From 641828c0358adb6d462bdee23f3eddaffc1f705d Mon Sep 17 00:00:00 2001 From: Igor Zinken <730069+igorski@users.noreply.github.com> Date: Sat, 29 Mar 2025 07:43:47 +0100 Subject: [PATCH] Move history management functions out of layer renderer and into history utility (#61) --- src/rendering/actors/layer-renderer.ts | 53 +--- src/utils/layer-history-util.ts | 54 +++- tests/unit/utils/layer-history-util.spec.ts | 288 +++++++++++++++----- 3 files changed, 288 insertions(+), 107 deletions(-) diff --git a/src/rendering/actors/layer-renderer.ts b/src/rendering/actors/layer-renderer.ts index c54d1f0..8811499 100644 --- a/src/rendering/actors/layer-renderer.ts +++ b/src/rendering/actors/layer-renderer.ts @@ -49,15 +49,13 @@ import { getDrawableCanvas, renderDrawableCanvas, disposeDrawableCanvas, sliceBrushPointers, createOverrideConfig } from "@/rendering/utils/drawable-canvas-utils"; import BrushFactory from "@/factories/brush-factory"; -import { getRendererForLayer } from "@/factories/renderer-factory"; -import { enqueueState } from "@/factories/history-state-factory"; import { createCanvas, canvasToBlob, cloneCanvas, globalToLocal, getPixelRatio } from "@/utils/canvas-util"; import { createSyncSnapshot } from "@/utils/document-util"; import { hasBlend, isDrawable, isMaskable, isRotated, isScaled } from "@/utils/layer-util"; import { blobToResource } from "@/utils/resource-manager"; import { getLastShape } from "@/utils/selection-util"; import { isShapeClosed } from "@/utils/shape-util"; -import { positionRendererFromHistory, restorePaintFromHistory } from "@/utils/layer-history-util"; +import { storeLayerPositionInHistory, storeMaskPositionInHistory, storePaintInHistory } from "@/utils/layer-history-util"; import type { BitMapperyState } from "@/store"; const HALF = 0.5; @@ -431,18 +429,8 @@ export default class LayerRenderer extends ZoomableSprite { const orgBlob = await canvasToBlob( original ); const orgState = blobToResource( orgBlob ); - const layer = this.layer; - const isMask = isMaskable( layer, this.getStore() ); + storePaintInHistory( this.layer, orgState, newState, isMaskable( this.layer, this.getStore() )); - enqueueState( `layerPaint_${layer.id}`, { - undo(): void { - restorePaintFromHistory( layer, orgState, isMask ); - }, - redo(): void { - restorePaintFromHistory( layer, newState, isMask ); - }, - resources: [ orgState, newState ], - }); return true; } @@ -465,30 +453,20 @@ export default class LayerRenderer extends ZoomableSprite { super.setBounds( x, y, width, height ); // store new value (for redo) - const newX = bounds.left; - const newY = bounds.top; + const newLeft = bounds.left; + const newTop = bounds.top; // update the Layer model by the relative offset // (because the renderer maintains an alternate position when the Layer is rotated) - const newLayerX = layer.left + ( newX - left ); - const newLayerY = layer.top + ( newY - top ); + const newLayerX = layer.left + ( newLeft - left ); + const newLayerY = layer.top + ( newTop - top ); layer.left = newLayerX; layer.top = newLayerY; - enqueueState( `layerPos_${layer.id}`, { - undo() { - positionRendererFromHistory( layer, left, top ); - layer.left = oldLayerX; - layer.top = oldLayerY; - }, - redo() { - positionRendererFromHistory( layer, newX, newY ); - layer.left = newLayerX; - layer.top = newLayerY; - } - }); + storeLayerPositionInHistory( this.layer, oldLayerX, oldLayerY, newLayerX, newLayerY, left, top, newLeft, newTop ); + this.invalidateBlendCache(); } @@ -574,20 +552,7 @@ export default class LayerRenderer extends ZoomableSprite { if ( this.layer.effects.mirrorY ) { newMaskY = -newMaskY; } - const commit = () => { - layer.maskX = newMaskX; - layer.maskY = newMaskY; - getRendererForLayer( layer )?.resetFilterAndRecache(); - }; - commit(); - enqueueState( `maskPos_${layer.id}`, { - undo() { - layer.maskX = maskX; - layer.maskY = maskY; - getRendererForLayer( layer )?.resetFilterAndRecache(); - }, - redo: commit - }); + storeMaskPositionInHistory( this.layer, maskX, maskY, newMaskX, newMaskY ); } else if ( this._isDragMode ) { super.handleMove( x, y, event ); return; diff --git a/src/utils/layer-history-util.ts b/src/utils/layer-history-util.ts index 0789ee7..4bda9b5 100644 --- a/src/utils/layer-history-util.ts +++ b/src/utils/layer-history-util.ts @@ -22,13 +22,63 @@ */ import { type Layer } from "@/definitions/document"; import { getRendererForLayer } from "@/factories/renderer-factory"; +import { enqueueState } from "@/factories/history-state-factory"; import { flushBlendedLayerCache, useBlendCaching } from "../rendering/cache/blended-layer-cache"; // NOTE we use getRendererForLayer() instead of passing the renderer by reference // as it is possible the renderer originally rendering the Layer has been disposed // and a new one has been created while traversing the change history -export function positionRendererFromHistory( layer: Layer, x: number, y: number ): void { +export function storeLayerPositionInHistory( + layer: Layer, oldLayerX: number, oldLayerY: number, newLayerX: number, newLayerY: number, + oldRendererX: number, oldRendererY: number, newRendererX: number, newRendererY: number, +): void { + enqueueState( `layerPos_${layer.id}`, { + undo(): void { + positionRendererFromHistory( layer, oldRendererX, oldRendererY ); + layer.left = oldLayerX; + layer.top = oldLayerY; + }, + redo(): void { + positionRendererFromHistory( layer, newRendererX, newRendererY ); + layer.left = newLayerX; + layer.top = newLayerY; + } + }); +} + +export function storeMaskPositionInHistory( layer: Layer, oldMaskX: number, oldMaskY: number, newMaskX: number, newMaskY: number ): void { + const commit = (): void => { + layer.maskX = newMaskX; + layer.maskY = newMaskY; + getRendererForLayer( layer )?.resetFilterAndRecache(); + }; + commit(); + enqueueState( `maskPos_${layer.id}`, { + undo(): void { + layer.maskX = oldMaskX; + layer.maskY = oldMaskY; + getRendererForLayer( layer )?.resetFilterAndRecache(); + }, + redo: commit + }); +} + +export function storePaintInHistory( layer: Layer, orgSourceBlobURL: string, newSourceBlobURL: string, isMask: boolean ): void { + enqueueState( `layerPaint_${layer.id}`, { + undo(): void { + restorePaintFromHistory( layer, orgSourceBlobURL, isMask ); + }, + redo(): void { + restorePaintFromHistory( layer, newSourceBlobURL, isMask ); + }, + resources: [ orgSourceBlobURL, newSourceBlobURL ], + }); +} + +/* internal methods */ + +function positionRendererFromHistory( layer: Layer, x: number, y: number ): void { const renderer = getRendererForLayer( layer ); if ( renderer ) { renderer.getBounds().left = x; @@ -40,7 +90,7 @@ export function positionRendererFromHistory( layer: Layer, x: number, y: number } } -export function restorePaintFromHistory( layer: Layer, sourceToRestore: string, isMask: boolean ): void { +function restorePaintFromHistory( layer: Layer, sourceToRestore: string, isMask: boolean ): void { const source = isMask ? layer.mask : layer.source; const ctx = source.getContext( "2d" ) as CanvasRenderingContext2D; ctx.clearRect( 0, 0, source.width, source.height ); diff --git a/tests/unit/utils/layer-history-util.spec.ts b/tests/unit/utils/layer-history-util.spec.ts index 600fdf2..59f5bde 100644 --- a/tests/unit/utils/layer-history-util.spec.ts +++ b/tests/unit/utils/layer-history-util.spec.ts @@ -3,15 +3,21 @@ import { createMockCanvasElement, mockZCanvas } from "../mocks"; mockZCanvas(); +import { type Layer } from "@/definitions/document"; import LayerFactory from "@/factories/layer-factory"; import LayerRenderer from "@/rendering/actors/layer-renderer"; -import { positionRendererFromHistory, restorePaintFromHistory } from "@/utils/layer-history-util"; +import { storeLayerPositionInHistory, storeMaskPositionInHistory, storePaintInHistory } from "@/utils/layer-history-util"; const mockGetRendererForLayer = vi.fn(); vi.mock( "@/factories/renderer-factory", () => ({ getRendererForLayer: vi.fn(( ...args ) => mockGetRendererForLayer( ...args )), })); +const mockEnqueueState = vi.fn(); +vi.mock( "@/factories/history-state-factory", () => ({ + enqueueState: vi.fn(( ...args ) => mockEnqueueState( ...args )), +})); + const mockFlushBlendedLayerCache = vi.fn(); let mockUseBlendCaching = false; vi.mock( "@/rendering/cache/blended-layer-cache", () => ({ @@ -21,13 +27,14 @@ vi.mock( "@/rendering/cache/blended-layer-cache", () => ({ })); describe( "Layer history utilities", () => { - const layer = LayerFactory.create({ - source: createMockCanvasElement(), - mask: createMockCanvasElement(), - }); + let layer: Layer; let layerRenderer: LayerRenderer; beforeEach(() => { + layer = LayerFactory.create({ + source: createMockCanvasElement(), + mask: createMockCanvasElement(), + }); layerRenderer = new LayerRenderer( layer ); mockGetRendererForLayer.mockReturnValue( layerRenderer ); }); @@ -36,83 +43,242 @@ describe( "Layer history utilities", () => { vi.resetAllMocks(); }); - describe( "When adjusting a Layer renderers position from a history state", () => { - it( "should update the existing bounds object directly without invoking the setter methods", () => { - const orgBounds = layerRenderer.getBounds(); - const { width, height } = orgBounds; - - positionRendererFromHistory( layer, 5, 7 ); + describe( "When storing a Layer renderers position as a history state", () => { + const oldLayerX = 5; + const oldLayerY = 6; + const newLayerX = 15; + const newLayerY = 16; + const oldRendererX = 2; + const oldRendererY = 3; + const newRendererX = 7; + const newRendererY = 8; - expect( orgBounds ).toEqual({ - left: 5, - top: 7, - width, - height, + it( "should enqueue the state in history", () => { + storeLayerPositionInHistory( + layer, oldLayerX, oldLayerY, newLayerX, newLayerY, + oldRendererX, oldRendererY, newRendererX, newRendererY + ); + expect( mockEnqueueState ).toHaveBeenCalledWith( + `layerPos_${layer.id}`, { + undo: expect.any( Function ), + redo: expect.any( Function ) + } + ); + }); + + describe( "and calling the undo or redo state", () => { + let undo: VoidFunction; + let redo: VoidFunction; + + beforeEach(() => { + storeLayerPositionInHistory( + layer, oldLayerX, oldLayerY, newLayerX, newLayerY, + oldRendererX, oldRendererY, newRendererX, newRendererY + ); + ({ undo, redo } = mockEnqueueState.mock.calls[ 0 ][ 1 ]); }); - }); - it( "should call the invalidate() method on the renderer to trigger a render", () => { - const invalidateSpy = vi.spyOn( layerRenderer, "invalidate" ); - - positionRendererFromHistory( layer, 5, 7 ); + it.each([ "undo", "redo" ]) + ( `should restore the position of the layer for the "%s" action`, ( action: string ) => { + layer.left = 1000; + layer.top = 1000; - expect( invalidateSpy ).toHaveBeenCalled(); - }); + ( action === "undo" ) ? undo() : redo(); - it( "should not invalidate the blended layer cache when blend caching is disabled", () => { - mockUseBlendCaching = false; + expect( layer.left ).toEqual( action === "undo" ? oldLayerX : newLayerX ); + expect( layer.top ).toEqual( action === "undo" ? oldLayerY : newLayerY ); + }); - positionRendererFromHistory( layer, 5, 7 ); - - expect( mockFlushBlendedLayerCache ).not.toHaveBeenCalled(); - }); + it.each([ "undo", "redo" ]) + ( `should update the existing bounds object directly without invoking the setter methods for the "%s" action`, + ( action: string ) => { + const bounds = layerRenderer.getBounds(); + const { width, height } = bounds; + + bounds.left = 1000; + bounds.top = 1000; + + ( action === "undo" ) ? undo() : redo(); - it( "should invalidate the blended layer cache when blend caching is enabled", () => { - mockUseBlendCaching = true; + expect( bounds ).toEqual({ + left: action === "undo" ? oldRendererX : newRendererX, + top: action === "undo" ? oldRendererY : newRendererY, + width, + height, + }); + }); - positionRendererFromHistory( layer, 5, 7 ); - - expect( mockFlushBlendedLayerCache ).toHaveBeenCalled(); + it.each([ "undo", "redo" ]) + ( `should call the invalidate() method on the renderer to trigger a render for the "%s" action`, ( action: string ) => { + const invalidateSpy = vi.spyOn( layerRenderer, "invalidate" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( invalidateSpy ).toHaveBeenCalled(); + }); + + it.each([ "undo", "redo" ]) + ( `should not invalidate the blended layer cache when blend caching is disabled for the "%s" action`, ( action: string ) => { + mockUseBlendCaching = false; + + ( action === "undo" ) ? undo() : redo(); + + expect( mockFlushBlendedLayerCache ).not.toHaveBeenCalled(); + }); + + it.each([ "undo", "redo" ]) + ( `should invalidate the blended layer cache when blend caching is enabled for the "%s" action`, ( action: string ) => { + mockUseBlendCaching = true; + + ( action === "undo" ) ? undo() : redo(); + + expect( mockFlushBlendedLayerCache ).toHaveBeenCalled(); + }); }); }); - describe( "When restoring a paint operation from state history", () => { - const MOCK_SOURCE = "blob:http://foo"; - let setSourceSpy; + describe( "When storing a Layer mask position as a history state", () => { + const oldMaskX = 5; + const oldMaskY = 6; + const newMaskX = 15; + const newMaskY = 16; - beforeEach(() => { - // instantly load an image on src assignment - setSourceSpy = vi.spyOn( global.Image.prototype, "src", "set" ).mockImplementation( function( _source: string ): void { - this.onload(); + it( "should enqueue the state in history", () => { + storeMaskPositionInHistory( layer, oldMaskX, oldMaskY, newMaskX, newMaskY ); + expect( mockEnqueueState ).toHaveBeenCalledWith( + `maskPos_${layer.id}`, { + undo: expect.any( Function ), + redo: expect.any( Function ) + } + ); + }); + + describe( "and calling the undo or redo state", () => { + let undo: VoidFunction; + let redo: VoidFunction; + + beforeEach(() => { + storeMaskPositionInHistory( layer, oldMaskX, oldMaskY, newMaskX, newMaskY ); + ({ undo, redo } = mockEnqueueState.mock.calls[ 0 ][ 1 ]); + }); + + it.each([ "undo", "redo" ]) + ( `should restore the position of the mask for the "%s" action`, ( action: string ) => { + layer.maskX = 1000; + layer.maskY = 1000; + + ( action === "undo" ) ? undo() : redo(); + + expect( layer.maskX ).toEqual( action === "undo" ? oldMaskX : newMaskX ); + expect( layer.maskY ).toEqual( action === "undo" ? oldMaskY : newMaskY ); + }); + + it.each([ "undo", "redo" ]) + ( `should recache the renderers filter on the "%s" action`, ( action: string ) => { + const rerenderSpy = vi.spyOn( layerRenderer, "resetFilterAndRecache" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( rerenderSpy ).toHaveBeenCalled(); }); }); + }); - it( "should set the layer source to the provided source value when restoring outside of the mask", () => { - const getMaskContextSpy = vi.spyOn( layer.mask, "getContext" ); + describe( "When storing a paint operation into state history", () => { + const MOCK_ORG_SOURCE = "blob:http://foo"; + const MOCK_NEW_SOURCE = "blob:http://bar"; - restorePaintFromHistory( layer, MOCK_SOURCE, false ); - - expect( getMaskContextSpy ).not.toHaveBeenCalled(); - - expect( layer.source.getContext( "2d" ).drawImage ).toHaveBeenCalledWith( expect.any( global.Image ), 0, 0 ); + it( "should enqueue the state in history including a resources list", () => { + storePaintInHistory( layer, MOCK_ORG_SOURCE, MOCK_NEW_SOURCE, true ); + expect( mockEnqueueState ).toHaveBeenCalledWith( + `layerPaint_${layer.id}`, { + undo: expect.any( Function ), + redo: expect.any( Function ), + resources: [ MOCK_ORG_SOURCE, MOCK_NEW_SOURCE ], + } + ); }); - it( "should set the layer mask to the provided source value when restoring inside of the mask", () => { - const getSourceContextSpy = vi.spyOn( layer.source, "getContext" ); - - restorePaintFromHistory( layer, MOCK_SOURCE, true ); + describe( "and calling the undo or redo state", () => { + let setSourceSpy; + let loadedSource: string; + let undo: VoidFunction; + let redo: VoidFunction; - expect( getSourceContextSpy ).not.toHaveBeenCalled(); - - expect( layer.mask.getContext( "2d" ).drawImage ).toHaveBeenCalledWith( expect.any( global.Image ), 0, 0 ); - }); + describe( "for a paint action outside of the Layer mask", () => { - it( "should request a filter invalidation and recache from the Layers renderer ", () => { - const recacheSpy = vi.spyOn( layerRenderer, "resetFilterAndRecache" ); + beforeEach(() => { + storePaintInHistory( layer, MOCK_ORG_SOURCE, MOCK_NEW_SOURCE, false ); + ({ undo, redo } = mockEnqueueState.mock.calls[ 0 ][ 1 ]); - restorePaintFromHistory( layer, MOCK_SOURCE, false ); + // instantly load an image on src assignment + setSourceSpy = vi.spyOn( global.Image.prototype, "src", "set" ).mockImplementation( function( _source: string ): void { + loadedSource = _source; + this.onload(); + }); + }); - expect( recacheSpy ).toHaveBeenCalledWith(); + it.each([ "undo", "redo" ]) + ( `should set the layer source to the provided source value when restoring outside of the mask for the "%s" action`, + ( action: string ) => { + const getMaskContextSpy = vi.spyOn( layer.mask, "getContext" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( getMaskContextSpy ).not.toHaveBeenCalled(); + + expect( layer.source.getContext( "2d" ).drawImage ).toHaveBeenCalledWith( expect.any( global.Image ), 0, 0 ); + + expect( loadedSource ).toEqual( action === "undo" ? MOCK_ORG_SOURCE : MOCK_NEW_SOURCE ); + }); + + it.each([ "undo", "redo" ]) + ( `should request a filter invalidation and recache from the Layers renderer for the "%s" action`, ( action: string ) => { + const recacheSpy = vi.spyOn( layerRenderer, "resetFilterAndRecache" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( recacheSpy ).toHaveBeenCalledWith(); + }); + }); + + describe( "for a paint action inside the Layer mask", () => { + + beforeEach(() => { + storePaintInHistory( layer, MOCK_ORG_SOURCE, MOCK_NEW_SOURCE, true ); + ({ undo, redo } = mockEnqueueState.mock.calls[ 0 ][ 1 ]); + + // instantly load an image on src assignment + setSourceSpy = vi.spyOn( global.Image.prototype, "src", "set" ).mockImplementation( function( _source: string ): void { + loadedSource = _source; + this.onload(); + }); + }); + + it.each([ "undo", "redo" ]) + ( `should set the layer mask to the provided source value when restoring inside of the mask for the "%s" action`, + ( action: string ) => { + const getSourceContextSpy = vi.spyOn( layer.source, "getContext" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( getSourceContextSpy ).not.toHaveBeenCalled(); + + expect( layer.mask.getContext( "2d" ).drawImage ).toHaveBeenCalledWith( expect.any( global.Image ), 0, 0 ); + + expect( loadedSource ).toEqual( action === "undo" ? MOCK_ORG_SOURCE : MOCK_NEW_SOURCE ); + }); + + it.each([ "undo", "redo" ]) + ( `should request a filter invalidation and recache from the Layers renderer for the "%s" action`, + ( action: string ) => { + const recacheSpy = vi.spyOn( layerRenderer, "resetFilterAndRecache" ); + + ( action === "undo" ) ? undo() : redo(); + + expect( recacheSpy ).toHaveBeenCalledWith(); + }); + }); }); }); -}); \ No newline at end of file +});