Merge pull request #15206 from calixteman/simplify_command_manager

[Editor] Simplify the command manager

src/display/editor/tools.js

@@ -54,21 +54,27 @@ class IdManager {
 class CommandManager {
   #commands = [];
 
-  #maxSize = 100;
+  #maxSize;
 
-  // When the position is NaN, it means the buffer is empty.
+  #position = -1;
-  #position = NaN;
 
-  #start = 0;
+  constructor(maxSize = 128) {
+    this.#maxSize = maxSize;
+  }
+
+  /**
+   * @typedef {Object} addOptions
+   * @property {function} cmd
+   * @property {function} undo
+   * @property {boolean} mustExec
+   * @property {number} type
+   * @property {boolean} overwriteIfSameType
+   * @property {boolean} keepUndo
+   */
 
   /**
    * Add a new couple of commands to be used in case of redo/undo.
-   * @param {function} cmd
+   * @param {addOptions} options
-   * @param {function} undo
-   * @param {boolean} mustExec
-   * @param {number} type
-   * @param {boolean} overwriteIfSameType
-   * @param {boolean} keepUndo
    */
   add({
     cmd,
@@ -78,12 +84,18 @@ class CommandManager {
     overwriteIfSameType = false,
     keepUndo = false,
   }) {
+    if (mustExec) {
+      cmd();
+    }
+
     const save = { cmd, undo, type };
-    if (
+    if (this.#position === -1) {
-      overwriteIfSameType &&
+      this.#position = 0;
-      !isNaN(this.#position) &&
+      this.#commands.push(save);
-      this.#commands[this.#position].type === type
+      return;
-    ) {
+    }
+
+    if (overwriteIfSameType && this.#commands[this.#position].type === type) {
       // For example when we change a color we don't want to
       // be able to undo all the steps, hence we only want to
       // keep the last undoable action in this sequence of actions.
@@ -91,62 +103,41 @@ class CommandManager {
         save.undo = this.#commands[this.#position].undo;
       }
       this.#commands[this.#position] = save;
-      if (mustExec) {
-        cmd();
-      }
       return;
     }
-    const next = (this.#position + 1) % this.#maxSize;
-    if (next !== this.#start) {
-      if (this.#start < next) {
-        this.#commands = this.#commands.slice(this.#start, next);
-      } else {
-        this.#commands = this.#commands
-          .slice(this.#start)
-          .concat(this.#commands.slice(0, next));
-      }
-      this.#start = 0;
-      this.#position = this.#commands.length - 1;
-    }
-    this.#setCommands(save);
 
-    if (mustExec) {
+    const next = this.#position + 1;
-      cmd();
+    if (next === this.#maxSize) {
+      this.#commands.splice(0, 1);
+    } else {
+      this.#position = next;
+      if (next < this.#commands.length) {
+        this.#commands.splice(next);
       }
     }
 
+    this.#commands.push(save);
+  }
+
   /**
    * Undo the last command.
    */
   undo() {
-    if (isNaN(this.#position)) {
+    if (this.#position === -1) {
       // Nothing to undo.
       return;
     }
     this.#commands[this.#position].undo();
-    if (this.#position === this.#start) {
+    this.#position -= 1;
-      this.#position = NaN;
-    } else {
-      this.#position = (this.#maxSize + this.#position - 1) % this.#maxSize;
-    }
   }
 
   /**
    * Redo the last command.
    */
   redo() {
-    if (isNaN(this.#position)) {
+    if (this.#position < this.#commands.length - 1) {
-      if (this.#start < this.#commands.length) {
+      this.#position += 1;
-        this.#commands[this.#start].cmd();
+      this.#commands[this.#position].cmd();
-        this.#position = this.#start;
-      }
-      return;
-    }
-
-    const next = (this.#position + 1) % this.#maxSize;
-    if (next !== this.#start && next < this.#commands.length) {
-      this.#commands[next].cmd();
-      this.#position = next;
     }
   }
 
@@ -155,7 +146,7 @@ class CommandManager {
    * @returns {boolean}
    */
   hasSomethingToUndo() {
-    return !isNaN(this.#position);
+    return this.#position !== -1;
   }
 
   /**
@@ -163,29 +154,7 @@ class CommandManager {
    * @returns {boolean}
    */
   hasSomethingToRedo() {
-    if (isNaN(this.#position) && this.#start < this.#commands.length) {
+    return this.#position < this.#commands.length - 1;
-      return true;
-    }
-    const next = (this.#position + 1) % this.#maxSize;
-    return next !== this.#start && next < this.#commands.length;
-  }
-
-  #setCommands(cmds) {
-    if (this.#commands.length < this.#maxSize) {
-      this.#commands.push(cmds);
-      this.#position = isNaN(this.#position) ? 0 : this.#position + 1;
-      return;
-    }
-
-    if (isNaN(this.#position)) {
-      this.#position = this.#start;
-    } else {
-      this.#position = (this.#position + 1) % this.#maxSize;
-      if (this.#position === this.#start) {
-        this.#start = (this.#start + 1) % this.#maxSize;
-      }
-    }
-    this.#commands[this.#position] = cmds;
   }
 
   destroy() {
@@ -969,4 +938,10 @@ class AnnotationEditorUIManager {
   }
 }
 
-export { AnnotationEditorUIManager, bindEvents, ColorManager, KeyboardManager };
+export {
+  AnnotationEditorUIManager,
+  bindEvents,
+  ColorManager,
+  CommandManager,
+  KeyboardManager,
+};

test/unit/editor_spec.js

@@ -13,9 +13,85 @@
  * limitations under the License.
  */
 
+import { CommandManager } from "../../src/display/editor/tools.js";
 import { fitCurve } from "../../src/display/editor/ink.js";
 
 describe("editor", function () {
+  describe("Command Manager", function () {
+    it("should check undo/redo", function () {
+      const manager = new CommandManager(4);
+      let x = 0;
+      const makeDoUndo = n => ({ cmd: () => (x += n), undo: () => (x -= n) });
+
+      manager.add({ ...makeDoUndo(1), mustExec: true });
+      expect(x).toEqual(1);
+
+      manager.add({ ...makeDoUndo(2), mustExec: true });
+      expect(x).toEqual(3);
+
+      manager.add({ ...makeDoUndo(3), mustExec: true });
+      expect(x).toEqual(6);
+
+      manager.undo();
+      expect(x).toEqual(3);
+
+      manager.undo();
+      expect(x).toEqual(1);
+
+      manager.undo();
+      expect(x).toEqual(0);
+
+      manager.undo();
+      expect(x).toEqual(0);
+
+      manager.redo();
+      expect(x).toEqual(1);
+
+      manager.redo();
+      expect(x).toEqual(3);
+
+      manager.redo();
+      expect(x).toEqual(6);
+
+      manager.redo();
+      expect(x).toEqual(6);
+
+      manager.undo();
+      expect(x).toEqual(3);
+
+      manager.redo();
+      expect(x).toEqual(6);
+    });
+  });
+
+  it("should hit the limit of the manager", function () {
+    const manager = new CommandManager(3);
+    let x = 0;
+    const makeDoUndo = n => ({ cmd: () => (x += n), undo: () => (x -= n) });
+
+    manager.add({ ...makeDoUndo(1), mustExec: true }); // 1
+    manager.add({ ...makeDoUndo(2), mustExec: true }); // 3
+    manager.add({ ...makeDoUndo(3), mustExec: true }); // 6
+    manager.add({ ...makeDoUndo(4), mustExec: true }); // 10
+    expect(x).toEqual(10);
+
+    manager.undo();
+    manager.undo();
+    expect(x).toEqual(3);
+
+    manager.undo();
+    expect(x).toEqual(1);
+
+    manager.undo();
+    expect(x).toEqual(1);
+
+    manager.redo();
+    manager.redo();
+    expect(x).toEqual(6);
+    manager.add({ ...makeDoUndo(5), mustExec: true });
+    expect(x).toEqual(11);
+  });
+
   describe("fitCurve", function () {
     it("should return a function", function () {
       expect(typeof fitCurve).toEqual("function");