Unfortunately it is not that easy since there many interoperability problem. For example: borders has to be painted as
rotated. isHitA, convertSceneToLocal and other methods would have to count with rotation... Basically generic "rotate"
support for Widget is very hard to implement.

Having "RotatableWidget" class to be implemented by developer would solve lots of architectural problems but still you
would have to use "angle" and "rotationOriginPoint" properties for each RotatableWidget.

Then there is one more issue. Any Widget can have child Widgets, Unfortunately they would not be rendered "rotated" with
the parent widget rotation. Also the will not get layed out rotated...

For the issues above I am setting "target milestone" to future. Anyway feel free to contribute a patch for it and we may
include it to the library since there is support for "rotation" at all.

Ok, in future I'll try to submit some patches (if I'm able to do solve the issues).

I'd use this issue at the moment for brainstorming. What about forcing a "RotatableWidget" to use double-buffering for painting? I mean:

1. create a buffered Graphics2D
2. paint everything (borders, children, etc...) in the buffered Graphics, non rotated
3. get the bitmap from the Graphics2D, rotate and paint it

Actually painting is not a problem since you have Widget.paintBorder, Widget.paintBackground, Widget.paintWidget and
Widget.paintChildren. Therefore can put a code there for temporarily rotate Graphics2D during painting.
The problem is that isHitAt on the RotatableWidget and its children would not count with rotation of the
RotatableWidget. Also Layout implementation would not count with rotating since they are using
Widget.getPreferredLocation and Widget.getPreferredBounds which are return unrotated values.
P.S.: We should change "RotatableWidget" class name to sometime nicer.

Created attachment 122943 [details]
Test case main class

Draw a Shape.
Double click make it rotate.

Created attachment 122944 [details]
First widget class for the test case

Created attachment 122945 [details]
Second widget class for the test case

Created attachment 122946 [details]
Modified widget class

Modified widget class implementing Affine Transform for a widget.
Graphics gets a transformation applied before painting.

IsHitAt method was modified to take into account the affine transformation.

I forgot to mention
I have a big issue with clipping.
widget keep being clipped with its original boundaries unless i full revalidation occurs(When I drag the window outside the screen and back inside everything is OK

Created attachment 123838 [details]
Latest patch provided by bcallebaut

Created attachment 123898 [details]
Patch

Updated Patch

 - Field lookup made private.
 - Tried to solve the issue with clipping (see method Scene.getRealBounds).

Please check the changes. Thanks.

Created attachment 123901 [details]
Patch - Examples in module visual.examples

Patch with examples provided by bcallebaut (for module visual.examples).

Note: To use this patch, please clone repository main/contrib:
cd <path-to-your-repo> (e.g. /home/user/netbeans/core-main)
hg clone http://hg.netbeans.org/main/contrib/

I've modified the test case by adding more widgets and move actions.
Rotated widgets seems to work fine. But the scene is not rendered (or cleared) correctly if the widgets are moved quickly. If this is caused by the new changes, it should be solved.

Thanks to Benoit for provided test classes.

I think that we can now return to the original purpose of this enhancement.

(In reply to comment #0)
> I'm proposing to provide something such as ActionFactory.createRotateAction() -
> when enabled, the user will be able to rotate the widget as well as move 
> and resize it.
This is probably the main feature we should provide. The Widget improvements made it probably quite possible.

> I think that it's acceptable that not all the existing Widgets (e.g.
> LabelWidget, ComponentWidget) support this new feature, while others (e.g.
> ImageWidget) 
> should. Probably the best way to take advantage of the new feature would be by
> subclassing Widget and let the programmer do the work.
Here we should consider whether adding (public) AffineTransform to all Widget objects is necessary.
Maybe creating a subclass for this purpose would be safer.
Or providing method  getRotation() instead of getTransform() would be sufficient.

Benoit, what is your use case for rotating widgets? Are you going to use something like createRotateAction?

(In reply to comment #12)
> I think that we can now return to the original purpose of this enhancement.
> 
> (In reply to comment #0)
> > I'm proposing to provide something such as ActionFactory.createRotateAction() -
> > when enabled, the user will be able to rotate the widget as well as move 
> > and resize it.
> This is probably the main feature we should provide. The Widget improvements
> made it probably quite possible.

Agree. I have it in my test case.
I can add it to the ActionFactory and propose a patch.
> 
> > I think that it's acceptable that not all the existing Widgets (e.g.
> > LabelWidget, ComponentWidget) support this new feature, while others (e.g.
> > ImageWidget) 
> > should. Probably the best way to take advantage of the new feature would be by
> > subclassing Widget and let the programmer do the work.
> Here we should consider whether adding (public) AffineTransform to all Widget
> objects is necessary.
> Maybe creating a subclass for this purpose would be safer.
> Or providing method  getRotation() instead of getTransform() would be
> sufficient.

For ComponentWidget it makes sense. Not sure I agree on that for LabelWidget. 
Since my idea is to provide Visio like features missing in the Visual Library and since the Text widget in Visio can be rotated, I would make the LabelWidget rotatable. It also remove the need to have a 90° orientation property in the LabelWidget

> 
> Benoit, what is your use case for rotating widgets? Are you going to use
> something like createRotateAction?

My use case is to provide user actions to rotate/mirror a widget.

I am going to provide actions like RotateAction, MirrorVerticallyAction and MirrorHorizontallyAction.
So, getTransform() is better in this case.
My intend is to provide Visio like functionalities to the visual library that are currently missing (not all).

> > should. Probably the best way to take advantage of the new feature would be by
> > subclassing Widget and let the programmer do the work.
> Here we should consider whether adding (public) AffineTransform to all Widget
> objects is necessary.
> Maybe creating a subclass for this purpose would be safer.
> Or providing method  getRotation() instead of getTransform() would be
> sufficient.

I was thinking on using the lookup to provide this kind of feature. But it still mean that we need to add code to provide this feature in the main paint() method so in the Widget class.  Rotating a widget means that its shape changes for clipping and is not rectangular anymore and that the shape has to be transformed (rotated) after being translated in the paint code.

But it doesn't mean for me that the getTransform() method should be made public. Being protected is fine for me. Widget could even delegate that functionality to a final private object that is optionally put into its lookup making the rotate feature an option that can be enabled/disabled at runtime.
The widget subclass would have the option to add it to its lookup or not. Like that it is fully transparent to existing code and doesn't add extra API/interfaces to the Widget class.

By the way, I don't see any benefit for the Scene and LayerWidget classes to be rotatable/mirrorable

(In reply to comment #11)
> Created attachment 123901 [details]
> Patch - Examples in module visual.examples
> 
> Patch with examples provided by bcallebaut (for module visual.examples).
> 
> Note: To use this patch, please clone repository main/contrib:
> cd <path-to-your-repo> (e.g. /home/user/netbeans/core-main)
> hg clone http://hg.netbeans.org/main/contrib/
> 
> I've modified the test case by adding more widgets and move actions.
> Rotated widgets seems to work fine. But the scene is not rendered (or cleared)
> correctly if the widgets are moved quickly. If this is caused by the new
> changes, it should be solved.

There is an issue with the redraw optimization in the library. 
The drawing component clips the area to the boundaries of the Widget shape without transformation (assuming that this is a rectangle in Canvas coordinates).
I disabled that optimization as a workaround. 
This issue has to be solved but I don't have a deep enough understanding of the drawing process to provide a good enough solution.

> 
> Thanks to Benoit for provided test classes.

The patch and your opinions mentioned here should be reviewed and discussed.
I'm opening an API review for this issue.

Please, if you are interested in Visual Library, read about proposed changes and add your comments.

(In reply to comment #13)
> > Maybe creating a subclass for this purpose would be safer.
> > Or providing method  getRotation() instead of getTransform() would be
> > sufficient.
>
> For ComponentWidget it makes sense. Not sure I agree on that for LabelWidget.
> Since my idea is to provide Visio like features missing in the Visual 
> Library and since the Text widget in Visio can be rotated, I would make
> the LabelWidget rotatable. It also remove the need to have a 90° 
> orientation property in the LabelWidget.
I think that we will need to keep this property for compatibility reasons.

> I am going to provide actions like RotateAction, MirrorVerticallyAction and
> MirrorHorizontallyAction. So, getTransform() is better in this case.
Sure, I agree.

(In reply to comment #15)
> There is an issue with the redraw optimization in the library. 
> The drawing component clips the area to the boundaries of the Widget shape
> without transformation (assuming that this is a rectangle in Canvas
> coordinates).
> I disabled that optimization as a workaround. 
> This issue has to be solved but I don't have a deep enough understanding
> of the drawing process to provide a good enough solution.
Which optimization do you mean? How did you disable it?
Thank you.

> (In reply to comment #15)
> > There is an issue with the redraw optimization in the library. 
> > The drawing component clips the area to the boundaries of the Widget shape
> > without transformation (assuming that this is a rectangle in Canvas
> > coordinates).
> > I disabled that optimization as a workaround. 
> > This issue has to be solved but I don't have a deep enough understanding
> > of the drawing process to provide a good enough solution.
> Which optimization do you mean? How did you disable it?
> Thank you.

In Scene.java line 426 (method resolveRepaints ())
I replaced
  component.repaint (r);
by
  component.repaint (this.getBounds ());

This redraw only the area occupied by the widget.
The issue is that this area is supposed to be a rectangle which is not the case anymore with a rotated widget since the rectangle is rotated.

What I did is to force the Scene to be totally repainted. It obviously slow down repainting.

From my point of view, In the review, you should also discuss clarification of how painting and clipping are done in the Visual Library. This has a huge impact on non rectangular widgets and widgets that overflow their rectangular area (Visio allows that without trouble)

For reference original code lines 421 to 429
 // NOTE - maybe improves performance when component.repaint will be called for all widgets/rectangles separately
        if (repaintRegion != null) {
            Rectangle r = convertSceneToView (repaintRegion);
            r.grow (1, 1);
            if (component != null)
                component.repaint (r);
            repaintSatellite ();
            repaintRegion = null;
        }

Y01 ChainProvider has no useful javadoc.

Y02 A pattern when Lookup is passed into a constructor and it seeks for well known instances in there is more a mispattern. Why the constructor does not accept a Border, Layout and WidgetAction.ChainProvider to begin with? The Lookup is more appropriate when you want to "tunnel" an unknown API throw Widget, which is not the case here as far as I can tell.

Created attachment 125452 [details]
Modified Patch to Widget action. Added javadocs

The ChainProvider inner class implements the Provider pattern for the Chain class.

The idea is to let a Widget loads its action chains from a configuration object before use (at construction time or validation time).

> Y02 A pattern when Lookup is passed into a constructor and it seeks for well
> known instances in there is more a mispattern. Why the constructor does not
> accept a Border, Layout and WidgetAction.ChainProvider to begin with? The
> Lookup is more appropriate when you want to "tunnel" an unknown API throw
> Widget, which is not the case here as far as I can tell.

Let me explain why I did it this way.
* I didn't want to add another constructor in the Widget base class. The constructor using a Lookup is already present, so it is easy to reuse it to pass an unknown set of instances to the Widget. I agree this is not API tunneling

* I wanted to have a way to "configure" a widget (standard border,actions....) in an easy way without having to add after construction many lines of code. In that  scope I use the Lookup object as a kind of Map<Class,Object> with more than 1 value per key that can be shared by multiple widget classes. I could have defined a "WidgetConfiguration" class for that purpose but the Lookup is working fine. I have a WidgetFactory pattern implementation that can return a configured Widget with its own model, actions and border. The standard actions and border are passed through the WidgetFactory to the Widget.

* I would like to promote compositing over subclassing when addin features to a Widget.  ObjectScene an GraphPinScene are working very well but put constraint on how you develop your code. How do you do when part of your graph is Graph oriented while some other parts are not. A use case could be to have a big widget in a Directed graph representing a GUI panel with sub widgets that you can add/remove/interact with. Giving the option to compose behaviours using a Lookup mechanism is a clean way of doing it.


To summarize,
* I don't want to add constructor or other configuration methods to the base Widget class to avoid breaking existing code (API change)
* I have use case where the Lookup object is tunneled (WidgetFactory).
* I want to provide functionalities to widgets dynamically based on the context of their use, so without subclassing.
* I need a Cookie like functionality for that and Lookup is working fine like that.

Created attachment 126441 [details]
Patch

Two patches merged and updated for current sources.

> * I didn't want to add another constructor in the Widget base class. The
> constructor using a Lookup is already present, so it is easy to reuse it to
> pass an unknown set of instances to the Widget. I agree this is not API
> tunneling
Why not? It seems to be better if the constructor signature describes what objects are expected.
Passing border and layout is probably beneficial, because instantiation of possibly unused EmptyBorder and AbsoluteLayout can be skipped, and Lookup can be useful for putting custom objects ("cookies") to the widget.
But I'm not sure that passing ChainProviders to the constructor is really needed. Isn't using createActions() and getActions().addAction() sufficient?

> * I wanted to have a way to "configure" a widget (standard border,actions....)
> in an easy way without having to add after construction many lines of code. In
> that  scope I use the Lookup object as a kind of Map<Class,Object> with more
> than 1 value per key that can be shared by multiple widget classes.
You can create a helper method that sets the widget as you wish. It may be simpler and faster than creating a Lookup and then reading it during construction of each object.

> To summarize,
> * I don't want to add constructor or other configuration methods to the base
>   Widget class to avoid breaking existing code (API change)
Something like this should be safe:

    public Widget (Scene scene, Lookup lookup, Layout layout, Border border) {
        // ...
    }

    public Widget(Scene scene) {
        this(scene, Lookup.EMPTY, LayoutFactory.createAbsoluteLayout(),
                BorderFactory.createEmptyBorder());
    }

> * I have use case where the Lookup object is tunneled (WidgetFactory).
You can have a method (in your widget factory) that initializes newly created widget, e.g. sets actions. Is this solution not suitable for you?

    public static void initWidget(Widget w) {
        Chain tool1Actions = w.createActions("tool1");
        tool1Actions.addAction(SomeWidgetAction.create());
        // ...
    }

P.S. I'm sorry for the delay in answering.

(In reply to comment #20)
> * I would like to promote compositing over subclassing when addin features to a
> Widget.  

I like composition, but in such case consider creating a builder. It is the appropriate pattern for composing multiple features and then obtaining the instance.

(In reply to comment #22)
> > * I didn't want to add another constructor in the Widget base class. The
> > constructor using a Lookup is already present, so it is easy to reuse it to
> > pass an unknown set of instances to the Widget. I agree this is not API
> > tunneling
> Why not? It seems to be better if the constructor signature describes what
> objects are expected.
> Passing border and layout is probably beneficial, because instantiation of
> possibly unused EmptyBorder and AbsoluteLayout can be skipped, and Lookup can
> be useful for putting custom objects ("cookies") to the widget.
> But I'm not sure that passing ChainProviders to the constructor is really
> needed. Isn't using createActions() and getActions().addAction() sufficient?
> 


> > * I wanted to have a way to "configure" a widget (standard border,actions....)
> > in an easy way without having to add after construction many lines of code. In
> > that  scope I use the Lookup object as a kind of Map<Class,Object> with more
> > than 1 value per key that can be shared by multiple widget classes.
> You can create a helper method that sets the widget as you wish. It may be
> simpler and faster than creating a Lookup and then reading it during
> construction of each object.
> 

> Something like this should be safe:
For me it is good. I just don't want to break existing compiled code.


> > * I have use case where the Lookup object is tunneled (WidgetFactory).
> You can have a method (in your widget factory) that initializes newly created
> widget, e.g. sets actions. Is this solution not suitable for you?

A widget factory is in my plan. I don't know yet how to fit it in the general API. I still have to think it and try.

Will anyone continue driving the review to its integration?