Skip to content

Fix undo/redo bug #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix undo/redo bug #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b432b9c
fix:improve history behavior
heruiwoniou Sep 9, 2024
f22a27d
fix:fixed cannot undo to empty state bug
heruiwoniou Sep 17, 2024
69e0b1b
fix:improve loadPath function
heruiwoniou Sep 17, 2024
1d6f383
fix timing issue with setting history
ecfairle Apr 6, 2025
03637e4
add test for undo after clear
ecfairle Apr 6, 2025
bfe5994
Fix undo behavior with loadPaths and add tests
ecfairle Apr 20, 2025
efc3b2c
Fix reset canvas history logic
ecfairle Apr 20, 2025
24515b8
Use event queue for undo/redo/etc.
ecfairle May 11, 2025
8a85d56
Fix order of operations
ecfairle May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 88 additions & 27 deletions packages/react-sketch-canvas/src/ReactSketchCanvas/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from "react";
import {useCallback} from "react";
import { Canvas } from "../Canvas";
import {
CanvasPath,
Expand All @@ -9,6 +10,11 @@ import {
import { CanvasRef } from "../Canvas/types";
import { ReactSketchCanvasProps, ReactSketchCanvasRef } from "./types";

type Operation = {
type: 'undo' | 'redo' | 'clear' | 'loadPaths';
payload?: CanvasPath[];
};

/**
* ReactSketchCanvas is a wrapper around Canvas component to provide a controlled way to manage the canvas paths.
* It provides a set of methods to manage the canvas paths, undo, redo, clear and reset the canvas.
Expand Down Expand Up @@ -50,9 +56,19 @@ export const ReactSketchCanvas = React.forwardRef<
const svgCanvas = React.createRef<CanvasRef>();
const [drawMode, setDrawMode] = React.useState<boolean>(true);
const [isDrawing, setIsDrawing] = React.useState<boolean>(false);
const [resetStack, setResetStack] = React.useState<CanvasPath[]>([]);
const [undoStack, setUndoStack] = React.useState<CanvasPath[]>([]);
const [history, setHistory] = React.useState<CanvasPath[][]>([[]]);
const [historyPos, setHistoryPos] = React.useState<number>(0);
const [historySynced, setHistorySynced] = React.useState<boolean>(false);
const [currentPaths, setCurrentPaths] = React.useState<CanvasPath[]>([]);
const [operationQueue, setOperationQueue] = React.useState<Operation[]>([]);
const [isProcessingQueue, setIsProcessingQueue] = React.useState(false);

const addLastStroke = useCallback(():void => {
if (!historySynced) {
setHistory(his => [...his.slice(0, historyPos), [...currentPaths]]);
setHistorySynced(true);
}
}, [currentPaths, historyPos, historySynced]);

const liftStrokeUp = React.useCallback((): void => {
const lastStroke = currentPaths.slice(-1)?.[0] ?? null;
Expand All @@ -75,32 +91,75 @@ export const ReactSketchCanvas = React.forwardRef<
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [currentPaths]);

const processQueue = React.useCallback(async () => {
if (isProcessingQueue || operationQueue.length === 0) return;

setIsProcessingQueue(true);
const operation = operationQueue[0];

try {
switch (operation.type) {
case 'undo':
if (historyPos > 0) {
addLastStroke();
setCurrentPaths(history[historyPos - 1]);
setHistoryPos(pos => pos - 1);
}
break;
case 'redo':
if (historyPos < history.length - 1) {
addLastStroke();
setCurrentPaths(history[historyPos + 1]);
setHistoryPos(pos => pos + 1);
}
break;
case 'clear':
addLastStroke();
setCurrentPaths([]);
setHistory(his => [...his.slice(0, historyPos + 1), []]);
setHistoryPos(pos => pos + 1);
break;
case 'loadPaths':
if (operation.payload) {
addLastStroke();
setCurrentPaths((paths) => {
const newPaths = [...paths, ...operation.payload!];
setHistory(his => {
const newHistoryPos = historyPos + 1;
setHistoryPos(newHistoryPos);
return [...his.slice(0, newHistoryPos), newPaths];
});
return newPaths;
});
}
break;
}
} finally {
setOperationQueue(queue => queue.slice(1));
setIsProcessingQueue(false);
}
}, [operationQueue, isProcessingQueue, historyPos, history, currentPaths, addLastStroke]);

React.useEffect(() => {
processQueue();
}, [processQueue, operationQueue]);

const enqueueOperation = useCallback((operation: Operation) => {
setOperationQueue(queue => [...queue, operation]);
}, []);

React.useImperativeHandle(ref, () => ({
eraseMode: (erase: boolean): void => {
setDrawMode(!erase);
},
clearCanvas: (): void => {
setResetStack([...currentPaths]);
setCurrentPaths([]);
enqueueOperation({ type: 'clear' });
},
undo: (): void => {
// If there was a last reset then
if (resetStack.length !== 0) {
setCurrentPaths([...resetStack]);
setResetStack([]);

return;
}

setUndoStack((paths) => [...paths, ...currentPaths.slice(-1)]);
setCurrentPaths((paths) => paths.slice(0, -1));
enqueueOperation({ type: 'undo' });
},
redo: (): void => {
// Nothing to Redo
if (undoStack.length === 0) return;

setCurrentPaths((paths) => [...paths, ...undoStack.slice(-1)]);
setUndoStack((paths) => paths.slice(0, -1));
enqueueOperation({ type: 'redo' });
},
exportImage: (
imageType: ExportImageType,
Expand Down Expand Up @@ -139,7 +198,7 @@ export const ReactSketchCanvas = React.forwardRef<
}
}),
loadPaths: (paths: CanvasPath[]): void => {
setCurrentPaths((path) => [...path, ...paths]);
enqueueOperation({ type: 'loadPaths', payload: paths });
},
getSketchingTime: (): Promise<number> =>
new Promise<number>((resolve, reject) => {
Expand All @@ -164,15 +223,15 @@ export const ReactSketchCanvas = React.forwardRef<
}
}),
resetCanvas: (): void => {
setResetStack([]);
setUndoStack([]);
setHistory([]);
setHistoryPos(0);
setCurrentPaths([]);
setOperationQueue([]);
},
}));
}), [currentPaths, history, historyPos, svgCanvas, withTimestamp, addLastStroke, enqueueOperation]);

const handlePointerDown = (point: Point, isEraser = false): void => {
setIsDrawing(true);
setUndoStack([]);

const isDraw = !isEraser && drawMode;

Expand All @@ -190,7 +249,9 @@ export const ReactSketchCanvas = React.forwardRef<
endTimestamp: 0,
};
}

addLastStroke();
setHistoryPos(pos => pos + 1);
setHistorySynced(false);
setCurrentPaths((paths) => [...paths, stroke]);
};

Expand All @@ -210,14 +271,14 @@ export const ReactSketchCanvas = React.forwardRef<
return;
}

const currentStroke = currentPaths.slice(-1)?.[0] ?? null;

setIsDrawing(false);

if (!withTimestamp) {
return;
}

const currentStroke = currentPaths.slice(-1)?.[0] ?? null;

if (currentStroke === null) {
return;
}
Expand Down
145 changes: 145 additions & 0 deletions packages/tests/src/actions/undoRedo.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { expect, test } from "@playwright/experimental-ct-react";

import { drawEraserLine, drawLine, getCanvasIds } from "../commands";
import { WithUndoRedoButtons } from "../stories/WithUndoRedoButtons";
import penStrokes from "../fixtures/penStroke.json";

test.use({ viewport: { width: 500, height: 500 } });

Expand All @@ -10,6 +11,7 @@ const undoButtonId = "undo-button";
const redoButtonId = "redo-button";
const clearCanvasButtonId = "clear-canvas-button";
const resetCanvasButtonId = "reset-canvas-button";
const loadPathsButtonId = "load-paths-button";

const { firstStrokeGroupId, eraserStrokeGroupId } = getCanvasIds(canvasId);

Expand All @@ -22,6 +24,7 @@ test.describe("undo", () => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down Expand Up @@ -53,6 +56,7 @@ test.describe("undo", () => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down Expand Up @@ -100,6 +104,7 @@ test.describe("redo", () => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down Expand Up @@ -137,6 +142,7 @@ test.describe("redo", () => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down Expand Up @@ -193,6 +199,7 @@ test("should still keep the stack on clearCanvas", async ({ mount }) => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down Expand Up @@ -250,6 +257,143 @@ test("should still keep the stack on clearCanvas", async ({ mount }) => {
).toHaveCount(1);
});

test("should undo a stroke after clear canvas", async ({ mount }) => {
const component = await mount(
<WithUndoRedoButtons
id={canvasId}
undoButtonId={undoButtonId}
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

const canvas = component.locator(`#${canvasId}`);
const undoButton = component.locator(`#${undoButtonId}`);
const clearCanvasButton = component.locator(`#${clearCanvasButtonId}`);

await drawLine(canvas, {
length: 50,
originX: 0,
originY: 10,
});

await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(1);

// Clear 1 stroke
await clearCanvasButton.click();
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(0);

await drawLine(canvas, {
length: 50,
originX: 10,
originY: 10,
});
await drawLine(canvas, {
length: 50,
originX: 20,
originY: 10,
});
await drawLine(canvas, {
length: 50,
originX: 30,
originY: 10,
});

// Undo 1 of 3 new strokes => 2 left
await undoButton.click();
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(2);
});

test("should undo loaded paths", async ({ mount }) => {
const component = await mount(
<WithUndoRedoButtons
id={canvasId}
undoButtonId={undoButtonId}
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
loadPathsButtonId={loadPathsButtonId}
paths={penStrokes}
/>,
);

const canvas = component.locator(`#${canvasId}`);
const undoButton = component.locator(`#${undoButtonId}`);
const loadPathsButton = component.locator(`#${loadPathsButtonId}`);

await drawLine(canvas, {
length: 50,
originX: 0,
originY: 10,
});

await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(1);

await loadPathsButton.click();

// Load 1 stroke + 1 existing = 2
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(2);

// Undo load action should reset to 1 original stroke
await undoButton.click();
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(1);
});

test("should undo draw after load", async ({ mount }) => {
const component = await mount(
<WithUndoRedoButtons
id={canvasId}
undoButtonId={undoButtonId}
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
loadPathsButtonId={loadPathsButtonId}
paths={penStrokes}
/>,
);

const canvas = component.locator(`#${canvasId}`);
const undoButton = component.locator(`#${undoButtonId}`);
const loadPathsButton = component.locator(`#${loadPathsButtonId}`);
await loadPathsButton.click();

// Load 1 stroke + 1 existing = 2
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(1);

await drawLine(canvas, {
length: 50,
originX: 0,
originY: 10,
});

// Load 1 stroke + 1 new stroke = 2
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(2);

// Undo => 2 total - 1 last stroke = 1
await undoButton.click();
await expect(
component.locator(firstStrokeGroupId).locator("path"),
).toHaveCount(1);
});

test("should clear the stack on resetCanvas", async ({ mount }) => {
const component = await mount(
<WithUndoRedoButtons
Expand All @@ -258,6 +402,7 @@ test("should clear the stack on resetCanvas", async ({ mount }) => {
redoButtonId={redoButtonId}
clearCanvasButtonId={clearCanvasButtonId}
resetCanvasButtonId={resetCanvasButtonId}
paths={penStrokes}
/>,
);

Expand Down
Loading