From cc81db0f9dedf41829ee72ad1c84450ea7ace552 Mon Sep 17 00:00:00 2001
From: Abdo <abdo@abdnh.net>
Date: Tue, 9 Jan 2024 04:19:46 +0300
Subject: [PATCH] Fix undo handling of group and some other IO tools (#2931)

* Fix undo handling of group and some other IO tools

* Emit change signal inside onObjectModified

* Fix group lost after moving group then undoing

* Skip undo entry if canvas has not changed

The onObjectModified() call I added in a previous commit to deleteDuplicateTools results in a duplicate undo entry for the delete tool. Checking for duplicate entries seems simpler than having to think about where onObjectModified() should be called exactly

* Fix extra undo entry added after ungroup
---
 ts/image-occlusion/Toolbar.svelte          |  4 +++-
 ts/image-occlusion/tools/lib.ts            |  1 +
 ts/image-occlusion/tools/tool-undo-redo.ts | 16 +++++++++++++---
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/ts/image-occlusion/Toolbar.svelte b/ts/image-occlusion/Toolbar.svelte
index 0827ca822..c0f0cb9fb 100644
--- a/ts/image-occlusion/Toolbar.svelte
+++ b/ts/image-occlusion/Toolbar.svelte
@@ -301,6 +301,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                     tooltip="{tool.tooltip()} ({getPlatformString(tool.shortcut)})"
                     on:click={() => {
                         tool.action(canvas);
+                        undoStack.onObjectModified();
                     }}
                 >
                     {@html tool.icon}
@@ -328,7 +329,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                     tooltip="{tool.tooltip()} ({getPlatformString(tool.shortcut)})"
                     on:click={() => {
                         tool.action(canvas);
-                        emitChangeSignal();
+                        undoStack.onObjectModified();
                     }}
                 >
                     {@html tool.icon}
@@ -368,6 +369,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                 )})"
                 on:click={() => {
                     alignTool.action(canvas);
+                    undoStack.onObjectModified();
                 }}
             >
                 {@html alignTool.icon}
diff --git a/ts/image-occlusion/tools/lib.ts b/ts/image-occlusion/tools/lib.ts
index 07cd8439a..4ac6c84fe 100644
--- a/ts/image-occlusion/tools/lib.ts
+++ b/ts/image-occlusion/tools/lib.ts
@@ -81,6 +81,7 @@ export const unGroupShapes = (canvas: fabric.Canvas): void => {
     const group = canvas.getActiveObject();
     const items = group.getObjects();
     group._restoreObjectsState();
+    group.destroyed = true;
     canvas.remove(group);
 
     items.forEach((item) => {
diff --git a/ts/image-occlusion/tools/tool-undo-redo.ts b/ts/image-occlusion/tools/tool-undo-redo.ts
index 20a07c76e..13af962f1 100644
--- a/ts/image-occlusion/tools/tool-undo-redo.ts
+++ b/ts/image-occlusion/tools/tool-undo-redo.ts
@@ -19,7 +19,7 @@ type UndoState = {
     redoable: boolean;
 };
 
-const shapeType = ["rect", "ellipse", "i-text"];
+const shapeType = ["rect", "ellipse", "i-text", "group"];
 
 const validShape = (shape: fabric.Object): boolean => {
     if (shape.width <= 5 || shape.height <= 5) {
@@ -49,7 +49,12 @@ class UndoStack {
     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));
+        this.canvas.on("object:removed", (opts) => {
+            // `destroyed` is a custom property set on groups in the ungrouping routine to avoid adding a spurious undo entry
+            if (!opts.target.group && !opts.target.destroyed) {
+                this.maybePush(opts);
+            }
+        });
     }
 
     reset(): void {
@@ -94,6 +99,7 @@ class UndoStack {
 
     onObjectModified(): void {
         this.push();
+        emitChangeSignal();
     }
 
     private maybePush(opts): void {
@@ -103,8 +109,12 @@ class UndoStack {
     }
 
     private push(): void {
+        const entry = JSON.stringify(this.canvas);
+        if (entry === this.stack[this.stack.length - 1]) {
+            return;
+        }
         this.stack.length = this.index + 1;
-        this.stack.push(JSON.stringify(this.canvas));
+        this.stack.push(entry);
         this.index++;
         this.updateState();
     }