<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits
Jim Graham
james.graham at oracle.com
Fri Feb 15 15:07:03 PST 2013
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...
...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