Fix media editor undo state bugs

ts/components/MediaEditor.tsx

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 
 import Measure from 'react-measure';
-import React, { useEffect, useRef, useState } from 'react';
+import React, { useEffect, useState } from 'react';
 import classNames from 'classnames';
 import { createPortal } from 'react-dom';
 import { fabric } from 'fabric';
@@ -27,6 +27,7 @@ import { MediaEditorFabricPencilBrush } from '../mediaEditor/MediaEditorFabricPe
 import { MediaEditorFabricCropRect } from '../mediaEditor/MediaEditorFabricCropRect';
 import { MediaEditorFabricIText } from '../mediaEditor/MediaEditorFabricIText';
 import { MediaEditorFabricSticker } from '../mediaEditor/MediaEditorFabricSticker';
+import { fabricEffectListener } from '../mediaEditor/fabricEffectListener';
 import { getRGBA, getHSL } from '../mediaEditor/util/color';
 import {
   TextStyle,
@@ -40,6 +41,16 @@ export type PropsType = {
   onDone: (data: Uint8Array) => unknown;
 } & Pick<StickerButtonProps, 'installedPacks' | 'recentStickers'>;
 
+const INITIAL_IMAGE_STATE: ImageStateType = {
+  angle: 0,
+  cropX: 0,
+  cropY: 0,
+  flipX: false,
+  flipY: false,
+  height: 0,
+  width: 0,
+};
+
 enum EditMode {
   Crop = 'Crop',
   Draw = 'Draw',
@@ -85,19 +96,18 @@ export const MediaEditor = ({
   const [fabricCanvas, setFabricCanvas] = useState<fabric.Canvas | undefined>();
   const [image, setImage] = useState<HTMLImageElement>(new Image());
 
-  const isRestoringImageState = useRef(false);
-
   const canvasId = useUniqueId();
 
-  const [imageState, setImageState] = useState<ImageStateType>({
+  const [imageState, setImageState] =
-    angle: 0,
+    useState<ImageStateType>(INITIAL_IMAGE_STATE);
-    cropX: 0,
+
-    cropY: 0,
+  // History state
-    flipX: false,
+  const { canRedo, canUndo, redoIfPossible, takeSnapshot, undoIfPossible } =
-    flipY: false,
+    useFabricHistory({
-    height: image.height,
+      fabricCanvas,
-    width: image.width,
+      imageState,
-  });
+      setImageState,
+    });
 
   // Initial image load and Fabric canvas setup
   useEffect(() => {
@@ -114,11 +124,14 @@ export const MediaEditor = ({
       const canvas = new fabric.Canvas(canvasId);
       canvas.selection = false;
       setFabricCanvas(canvas);
-      setImageState(curr => ({
+
-        ...curr,
+      const newImageState = {
+        ...INITIAL_IMAGE_STATE,
         height: img.height,
         width: img.width,
-      }));
+      };
+      setImageState(newImageState);
+      takeSnapshot('initial state', newImageState, canvas);
     };
     img.onerror = () => {
       // This is a bad experience, but it should be impossible.
@@ -130,9 +143,7 @@ export const MediaEditor = ({
       img.onload = noop;
       img.onerror = noop;
     };
-  }, [canvasId, fabricCanvas, imageSrc, onClose]);
+  }, [canvasId, fabricCanvas, imageSrc, onClose, takeSnapshot]);
-
-  const history = useFabricHistory(fabricCanvas);
 
   // Keyboard support
   useEffect(() => {
@@ -155,22 +166,8 @@ export const MediaEditor = ({
         ev => isCmdOrCtrl(ev) && ev.key === 't',
         () => setEditMode(EditMode.Text),
       ],
-      [
+      [ev => isCmdOrCtrl(ev) && ev.key === 'z', undoIfPossible],
-        ev => isCmdOrCtrl(ev) && ev.key === 'z',
+      [ev => isCmdOrCtrl(ev) && ev.shiftKey && ev.key === 'z', redoIfPossible],
-        () => {
-          if (history?.canUndo()) {
-            history?.undo();
-          }
-        },
-      ],
-      [
-        ev => isCmdOrCtrl(ev) && ev.shiftKey && ev.key === 'z',
-        () => {
-          if (history?.canRedo()) {
-            history?.redo();
-          }
-        },
-      ],
       [
         ev => ev.key === 'Escape',
         () => {
@@ -308,20 +305,7 @@ export const MediaEditor = ({
     return () => {
       document.removeEventListener('keydown', handleKeydown);
     };
-  }, [fabricCanvas, history]);
+  }, [fabricCanvas, redoIfPossible, undoIfPossible]);
-
-  // Take a snapshot of history whenever imageState changes
-  useEffect(() => {
-    if (
-      !imageState.height ||
-      !imageState.width ||
-      isRestoringImageState.current
-    ) {
-      isRestoringImageState.current = false;
-      return;
-    }
-    history?.takeSnapshot(imageState);
-  }, [history, imageState]);
 
   const [containerWidth, setContainerWidth] = useState(0);
   const [containerHeight, setContainerHeight] = useState(0);
@@ -359,8 +343,6 @@ export const MediaEditor = ({
     drawFabricBackgroundImage({ fabricCanvas, image, imageState });
   }, [fabricCanvas, image, imageState]);
 
-  const [canRedo, setCanRedo] = useState(false);
-  const [canUndo, setCanUndo] = useState(false);
   const [canCrop, setCanCrop] = useState(false);
   const [cropAspectRatioLock, setCropAspectRatioLock] = useState(false);
   const [drawTool, setDrawTool] = useState<DrawTool>(DrawTool.Pen);
@@ -369,64 +351,22 @@ export const MediaEditor = ({
   const [sliderValue, setSliderValue] = useState<number>(0);
   const [textStyle, setTextStyle] = useState<TextStyle>(TextStyle.Regular);
 
-  // Check if we can undo/redo & restore the image state on undo/undo
-  useEffect(() => {
-    if (!history) {
-      return;
-    }
-
-    function refreshUndoState() {
-      if (!history) {
-        return;
-      }
-
-      setCanUndo(history.canUndo());
-      setCanRedo(history.canRedo());
-    }
-
-    function restoreImageState(prevImageState: ImageStateType) {
-      isRestoringImageState.current = true;
-      setImageState(curr => ({ ...curr, ...prevImageState }));
-    }
-
-    function takeSnapshot() {
-      history?.takeSnapshot({ ...imageState });
-    }
-
-    history.on('appliedState', restoreImageState);
-    history.on('historyChanged', refreshUndoState);
-    history.on('pleaseTakeSnapshot', takeSnapshot);
-
-    return () => {
-      history.off('appliedState', restoreImageState);
-      history.off('historyChanged', refreshUndoState);
-      history.off('pleaseTakeSnapshot', takeSnapshot);
-    };
-  }, [history, imageState]);
-
   // If you select a text path auto enter edit mode
   useEffect(() => {
     if (!fabricCanvas) {
       return;
     }
-
+    return fabricEffectListener(
-    function updateEditMode() {
+      fabricCanvas,
-      if (fabricCanvas?.getActiveObject() instanceof MediaEditorFabricIText) {
+      ['selection:created', 'selection:updated', 'selection:cleared'],
-        setEditMode(EditMode.Text);
+      () => {
-      } else if (editMode === EditMode.Text) {
+        if (fabricCanvas?.getActiveObject() instanceof MediaEditorFabricIText) {
-        setEditMode(undefined);
+          setEditMode(EditMode.Text);
+        } else if (editMode === EditMode.Text) {
+          setEditMode(undefined);
+        }
       }
-    }
+    );
-
-    fabricCanvas.on('selection:created', updateEditMode);
-    fabricCanvas.on('selection:updated', updateEditMode);
-    fabricCanvas.on('selection:cleared', updateEditMode);
-
-    return () => {
-      fabricCanvas.off('selection:created', updateEditMode);
-      fabricCanvas.off('selection:updated', updateEditMode);
-      fabricCanvas.off('selection:cleared', updateEditMode);
-    };
   }, [editMode, fabricCanvas]);
 
   // Ensure scaling is in locked|unlocked state only when cropping
@@ -769,15 +709,13 @@ export const MediaEditor = ({
               return;
             }
 
-            setImageState({
+            const newImageState = {
-              angle: 0,
+              ...INITIAL_IMAGE_STATE,
-              cropX: 0,
-              cropY: 0,
-              flipX: false,
-              flipY: false,
               height: image.height,
               width: image.width,
-            });
+            };
+            setImageState(newImageState);
+            takeSnapshot('reset', newImageState);
           }}
           type="button"
         >
@@ -808,12 +746,14 @@ export const MediaEditor = ({
               obj.setCoords();
             });
 
-            setImageState(curr => ({
+            const newImageState = {
-              ...curr,
+              ...imageState,
-              angle: (curr.angle + 270) % 360,
+              angle: (imageState.angle + 270) % 360,
-              height: curr.width,
+              height: imageState.width,
-              width: curr.height,
+              width: imageState.height,
-            }));
+            };
+            setImageState(newImageState);
+            takeSnapshot('rotate', newImageState);
           }}
           type="button"
         />
@@ -825,12 +765,14 @@ export const MediaEditor = ({
               return;
             }
 
-            setImageState(curr => ({
+            const newImageState = {
-              ...curr,
+              ...imageState,
-              ...(curr.angle % 180
+              ...(imageState.angle % 180
-                ? { flipY: !curr.flipY }
+                ? { flipY: !imageState.flipY }
-                : { flipX: !curr.flipX }),
+                : { flipX: !imageState.flipX }),
-            }));
+            };
+            setImageState(newImageState);
+            takeSnapshot('flip', newImageState);
           }}
           type="button"
         />
@@ -863,8 +805,13 @@ export const MediaEditor = ({
               return;
             }
 
-            setImageState(curr => getNewImageStateFromCrop(curr, pendingCrop));
+            const newImageState = getNewImageStateFromCrop(
+              imageState,
+              pendingCrop
+            );
+            setImageState(newImageState);
             moveFabricObjectsForCrop(fabricCanvas, pendingCrop);
+            takeSnapshot('crop', newImageState);
             setEditMode(undefined);
           }}
           type="button"
@@ -1031,7 +978,7 @@ export const MediaEditor = ({
                 if (editMode === EditMode.Crop) {
                   setEditMode(undefined);
                 }
-                history?.undo();
+                undoIfPossible();
               }}
               type="button"
             />
@@ -1043,7 +990,7 @@ export const MediaEditor = ({
                 if (editMode === EditMode.Crop) {
                   setEditMode(undefined);
                 }
-                history?.redo();
+                redoIfPossible();
               }}
               type="button"
             />

ts/mediaEditor/fabricEffectListener.ts

@@ -0,0 +1,23 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import type { fabric } from 'fabric';
+
+/**
+ * A helper for setting Fabric events inside of React `useEffect`s.
+ */
+export function fabricEffectListener(
+  target: fabric.IObservable<unknown>,
+  eventNames: ReadonlyArray<string>,
+  handler: (event: fabric.IEvent) => unknown
+): () => void {
+  for (const eventName of eventNames) {
+    target.on(eventName, handler);
+  }
+
+  return () => {
+    for (const eventName of eventNames) {
+      target.off(eventName, handler);
+    }
+  };
+}

ts/mediaEditor/useFabricHistory.ts

@@ -1,152 +1,221 @@
 // Copyright 2021 Signal Messenger, LLC
 // SPDX-License-Identifier: AGPL-3.0-only
 
-import { useEffect, useState } from 'react';
+import { useCallback, useEffect, useState } from 'react';
 import { fabric } from 'fabric';
-import EventEmitter from 'events';
+
+import * as log from '../logging/log';
 
 import type { ImageStateType } from './ImageStateType';
 import { MediaEditorFabricIText } from './MediaEditorFabricIText';
 import { MediaEditorFabricPath } from './MediaEditorFabricPath';
 import { MediaEditorFabricSticker } from './MediaEditorFabricSticker';
-
+import { fabricEffectListener } from './fabricEffectListener';
-export function useFabricHistory(
+import { strictAssert } from '../util/assert';
-  canvas: fabric.Canvas | undefined
-): FabricHistory | undefined {
-  const [history, setHistory] = useState<FabricHistory | undefined>();
-
-  // We need this type of precision so that when serializing/deserializing
-  // the floats don't get rounded off and we maintain proper image state.
-  // http://fabricjs.com/fabric-gotchas
-  fabric.Object.NUM_FRACTION_DIGITS = 16;
-
-  // Attach our custom classes to the global Fabric instance. Unfortunately, Fabric
-  //   doesn't make it easy to deserialize into a custom class without polluting the
-  //   global namespace. See <http://fabricjs.com/fabric-intro-part-3#subclassing>.
-  Object.assign(fabric, {
-    MediaEditorFabricIText,
-    MediaEditorFabricPath,
-    MediaEditorFabricSticker,
-  });
-
-  useEffect(() => {
-    if (canvas) {
-      const fabricHistory = new FabricHistory(canvas);
-      setHistory(fabricHistory);
-    }
-  }, [canvas]);
-
-  return history;
-}
-
-const LIMIT = 1000;
 
 type SnapshotStateType = {
   canvasState: string;
   imageState: ImageStateType;
 };
 
-export class FabricHistory extends EventEmitter {
+const SNAPSHOT_LIMIT = 1000;
-  private readonly canvas: fabric.Canvas;
 
-  private highWatermark: number;
+/**
-  private isTimeTraveling: boolean;
+ * A helper hook to manage `<MediaEditor>`'s undo/redo state.
-  private snapshots: Array<SnapshotStateType>;
+ *
+ * There are 3 pieces of state here:
+ *
+ * 1. `snapshots`, which include the "canvas state" (i.e., where all the objects are) and
+ *    the "image state" (i.e., the dimensions/angle of the image). Once the image has
+ *    loaded, this will always have a length of at least 1.
+ * 2. `highWatermark`, representing the snapshot that we *want* to be applied. If the
+ *    user never hits Undo, this will always be `snapshots.length`.
+ * 3. `appliedHighWatermark`, representing the snapshot that *is* applied. Because undo
+ *    and redo are asynchronous, this can lag behind `highWatermark`. The user is in the
+ *    middle of a "time travel" if `highWatermark !== appliedHighWatermark`.
+ *
+ * When the user performs a normal operation (such as adding an object or cropping), we
+ * add a new snapshot and update `highWatermark` and `appliedHighWatermark` all at once.
+ * We can do this because it's a synchronous operation.
+ *
+ * When the user performs an undo/redo, we immediately update `highWatermark`, then
+ * asynchronously perform the operation, then update `appliedHighWatermark`. You can't
+ * undo/redo if you're already time traveling to help avoid race conditions.
+ */
+export function useFabricHistory({
+  fabricCanvas,
+  imageState,
+  setImageState,
+}: {
+  fabricCanvas: fabric.Canvas | undefined;
+  imageState: Readonly<ImageStateType>;
+  setImageState: (_: ImageStateType) => unknown;
+}): {
+  canRedo: boolean;
+  canUndo: boolean;
+  redoIfPossible: () => void;
+  takeSnapshot: (
+    logMessage: string,
+    imageState: ImageStateType,
+    canvasOverride?: fabric.Canvas
+  ) => void;
+  undoIfPossible: () => void;
+} {
+  // These are all in one object, instead of three `useState` calls, because we often
+  //   need to update them all at once based on the previous state.
+  const [state, setState] = useState<
+    Readonly<{
+      snapshots: ReadonlyArray<SnapshotStateType>;
+      highWatermark: number;
+      appliedHighWatermark: number;
+    }>
+  >({
+    snapshots: [],
+    highWatermark: 0,
+    appliedHighWatermark: 0,
+  });
 
-  constructor(canvas: fabric.Canvas) {
+  const { highWatermark, snapshots } = state;
-    super();
+  const isTimeTraveling = getIsTimeTraveling(state);
+  const desiredSnapshot: undefined | SnapshotStateType =
+    snapshots[highWatermark - 1];
 
-    this.canvas = canvas;
+  const takeSnapshotInternal = useCallback((snapshot: SnapshotStateType) => {
-    this.highWatermark = 0;
+    setState(oldState => {
-    this.isTimeTraveling = false;
+      const newSnapshots = oldState.snapshots.slice(0, oldState.highWatermark);
-    this.snapshots = [];
+      newSnapshots.push(snapshot);
-
+      while (newSnapshots.length > SNAPSHOT_LIMIT) {
-    this.canvas.on('object:added', this.onObjectModified.bind(this));
+        newSnapshots.shift();
-    this.canvas.on('object:modified', this.onObjectModified.bind(this));
+      }
-    this.canvas.on('object:removed', this.onObjectModified.bind(this));
+      return {
-  }
+        snapshots: newSnapshots,
-
+        highWatermark: newSnapshots.length,
-  private applyState({ canvasState, imageState }: SnapshotStateType): void {
+        appliedHighWatermark: newSnapshots.length,
-    this.canvas.loadFromJSON(canvasState, () => {
+      };
-      this.emit('appliedState', imageState);
-      this.emit('historyChanged');
-      this.isTimeTraveling = false;
     });
-  }
+  }, []);
+  const takeSnapshot = useCallback(
+    (
+      logMessage: string,
+      newImageState: ImageStateType,
+      canvasOverride?: fabric.Canvas
+    ) => {
+      const canvas = canvasOverride || fabricCanvas;
+      strictAssert(
+        canvas,
+        'Media editor: tried to take a snapshot without a canvas'
+      );
+      log.info(
+        `Media editor: taking snapshot of image state from ${logMessage}`
+      );
+      takeSnapshotInternal({
+        canvasState: getCanvasState(canvas),
+        imageState: newImageState,
+      });
+    },
+    [fabricCanvas, takeSnapshotInternal]
+  );
+  const undoIfPossible = useCallback(() => {
+    log.info('Media editor: undoing');
+    setState(oldState =>
+      getIsTimeTraveling(oldState)
+        ? oldState
+        : {
+            ...oldState,
+            highWatermark: Math.max(oldState.highWatermark - 1, 1),
+          }
+    );
+  }, []);
+  const redoIfPossible = useCallback(() => {
+    log.info('Media editor: redoing');
+    setState(oldState =>
+      getIsTimeTraveling(oldState)
+        ? oldState
+        : {
+            ...oldState,
+            highWatermark: Math.min(
+              oldState.highWatermark + 1,
+              oldState.snapshots.length
+            ),
+          }
+    );
+  }, []);
 
-  private getState(): string {
+  // Global Fabric overrides
-    return JSON.stringify(this.canvas.toDatalessJSON());
+  useEffect(() => {
-  }
+    // We need this type of precision so that when serializing/deserializing
+    // the floats don't get rounded off and we maintain proper image state.
+    // http://fabricjs.com/fabric-gotchas
+    fabric.Object.NUM_FRACTION_DIGITS = 16;
 
-  private onObjectModified({ target }: fabric.IEvent): void {
+    // Attach our custom classes to the global Fabric instance. Unfortunately, Fabric
-    if (target?.excludeFromExport) {
+    //   doesn't make it easy to deserialize into a custom class without polluting the
+    //   global namespace. See <http://fabricjs.com/fabric-intro-part-3#subclassing>.
+    Object.assign(fabric, {
+      MediaEditorFabricIText,
+      MediaEditorFabricPath,
+      MediaEditorFabricSticker,
+    });
+  }, []);
+
+  // Moving between different snapshots
+  useEffect(() => {
+    if (!fabricCanvas || !isTimeTraveling || !desiredSnapshot) {
       return;
     }
+    log.info(`Media editor: time-traveling to snapshot ${highWatermark}`);
+    fabricCanvas.loadFromJSON(desiredSnapshot.canvasState, () => {
+      setImageState(desiredSnapshot.imageState);
+      setState(oldState => ({
+        ...oldState,
+        appliedHighWatermark: highWatermark,
+      }));
+    });
+  }, [
+    desiredSnapshot,
+    fabricCanvas,
+    highWatermark,
+    isTimeTraveling,
+    setImageState,
+  ]);
 
-    this.emit('pleaseTakeSnapshot');
+  // Taking snapshots when objects are added, modified, and removed
-  }
+  useEffect(() => {
-
+    if (!fabricCanvas || isTimeTraveling) {
-  private getUndoState(): SnapshotStateType | undefined {
-    if (!this.canUndo()) {
       return;
     }
+    return fabricEffectListener(
+      fabricCanvas,
+      ['object:added', 'object:modified', 'object:removed'],
+      ({ target }) => {
+        if (isTimeTraveling || target?.excludeFromExport) {
+          return;
+        }
+        log.info('Media editor: taking snapshot from object change');
+        takeSnapshotInternal({
+          canvasState: getCanvasState(fabricCanvas),
+          imageState,
+        });
+      }
+    );
+  }, [takeSnapshotInternal, fabricCanvas, isTimeTraveling, imageState]);
 
-    this.highWatermark -= 1;
+  return {
-    return this.snapshots[this.highWatermark];
+    canRedo: highWatermark < snapshots.length,
-  }
+    canUndo: highWatermark > 1,
-
+    redoIfPossible,
-  private getRedoState(): SnapshotStateType | undefined {
+    takeSnapshot,
-    if (this.canRedo()) {
+    undoIfPossible,
-      this.highWatermark += 1;
+  };
-    }
+}
-
+
-    return this.snapshots[this.highWatermark];
+function getCanvasState(fabricCanvas: fabric.Canvas): string {
-  }
+  return JSON.stringify(fabricCanvas.toDatalessJSON());
-
+}
-  public takeSnapshot(imageState: ImageStateType): void {
+
-    if (this.isTimeTraveling) {
+function getIsTimeTraveling({
-      return;
+  highWatermark,
-    }
+  appliedHighWatermark,
-
+}: Readonly<{ highWatermark: number; appliedHighWatermark: number }>): boolean {
-    if (this.canRedo()) {
+  return highWatermark !== appliedHighWatermark;
-      this.snapshots.splice(this.highWatermark + 1, this.snapshots.length);
-    }
-
-    this.snapshots.push({ canvasState: this.getState(), imageState });
-    if (this.snapshots.length > LIMIT) {
-      this.snapshots.shift();
-    }
-    this.highWatermark = this.snapshots.length - 1;
-    this.emit('historyChanged');
-  }
-
-  public undo(): void {
-    const undoState = this.getUndoState();
-
-    if (!undoState) {
-      return;
-    }
-
-    this.isTimeTraveling = true;
-    this.applyState(undoState);
-  }
-
-  public redo(): void {
-    const redoState = this.getRedoState();
-
-    if (!redoState) {
-      return;
-    }
-
-    this.isTimeTraveling = true;
-    this.applyState(redoState);
-  }
-
-  public canUndo(): boolean {
-    return this.highWatermark > 0;
-  }
-
-  public canRedo(): boolean {
-    return this.highWatermark < this.snapshots.length - 1;
-  }
 }

ts/util/lint/exceptions.json

@@ -7461,13 +7461,6 @@
     "updated": "2020-02-14T20:02:37.507Z",
     "reasonDetail": "Used only to set focus"
   },
-  {
-    "rule": "React-useRef",
-    "path": "ts/components/MediaEditor.tsx",
-    "line": "  const isRestoringImageState = useRef(false);",
-    "reasonCategory": "usageTrusted",
-    "updated": "2021-12-01T01:13:59.892Z"
-  },
   {
     "rule": "React-useRef",
     "path": "ts/components/Modal.tsx",