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

[Artem Ananiev]

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()...

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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>>
>
>