<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