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.

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

feat(LayoutManager): Expose objects registration #9661

Merged
merged 7 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

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

## [next]

- [draft]: regisrtation on demand [#9661](https://github.com/fabricjs/fabric.js/pull/9661)
- fix(Object): support specyfing toCanvasElement canvas [#9652](https://github.com/fabricjs/fabric.js/pull/9652)
- ci(): no `src` imports [#9657](https://github.com/fabricjs/fabric.js/pull/9657)
- fix(textStyles): Split text into graphemes correctly [#9646](https://github.com/fabricjs/fabric.js/pull/9646)
Expand Down
168 changes: 126 additions & 42 deletions src/LayoutManager/LayoutManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { Point } from '../Point';
import { StaticCanvas } from '../canvas/StaticCanvas';
import { Group } from '../shapes/Group';
import { FabricObject } from '../shapes/Object/FabricObject';
import { Rect } from '../shapes/Rect';
import { LayoutManager } from './LayoutManager';
import { ClipPathLayout } from './LayoutStrategies/ClipPathLayout';
import { FitContentLayout } from './LayoutStrategies/FitContentLayout';
import { FixedLayout } from './LayoutStrategies/FixedLayout';
import {
Expand All @@ -12,12 +14,7 @@ import {
LAYOUT_TYPE_INITIALIZATION,
LAYOUT_TYPE_REMOVED,
} from './constants';
import type {
LayoutContext,
LayoutResult,
LayoutTrigger,
StrictLayoutContext,
} from './types';
import type { LayoutContext, LayoutResult, StrictLayoutContext } from './types';

describe('Layout Manager', () => {
it('should set fit content strategy by default', () => {
Expand Down Expand Up @@ -158,7 +155,7 @@ describe('Layout Manager', () => {
expect(performLayout.mock.calls).toMatchObject([
[
{
e: { target: object, ...event },
e: { ...event },
target,
trigger: 'modified',
type: 'object_modified',
Expand All @@ -182,39 +179,24 @@ describe('Layout Manager', () => {
expect(performLayout).not.toHaveBeenCalled();
});
});

it.each([
{ trigger: LAYOUT_TYPE_INITIALIZATION, action: 'subscribe' },
{ trigger: LAYOUT_TYPE_ADDED, action: 'subscribe' },
{ trigger: LAYOUT_TYPE_REMOVED, action: 'unsubscribe' },
] as {
trigger: LayoutTrigger;
action: 'subscribe' | 'unsubscribe';
}[])(
'$trigger trigger should $action targets and call target hooks',
({ action }) => {
const lifecycle: jest.SpyInstance[] = [];
Copy link
Member Author

@asturur asturur Feb 10, 2024

Choose a reason for hiding this comment

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

this 3 tests were failing because i wrote bad code. And it was not immediate to me to understand what they were testing for 2 reasons:
The connection between the array of data and the test variables force me to look up and down every time to understandand what is action and what is trigger, and the lifecycle array was a convoluted way to spell 3 times what has been calle, but without clearly saying with what has been called.

In general we should keep those array method for tests either for simple cases [true, false] or for large cases in which we need to use points or something else.

In the other cases i really prefer to spell clearly what the test is doing


const manager = new LayoutManager();

const targets = [
describe('triggers and event subscriptions', () => {
let manager: LayoutManager;
let targets: FabricObject[];
let target: Group;
let context: StrictLayoutContext;
beforeEach(() => {
manager = new LayoutManager();

targets = [
new Group([new FabricObject()], { layoutManager: manager }),
new FabricObject(),
];
const target = new Group(targets, { layoutManager: manager });
const canvasFire = jest.fn();
target.canvas = { fire: canvasFire };
const targetFire = jest.spyOn(target, 'fire').mockImplementation(() => {
lifecycle.push(targetFire);
});
target = new Group(targets, { layoutManager: manager });
target.canvas = { fire: jest.fn() };

const subscription = jest
.spyOn(manager, action)
.mockImplementation(() => {
lifecycle.push(subscription);
});
jest.spyOn(target, 'fire');

const context: StrictLayoutContext = {
context = {
bubbles: true,
strategy: manager.strategy,
type: LAYOUT_TYPE_INITIALIZATION,
Expand All @@ -225,18 +207,53 @@ describe('Layout Manager', () => {
this.bubbles = false;
},
};
});
it(`initialization trigger should subscribe targets and call target hooks`, () => {
jest.spyOn(manager, 'subscribe');
context.type = LAYOUT_TYPE_INITIALIZATION;
manager['onBeforeLayout'](context);

expect(lifecycle).toEqual([subscription, subscription, targetFire]);
expect(targetFire).toBeCalledWith('layout:before', {
expect(manager['subscribe']).toHaveBeenCalledTimes(targets.length);
expect(manager['subscribe']).toHaveBeenCalledWith(targets[0], context);
expect(target.fire).toBeCalledWith('layout:before', {
context,
});
expect(canvasFire).toBeCalledWith('object:layout:before', {
expect(target.canvas.fire).toBeCalledWith('object:layout:before', {
context,
target,
});
}
);
});
it(`object removed trigger should unsubscribe targets and call target hooks`, () => {
jest.spyOn(manager, 'unsubscribe');
context.type = LAYOUT_TYPE_REMOVED;
manager['onBeforeLayout'](context);
expect(manager['unsubscribe']).toHaveBeenCalledTimes(targets.length);
expect(manager['unsubscribe']).toHaveBeenCalledWith(
targets[0],
context
);
expect(target.fire).toBeCalledWith('layout:before', {
context,
});
expect(target.canvas.fire).toBeCalledWith('object:layout:before', {
context,
target,
});
});
it(`object added trigger should subscribe targets and call target hooks`, () => {
jest.spyOn(manager, 'subscribe');
context.type = LAYOUT_TYPE_ADDED;
manager['onBeforeLayout'](context);
expect(manager['subscribe']).toHaveBeenCalledTimes(targets.length);
expect(manager['subscribe']).toHaveBeenCalledWith(targets[0], context);
expect(target.fire).toBeCalledWith('layout:before', {
context,
});
expect(target.canvas.fire).toBeCalledWith('object:layout:before', {
context,
target,
});
});
});

it('passing deep should layout the entire tree', () => {
const manager = new LayoutManager();
Expand Down Expand Up @@ -739,6 +756,73 @@ describe('Layout Manager', () => {
).toMatchObject([child]);
});

describe('fromObject restore', () => {
const createTestData = (type: string) => ({
width: 2,
height: 3,
left: 6,
top: 4,
strokeWidth: 0,
objects: [
new Rect({
width: 100,
height: 100,
top: 0,
left: 0,
strokeWidth: 0,
}).toObject(),
new Rect({
width: 100,
height: 100,
top: 0,
left: 0,
strokeWidth: 0,
}).toObject(),
],
clipPath: new Rect({
width: 50,
height: 50,
top: 0,
left: 0,
strokeWidth: 0,
}).toObject(),
layoutManager: {
type: 'layoutManager',
strategy: type,
},
});
describe('Fitcontent layout', () => {
it('should subscribe objects', async () => {
const group = await Group.fromObject(
createTestData(FitContentLayout.type)
);
expect(
Array.from(group.layoutManager['_subscriptions'].keys())
).toMatchObject(group.getObjects());
});
});
describe('FixedLayout layout', () => {
it('should subscribe objects', async () => {
const group = await Group.fromObject(
createTestData(FixedLayout.type)
);
expect(
Array.from(group.layoutManager['_subscriptions'].keys())
).toMatchObject(group.getObjects());
});
});
describe('ClipPathLayout layout', () => {
it('should subscribe objects', async () => {
const group = await Group.fromObject(
createTestData(ClipPathLayout.type)
);
expect(
Array.from(group.layoutManager['_subscriptions'].keys())
).toMatchObject(group.getObjects());
});
});
});

test.each([true, false])(
'initialization edge case, legacy layout %s',
(legacy) => {
Expand All @@ -751,7 +835,7 @@ describe('Layout Manager', () => {
width: 200,
height: 200,
strokeWidth: 0,
layoutManager: !legacy ? new LayoutManager() : undefined,
layoutManager: legacy ? undefined : new LayoutManager(),
});
expect(group).toMatchObject({ width: 200, height: 200 });
expect(child.getRelativeCenterPoint()).toMatchObject({ x: 0, y: 0 });
Expand Down
25 changes: 17 additions & 8 deletions src/LayoutManager/LayoutManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ import {
LAYOUT_TYPE_OBJECT_MODIFIED,
LAYOUT_TYPE_OBJECT_MODIFYING,
} from './constants';
import type { LayoutContext, LayoutResult, StrictLayoutContext } from './types';
import type {
LayoutContext,
LayoutResult,
RegistrationContext,
StrictLayoutContext,
} from './types';
import { classRegistry } from '../ClassRegistry';

const LAYOUT_MANAGER = 'layoutManager';
Expand Down Expand Up @@ -59,14 +64,14 @@ export class LayoutManager {
/**
* subscribe to object layout triggers
*/
protected subscribe(object: FabricObject, context: StrictLayoutContext) {
protected subscribe(object: FabricObject, context: { target: Group }) {
const { target } = context;
this.unsubscribe(object, context);
const disposers = [
object.on('modified', (e) =>
this.performLayout({
trigger: 'modified',
e: { ...e, target: object },
e,
Copy link
Member Author

Choose a reason for hiding this comment

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

in the object modified, target is already available in the event, we don't need to repeat it. At leat this says typescript

Copy link
Member Author

Choose a reason for hiding this comment

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

on line 88 ( now 93 ) we do the same for the other event, but in my opinion we shouldn't. If we want to forward the original event that is fine, but we are adding the target itself in the object event, we don't need to copy it in the forwarded event.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember something about this.
If you wish to do this you should add tests to verify this.
Notice that we add a target prop to this object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that there is a target prop to this object, that is why i don't see any reason to duplicate it on the event.
Probably also e should be called 'triggerEventData' or something that can let us know what 'e' is because is not clear. The only purpose of this 'e' is to be found later inside 'layout:after' event, correct? What else?

type: LAYOUT_TYPE_OBJECT_MODIFIED,
target,
})
Expand Down Expand Up @@ -99,13 +104,17 @@ export class LayoutManager {
* unsubscribe object layout triggers
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected unsubscribe(object: FabricObject, context?: StrictLayoutContext) {
protected unsubscribe(object: FabricObject, context?: { target: Group }) {
(this._subscriptions.get(object) || []).forEach((d) => d());
this._subscriptions.delete(object);
}

unsubscribeTarget(target: Group) {
target.forEachObject((object) => this.unsubscribe(object));
unsubscribeTargets(context: RegistrationContext) {
context.targets.forEach((object) => this.unsubscribe(object, context));
}

subscribeTargets(context: RegistrationContext) {
context.targets.forEach((object) => this.subscribe(object, context));
}

protected onBeforeLayout(context: StrictLayoutContext) {
Expand All @@ -116,9 +125,9 @@ export class LayoutManager {
context.type === LAYOUT_TYPE_INITIALIZATION ||
context.type === LAYOUT_TYPE_ADDED
) {
context.targets.forEach((object) => this.subscribe(object, context));
this.subscribeTargets(context);
} else if (context.type === LAYOUT_TYPE_REMOVED) {
context.targets.forEach((object) => this.unsubscribe(object, context));
this.unsubscribeTargets(context);
}
// fire layout event (event will fire only for layouts after initialization layout)
target.fire('layout:before', {
Expand Down
5 changes: 5 additions & 0 deletions src/LayoutManager/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ export type StrictLayoutContext = LayoutContext & {
stopPropagation(): void;
};

export type RegistrationContext = {
targets: FabricObject[];
target: Group;
};

export type LayoutBeforeEvent = {
context: StrictLayoutContext;
};
Expand Down
9 changes: 8 additions & 1 deletion src/shapes/Group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,10 @@ export class Group
}

dispose() {
this.layoutManager.unsubscribeTarget(this);
this.layoutManager.unsubscribeTargets({
targets: this.getObjects(),
target: this,
});
this._activeObjects = [];
this.forEachObject((object) => {
this._watchObject(false, object);
Expand Down Expand Up @@ -672,6 +675,10 @@ export class Group
} else {
group.layoutManager = new LayoutManager();
}
group.layoutManager.subscribeTargets({
target: group,
targets: group.getObjects(),
});
group.setCoords();
return group;
});
Expand Down
2 changes: 1 addition & 1 deletion src/shapes/Object/types/SerializedObjectProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export interface SerializedObjectProps extends BaseProps, FillStrokeProps {
* If you want 0,0 of a clipPath to align with an object center, use clipPath.originX/Y to 'center'
* @type FabricObject
*/
clipPath?: Partial<SerializedObjectProps> & ClipPathProps;
clipPath?: Partial<SerializedObjectProps & ClipPathProps>;
}

export interface ClipPathProps {
Expand Down