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