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

Anton V. Tarasov anton.tarasov at oracle.com
Fri Feb 15 01:56:21 PST 2013


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