<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits
Anton V. Tarasov
anton.tarasov at oracle.com
Mon Feb 18 22:58:37 PST 2013
On 16.02.2013 3:07, Jim Graham wrote:
> Thanks Anton,
>
> I just noticed that the lines look pretty long. The JDK style guidelines indicate 80 character
> lines:
>
> -------
> 4.1 Line Length
> Avoid lines longer than 80 characters, since they're not handled well by many terminals and tools.
>
> Note: Examples for use in documentation should have a shorter line length-generally no more than
> 70 characters.
> -------
>
> Sorry not to have noticed or mentioned that earlier, if you fix the line lengths I don't think I
> need to review it any more...
No problem, I fixed it )
Thanks!
Anton.
>
> ...jim
>
> On 2/15/13 1:56 AM, Anton V. Tarasov wrote:
>> Hi Jim,
>>
>> I agree with the rest of your comments, fixed it.
>>
>> Here's the latest:
>>
>> http://cr.openjdk.java.net/~ant/8006406/webrev.8
>>
>> http://cr.openjdk.java.net/~ant/8006406/javadoc.8
>>
>> Thanks!
>>
>> Anton.
>>
>> On 2/15/13 3:56 AM, Jim Graham wrote:
>>> Great Anton,
>>>
>>> Some spelling fixes.
>>>
>>> occures -> occurs
>>>
>>> {@code {i, j}} -> {@code (i, j)}
>>> (I usually use parens to encapculate coordinates, but maybe there are
>>> conventions in other places that use braces?)
>>>
>>> It might be good to put parens around the (0 <= i/j < width) formulas?
>>> But that would depend on how it looks in a browser and I didn't
>>> compile and javadoc the code to see that - if it looks fine without
>>> the parens then that's good.
>>>
>>> But, the description(s) look(s) good and accurate. Thanks!
>>>
>>> ...jim
>>>
>>> On 2/14/13 5:51 AM, Anton V. Tarasov wrote:
>>>> Hi Jim,
>>>>
>>>> On 2/14/13 6:26 AM, Jim Graham wrote:
>>>>> I've been busy with FX things, but I just got a chance to look at some
>>>>> of the new interfaces.
>>>>
>>>> Ok, thanks =)
>>>>
>>>>> Here are some (hopefully) minor comments:
>>>>>
>>>>> LightweightContent:
>>>>>
>>>>> You never really define "image origin". The imageBufferReset() takes
>>>>> an x,y, but it doesn't state what those are referring to. Is that the
>>>>> x,y on the screen/scene where the image should be rendered to? Are
>>>>> they the values to use to figure out what the starting offset in the
>>>>> data array for the data for the image should be? One thing that would
>>>>> help would be to include a formula in the method comments that
>>>>> indicates how the data for pixels is retrieved from the buffer so
>>>>> there is no confusion, something like:
>>>>>
>>>>> -----
>>>>> The {w} and {h} should match the width and height of the component
>>>>> returned from {getComponent()} with the pixel at the origin of the
>>>>> component, {(0, 0)} in the coordinate space of the component,
>>>>> appearing at {data[y * linestride + x]}. All indices {data[(y + j) *
>>>>> linestride + (x + i)]} where {0 <= i < w} and {0 <= j < h} will
>>>>> represent valid pixel data for the component.
>>>>> -----
>>>>>
>>>>> Did I interpret that correctly?
>>>>
>>>> Yes, the formula is correct. I've put it into the doc:
>>>>
>>>> /**
>>>> * {@code JLightweightFrame} calls this method to notify the client
>>>> application that a new data buffer
>>>> * has been set as a content pixel buffer. Typically this occures
>>>> when a buffer of a larger size is
>>>> * created in response to a content resize event. The method
>>>> reports a reference to the pixel data buffer,
>>>> * the content image bounds within the buffer and the line stride
>>>> of the buffer. These values have the
>>>> * following correlation.
>>>> * <p>
>>>> * The {@code width} and {@code height} matches the size of the
>>>> content (the component returned from the
>>>> * {@link #getComponent} method). The {@code x} and {@code y} is
>>>> the origin of the content, {@code {0, 0}}
>>>> * in the coordinate space of the content, appearing at {@code
>>>> data[y * linestride + x]} in the buffer.
>>>> * All indices {@code data[(y + j) * linestride + (x + i)]} where
>>>> {@code 0 <= i < width} and {@code 0 <= j < height}
>>>> * will represent valid pixel data, {@code {i, j}} in the
>>>> coordinate space of the content.
>>>> *
>>>> * @param data the content pixel data buffer of INT_ARGB_PRE type
>>>> * @param x the x coordinate of the image
>>>> * @param y the y coordinate of the image
>>>> * @param width the width of the image
>>>> * @param height the height of the image
>>>> * @param linestride the line stride of the pixel buffer
>>>> */
>>>> public void imageBufferReset(int[] data, int x, int y, int width,
>>>> int height, int linestride);
>>>>
>>>> Is that enough clear now?
>>>>
>>>>>
>>>>> Then when you refer to xywh in imageReshaped I'm guessing it is just
>>>>> supplying 4 new parameters to replace the identical parameters that
>>>>> were in the Reset() method?
>>>>
>>>> That's right. There're three distinct events: buffer recreation (always
>>>> or usually connected with new image bounds), image reshape (may not be
>>>> connected to the first event)
>>>> and image update (may not be connected to the first and second events).
>>>> So, I thought all the three events should be reflected separately.
>>>>
>>>>> Then in imageUpdated(), are the xywh relative to the coordinate system
>>>>> of the Component? Or are they in the same space as the original xywh
>>>>> were supplied to imageBufferReset? When you say they are "relative to
>>>>> the origin" I think you mean the former. The thing that makes it
>>>>> difficult to describe that is that you have the parameters to Reset
>>>>> and Reshape both named x,y and the parameters to Updated are also
>>>>> named x,y and one set of x,y parameters is relative to the other set
>>>>> and you end up having to say "The x and y are relative to the x and
>>>>> y". One of the sets of parameters should be renamed to make it easier
>>>>> to discuss how they relate. Some sort of "All indices in the range
>>>>> ..." statement would help to show how all of the numbers relate to
>>>>> each other.
>>>>
>>>> OK, I put it into the formula:
>>>>
>>>> /**
>>>> * {@code JLightweightFrame} calls this method to notify the client
>>>> application that a part of
>>>> * the content image, or the whole image has been updated. The
>>>> method reports bounds of the
>>>> * rectangular dirty region. The {@code dirtyX} and {@code dirtyY}
>>>> is the origin of the dirty
>>>> * rectangle, which is relative to the origin of the content,
>>>> appearing at
>>>> * {@code data[(y + dirtyY] * linestride + (x + dirtyX)] in the
>>>> pixel buffer
>>>> * (see {@link #imageBufferReset}). All indices {@code data[(y +
>>>> dirtyY + j] * linestride +
>>>> * (x + dirtyX + i)]} where {@code 0 <= i < dirtyWidth} and {@code
>>>> 0 <= j < dirtyHeight}
>>>> * will represent valid pixel data, {@code {i, j}} in the
>>>> coordinate space of the dirty rectangle.
>>>> *
>>>> * @param dirtyX the x coordinate of the dirty rectangle, relative
>>>> to the image origin
>>>> * @param dirtyY the y coordinate of the dirty rectangle, relative
>>>> to the image origin
>>>> * @param dirtyWidth the width of the dirty rectangle
>>>> * @param dirtyHeight the height of the dirty rectangle
>>>> *
>>>> * @see #imageBufferReset
>>>> * @see #imageReshaped
>>>> */
>>>> public void imageUpdated(int dirtyX, int dirtyY, int dirtyWidth,
>>>> int dirtyHeight);
>>>>
>>>>> In SwingNode:
>>>>>
>>>>> Why is getContent() not just "return content;"?
>>>>
>>>> Actually, I had some more complicated construction there (wrapped
>>>> content ref), in which I needed the local copy, then I reduced it, but
>>>> missed the fact that it's even simpler now.
>>>> Thanks for noticing, I'll fix it.
>>>>
>>>>>
>>>>> Have you discussed the threading issues with anyone in FX? There is a
>>>>> big discussion right now on the appropriate threads for various
>>>>> activities...
>>>>
>>>> Not yet. I think we can start this discussion in the context of the
>>>> review of the fx part (which actually does the threading stuff).
>>>>
>>>> Thanks,
>>>> Anton.
>>>>
>>>>
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 2/13/13 4:57 AM, Anton V. Tarasov wrote:
>>>>>> Hi Sergey,
>>>>>>
>>>>>> new webrev: http://cr.openjdk.java.net/~ant/8006406/webrev.7
>>>>>>
>>>>>> On 2/12/13 4:57 PM, Sergey Bylokhov wrote:
>>>>>>> Hi, Anton.
>>>>>>> Notes about implementation:
>>>>>>> 1 Seems some code was changed for debug simplifications or changes
>>>>>>> from previous implementations. It would be good to revert them back.
>>>>>>> (Ex /LWComponentPeer.bounds).
>>>>>> Fixed all such occurrences (replaced with public "get" methods where
>>>>>> available). Also, added "protected initializeBase(..)" method for
>>>>>> field
>>>>>> only initialization.
>>>>>>
>>>>>>> 2 Probably it would be good to move grab/ungrab implementation from
>>>>>>> LWToolkit/WToolkit to SunToolkit? It looks unclear why we need so
>>>>>>> many
>>>>>>> grab/ungrab/grabFocus/ungrabFocus methods with the same
>>>>>>> implementation
>>>>>>> in the different places.
>>>>>> We don't really need so much grabs and I will clean it when (and
>>>>>> if) we
>>>>>> publish the grab API. Please, see my replies to Anthony on this
>>>>>> subject.
>>>>>>
>>>>>>> 3 I suggest make all methods in LightweightFrame final if possible.
>>>>>> Ok, I made toplevel related methods final. I'm not sure we should make
>>>>>> final all the rest... (and by the way, the extender JLF class is
>>>>>> final).
>>>>>>
>>>>>>> 4 JLightweightFrame.rootPane could be final
>>>>>> Did.
>>>>>>
>>>>>>> 5 JLightweightFrame.getGraphics() probably graphics should be
>>>>>>> initialized by correct window fonts/background/foreground? Also when
>>>>>>> you create backbuffer probably it should be filled by background
>>>>>>> color? Note that if transparent images are supported you should be
>>>>>>> aware about composite.
>>>>>>
>>>>>> Ok, I did:
>>>>>>
>>>>>> 1) set transparent background for JLightweightFrame
>>>>>> 2) set font/background/foreground for the Graphics.
>>>>>>
>>>>>> Now I think I shouldn't specially care about the composite (am I
>>>>>> right?).
>>>>>>
>>>>>>> 6 JLightweightFrame.initInterior you shouldn't dispose graphics.
>>>>>>
>>>>>> Yes, it seems this adheres to the javadoc:
>>>>>>
>>>>>> * Graphics objects which are provided as arguments to the
>>>>>> * <code>paint</code> and <code>update</code> methods
>>>>>> * of components are automatically released by the system when
>>>>>> * those methods return. For efficiency, programmers should
>>>>>> * call <code>dispose</code> when finished using
>>>>>> * a <code>Graphics</code> object only if it was created
>>>>>> * directly from a component or another <code>Graphics</code>
>>>>>> object.
>>>>>>
>>>>>> Fixed it.
>>>>>>
>>>>>>> 7 JLightweightFrame.reshape width * height could be changed to
>>>>>>> width |
>>>>>>> height ?
>>>>>> No =) It rather could be changed to w & h, but in order not to
>>>>>> confuse a
>>>>>> reader, I've changed it to w == 0 || h == 0.
>>>>>>
>>>>>>>
>>>>>>> 8 JLightweightFrame.reshape you did not flush old backbuffer.
>>>>>>
>>>>>> Did.
>>>>>>
>>>>>> Also, I had to override LWWindowPeer.updateCursorImmediately() in
>>>>>> LWLFP
>>>>>> to workaround the deadlock I faced on Mac.
>>>>>> The deadlock has the following nature:
>>>>>>
>>>>>> - EDT: holding the paintLock (a shared lock b/w JLF and SwingNode),
>>>>>> and
>>>>>> the cursor manager dives to native code and tries to invoke a
>>>>>> method on
>>>>>> Main (FX App) thread.
>>>>>> - FX Renderer: is about to render a SwingNode content, waiting on the
>>>>>> paintLock.
>>>>>> - FX App: (as far as I can guess) waiting for the Renderer to finish.
>>>>>>
>>>>>> I can start looking for the solution in parallel with the review,
>>>>>> and if
>>>>>> not yet found, I'd push the first patch with cursor updates disabled.
>>>>>>
>>>>>> Thanks!
>>>>>> Anton.
>>>>>>
>>>>>>>
>>>>>>> 08.02.2013 21:27, Anton V. Tarasov wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Please, review the changes for the CR:
>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006406
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~ant/8006406/webrev.6
>>>>>>>>
>>>>>>>> It introduces sun.swing.JLightweightFrame class, aimed at
>>>>>>>> lightweight
>>>>>>>> embedding of Swing components into java-based toolkits.
>>>>>>>> The primary target is JavaFX toolkit, however the class is not
>>>>>>>> limited to this usage and the API it provides is quite generic.
>>>>>>>>
>>>>>>>> Below I'm giving a link to the jfx side implementation. This
>>>>>>>> implementation should not be reviewed in this thread (it is in a
>>>>>>>> pre-review phase),
>>>>>>>> it should just clarify how the introduced API is supposed to be
>>>>>>>> used.
>>>>>>>> Namely, SwingNode.SwingNodeContent which implements
>>>>>>>> sun.swing.LightweightContent and forwards requests from
>>>>>>>> sun.swing.JLightweightFrame to NGExternalNode which does the
>>>>>>>> rendering.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~ant/RT-27887/webrev.1
>>>>>>>>
>>>>>>>> Some comments on the awt/swing part:
>>>>>>>>
>>>>>>>> - Only Win and Mac implementation is currently available, X11 will
>>>>>>>> come lately.
>>>>>>>>
>>>>>>>> - Win implementation uses a heavyweight window behind the
>>>>>>>> lightweight
>>>>>>>> frame, while it is not actually needed for lightweight embedding.
>>>>>>>> This is due to the architecture of the Win AWT peers which are
>>>>>>>> strongly tight to the native code, and it's not a trivial task to
>>>>>>>> separate them.
>>>>>>>> On Mac the lightweight frame peer is truly lightweight, meaning
>>>>>>>> that it doesn't create an NSWindow object behind it. The Mac port LW
>>>>>>>> abstraction
>>>>>>>> allows to override and substitute CPlatform* classes with their
>>>>>>>> lightweight stubs.
>>>>>>>>
>>>>>>>> - LightweightFrame, among others, introduces two new methods -
>>>>>>>> grabFocus() and ungrabFocus(boolean). Ideally, these methods should
>>>>>>>> go to
>>>>>>>> the super Window class where the grab API becomes public
>>>>>>>> (which is
>>>>>>>> a long-term project...). Current host of the grab API is SunToolkit,
>>>>>>>> which
>>>>>>>> now forwards the calls to LightweightFrame. This is necessary to
>>>>>>>> intercommunicate with the client when grab/ungrab happens on both
>>>>>>>> sides.
>>>>>>>>
>>>>>>>> - Unresolved issues exist, like modal dialogs, d&d etc. They are to
>>>>>>>> be addressed further.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Anton.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
More information about the awt-dev
mailing list