<AWT Dev> <Swing Dev> Fw: review request for6899434:Addaffinetransformsupport to JLayer

Artem Ananiev Artem.Ananiev at Sun.COM
Tue Feb 16 04:49:46 PST 2010


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.

2. Could toRenderedSpace() throw NoninvertibleTransformException as well?

3. getRenderedSpaceShift() - should we traverse containers up to null or 
up to the nativeContainer?

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