<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits

Jim Graham james.graham at oracle.com
Thu Feb 14 15:56:03 PST 2013

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!


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