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

[Anthony Petrov]

Hi Piet,

The version #2 looks very good.

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.


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

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.

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.

Otherwise looks fine. Thanks!

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