Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12d0773
Correctly dispose layoutManager
jiayihu Apr 4, 2024
342694b
Update changelog
jiayihu Apr 4, 2024
5351cc8
Merge branch 'master' into fix/dispose-layout-manager
asturur Apr 9, 2024
79b3484
Call unsubscribeTargets and add test
jiayihu Apr 14, 2024
195e0ba
Add comment
asturur Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- fix(Group): Correctly dispose layoutManager [#9787](https://github.com/fabricjs/fabric.js/pull/9787)
- fix(Canvas): Fix searchPossibleTargets for non-interactive nested targets [#9762](https://github.com/fabricjs/fabric.js/pull/9762)
- test(): Rename svg tests [#9775](https://github.com/fabricjs/fabric.js/pull/9775)
- refactor(): `_findTargetCorner` is now called `findControl` and returns the key and the control and the coordinates [#9668](https://github.com/fabricjs/fabric.js/pull/9668)
Expand Down
28 changes: 28 additions & 0 deletions src/shapes/Group.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,34 @@ describe('Group', () => {
expect(group.height).toBe(100);
});

test('disposes the LayoutManager subscriptions', () => {
class CustomLayoutManager extends LayoutManager {
private disposers: VoidFunction[] = [];

attachListener(subscription: VoidFunction) {
this.disposers.push(subscription);
}

dispose(): void {
this.disposers.forEach((d) => d());
this.disposers = [];

super.dispose();
}
}

const objs = [new FabricObject(), new FabricObject()];
const layoutManager = new CustomLayoutManager(new FitContentLayout());
const group = new Group(objs, { layoutManager });

const subscription = jest.fn();
layoutManager.attachListener(subscription);

group.dispose();

expect(subscription).toHaveBeenCalled();
});

describe('With fit-content layout manager', () => {
test('will serialize correctly without default values', async () => {
const { group } = makeGenericGroup({
Expand Down
3 changes: 3 additions & 0 deletions src/shapes/Group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,9 @@ export class Group
targets: this.getObjects(),
target: this,
});
// dispose additional listeners that may have been
// added by the developer on the layoutManager #9787
this.layoutManager.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I surfaced this when that happened IIRC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this._activeObjects = [];
this.forEachObject((object) => {
this._watchObject(false, object);
Expand Down
Loading