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

[Piet Blok]

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