From 3742fa9f0c3118de1c23bf27198975eea62e09bb Mon Sep 17 00:00:00 2001
From: Hikaru Y <hkrysg@gmail.com>
Date: Sun, 10 Sep 2023 12:26:41 +0900
Subject: [PATCH] Fix some issues with undo/redo in mask editor (#2649)

Issues:
- The `change` event was not dispatched in MaskEditor.svelte when an
undo/redo was performed. Therefore, if the user then closed the editor
or switched to another note without performing an operation that would
cause the `change` event to be dispatched, the undone or redone changes
were not saved to DB.
- When `IOMode.kind === "edit"` (i.e., Edit Current or Browse), the
beginning of the undo history was a blank canvas, not a canvas with
existing masks. Therefore, if you continued to undo to the beginning of
the history, the masks that existed when you opened the editor would be
lost, and they would not be restored even when you performed a redo.
- In the 'Add' dialog, the undo history was not reset when starting to
create a new IO note after adding an IO note.

Also add a small UI improvement:
The undo/redo buttons are now disabled when there is no action to
undo/redo.
---
 ts/image-occlusion/MaskEditor.svelte       |  12 ++
 ts/image-occlusion/Toolbar.svelte          |   9 +-
 ts/image-occlusion/mask-editor.ts          |   6 +-
 ts/image-occlusion/tools/tool-ellipse.ts   |   6 +-
 ts/image-occlusion/tools/tool-polygon.ts   |   9 +-
 ts/image-occlusion/tools/tool-rect.ts      |   6 +-
 ts/image-occlusion/tools/tool-undo-redo.ts | 146 ++++++++++++++-------
 7 files changed, 126 insertions(+), 68 deletions(-)

diff --git a/ts/image-occlusion/MaskEditor.svelte b/ts/image-occlusion/MaskEditor.svelte
index c6c74532f..a89625923 100644
--- a/ts/image-occlusion/MaskEditor.svelte
+++ b/ts/image-occlusion/MaskEditor.svelte
@@ -2,6 +2,16 @@
 Copyright: Ankitects Pty Ltd and contributors
 License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 -->
+<script context="module" lang="ts">
+    import { writable } from "svelte/store";
+
+    const changeSignal = writable(Symbol());
+
+    export function emitChangeSignal() {
+        changeSignal.set(Symbol());
+    }
+</script>
+
 <script lang="ts">
     import type { PanZoom } from "panzoom";
     import panzoom from "panzoom";
@@ -28,6 +38,8 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
         dispatch("change", { canvas });
     }
 
+    $: $changeSignal, onChange();
+
     function init(node) {
         instance = panzoom(node, {
             bounds: true,
diff --git a/ts/image-occlusion/Toolbar.svelte b/ts/image-occlusion/Toolbar.svelte
index 511e22a28..17d6c2ff3 100644
--- a/ts/image-occlusion/Toolbar.svelte
+++ b/ts/image-occlusion/Toolbar.svelte
@@ -29,7 +29,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
         zoomTools,
     } from "./tools/more-tools";
     import { tools } from "./tools/tool-buttons";
-    import { undoRedoTools } from "./tools/tool-undo-redo";
+    import { undoRedoTools, undoStack } from "./tools/tool-undo-redo";
 
     export let canvas;
     export let instance;
@@ -172,9 +172,10 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                     ? 'left-border-radius'
                     : 'right-border-radius'}"
                 {iconSize}
-                on:click={() => {
-                    tool.action(canvas);
-                }}
+                on:click={tool.action}
+                disabled={tool.name === "undo"
+                    ? !$undoStack.undoable
+                    : !$undoStack.redoable}
             >
                 {@html tool.icon}
             </IconButton>
diff --git a/ts/image-occlusion/mask-editor.ts b/ts/image-occlusion/mask-editor.ts
index 438eafd3b..b808ba0dd 100644
--- a/ts/image-occlusion/mask-editor.ts
+++ b/ts/image-occlusion/mask-editor.ts
@@ -13,7 +13,7 @@ import { notesDataStore, tagsWritable, zoomResetValue } from "./store";
 import Toast from "./Toast.svelte";
 import { addShapesToCanvasFromCloze } from "./tools/add-from-cloze";
 import { enableSelectable, moveShapeToCanvasBoundaries } from "./tools/lib";
-import { undoRedoInit } from "./tools/tool-undo-redo";
+import { undoStack } from "./tools/tool-undo-redo";
 import type { Size } from "./types";
 
 export const setupMaskEditor = async (
@@ -34,6 +34,7 @@ export const setupMaskEditor = async (
         image.height = size.height;
         image.width = size.width;
         setCanvasZoomRatio(canvas, instance);
+        undoStack.reset();
     };
 
     return canvas;
@@ -75,6 +76,7 @@ export const setupMaskEditorForEdit = async (
         addShapesToCanvasFromCloze(canvas, clozeNote.occlusions);
         enableSelectable(canvas, true);
         addClozeNotesToTextEditor(clozeNote.header, clozeNote.backExtra, clozeNote.tags);
+        undoStack.reset();
         window.requestAnimationFrame(() => {
             image.style.visibility = "visible";
         });
@@ -87,11 +89,11 @@ function initCanvas(onChange: () => void): fabric.Canvas {
     const canvas = new fabric.Canvas("canvas");
     tagsWritable.set([]);
     globalThis.canvas = canvas;
+    undoStack.setCanvas(canvas);
     // enables uniform scaling by default without the need for the Shift key
     canvas.uniformScaling = false;
     canvas.uniScaleKey = "none";
     moveShapeToCanvasBoundaries(canvas);
-    undoRedoInit(canvas);
     canvas.on("object:modified", onChange);
     canvas.on("object:removed", onChange);
     return canvas;
diff --git a/ts/image-occlusion/tools/tool-ellipse.ts b/ts/image-occlusion/tools/tool-ellipse.ts
index ff5a4f94a..0edd3634a 100644
--- a/ts/image-occlusion/tools/tool-ellipse.ts
+++ b/ts/image-occlusion/tools/tool-ellipse.ts
@@ -4,9 +4,7 @@
 import { fabric } from "fabric";
 
 import { BORDER_COLOR, disableRotation, SHAPE_MASK_COLOR, stopDraw } from "./lib";
-import { objectAdded } from "./tool-undo-redo";
-
-const addedEllipseIds: string[] = [];
+import { undoStack } from "./tool-undo-redo";
 
 export const drawEllipse = (canvas: fabric.Canvas): void => {
     canvas.selectionColor = "rgba(0, 0, 0, 0)";
@@ -117,6 +115,6 @@ export const drawEllipse = (canvas: fabric.Canvas): void => {
         }
 
         ellipse.setCoords();
-        objectAdded(canvas, addedEllipseIds, ellipse.id);
+        undoStack.onObjectAdded(ellipse.id);
     });
 };
diff --git a/ts/image-occlusion/tools/tool-polygon.ts b/ts/image-occlusion/tools/tool-polygon.ts
index af058d5f7..dd32cf8c0 100644
--- a/ts/image-occlusion/tools/tool-polygon.ts
+++ b/ts/image-occlusion/tools/tool-polygon.ts
@@ -5,7 +5,7 @@ import { fabric } from "fabric";
 import type { PanZoom } from "panzoom";
 
 import { BORDER_COLOR, disableRotation, SHAPE_MASK_COLOR } from "./lib";
-import { objectAdded, saveCanvasState } from "./tool-undo-redo";
+import { undoStack } from "./tool-undo-redo";
 
 let activeLine;
 let activeShape;
@@ -13,7 +13,6 @@ let linesList: fabric.Line = [];
 let pointsList: fabric.Circle = [];
 let drawMode = false;
 let zoomValue = 1;
-const addedPolygonIds: string[] = [];
 
 export const drawPolygon = (canvas: fabric.Canvas, panzoom: PanZoom): void => {
     canvas.selectionColor = "rgba(0, 0, 0, 0)";
@@ -190,12 +189,12 @@ const generatePolygon = (canvas: fabric.Canvas, pointsList): void => {
         disableRotation(polygon);
         canvas.add(polygon);
         // view undo redo tools
-        objectAdded(canvas, addedPolygonIds, polygon.id);
+        undoStack.onObjectAdded(polygon.id);
     }
 
     polygon.on("modified", () => {
         modifiedPolygon(canvas, polygon);
-        saveCanvasState(canvas);
+        undoStack.onObjectModified();
     });
 
     toggleDrawPolygon(canvas);
@@ -223,7 +222,7 @@ const modifiedPolygon = (canvas: fabric.Canvas, polygon: fabric.Polygon): void =
 
     polygon1.on("modified", () => {
         modifiedPolygon(canvas, polygon1);
-        saveCanvasState(canvas);
+        undoStack.onObjectModified();
     });
 
     canvas.remove(polygon);
diff --git a/ts/image-occlusion/tools/tool-rect.ts b/ts/image-occlusion/tools/tool-rect.ts
index fc63d103b..b3ccc85b3 100644
--- a/ts/image-occlusion/tools/tool-rect.ts
+++ b/ts/image-occlusion/tools/tool-rect.ts
@@ -4,9 +4,7 @@
 import { fabric } from "fabric";
 
 import { BORDER_COLOR, disableRotation, SHAPE_MASK_COLOR, stopDraw } from "./lib";
-import { objectAdded } from "./tool-undo-redo";
-
-const addedRectangleIds: string[] = [];
+import { undoStack } from "./tool-undo-redo";
 
 export const drawRectangle = (canvas: fabric.Canvas): void => {
     canvas.selectionColor = "rgba(0, 0, 0, 0)";
@@ -111,6 +109,6 @@ export const drawRectangle = (canvas: fabric.Canvas): void => {
         }
 
         rect.setCoords();
-        objectAdded(canvas, addedRectangleIds, rect.id);
+        undoStack.onObjectAdded(rect.id);
     });
 };
diff --git a/ts/image-occlusion/tools/tool-undo-redo.ts b/ts/image-occlusion/tools/tool-undo-redo.ts
index 78176e5bd..322c70abe 100644
--- a/ts/image-occlusion/tools/tool-undo-redo.ts
+++ b/ts/image-occlusion/tools/tool-undo-redo.ts
@@ -2,89 +2,137 @@
 // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
 import type fabric from "fabric";
+import { writable } from "svelte/store";
 
 import { mdiRedo, mdiUndo } from "../icons";
+import { emitChangeSignal } from "../MaskEditor.svelte";
 
 /**
  * Undo redo for rectangle and ellipse handled here,
  * view tool-polygon for handling undo redo in case of polygon
  */
 
-let lockHistory = false;
-const undoHistory: string[] = [];
-const redoHistory: string[] = [];
+type UndoState = {
+    undoable: boolean;
+    redoable: boolean;
+};
 
 const shapeType = ["rect", "ellipse"];
 
-export const undoRedoInit = (canvas: fabric.Canvas): void => {
-    undoHistory.push(JSON.stringify(canvas));
-
-    canvas.on("object:modified", function(o) {
-        if (lockHistory) return;
-        if (!validShape(o.target as fabric.Object)) return;
-        saveCanvasState(canvas);
-    });
-
-    canvas.on("object:removed", function(o) {
-        if (lockHistory) return;
-        if (!validShape(o.target as fabric.Object)) return;
-        saveCanvasState(canvas);
-    });
-};
-
 const validShape = (shape: fabric.Object): boolean => {
     if (shape.width <= 5 || shape.height <= 5) return false;
     if (shapeType.indexOf(shape.type) === -1) return false;
     return true;
 };
 
-export const undoAction = (canvas: fabric.Canvas): void => {
-    if (undoHistory.length > 0) {
-        lockHistory = true;
-        if (undoHistory.length > 1) redoHistory.push(undoHistory.pop() as string);
-        const content = undoHistory[undoHistory.length - 1];
-        canvas.loadFromJSON(content, function() {
-            canvas.renderAll();
-            lockHistory = false;
+class UndoStack {
+    private stack: string[] = [];
+    private index = -1;
+    private canvas: fabric.Canvas | undefined;
+    private locked = false;
+    private shapeIds = new Set<string>();
+    /** used to make the toolbar buttons reactive */
+    private state = writable<UndoState>({ undoable: false, redoable: false });
+    subscribe: typeof this.state.subscribe;
+
+    constructor() {
+        // allows an instance of the class to act as a store
+        this.subscribe = this.state.subscribe;
+    }
+
+    setCanvas(canvas: fabric.Canvas): void {
+        this.canvas = canvas;
+        this.canvas.on("object:modified", (opts) => this.maybePush(opts));
+        this.canvas.on("object:removed", (opts) => this.maybePush(opts));
+    }
+
+    reset(): void {
+        this.shapeIds.clear();
+        this.stack.length = 0;
+        this.index = -1;
+        this.push();
+        this.updateState();
+    }
+
+    private canUndo(): boolean {
+        return this.index > 0;
+    }
+
+    private canRedo(): boolean {
+        return this.index < this.stack.length - 1;
+    }
+
+    private updateState(): void {
+        this.state.set({
+            undoable: this.canUndo(),
+            redoable: this.canRedo(),
         });
     }
-};
 
-export const redoAction = (canvas: fabric.Canvas): void => {
-    if (redoHistory.length > 0) {
-        lockHistory = true;
-        const content = redoHistory.pop() as string;
-        undoHistory.push(content);
-        canvas.loadFromJSON(content, function() {
-            canvas.renderAll();
-            lockHistory = false;
+    private updateCanvas(): void {
+        this.locked = true;
+        this.canvas?.loadFromJSON(this.stack[this.index], () => {
+            this.canvas?.renderAll();
+            emitChangeSignal();
+            this.locked = false;
         });
     }
-};
 
-export const objectAdded = (canvas: fabric.Canvas, shapeIdList: string[], shapeId: string): void => {
-    if (shapeIdList.includes(shapeId)) {
-        return;
+    onObjectAdded(id: string): void {
+        if (!this.shapeIds.has(id)) {
+            this.push();
+        }
+        this.shapeIds.add(id);
     }
 
-    shapeIdList.push(shapeId);
-    saveCanvasState(canvas);
-};
+    onObjectModified(): void {
+        this.push();
+    }
 
-export const saveCanvasState = (canvas: fabric.Canvas): void => {
-    undoHistory.push(JSON.stringify(canvas));
-    redoHistory.length = 0;
-};
+    private maybePush(opts): void {
+        if (
+            !this.locked
+            && validShape(opts.target as fabric.Object)
+        ) {
+            this.push();
+        }
+    }
+
+    private push(): void {
+        this.stack.length = this.index + 1;
+        this.stack.push(JSON.stringify(this.canvas));
+        this.index++;
+        this.updateState();
+    }
+
+    undo(): void {
+        if (this.canUndo()) {
+            this.index--;
+            this.updateState();
+            this.updateCanvas();
+        }
+    }
+
+    redo(): void {
+        if (this.canRedo()) {
+            this.index++;
+            this.updateState();
+            this.updateCanvas();
+        }
+    }
+}
+
+export const undoStack = new UndoStack();
 
 export const undoRedoTools = [
     {
         name: "undo",
         icon: mdiUndo,
-        action: undoAction,
+        action: () => undoStack.undo(),
     },
     {
         name: "redo",
         icon: mdiRedo,
-        action: redoAction,
+        action: () => undoStack.redo(),
     },
 ];