<Swing Dev> <AWT Dev> Fw:reviewrequestfor6899434:Addaffinetransformsupport to JLayer

[Piet Blok]

Hi All,



> Hello Piet and the team
>
> While the AWT part of the fix is clear and almost done
> there are some alternatives
> how we should make the best out of it on the Swing side
>
> To make the best decision and get the early feedback from the community
> I propose to split the generic support of mouse events
> from adding getTransform()/setTransform() on the Swing side.
>
> This is one of the fix which is better to be done step by step.
>
> I filed a new RFE #6929295:
> Generic support of mouse event transformation for AWT/Swing
>
> Fixing it will enable the users to easily use
> affine transforms for custom Swing components,
> which is a fine addition itself.
>
> The next steps will be certainly discussed
> shortly after that
>
> Piet, could you please make a new webrev for 6929295?
>
> It should be basically the same fix as for 6899434
> but with no changes in JLayer

Done. Please see here http://www.pbjar.org/OpenJDK/6929295/webrev/

(It is created from the same respository, hence the misleading name insiade 
the webrev)

I couldn't find rfe 6929295 in the bug database. Perhaps it's not yet 
publicly available?

Changes versus the last version v3.2:

1) Added a check on nativeContainer (I don't think it is necessary, but it 
doesn't hurt)
2) Edited the API doc for the two new methods

>
> It would be really great if we manage to do only with
> toLayoutSpace()/toRenderedSpace() methods
> without using JComponent.inverseTransformVisibleRectangle()
>
> I hope you can emulate it by transforming the corner points of the 
> rectangle with toLayoutSpace() method and using its bounds
> inside the JComponent.computeVisibleRect() method.

Technically such a computation is possible. However, the 
JLayer.inverseTransformVisibleRectangle() does something more:
it intersect the resulting rectangle with the transformed view bounds. But 
let's discuss that later.

>
> For the default case (no transforms, rectangles only) it will be cheap
> and will simplify the usage of the new feature for the Swing developers.
>
> Thanks
> alexp
>
>>
>> Hi Artem
>>
>>
>>>
>>> On 2/16/2010 6:12 PM, Piet Blok wrote:
>>>> Hi Artem
>>>>
>>>>> Hi, Piet,
>>>>>
>>>>> the fix looks pretty good in general. Some small comments:
>>>>>
>>>>> 1. As we're about to introduce 2 new public methods into Container, we
>>>>> need to provide a description of "layout space" and "render space". I
>>>>> hope you or Alex will take care of this.
>>>>
>>>> What about the following descriptions?
>>>>
>>>> Layout space refers to user space (coordinates in the layout system).
>>>> Rendered space refers to device space (coordinates on the screen).
>>>> Please see (link to) AffineTransform.
>>>
>>> Probably, we should mention that layout coordinates are the ones used
>>> by LayoutManager and other Container stuff. For example,
>>> Component.getBounds() always returns layout rectangle, not rendered one.
>>
>> Ok, good addition. I'll add that clarification.
>>
>>>
>>>>> 2. Could toRenderedSpace() throw NoninvertibleTransformException as
>>>>> well?
>>>>
>>>> No. toRenderedSpace() is a direct transformation with any of
>>>> AffineTransform's transform() methods.
>>>> This in contrast to inverse operations as used in toLayoutSpace(), like
>>>> AffineTransform's createInverse() and inverseTransform() methods that
>>>> may throw this exception.
>>>
>>> It depends on what we consider a forward transform and a reverse
>>> transform :) And don't forget there may be non-affine transforms...
>>
>> Any transform, be it affine or non-affine is able by definition to
>> transform, otherwise it's not a transform :-)
>>
>> Only inverse transform can be problematic. That is, when two different
>> points, after transformation, result in the same point. Valid during
>> transform. But impossible to do an inverse. Analogous to multiplying by
>> zero is valid, but the operation can't be inversed.
>>
>>>
>>>>> 3. getRenderedSpaceShift() - should we traverse containers up to null
>>>>> or up to the nativeContainer?
>>>>
>>>> Good point. For testing I added a test "not nativeContainer" to the 
>>>> null
>>>> test and everything still works (in my own test suite that is far more
>>>> complex than the provided jtreg test cases).
>>>>
>>>> getRenderedSpaceShift() is invoked from eventDispatched. It should do a
>>>> traversal that is analogous to the traversal there.
>>>>
>>>> In general, I'm not sure about the role of "nativeContainer" and how it
>>>> is used. For example, I don't know if (one or more) native containers
>>>> can be present in the hierachy between a Window and the lowest child
>>>> component. Or is the top Window always the native container? If this is
>>>> not the case, could you depict some hierarchy example where a native
>>>> container is a child somewhere in the hierarchy of a JLayer's view? 
>>>> Then
>>>> I can do a more comprehensive test.
>>>
>>> nativeContainer is a basic part of LightweightDispatcher machinery: it
>>> is the container, always heavyweight, which is served by the
>>> dispatcher. An obvious example is how all the mouse events are
>>> dispatched: we (AWT) receive events for heavyweight components only as
>>> the underlying OS isn't aware of our lightweight Swing components.
>>> When the event is dispatched to a heavyweight container, it's
>>> lightweight dispatcher retargets it to a proper lightweight child.
>>>
>>> Given that we won't support transformations for heavyweight components
>>> (BTW, it should also be reflected in JavaDoc), it's enough to care
>>> about nativeContainer children only.
>>
>>
>> 1) I'll add a remark to the new public methods that they only apply to
>> lightweight components.
>> 2) I'll study your remarks and decide if I'll add a check on
>> nativeContainer.
>>
>> I'll let you know if and when a version 3.2 is available.
>>
>> Thanks,
>> Piet
>>
>>>
>>>> Please let me know, so I can prepare a version 3.2 if needed (and add
>>>> the descriptions at the same time)
>>>
>>> 3.2 is only required if you make any significant changes based on the
>>> current discussion.
>>>
>>> Thanks,
>>>
>>> Artem
>>>
>>>> Thanks,
>>>> Piet.
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Artem
>>>>>
>>>>> On 2/12/2010 10:53 AM, Piet Blok wrote:
>>>>>>
>>>>>> 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
>