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

Piet Blok pbhome at ziggo.nl
Thu Feb 11 23:53:38 PST 2010


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