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

Fri Feb 12 14:56:21 UTC 2010

> Hello Piet
>
> I am fine with 3rd version,
> waiting for the comments from AWT guys

small correction:

I meant 3.1 version (the latest one)

Thanks again
alexp

>
> Thanks!
> alexp
>
>>
>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>