<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits
Anton V. Tarasov
anton.tarasov at oracle.com
Tue Feb 19 02:14:39 PST 2013
On 19.02.2013 13:24, Anthony Petrov wrote:
> On 2/18/2013 18:10, Anton V. Tarasov wrote:
>> On 18.02.2013 16:50, Anthony Petrov wrote:
>>> As far as I know there's no RFEs filed that request to provide public API for grab/ungrab
>>> functionality. Also, it isn't clear whether this API could be useful for GUI applications.
>>
>> Current grab API was created as a draft, with an intention to make it public later. However
>> you're right, there's no RFE which could prove this fact (but we did have in our todo list :)).
>
> Right. My point is that I doubt this API could be useful for application developers.
>
>
>>> As such I don't find it ultimately needed to extend the WindowPeer interface with
>>> grabFocus/ungrabFocus() methods, nor add these methods as public interface to the
>>> LightweightFrame class at this time.
>>>
>>> Instead, I propose to introduce a sun.awt.FocusGrabbable interface with two methods: grabFocus()
>>> and ungrabFocus(). This interface needs to be implemented by the JLightweightFrame class only,
>>> where we can provide reasonable implementation by redirecting the calls to FX. In
>>> SunToolkit.grab()/ungrab() we should check for instanceof FocusGrabbable, and call the methods
>>> appropriately. Otherwise, if the window doesn't implement this interface, we proceed as usual.
>>>
>>> This would eliminate changes to the WindowPeer, remove the unneeded
>>> LightweightFrame.grab/ungrabFocus() methods, and generally simplify the fix (e.g. we won't need
>>> any changes in XWindowPeer, etc.)
>>>
>>> Opinions?
>>
>> At least, your suggestion bring us back to the discussion about the direction of the call chain.
>> What should a user expect from the JLF.grabFocus() method? I think the same as from the
>> SunToolkit.grab(JLF), right?
>> However, your version of JLF.grabFocus() looks more like a callback - JLF.notifyGrabbed. It won't
>> do real grab, but will tell the frame that someone has grabbed focus on it. Evolving this
>> approach, I'd say that,
>> from AWT perspective, it would be more natural to have a listener - GrabListener, and
>> GrabEvent/UngrabEvent appropriately. In which case, we would need to add a GrabEvent class, a
>> pair for the existing UngrabEvent.
>> This seems consistent to me (with AWT standarts). But at the same time, IMHO, this doesn't worth
>> the candle. And so, I still think the original approach better serves the needs.
>
> I think I didn't provide you with any version of "my" JLF.grabFocus() yet, so here it is:
>
> public interface sun.awt.FocusGrabbable {
> void grabFocus();
> void ungrabFocus(boolean postEvent);
> }
>
> As you can see, these are very much the same grab/ungrabFocus() methods that you have in your
> current fix. I suggest to implement this interface in the JLightweightFrame class only, since this
> is where they are really needed, and their implementation is entirely different from others
> (including the LightweightFrame, which in the end does the same thing as e.g. Frame, currently).
>
>
>> By the way, current SunToolkit.ungrab() implementation does ungrab silently, without posting
>> UngrabEvent. This is not enough for FX SwingNode. And thus, we anyway need
>> SunToolkit.ungrab(boolean) or
>> SunToolkit.ungrabAndPost() or something of the like. LightweightFrame.ungrabFocus(boolean) looks
>> better to me.
>
> Now, in e.g. WToolkit the grab() method should look like:
>
> 987 public void grab(Window w) {
> 991 if (w instanceof FocusGrabbable) {
> 992 ((FocusGrabbable)w).grabFocus();
> 993 return;
> 994 }
> 995 if (w.getPeer() != null) {
> 996 ((WWindowPeer)w.getPeer()).grab();
> 997 }
> 998 }
>
> and a similar implementation with a check for FocusGrabbable for the ungrab().
>
> I hope the above clarifies my proposal well enough.
Anthony,
Sorry, but I don't understand your proposal.
In the fix, LightweightFrame does the following:
113 public void grabFocus() {
114 ((WindowPeer)getPeer()).grabFocus();
115 }
In the above code you call JLF.grabFocus() and then return. At the same time, you're rejecting
grab/ungrab in WindowPeer, from what I can conclude you're not going to call the peer (line 114 above).
So, how JLF.grabFocus() will _grab_? (Currently, only a peer does grab/ungrab logic).
When SunToolkit.grab(Window) is called for JLF it should do the following:
1. grab like a generic frame
2. notify (the client) via LightweightContent instance about the grab
I'd understand the following logic:
public void grab(Window w) {
if (w instanceof FocusGrabbable) {
((FocusGrabbable)w).grabFocus(); // 2. notify
}
if (w.getPeer() != null) {
((WWindowPeer)w.getPeer()).grab(); // 1. do grab
}
}
in which case it notifies and then grabs. But, as I wrote before, here the name grabFocus() doesn't
seem correct, but something like notifyGrabbed() would make sense.
>
>
>> I don't think having it in WindowPeer is a big price. Moreover, the implementation is already in
>> the peers. I just added the methods to the peer interfaces.
>
> I don't think this is necessary at this moment. Besides, not adding them to the interfaces allows
> us to avoid implementing them in XWindowPeer, WWindowPeer, and LWWindowPeer, too. Also, we
> absolutely don't need public grab/ungrabFocus() methods in LightweightFrame, too. Hence, as you
> can see, the fix could be simplified.
All *WindowPeer classes _already_ implement grab/ungrab functionality. What I'm doing is just
refining the interface.
Please, clarify your point...
Thanks,
Anton.
>
>
>> And also, com.sun.javafx.tk.TKStage has grabFocus() and ungrabFocus() methods (the latter does
>> send the event with no option). Why not in AWT? At worst, it can stay in sun.awt.LightweightFrame
>> forever.
>
> TKStage is a peer interface, it is not a part of public FX API. Also, AWT has its own, "legacy"
> code structure, and I don't see a reason to change it anyhow at this time.
>
> --
> best regards,
> Anthony
>
>>
>> Thanks,
>> Anton.
>>
>>
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 02/15/13 13:56, 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