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

[Jim Graham]

I've been busy with FX things, but I just got a chance to look at some 
of the new interfaces.  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?

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?

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.

In SwingNode:

Why is getContent() not just "return content;"?

Have you discussed the threading issues with anyone in FX?  There is a 
big discussion right now on the appropriate threads for various 
activities...

			...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.
>>>
>>
>>
>