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

[Piet Blok]

Hi Artem,

Thanks for the detailed evaluation.


> Hi, Piet,
>
> let's try to find if these two approaches could be combined together. What 
> I personally like is:
>
> 1. There's no changed in Component (approach 1).
>
> 2. AWTAccessor is not changed and not used (approach 1).
>
> 3. Small changes in Container (approach 2).

Ok, let's try to do it without AWTAccessor and keep the changes in Container 
as small as possible.

>
> As you correctly noticed, we need some (private or public) API to do both 
> forward and reverse transforms - to find a component under mouse and to 
> send a translated mouse event. Why couldn't we make the following minimal 
> changes then:
>
> a. Introduce a new protected method toLayoutSpace() - or whatever it's 
> named - which does nothing by default, but overridden in JLayer to respect 
> its transform.

Ok (like in approach 1). The toLayoutSpace(Point) in Container will become 
protected and will do nothing. JLayer will override it.

>
> b. Make findComponentAt() protected. It will then be overridden in JLayer 
> to respect its inverse transform.

I'm not so sure. First of all, there are two methods in Container that need 
adustment:

- findComponentAt Impl(used for lookup of a component that provides a 
cursor)
- getMouseEventTargetImpl (used for lookup of a target component for a 
MouseEvent)

Both methods use a different lookup approach and follow a chain of method 
invocations. Both are quite complicated and have some parameters that 
influence the lookup. Overriding them in JLayer would make it necessary to 
copy all that code in JLayer and just make one tiny adjustment for the 
transform.

Besides, why did we introduce the toLayoutSpace(Point) method? That method 
is intended to be used to have the Point transformed, like in version 2 
(which you liked) but without the AWTAccessor. That method is introduced 
exactly for that purpose!

>
> As a side effect, this would give the developers a flexibility to use 
> non-affine transforms. They will even be able to use different transforms 
> for forward and reverse operations, but I doubt anyone will want this (or 
> we could add an explicit warning to JavaDoc about this).

Hmm.. I share your doubts, but nevertheless.

You forget one thing to mention: the retargeting in LightweightDispatcher 
also needs adjustment. In version 2 (which you liked) a call is made to a 
private static methods toLayoutSpace(MouseEvent) which uses the AWTAccessor.

This method can be adapted to use the Container.toLayoutSpace(Point) method 
to update the MouseEvent. The AWTAccessor pattern is then obsolete.

So, in summary,

1) The implementation in version 2 will be used but without the AWTAccessor.
:
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.

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