Re: Fw:reviewrequestfor6899434:Addaffinetransformsupport to JLayer

[Alexander Potochkin]

Hi Alex, Anthony and others,

Hello Piet

Aha, I misinterpreted the request to only an awt change.

I'll start working on an uptaed implementation on the swing side
(RepaintManager, JPopupMenu, ToolTipManager and JComponent), only using
toLayoutSpace and toRendereredSpace in Container.
Then I'll also be able to create a simple test with some custom
transforming component.

This will take some more time :-)

Sure

Thanks much!
alexp

Thanks Piet

Hello Piet

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/

The main idea is to enable users to easily use transforms in custom
Swing components, so we need all the stuff for the RM, ToolTipManager
etc..

just JLayer is to be excluded

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

It usually takes a couple of days to show up in the database

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.

Is it possible to implement this computation right now?

I am afraid we also need at least one simple test
with a custom Swing component with overridden
toLayoutSpace()/toRenderedSpace()

Thanks much and sorry for the confusion!

alexp

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