Move history management functions out of layer renderer and into history utility (#61)

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;

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 );

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();
+                });
+            });
         });
     });
-});
+});