Rethink CanvasItem class and Transform Panel bindings

[Alecaddd]

This is more a of a general issue to discuss basic architecture and future improvements to a substantial area of the application.

The CanvasItem problem

We currently have in place a base abstract class called CanvasItem which we use to build all the different items on top of it (Rectangle, Ellipse, Image, etc.).
The problems for this approach are starting to show as we implement more and more features and we make the interactions and transformations with the Canvas more complex.
These are few highlights of what we stumbled upon so far:

• Not all the attributes from the base class are necessary/used in the child classes, but we're forced to declare anyway.
• Some important attributes (eg. X and Y) are not directly accessible from the base object, so we need to typecast the proper class or create helper methods.
• Unique classes like Images and Artboards, have substantially different needs and don't leverage most of the shared methods coming from the base object.

How can we improve this?

The Transform Panel problem

The Transform panel allows to edit item's attributes (x, y, width, height, etc) through input fields.
These attributes can be also modified directly on the canvas, by moving and resizing the selected items.
These fields need a bidirectional binding with the specific attribute (eg. the X input is binded to the item's X attribute), so when one changes, the other follows accordingly, and proper signals are emitted to allow the UI to react accordingly (eg. the selection bounds follow, the artboards get updated, etc.).

Since items can be manipulated directly from the Canvas, we need to deactivate some of these signals and bindings to not fall into an infinite loop of, the item gets updated, the input field receives the signal which updates the value, the input field emits the signal because the value changed, the item receives the signal to updated the value, the item emits the signal with the new value, the input receives the signal... and it goes on and on and on.

We solved this issue by implementing some conditions and by unbinding properties at the right time, but this solution causes some problems:

• Whenever an item gets moved, we check if it falls inside or outside an artboard to change hierarchy. We do this when the mouse button gets released. Doing this on the X and Y binding in the Transform Panel, causes that condition to run for every moved pixel. Pretty intense in terms of resources.
• For Image, we rescale the pixbuf after an image gets scaled down to lower the quality and speed up the canvas. We do this when the mouse button gets released. Doing this on the WIDTH and HEIGHT bindings in the Transform Panel, causes that condition to run for every changed pixel. Pretty intense in terms of resources.

These wouldn't be critical issues if we assume the user mainly interact with the canvas and changes those values sporadically, maybe a few pixels at a time, but we can't assume that.
Sometimes the user might focus on the X input and hold down the arrow up key, causing hundreds of triggers consecutively.

How can we improve this?

[jtrees]

The Transform Panel problem

I think the problems you describe here are all the result of the one thing:

"Two-way data binding is the Devil."

Ember.js used to rely on two-way data binding until they figured this out and eventually replaced it with what they call "Data Down, Actions Up" [1] [2].

Effectively, this means having a data binding that goes in one direction ("down") and sending messages ("actions") in the other direction ("up") to trigger data updates.

DDAU Example

Consider a setup with the following components:

        _---App---_
       /           \
      |             |
TransformPanel    Canvas
                    |
                    |
                  Shape

The App component has a bunch of data (the global app state) and passes some of this data on to the TransformPanel and Canvas components. This data is bound in one direction:

So, whenever the data is updated in App, that change is immediately propagated down to TransformPanel and Canvas. However, the latter components can mutate the data as they wish, it will not affect App. They effectively have their own copy of the data.

Conversely, if a change in a lower component should affect a component further up, an action (basically an event, optionally with data) can be sent upwards. Upon receiving this action, the relevant component can decide to update any data, triggering a flow back down.

For example, if the x coordinate of a shape is changed in TranformPanel, the following would take place:

1. TransformPanel sends an action up to App (something like x_changed(new_value: 42))
2. App receives the action and updates the x coordinate of the relevant shape in the global state.
3. The one-way data binding sends the updated x coordinate down to Canvas.
4. The one-way data binding of Canvas sends the received x coordinate down to Shape.
5. Shape redraws itself to match the new state (data) it has.

Benefits

This approach has a number of advantages:

1. Clearer separation of concerns
2. Single source of truth
3. No data loops and undefined states
4. Better testability
5. Easier to reason about
6. Easier to track state changes

Whereas a two-way binding between TransformPanel and Shape can lead to conflicts (e.g. What if for some reason x is 1 in TransformPanel and x is 2 in Shape? Who wins?), DDUP forces you to put shared state in a single "higher" location so at every point in time the application state is unambiguous.

[1] https://dockyard.com/blog/2015/10/14/best-practices-data-down-actions-up
[2] https://discuss.emberjs.com/t/rationale-for-data-down-actions-up/13830/2

[Alecaddd]

After only 273 days, I'm starting to work on this.
I am fast >_>

[donadigo]

Here we go:

The canvas problem:
It seems like this is an issue of inheritance vs composition. While inheritance is really nice for simple implementations, it starts to fall short when it comes to complex systems like here. The more objects you implement, the more you have to fight with classes that you inherit, trying to find similarities and then somehow adapting them to fit each other which creates a mess. Which is why e.g many game engines like Unity or Unreal Engine prefer not to do that and go the composition way. Instead of inheriting a base object, they define it as just a container of "components". Each component can do something different with the parent base. I think this would be nicest to illustrate with an example:

public class Component : Object {
    public unowned CanvasItem item { get; construct set; }
}

// Implements transform for a CanvasItem
public class Transform : Component {
    public double x { get; set; }
    public double y { get; set; }

    public Transform (CanvasItem item) {
        this.item = item;
    }

    // ...
}

// Implements rectangle for a CanvasItem
public class RectangleRenderer : Component {
    public double width { get; set; }
    public double height { get; set; }

    public RectangleRenderer (CanvasItem item) {
        this.item = item;
    }

    public void render () {
        var transform = get_component<Transform> (typeof (Transform));
        // render rectangle at (transform.x, transform.y)
    }
    // ...
}

public class CanvasItem : Goo.CanvasItemSimple, Goo.CanvasItem {
    public Gee.ArrayList<Component> components { get; construct; }

    construct {
        components = new Gee.ArrayList<Component> ();
        components.add (new Transform (this));
    }

    public T? get_component<T> (T type) {
        foreach (unowned Component comp in components) {
            if (comp.get_type () == type) {
                return comp;
            }
        }

        return null;
    }
}

As you can see here, the CanvasItem now only holds components that make it up which allows for cleaner implementations of all things. If a component requires access to another, it simply calls get_component on the parent item to retrieve it. Whether or not kind of system is possible to implement with Goo, I don't really know.

[Alecaddd]

@donadigo would you be willing to create a quick PR to test this approach?
Something really simple, like a component that handles the X & Y coordinates?
I don't know how doable this could be, but I really like it and I'd like a have a blueprint to start building on it, just to avoid messing it up 😅

[Alecaddd]

I started working on this and holy cow how easy is to create items by relying on components.
I'll ping you once the PR is ready so you can see if I messed something up

[donadigo]

Appreciate it and will definitely comment. Sorry that I haven't responded before, quite busy with my studies right now.

[be1]

@jtrees «Data Down, Actions Up» is to me a simple and safe way of property binding propagation. Thanks for sharing.

[12people]

Just a note about object composition – it might be helpful to get inspired by the Sketch file format schema, which gives you an idea of how Sketch composes items: https://github.com/sketch-hq/sketch-file-format

[Alecaddd]

@jtrees thank you so much for the detailed explanation.
I was looking at the "Data Down, Actions Up" a while ago, but then I decided to go with the 2 way bindings because it made the code leaner. But indeed, now that approach doesn't really work and doesn't scale well for our situation.
I'll create a branch to refactor the transform panel with this approach. Would it be okay for you if I ping you on that upcoming PR so we can continue the conversation, and maybe you could take a quick look at the code to see if I'm not messing anything up?

@donadigo ah, that's exactly what I was looking for, thank.
I think this is extremely doable as Goo.CanvasItems can be easily extended and need very few arguments to be properly initialized. I suspect it won't really matter how we handle the various components as long as we then apply the transformed attributes to the base canvasitem object.
I'll start working on this probably right after the first alpha release, as it doesn't affect the users but it's only an architecture refactor.

Thank you all for the suggestions and insights, I love Open Source!

[jtrees]

Would it be okay for you if I ping you on that upcoming PR so we can continue the conversation, and maybe you could take a quick look at the code to see if I'm not messing anything up?

Sure!

[Alecaddd]

Just a heads up to the folks participating in this discussion.

The "Data Down, Action Up" rework has just landed on Master and holy smokes it makes a huge difference.
No more coordinates conflicts, and a single source of truth (the Coordinates State Manager) to handle the position transformations.

The transition from inheritance to composition is under way and I should be able to create a WIP PR in a couple of weeks.
Since all our items are tied to every single aspect of the application, it wasn't possible to create small PRs without destroying everything, so I decided to embark on a single massive rewrite.
This will be longer and harder, but at least we can immediately test everything at once, rather than shipping something half broken while we keep working.

Stay tuned for more awesome stuff!

[Alecaddd]

All of things listed above have been done 🎉

✔️ CanvasItem uses components and we dropped inheritance.
✔️ A new CoordinatesManager has been implemented, which takes care of handling items' coordinates and sending signals to the rest of the application to update everything.

Thank you all for participating and for your invaluable insights!

I'll keep this discussion open just in case something more comes up.