Refactor DraggableBox / GNodeSkin

[eckig]

Essentially the GNodeSkin consists (mainly) of a DraggableBox so it is about both of them.

I would like to suggest to refactor the DraggableBox to extend Region instead of StackPane. It is not a very good idea to stack multiple layers on top of each other just to achieve some borders. The number of Nodes (JavaFX) in the SceneGraph should be kept as low as possible.

With CSS and the Regions css-property "-fx-background-color" you can stack multiple colors on top of each other to get multiple borders and/or background colors, I stamped together a little example replicating the look of a default JavaFX button for a Region:

SSCE:

public class Main extends Application {
    @Override
    public void start(Stage primaryStage) {
        BorderPane root = new BorderPane();

        Region draggableBox = new Region();
        draggableBox.setMaxSize(150, 90);
        draggableBox.resize(150, 90);
        draggableBox.getStyleClass().add("draggable-box");

        root.setTop(new Button("Test"));
        root.setCenter(draggableBox);

        Scene scene = new Scene(root,400,400);
        scene.getStylesheets().add(getClass().getResource("application.css").toExternalForm());
        primaryStage.setScene(scene);
        primaryStage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }
}

and the CSS for the Region:

.draggable-box {
    -fx-background-color: -fx-outer-border, -fx-inner-border, -fx-body-color;
    -fx-background-insets: 0, 1, 2;
    -fx-background-radius: 3, 3, 2, 1;
    -fx-padding: 4 8 4 8;
}

And another feature request: If you add this (or I may create a pull request), please expose the width and height properties of this Region to the GNodeSkin to make custom layouting easier with bindings.

[rmfisher]

It's currently a StackPane so children can be added. Connectors, for instance. Or any custom content.

Your solution with CSS applied directly to the root node would typically be the best one. The only reason I kept these border and background rectangles is due to the following issue:

https://javafx-jira.kenai.com/browse/RT-381

Here's my question about it on the JavaFX mailing list some time ago:

http://comments.gmane.org/gmane.comp.java.openjdk.openjfx.devel/5456

My workaround was to use 2 rectangles, a 'border' rectangle with a drop shadow and a null fill, and above it a 'background' rectangle with a null stroke and a partially-transparent fill.

These rectangles could probably be moved from the DraggableBox class into the skin classes, if it was particularly important. However you can easily set them both to visible=false and use the CSS styling in the above example on the StackPane.

As for exposing the width and height properties, the GNodeSkin can currently call:

getRoot().widthProperty()
getRoot().heightProperty()

In any case the documentation should be made clearer and I appreciate the input.

[eckig]

Aah okay, did not know that issue. But you can use background colors to achieve a subtle drop shadow:

.draggable-box {
    -fx-background-color: rgba(0,0,0,0.05), -fx-outer-border, -fx-inner-border, -fx-body-color;
    -fx-background-insets: 0 -2 -2 0, 0, 1, 2;
    -fx-background-radius: 3px, 3px, 2px, 1px;
    -fx-padding: 4 8 4 8;
    -fx-opacity: 0.9;
}

If I find the time to experiment further, there might even be a solution for the selection effect into the same background color stack.

I can only recommend you to refactor this, not force you to do so. But here is another argument: Today I fiddled around with the GNodeSkin and found it very hard to correctly size the node because of the way you intertwined the width properties of the parent stackpane with the child rectangles. It would be much easier with only one element..

But thank you anyway for your insights and for this graph API!

[rmfisher]

Ok, I will see if I can move these 2 rectangles outside the DraggableBox class. They should be part of the skin, you should not be forced to use them if you don't want to.

Just out of interest, what exactly are you trying to set the size of? Currently the skin should set the DraggableBox's size when intialized via:

getRoot().getBorderRectangle().setWidth(node.getWidth())
getRoot().getBorderRectangle().setHeight(node.getHeight())

And after that the nodes are sized manually by the user.

[eckig]

I explored the samples and created my own skin based on them. Then I set a larger minimum size to display all elements. Now there is some issue with the node skin when you set a minimum size larger than the current size (initial state). My best guess currently is that this is caused by binding the StackPanes width to the border rectangles width.

And that is part of the problem. Giving the user access to a full JavaFX Node (getRoot()) but know here comes the API and says "No no, don't touch this" encouraging another way of doing things. No offense meant, it is just my personal opinion - It looks just a bit overcomplicated to manage all these width properties (Parent + 2 Rectangles) when there could be an easier way (let JavaFX manage this for you) ;-)

[rmfisher]

Yes, it's a good point. I will try to simplify the API there. Thanks again for the feedback :)

[eckig]

Any progress? - No pressure :-) Otherwise I could refactor the classes and provide a pull request?

[rmfisher]

Unfortunately I won't be able to make further changes or accept pull requests in the next few weeks, as we are still clarifying what our exact process for accepting pull requests will be.

In the meantime I think you can simply set these rectangles to visible=false if you don't want to deal with them. Of course this is not a good long term solution and I will definitely try to refactor as soon as I can.

[eckig]

That is sad to hear.

In that case I will create another fork and will do it on my own..

[rmfisher]

If you do this please let me know because I tried it briefly and did not have any immediate success. I would be interested to see how you did it.

Again, sorry about the slow response. I'm a bit swamped right now and not able to dedicate as much time to this as I would like.

[eckig]

It was fairly easy to refactor the draggable Box: https://github.com/eckig/graph-editor/commit/2a966e06e5fb091fc1c406a4e98a28c5226b89e9

There are two things that need to be done:

1. Change the API docs on the affected classes to correctly reflect the changes made.
2. The custom demo skins have only been corrected to be able to compile. The css files need to be changed, too.

[eckig]

Okay, clean up is mostly done: https://github.com/eckig/graph-editor/commit/4fa11657ac9f3aeafc733f2973558bb4cf8546e9

More Commits added: https://github.com/eckig/graph-editor/commits/DraggableBox

[rmfisher]

Incorporated into the main branch. Thanks.