<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 01:43:43 PST 2013
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