fix(Group, LayoutManager) Correctly dispose LayoutManager when disposing the Group

[jiayihu]

If I'm not wrong, it seems that LayoutManager.dispose is never called actually. Disposing of the layout manager completely is safer than unsubscribing the Group objects:

1. There may be other target subscriptions left behind due to other bugs
2. dispose in an extended LayoutManager class takes care of disposing other things

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[asturur]

let me see what dispose does because i do not know.

[asturur]

i ll try to write it here as sort of loud thinking.

In the case of groups i do not see any difference.
Is it clear that the dispose() approch is statefull ( kill every disposer that we currently have ) while the current approach is more optimistic ( hopefully we have disposers only for current object of the groups and we didn't mess up during the group lifecycle ).

For activeSelections it may be a little bit different ( description to follow )

[jiayihu]

You're right, I didn't notice ActiveSelectionLayoutManager has its peculiar subscriptions. I tried to do this:

dispose(): void {
    this.unsubscribeTargets({
      targets: this.getObjects(),
      target: this,
    });

    super.dispose()
}

But I'm missing the ActiveSelection reference from the LayoutManager dispose method. This is something I've noticed working with the LayoutManager.

[asturur]

Yes but we could have left a bug here or there.

if the current group code is better than dispose, it should become the dispose code.

[asturur]

I m not sure this is worth anyone time if you don't have a bug in your hands.
I think we want to move away from the subscription model inside the group for something that will register the necessary events on selection of the obejcts and not all the time. So the lifecycle of the events handler will be different.
For now i would say if is not broken let's not touch it

[jiayihu]

I m not sure this is worth anyone time if you don't have a bug in your hands.

I have a LayoutManager subclass that has listeners to dispose of, that's why I'd like dispose to be called. And I think it should be called. Currently it's not called anywhere, it's clearly wrong.

But I can call dispose myself, no problem.

[jiayihu]

This would fix the problem without regressions:

// Group.ts

dispose() {
  this.layoutManager.unsubscribeTargets({
      targets: this.getObjects(),
      target: this,
  });
  this.layoutManager.dispose()
}

What do you think?

[asturur]

I m not sure this is worth anyone time if you don't have a bug in your hands.

I have a LayoutManager subclass that has listeners to dispose of, that's why I'd like dispose to be called. And I think it should be called. Currently it's not called anywhere, it's clearly wrong.

But I can call dispose myself, no problem.

i m trying to understand why the

  this.layoutManager.unsubscribeTargets({
      targets: this.getObjects(),
      target: this,
  });

is not calling the disposal part for you.

[jiayihu]

What do you mean? It's not calling .dispose isn't it? So I can't do something like:

export class MyLayoutManager {
  constructor(strategy) {
    super(strategy)
   
   this.disposers = [
     canvas.on("something", () => { ... })
   ]
  }

  dispose() {
    this.disposers = []
  }
}

So my issue is being with layoutManager.dispose being never called and the disposal being done only with unsubscribeTargets.

[asturur]

it seems to me that when we call unsubscribe targets with all the objects as targets, we effectively wipe away all the disposers. don't we?

[jiayihu]

How do you clear the custom disposers?

[asturur]

i do not know i think your change is fine, i m just trying to catch up with this usage.
If all the listeners are added with subscribeTargets i was expecting unsubscribeTargets to clean up.
That's all

I'm trying to find out why that is not the case.

[asturur]

so please add the part you deleted from the group inside the layout manager dispose method. try to wrap your case in a jest test if possible, try to represent your use case if possible rather than a generic function test, and write inside dispose why we do both unsubscribe and an extra run on the disposers

[jiayihu]

Done, I've added a test as well but didn't do my use case. My use case was basically this: when dragging an object over a Group, I want the Group to resize automatically to fit including the dragged object. So I need to add an extra listener mousemove to the Group.
Adding a test that reproduces that seemee too much, even just doing the relayouting thing because it does manually several calculations as LayoutStrategy#calcBoundingBox. I hope the simple test is enough.

[asturur]

Yes also it would have required a full e2e test and since is not a functionality supported out of the box we can't really test for that.
But it would make a nice example or demo

[ShaMan123]

For the mentioned use case: why not calling triggerLayout? I might be missing out on something.
I don't think we should merge this, see comment

[ShaMan123]

My only thought about this is that the layout manager isn't bound to a single group so if an app uses a single layout manager for multiple groups this will break it so this PR forces a unique layout manager for each group but that is not strictly necessary, I designed it to be decoupled.

[jiayihu]

I never knew about sharing LayoutManager. How would one dispose it then?

[jiayihu]

If you mean to leave disposing to the app-level, then dispose in LayoutManager should be noop and have a comment explaining that the developer should do it accordingly to their needs, e.g. whether to dispose it on group disposal or canvas disposal.

[asturur]

My only thought about this is that the layout manager isn't bound to a single group so if an app uses a single layout manager for multiple groups this will break it so this PR forces a unique layout manager for each group but that is not strictly necessary, I designed it to be decoupled.

The sharing part was one of the contested parts of the design.
The group didn't have the layout manager, it had a bunch of methods to do the work of the layoutManager.
I asked if we could put those methods in a scoped box because with all the different ideas of layouts and functionality they were growing out of hand, and it was supposed to be a way to reduce clutter in the group. Period. The sharing between group is something that was added while you implemented the layoutManager but the usefulness of being shared isn't really clarified. It adds a lot of constrain to how this code should behave but what does it adds?
Indeed one of the first changes was to make every group generate its own layoutManager.

Can you make a case for the sharing part?

[ShaMan123]

This is the same as controls.
I did it like this because I know that is important for you.

class Group {
  static ownDefaults = {
    layoutManager: new LayoutManager() // shared across all instances
  }
}

[ShaMan123]

Indeed one of the first changes was to make every group generate its own layoutManager.

I surfaced this when that happened IIRC

[asturur]

It is not important for me, and i think it adds complexity both to signatures and usage.
The layout manager is just the group logic tucked away in a class. I don't see any need for sharing it across instances.

[jiayihu]

For the mentioned use case: why not calling triggerLayout? I might be missing out on something.

That's not the point, it's just an example. Think about the fact that you have a dispose method on the LayoutManager but it never gets called. Isn't that an issue? Whatever use case you might have for adding additional listeners in the LayoutManager, there is no way to dispose them.