From 33b92d9df82a42676e0a7ece9c67368ac0989bf9 Mon Sep 17 00:00:00 2001 From: Shachar <34343793+ShaMan123@users.noreply.github.com> Date: Thu, 11 Jan 2024 11:07:32 +0200 Subject: [PATCH] feat(LayoutManager): BREAKING remove `shouldResetTransform` logic from layoutManager (#9581) --- CHANGELOG.md | 1 + src/LayoutManager/LayoutManager.spec.ts | 48 ++++--------------- src/LayoutManager/LayoutManager.ts | 6 --- .../LayoutStrategies/LayoutStrategy.ts | 7 --- .../__snapshots__/LayoutManager.spec.ts.snap | 8 ++-- src/shapes/ActiveSelection.spec.ts | 43 +++-------------- src/shapes/ActiveSelection.ts | 4 +- 7 files changed, 23 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fa1fa01ef..b71dd3aa023 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [next] +- BREAKING BETA(LayoutManager): rm `shouldResetTransform` [#9581](https://github.com/fabricjs/fabric.js/pull/9581) - refactor(): rm active selection ref [#9561](https://github.com/fabricjs/fabric.js/pull/9561) - feat(Next.js sandbox): simpler canvas hook [#9577](https://github.com/fabricjs/fabric.js/pull/9577) - fix(): fix modify polygon points with zero sized polygons ( particular case of axis oriented lines ) [#9575](https://github.com/fabricjs/fabric.js/pull/9575) diff --git a/src/LayoutManager/LayoutManager.spec.ts b/src/LayoutManager/LayoutManager.spec.ts index edcc8876704..3ef954a6219 100644 --- a/src/LayoutManager/LayoutManager.spec.ts +++ b/src/LayoutManager/LayoutManager.spec.ts @@ -305,19 +305,27 @@ describe('Layout Manager', () => { relativeCorrection: new Point(-30, -40), }); - const target = new Group([], { scaleX: 2, scaleY: 0.5, angle: 30 }); + const rect = new FabricObject({ width: 50, height: 50 }); + const target = new Group([rect], { scaleX: 2, scaleY: 0.5, angle: 30 }); const context: StrictLayoutContext = { bubbles: true, strategy: manager.strategy, target, + targets: [rect], ...options, stopPropagation() { this.bubbles = false; }, }; - expect(manager['getLayoutResult'](context)).toMatchSnapshot(); + expect(manager['getLayoutResult'](context)).toMatchSnapshot({ + cloneDeepWith: (value: any) => { + if (value instanceof Point) { + return new Point(Math.round(value.x), Math.round(value.y)); + } + }, + }); }); }); @@ -475,12 +483,6 @@ describe('Layout Manager', () => { const canvasFire = jest.fn(); target.canvas = { fire: canvasFire }; - const shouldResetTransform = jest - .spyOn(manager.strategy, 'shouldResetTransform') - .mockImplementation(() => { - lifecycle.push(shouldResetTransform); - }); - const context: StrictLayoutContext = { bubbles, strategy: manager.strategy, @@ -501,11 +503,9 @@ describe('Layout Manager', () => { manager['onAfterLayout'](context, layoutResult); expect(lifecycle).toEqual([ - shouldResetTransform, targetFire, ...(bubbles ? [parentPerformLayout] : []), ]); - expect(shouldResetTransform).toBeCalledWith(context); expect(targetFire).toBeCalledWith('layout:after', { context, result: layoutResult, @@ -527,34 +527,6 @@ describe('Layout Manager', () => { } ); - test.each([true, false])('reset target transform %s', (reset) => { - const targets = [new Group([new FabricObject()]), new FabricObject()]; - const target = new Group(targets); - target.left = 50; - - const manager = new LayoutManager(); - jest - .spyOn(manager.strategy, 'shouldResetTransform') - .mockImplementation(() => { - return reset; - }); - - const context: StrictLayoutContext = { - bubbles: true, - strategy: manager.strategy, - type: LAYOUT_TYPE_REMOVED, - target, - targets, - prevStrategy: undefined, - stopPropagation() { - this.bubbles = false; - }, - }; - manager['onAfterLayout'](context); - - expect(target.left).toBe(reset ? 0 : 50); - }); - test('bubbling', () => { const manager = new LayoutManager(); const manager2 = new LayoutManager(); diff --git a/src/LayoutManager/LayoutManager.ts b/src/LayoutManager/LayoutManager.ts index 6494d56def9..7bd78b6be99 100644 --- a/src/LayoutManager/LayoutManager.ts +++ b/src/LayoutManager/LayoutManager.ts @@ -4,7 +4,6 @@ import { CENTER, iMatrix } from '../constants'; import type { Group } from '../shapes/Group'; import type { FabricObject } from '../shapes/Object/FabricObject'; import { invertTransform } from '../util/misc/matrix'; -import { resetObjectTransform } from '../util/misc/objectTransforms'; import { resolveOrigin } from '../util/misc/resolveOrigin'; import { FitContentLayout } from './LayoutStrategies/FitContentLayout'; import type { LayoutStrategy } from './LayoutStrategies/LayoutStrategy'; @@ -262,11 +261,6 @@ export class LayoutManager { ...bubblingContext } = context; const { canvas } = target; - if (strategy.shouldResetTransform(context)) { - resetObjectTransform(target); - target.left = 0; - target.top = 0; - } // fire layout event (event will fire only for layouts after initialization layout) target.fire('layout:after', { diff --git a/src/LayoutManager/LayoutStrategies/LayoutStrategy.ts b/src/LayoutManager/LayoutStrategies/LayoutStrategy.ts index a7bb9bf0e95..12fdc395577 100644 --- a/src/LayoutManager/LayoutStrategies/LayoutStrategy.ts +++ b/src/LayoutManager/LayoutStrategies/LayoutStrategy.ts @@ -61,13 +61,6 @@ export abstract class LayoutStrategy { return result.size; } - /** - * called from the `onAfterLayout` hook - */ - shouldResetTransform(context: StrictLayoutContext) { - return context.target.size() === 0; - } - /** * Override this method to customize layout. */ diff --git a/src/LayoutManager/__snapshots__/LayoutManager.spec.ts.snap b/src/LayoutManager/__snapshots__/LayoutManager.spec.ts.snap index d43051c9e2b..741508b80e1 100644 --- a/src/LayoutManager/__snapshots__/LayoutManager.spec.ts.snap +++ b/src/LayoutManager/__snapshots__/LayoutManager.spec.ts.snap @@ -7,12 +7,12 @@ exports[`Layout Manager getLayoutResult imperative trigger 1`] = ` "y": 100, }, "offset": Point { - "x": -70, - "y": -120, + "x": -42, + "y": -113, }, "prevCenter": Point { - "x": 0, - "y": 0, + "x": 38, + "y": 37, }, "result": { "center": Point { diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index 9ee419c650a..2b6322ec98d 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -13,36 +13,7 @@ describe('ActiveSelection', () => { ); }); - it('clearing active selection objects resets transform', () => { - const obj = new FabricObject({ - left: 100, - top: 100, - width: 100, - height: 100, - }); - const selection = new ActiveSelection([obj], { - left: 200, - top: 200, - angle: 45, - skewX: 0.5, - skewY: -0.5, - }); - selection.remove(obj); - expect(selection).toMatchObject({ - left: 0, - top: 0, - angle: 0, - scaleX: 1, - scaleY: 1, - skewX: 0, - skewY: 0, - flipX: false, - flipY: false, - _objects: [], - }); - }); - - it('deselect removes all objects and resets transform', () => { + it('deselect removes all objects', () => { const selection = new ActiveSelection([], { left: 200, top: 100, @@ -52,9 +23,9 @@ describe('ActiveSelection', () => { selection.onDeselect(); expect(spy).toHaveBeenCalled(); expect(selection).toMatchObject({ - left: 0, - top: 0, - angle: 0, + left: 200, + top: 100, + angle: 45, scaleX: 1, scaleY: 1, skewX: 0, @@ -64,11 +35,11 @@ describe('ActiveSelection', () => { _objects: [], }); selection.add(new FabricObject({ left: 50, top: 50, strokeWidth: 0 })); - expect(selection.item(0).getCenterPoint()).toEqual({ x: 50, y: 50 }); + const { x, y } = selection.item(0).getCenterPoint(); + expect({ x: Math.round(x), y: Math.round(y) }).toEqual({ x: 50, y: 50 }); }); - // remove skip once #9152 is merged - it.skip('should not set coords in the constructor', () => { + it('should not set coords in the constructor', () => { const spy = jest.spyOn(ActiveSelection.prototype, 'setCoords'); new ActiveSelection([ new FabricObject({ diff --git a/src/shapes/ActiveSelection.ts b/src/shapes/ActiveSelection.ts index cb8d9822841..bb8d763c8e0 100644 --- a/src/shapes/ActiveSelection.ts +++ b/src/shapes/ActiveSelection.ts @@ -146,9 +146,7 @@ export class ActiveSelection extends Group { } /** - * If returns true, deselection is cancelled. - * @since 2.0.0 - * @return {Boolean} [cancel] + * @override remove all objects */ onDeselect() { this.removeAll();