<AWT Dev> <Swing Dev> Fw: reviewrequestfor6899434:Addaffinetransformsupport to JLayer

Alexander Potochkin Alexander.Potochkin at Sun.COM
Wed Feb 24 05:44:18 PST 2010


Hello Piet and the team

While the AWT part of the fix is clear and almost done
there are some alternatives
how we should make the best out of it on the Swing side

To make the best decision and get the early feedback from the community
I propose to split the generic support of mouse events
from adding getTransform()/setTransform() on the Swing side.

This is one of the fix which is better to be done step by step.

I filed a new RFE #6929295:
Generic support of mouse event transformation for AWT/Swing

Fixing it will enable the users to easily use
affine transforms for custom Swing components,
which is a fine addition itself.

The next steps will be certainly discussed
shortly after that

Piet, could you please make a new webrev for 6929295?

It should be basically the same fix as for 6899434
but with no changes in JLayer

It would be really great if we manage to do only with
toLayoutSpace()/toRenderedSpace() methods
without using JComponent.inverseTransformVisibleRectangle()

I hope you can emulate it by transforming the corner points of the 
rectangle with toLayoutSpace() method and using its bounds
inside the JComponent.computeVisibleRect() method.

For the default case (no transforms, rectangles only) it will be cheap
and will simplify the usage of the new feature for the Swing developers.

Thanks
alexp

>
> Hi Artem
>
>
>>
>> On 2/16/2010 6:12 PM, Piet Blok wrote:
>>> Hi Artem
>>>
>>>> Hi, Piet,
>>>>
>>>> the fix looks pretty good in general. Some small comments:
>>>>
>>>> 1. As we're about to introduce 2 new public methods into Container, we
>>>> need to provide a description of "layout space" and "render space". I
>>>> hope you or Alex will take care of this.
>>>
>>> What about the following descriptions?
>>>
>>> Layout space refers to user space (coordinates in the layout system).
>>> Rendered space refers to device space (coordinates on the screen).
>>> Please see (link to) AffineTransform.
>>
>> Probably, we should mention that layout coordinates are the ones used
>> by LayoutManager and other Container stuff. For example,
>> Component.getBounds() always returns layout rectangle, not rendered one.
>
> Ok, good addition. I'll add that clarification.
>
>>
>>>> 2. Could toRenderedSpace() throw NoninvertibleTransformException as
>>>> well?
>>>
>>> No. toRenderedSpace() is a direct transformation with any of
>>> AffineTransform's transform() methods.
>>> This in contrast to inverse operations as used in toLayoutSpace(), like
>>> AffineTransform's createInverse() and inverseTransform() methods that
>>> may throw this exception.
>>
>> It depends on what we consider a forward transform and a reverse
>> transform :) And don't forget there may be non-affine transforms...
>
> Any transform, be it affine or non-affine is able by definition to
> transform, otherwise it's not a transform :-)
>
> Only inverse transform can be problematic. That is, when two different
> points, after transformation, result in the same point. Valid during
> transform. But impossible to do an inverse. Analogous to multiplying by
> zero is valid, but the operation can't be inversed.
>
>>
>>>> 3. getRenderedSpaceShift() - should we traverse containers up to null
>>>> or up to the nativeContainer?
>>>
>>> Good point. For testing I added a test "not nativeContainer" to the null
>>> test and everything still works (in my own test suite that is far more
>>> complex than the provided jtreg test cases).
>>>
>>> getRenderedSpaceShift() is invoked from eventDispatched. It should do a
>>> traversal that is analogous to the traversal there.
>>>
>>> In general, I'm not sure about the role of "nativeContainer" and how it
>>> is used. For example, I don't know if (one or more) native containers
>>> can be present in the hierachy between a Window and the lowest child
>>> component. Or is the top Window always the native container? If this is
>>> not the case, could you depict some hierarchy example where a native
>>> container is a child somewhere in the hierarchy of a JLayer's view? Then
>>> I can do a more comprehensive test.
>>
>> nativeContainer is a basic part of LightweightDispatcher machinery: it
>> is the container, always heavyweight, which is served by the
>> dispatcher. An obvious example is how all the mouse events are
>> dispatched: we (AWT) receive events for heavyweight components only as
>> the underlying OS isn't aware of our lightweight Swing components.
>> When the event is dispatched to a heavyweight container, it's
>> lightweight dispatcher retargets it to a proper lightweight child.
>>
>> Given that we won't support transformations for heavyweight components
>> (BTW, it should also be reflected in JavaDoc), it's enough to care
>> about nativeContainer children only.
>
>
> 1) I'll add a remark to the new public methods that they only apply to
> lightweight components.
> 2) I'll study your remarks and decide if I'll add a check on
> nativeContainer.
>
> I'll let you know if and when a version 3.2 is available.
>
> Thanks,
> Piet
>
>>
>>> Please let me know, so I can prepare a version 3.2 if needed (and add
>>> the descriptions at the same time)
>>
>> 3.2 is only required if you make any significant changes based on the
>> current discussion.
>>
>> Thanks,
>>
>> Artem
>>
>>> Thanks,
>>> Piet.
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 2/12/2010 10:53 AM, Piet Blok wrote:
>>>>>
>>>>> Hi Artem,
>>>>>
>>>>> The webrev for version 3.1:
>>>>> http://www.pbjar.org/OpenJDK/6899434/version-3.1/webrev/
>>>>>
>>>>>
>>>>>> Hi, Piet,
>>>>>>
>>>>>> the 3rd version looks really great! I haven't looked to Swing code
>>>>>> much, though :)
>>>>>>
>>>>>> A small suggestion is to unify getRenderedSpaceShift() and
>>>>>> getLayoutSpaceShift(), so they both accept a Component and a Point to
>>>>>> translate. Could they also be static methods? It seems not, as I
>>>>>> see a
>>>>>> reference to "nativeContainer" in getLayoutSpaceShift()...
>>>>>
>>>>> I unified both methods and made them static after adding
>>>>> nativeContainer
>>>>> as a parameter to both. Their signature is now:
>>>>>
>>>>> private static Point getXXXSpaceShift(Component target, Point
>>>>> dstPoint,
>>>>> Container nativeContainer)
>>>>>
>>>>> (the nativeContainer argument is used in only one of the methods)
>>>>>
>>>>> For the swing guys: in SwingUtilities and RepaintManager, where
>>>>> iterating over the parent hierarchy, I added a check to "parent !=
>>>>> null"
>>>>> to also check "!(parent instanceof Window)".
>>>>>
>>>>> Thanks,
>>>>> Piet
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Artem
>>>>>>
>>>>>> On 2/11/2010 5:25 PM, Piet Blok wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please find a third version for the webrev here:
>>>>>>> http://www.pbjar.org/OpenJDK/6899434/version-3/webrev/
>>>>>>>
>>>>>>> AWTAccessor removed again
>>>>>>>
>>>>>>> 2 protected methods for Container: toLayoutSpace(x,y) and
>>>>>>> toRenderedSpace(x,y), overridden in JLayer.
>>>>>>>
>>>>>>> Use getContainer() in getRenderedSpaceShift(), but getParent() in
>>>>>>> getLayoutSpaceShift(). The latter because it is called from
>>>>>>> retargetMouseEvent which itself uses getParent() when finding the
>>>>>>> hierarchy translation value.
>>>>>>>
>>>>>>> Indented the try block
>>>>>>>
>>>>>>> Added some jtreg test cases, one a manual test.
>>>>>>>
>>>>>>> Please review again
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Piet
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Hi Anthony,
>>>>>>>>
>>>>>>>>> Hi Piet,
>>>>>>>>>
>>>>>>>>> The version #2 looks very good.
>>>>>>>>
>>>>>>>> Looks, yes. Unfortunately, later I detected that it doesn't work.
>>>>>>>> It's
>>>>>>>> missing something. Oh yes, I carried out a comprehensive manual
>>>>>>>> test
>>>>>>>> but the test setup was wrong: I tested against the version 1! (A
>>>>>>>> jtreg
>>>>>>>> test was carried out against version 2 and was succesfull).
>>>>>>>>
>>>>>>>> I'll try to manually add a remark to that webrev to state that it's
>>>>>>>> invalid and should not be used.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/9/2010 4:30 PM Piet Blok wrote:
>>>>>>>>>> 1) The implementation in version 2 will be used but without the
>>>>>>>>>> AWTAccessor.
>>>>>>>>>
>>>>>>>>> So that the Component.transform is moved over to the JLayer class,
>>>>>>>>> right? That would be great.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 2) Container.toLayoutSpace(Point) will become protected and the
>>>>>>>>>> Container implementation does nothing.
>>>>>>>>>>
>>>>>>>>>> 3) LightweightDispatcher.toLayoutSpace(MouseEvent) will remain
>>>>>>>>>> private and static, but will be rewritten to use
>>>>>>>>>> Container.toLayoutSpace(Point) in a hierachy loop.
>>>>>>>>>>
>>>>>>>>>> 4) LightweightDispatcher.concatenateHierarchyTransforms() will of
>>>>>>>>>> course be removed.
>>>>>>>>>
>>>>>>>>> I like the proposal.
>>>>>>>>
>>>>>>>> As said, something was missing: A
>>>>>>>> Container.toRenderedSpace(point) is
>>>>>>>> needed as well. This method must return the normal transformed
>>>>>>>> point,
>>>>>>>> as opposed to toLayoutSpace() that returns the inverse transformed
>>>>>>>> point.
>>>>>>>>
>>>>>>>> And yes, like Artem pointed out in an earlier post, this leaves the
>>>>>>>> option open for implementers to choose for a transformation other
>>>>>>>> than
>>>>>>>> AffineTransform. Fish eye, some sinus, whatever. (Curious to
>>>>>>>> know how
>>>>>>>> one would implement the actual rendering, but that's beside the
>>>>>>>> point).
>>>>>>>>
>>>>>>>>>
>>>>>>>>> A minor comment regarding the code:
>>>>>>>>>
>>>>>>>>> src/share/classes/java/awt/Container.java
>>>>>>>>>> 4875 Component parent = comp.getParent();
>>>>>>>>>
>>>>>>>>> I suggest to use the getContainer() method instead. If the comp
>>>>>>>>> is a
>>>>>>>>> window, the getParent() may actually return an owner of the
>>>>>>>>> window,
>>>>>>>>> which we certainly don't want to deal with.
>>>>>>>>
>>>>>>>> Aha, wasn't aware of getContainer() (package private). Very good.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also, please make sure you format the code according the
>>>>>>>>> guidelines:
>>>>>>>>> in Container.java the code put in the new try{} blocks must be
>>>>>>>>> correctly indented.
>>>>>>>>
>>>>>>>> This I was wondering about: should I or shouldn't I (touch code
>>>>>>>> that
>>>>>>>> is otherwise not altered). Now I know, thanks.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Otherwise looks fine. Thanks!
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ok, I'm working on version 3. And this time actually testing
>>>>>>>> against
>>>>>>>> this same version 3! I'm still working on some more simple jtreg
>>>>>>>> test
>>>>>>>> cases and I'll change to getContainer() and indent correctly.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Piet
>>>>>>>>
>>>>>>>>> --
>>>>>>>>> best regards,
>>>>>>>>> Anthony
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Please let me know if you agree.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Piet
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Artem
>>>>>>>>>>>
>>>>>>>>>>> On 2/8/2010 2:27 PM, Piet Blok wrote:
>>>>>>>>>>>> Hi Artem,
>>>>>>>>>>>>
>>>>>>>>>>>> To demonstrate the implemention via the AWTAccessor pattern, I
>>>>>>>>>>>> created a
>>>>>>>>>>>> version 2 implementation:
>>>>>>>>>>>>
>>>>>>>>>>>> http://www.pbjar.org/OpenJDK/6899434/version-2/webrev/
>>>>>>>>>>>>
>>>>>>>>>>>> This implementation is much cleaner than the original one.
>>>>>>>>>>>>
>>>>>>>>>>>> Looking forward for yout comments,
>>>>>>>>>>>> Piet
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Artem,
>>>>>>>>>>>>>
>>>>>>>>>>>>> The problem with making existing methods public is that it
>>>>>>>>>>>>> solves
>>>>>>>>>>>>> only
>>>>>>>>>>>>> half of the problem at hand:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1) Locate the correct component (can be solved)
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2) Recalculating the mouse point from rendered space to layout
>>>>>>>>>>>>> space
>>>>>>>>>>>>> is not solved because the locating methods only return a
>>>>>>>>>>>>> component.
>>>>>>>>>>>>> Recalculation is needed to correctly set a mouse point in the
>>>>>>>>>>>>> new
>>>>>>>>>>>>> events, relative to the target component.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In my proposed implementation the shift caused by
>>>>>>>>>>>>> transformations is
>>>>>>>>>>>>> stored when looking up the target (for future use: creating
>>>>>>>>>>>>> new
>>>>>>>>>>>>> events
>>>>>>>>>>>>> from the original event). This future is quite an immediate
>>>>>>>>>>>>> future
>>>>>>>>>>>>> because creating a new event from an existing event will
>>>>>>>>>>>>> always be
>>>>>>>>>>>>> directly preceded by looking up that target event.
>>>>>>>>>>>>>
>>>>>>>>>>>>> An alternative would be to again iterate through the hierarchy
>>>>>>>>>>>>> and do
>>>>>>>>>>>>> the transformations. This must be done in
>>>>>>>>>>>>> LightweightDispatcher
>>>>>>>>>>>>> in the
>>>>>>>>>>>>> methods:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1) retargetMouseEvent (an inverse transform is needed, so the
>>>>>>>>>>>>> new
>>>>>>>>>>>>> Container method getConvertedPoint can be used)
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2) eventDispatched. Unfortunately here an ordinary
>>>>>>>>>>>>> transform is
>>>>>>>>>>>>> needed, so a second new Container method must be defined that
>>>>>>>>>>>>> does an
>>>>>>>>>>>>> ordinary transform.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But.... a completely different approach is also possible. I
>>>>>>>>>>>>> did
>>>>>>>>>>>>> this
>>>>>>>>>>>>> in an earlier version, so I know that it works. With this
>>>>>>>>>>>>> approach no
>>>>>>>>>>>>> new public or protected methods need to be introduced and no
>>>>>>>>>>>>> existing
>>>>>>>>>>>>> methods need to go public or protected. All remains private or
>>>>>>>>>>>>> package
>>>>>>>>>>>>> private.
>>>>>>>>>>>>>
>>>>>>>>>>>>> That approach is as follows:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1) Define the AffineTransform as a private field in Component.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2) Use the AWTAccessor pattern to make the transform
>>>>>>>>>>>>> available in
>>>>>>>>>>>>> Container and LightweightDispatcher and in swing classes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3) In Container and LightweightDispatcher, get the transform
>>>>>>>>>>>>> and do
>>>>>>>>>>>>> transformations when needed.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In my opinion, the solution with the AWTAccessor pattern is
>>>>>>>>>>>>> the
>>>>>>>>>>>>> cleanest. However, it requires Component and AWTAccessor to be
>>>>>>>>>>>>> touched.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please let me know what you think.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Piet
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi, Piet,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I haven't looked through the entire webrev and inspected
>>>>>>>>>>>>>> mostly an
>>>>>>>>>>>>>> AWT part of the fix. A question is whether it's possible to
>>>>>>>>>>>>>> get rid
>>>>>>>>>>>>>> of the new "conversionShift" field in Container, to make
>>>>>>>>>>>>>> transformations support really stateless?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Another option to consider is to make some of the existing
>>>>>>>>>>>>>> methods
>>>>>>>>>>>>>> (e.g. getMouseEventTargetImpl()) public instead of
>>>>>>>>>>>>>> introducing
>>>>>>>>>>>>>> new ones.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Artem
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 1/28/2010 8:21 PM, Piet Blok wrote:
>>>>>>>>>>>>>>> Hello all,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> review request for 6899434: Add affine transform support to
>>>>>>>>>>>>>>> JLayer
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The webrev: http://www.pbjar.org/OpenJDK/6899434/webrev/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The patch covers all the requested functionality. It is
>>>>>>>>>>>>>>> concentrated in
>>>>>>>>>>>>>>> JLayer class, keeping in mind to affect the library as
>>>>>>>>>>>>>>> little as
>>>>>>>>>>>>>>> possible.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 1) A setter and getter for the transform in JLayer
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 2) The paint method in JLayer has been adapted to use the
>>>>>>>>>>>>>>> transform
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 3) RepaintManager has been adapted to propagate repaint
>>>>>>>>>>>>>>> requests from
>>>>>>>>>>>>>>> the view or any of its children to the top level JLayer and
>>>>>>>>>>>>>>> have the
>>>>>>>>>>>>>>> dirty region transformed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 4) java.awt.Container and java.awt.LightweightDispatcher
>>>>>>>>>>>>>>> (both
>>>>>>>>>>>>>>> in the
>>>>>>>>>>>>>>> same source) have been adapted to redispatch MouseEvents to
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> intended
>>>>>>>>>>>>>>> target component. The lookup for the component that
>>>>>>>>>>>>>>> provides a
>>>>>>>>>>>>>>> custon
>>>>>>>>>>>>>>> cursor has also been adapted.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 5) To enable Container to do necessary reculculations, a
>>>>>>>>>>>>>>> protected
>>>>>>>>>>>>>>> method has been introduced that will be overridden by
>>>>>>>>>>>>>>> JLayer:
>>>>>>>>>>>>>>> protected Point getConvertedPoint(Point point).
>>>>>>>>>>>>>>> (If someone can suggest a better name for this method I'm
>>>>>>>>>>>>>>> glad
>>>>>>>>>>>>>>> to hear)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 6) A package private method in SwingUtilities has been added
>>>>>>>>>>>>>>> that helps
>>>>>>>>>>>>>>> JPopupMenu and ToolTipManager to find the correct popup
>>>>>>>>>>>>>>> location.
>>>>>>>>>>>>>>> JPopupMenu and ToolTipManager have been changed to use this
>>>>>>>>>>>>>>> new
>>>>>>>>>>>>>>> method
>>>>>>>>>>>>>>> in their calculations.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 7) Two jtreg tests have been added.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looking forward for comments.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Piet
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>




More information about the awt-dev mailing list