<AWT Dev> [8] Review request for CR 8006406: lightweight embedding in other Java UI toolkits
Anthony Petrov
anthony.petrov at oracle.com
Wed Feb 20 02:24:38 PST 2013
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