<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits
Anton V. Tarasov
anton.tarasov at oracle.com
Wed Feb 20 03:18:16 PST 2013
Great. Thanks!
Anton.
On 20.02.2013 14:24, Anthony Petrov wrote:
> I'm fine with the fix. Thank you.
>
> --
> best regards,
> Anthony
>
> On 2/20/2013 13:43, Anton V. Tarasov wrote:
>> Anthony, Sergey,
>>
>> Eventually, I'm ok with the idea to just forward grab/ungrab to the embedder, and just
>> symmetrically post UngrabEvent from the embedder.
>>
>> However, I suggest to implement this w/o the FocusGrabbable interface. I'm (yet) adding
>> focusGrab/focusUngrab to the LightweightFrame, but these methods are now abstract.
>> (remember that the LightweightFrame is an abstract class). JLightweightFrame implements them.
>>
>> For me, it seems naturally to have the methods in LF. The grab is an AWT stuff, LF forces its
>> extender to implement them, we work with LF on the AWT side and LF exposes complete interface.
>> The benefit is that we get rid of any "instanceof" check in this context.
>>
>> No grab/ungrab in the WindowPeer interface. I only override existing *WindowPeer.grab/ungrab in
>> *LightweightFramePeer and delegate the calls to the LF target.
>>
>> webrev: http://cr.openjdk.java.net/~ant/8006406/webrev.9
>>
>> Thanks,
>> Anton.
>>
>>
>> On 2/19/13 5:14 PM, Anton V. Tarasov wrote:
>>> On 19.02.2013 16:22, Anthony Petrov wrote:
>>>>
>>>> On 2/19/2013 16:05, Anton V. Tarasov wrote:
>>>>> On 19.02.2013 14:28, Anthony Petrov wrote:
>>>>>> (please find my reply inline)
>>>>>>
>>>>>> On 2/19/2013 14:14, Anton V. Tarasov wrote:
>>>>>>> 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 understand why you need to call WWindowPeer.grab(). In our use case, we embed Swing
>>>>>> content in an FX window. When Swing code requests to grab focus, we should simply redirect
>>>>>> this request to FX (i.e. call TKStage.grabFocus() for the FX window). That's all what is
>>>>>> needed. We don't need to also arm the AWT's native grab machinery in this case. We expect FX
>>>>>> to send us an UNGRAB event when the focus is ungrabbed.
>>>>>>
>>>>>> Why do you want to call WWindowPeer.grab()? What will break if you remove the following line:
>>>>>>
>>>>>> src/share/classes/sun/swing/JLightweightFrame.java:
>>>>>>> 102 super.grabFocus();
>>>>>>
>>>>>> ? I believe nothing should break, and this call is just unneeded here.
>>>>>
>>>>> I suppose you suggest to remove super.ungrabFocus(boolean) as well. Then, in the following
>>>>> scenario:
>>>>>
>>>>> 1. I show a swing popup in JLF, it calls SunToolkit.grab(JLF) which just forwards the grab to
>>>>> the fx stage.
>>>>>
>>>>> 2. I click in the title of the stage. It initiates ungrab, I intercept the FocusUngrabEvent
>>>>> and call SunToolkit.ungrab(true) (let's assume we have SunToolkit.ungrab(boolean)).
>>>>> It's no op, because AWT grab has not been armed. JLF won't receive UngrabEvent.
>>>>
>>>> FX's UNGRAB_FOCUS event should be translated directly into an AWT's UngrabEvent. We don't need
>>>> to call ungrab() upon receiving this event. We should just send a corresponding AWT event. This
>>>> is exactly what JFXPanel does, but the other way around. I've sent you a link (privately) to a
>>>> webrev that shows what we do for JFXPanel in this case.
>>>>
>>>>
>>>>> Or you think we should post UngrabEvent instead of calling SunToolkit.ungrab? From my point of
>>>>> view this is an interference into the mechanism. Yes, it's (currently) simple, but still this
>>>>> approach doesn't seem correct to me.
>>>>> The proposed fix is reusing the logic, not changing it, not half-copying. This also means less
>>>>> chances to face pitfalls.
>>>>
>>>> I don't see how this looks like "half-copying" to you. A request is translated into a request,
>>>> an event - into an event. Everything is symmetrical.
>>>
>>> Well, I think I see your point now. Let me think if this is indeed symmetrical...
>>>
>>> Thanks,
>>> Anton.
>>>
>>>>
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>>>
>>>>> Thanks,
>>>>> Anton.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> I'd be OK if you removed the old methods (e.g. WWindowPeer.grab()) and used the new ones
>>>>>> (grabFocus()) exclusively. However, my above proposal suggests that we just don't need to
>>>>>> change this part at all.
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>>>
>>>>>>> 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