From 3f8144ef85c4e3e16fdbd848796258bab9be639f Mon Sep 17 00:00:00 2001 From: Pete Hunt Date: Sat, 14 Mar 2020 20:46:57 -0700 Subject: [PATCH] Fix many syncing issues (#952) --- .gitignore | 2 + src/actions/actionCanvas.tsx | 7 +- src/actions/actionHistory.tsx | 40 ++++++++-- src/appState.ts | 1 - src/components/App.tsx | 135 +++++++++++++++++----------------- src/data/index.ts | 4 +- src/data/restore.ts | 3 +- src/element/index.ts | 28 +++++++ src/element/mutateElement.ts | 23 +++++- src/element/newElement.ts | 2 + src/history.ts | 5 ++ src/renderer/renderScene.ts | 4 +- src/scene/comparisons.ts | 6 ++ src/scene/selection.ts | 15 +--- src/types.ts | 1 - 15 files changed, 176 insertions(+), 100 deletions(-) diff --git a/.gitignore b/.gitignore index d52cc684..8a775d78 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,5 @@ static yarn-debug.log* yarn-error.log* yarn.lock +.envrc +firebase/ diff --git a/src/actions/actionCanvas.tsx b/src/actions/actionCanvas.tsx index c84e732e..b5e54df9 100644 --- a/src/actions/actionCanvas.tsx +++ b/src/actions/actionCanvas.tsx @@ -9,6 +9,7 @@ import { KEYS } from "../keys"; import { getShortcutKey } from "../utils"; import useIsMobile from "../is-mobile"; import { register } from "./register"; +import { newElementWith } from "../element/mutateElement"; export const actionChangeViewBackgroundColor = register({ name: "changeViewBackgroundColor", @@ -33,9 +34,11 @@ export const actionChangeViewBackgroundColor = register({ export const actionClearCanvas = register({ name: "clearCanvas", commitToHistory: () => true, - perform: () => { + perform: elements => { return { - elements: [], + elements: elements.map(element => + newElementWith(element, { isDeleted: true }), + ), appState: getDefaultAppState(), }; }, diff --git a/src/actions/actionHistory.tsx b/src/actions/actionHistory.tsx index df29310f..b5d818d6 100644 --- a/src/actions/actionHistory.tsx +++ b/src/actions/actionHistory.tsx @@ -7,8 +7,11 @@ import { SceneHistory } from "../history"; import { ExcalidrawElement } from "../element/types"; import { AppState } from "../types"; import { KEYS } from "../keys"; +import { getElementMap } from "../element"; +import { newElementWith } from "../element/mutateElement"; const writeData = ( + prevElements: readonly ExcalidrawElement[], appState: AppState, updater: () => { elements: ExcalidrawElement[]; appState: AppState } | null, ) => { @@ -19,13 +22,32 @@ const writeData = ( !appState.draggingElement ) { const data = updater(); + if (data === null) { + return {}; + } - return data === null - ? {} - : { - elements: data.elements, - appState: { ...appState, ...data.appState }, - }; + const prevElementMap = getElementMap(prevElements); + const nextElements = data.elements; + const nextElementMap = getElementMap(nextElements); + return { + elements: nextElements + .map(nextElement => + newElementWith( + prevElementMap[nextElement.id] || nextElement, + nextElement, + ), + ) + .concat( + prevElements + .filter( + prevElement => !nextElementMap.hasOwnProperty(prevElement.id), + ) + .map(prevElement => + newElementWith(prevElement, { isDeleted: true }), + ), + ), + appState: { ...appState, ...data.appState }, + }; } return {}; }; @@ -37,7 +59,8 @@ type ActionCreator = (history: SceneHistory) => Action; export const createUndoAction: ActionCreator = history => ({ name: "undo", - perform: (_, appState) => writeData(appState, () => history.undoOnce()), + perform: (elements, appState) => + writeData(elements, appState, () => history.undoOnce()), keyTest: testUndo(false), PanelComponent: ({ updateData }) => ( ({ export const createRedoAction: ActionCreator = history => ({ name: "redo", - perform: (_, appState) => writeData(appState, () => history.redoOnce()), + perform: (elements, appState) => + writeData(elements, appState, () => history.redoOnce()), keyTest: testUndo(true), PanelComponent: ({ updateData }) => ( { socketInitialized: boolean = false; // we don't want the socket to emit any updates until it is fully initalized roomID: string | null = null; roomKey: string | null = null; + lastBroadcastedOrReceivedSceneVersion: number = -1; actionManager: ActionManager; canvasOnlyActions = ["selectAll"]; @@ -275,15 +280,11 @@ export class App extends React.Component { iv, ); - let deletedIds = this.state.deletedIds; switch (decryptedData.type) { case "INVALID_RESPONSE": return; case "SCENE_UPDATE": - const { - elements: remoteElements, - appState: remoteAppState, - } = decryptedData.payload; + const { elements: remoteElements } = decryptedData.payload; const restoredState = restore(remoteElements || [], null, { scrollToContent: true, }); @@ -295,32 +296,7 @@ export class App extends React.Component { } else { // create a map of ids so we don't have to iterate // over the array more than once. - const localElementMap = elements.reduce( - ( - acc: { [key: string]: ExcalidrawElement }, - element: ExcalidrawElement, - ) => { - acc[element.id] = element; - return acc; - }, - {}, - ); - - deletedIds = { ...deletedIds }; - - for (const [id, remoteDeletedEl] of Object.entries( - remoteAppState.deletedIds, - )) { - if ( - !localElementMap[id] || - // don't remove local element if it's newer than the one - // deleted on remote - remoteDeletedEl.version >= localElementMap[id].version - ) { - deletedIds[id] = remoteDeletedEl; - delete localElementMap[id]; - } - } + const localElementMap = getElementMap(elements); // Reconcile elements = restoredState.elements @@ -342,17 +318,27 @@ export class App extends React.Component { ) { elements.push(localElementMap[element.id]); delete localElementMap[element.id]; - } else { - if (deletedIds.hasOwnProperty(element.id)) { - if (element.version > deletedIds[element.id].version) { - elements.push(element); - delete deletedIds[element.id]; - delete localElementMap[element.id]; - } + } else if ( + localElementMap.hasOwnProperty(element.id) && + localElementMap[element.id].version === element.version && + localElementMap[element.id].versionNonce !== + element.versionNonce + ) { + // resolve conflicting edits deterministically by taking the one with the lowest versionNonce + if ( + localElementMap[element.id].versionNonce < + element.versionNonce + ) { + elements.push(localElementMap[element.id]); } else { + // it should be highly unlikely that the two versionNonces are the same. if we are + // really worried about this, we can replace the versionNonce with the socket id. elements.push(element); - delete localElementMap[element.id]; } + delete localElementMap[element.id]; + } else { + elements.push(element); + delete localElementMap[element.id]; } return elements; @@ -360,9 +346,15 @@ export class App extends React.Component { // add local elements that weren't deleted or on remote .concat(...Object.values(localElementMap)); } - this.setState({ - deletedIds, - }); + this.lastBroadcastedOrReceivedSceneVersion = getDrawingVersion( + elements, + ); + // We haven't yet implemented multiplayer undo functionality, so we clear the undo stack + // when we receive any messages from another peer. This UX can be pretty rough -- if you + // undo, a user makes a change, and then try to redo, your element(s) will be lost. However, + // right now we think this is the right tradeoff. + history.clear(); + this.setState({}); if (this.socketInitialized === false) { this.socketInitialized = true; } @@ -370,13 +362,13 @@ export class App extends React.Component { case "MOUSE_LOCATION": const { socketID, pointerCoords } = decryptedData.payload; this.setState(state => { - if (state.collaborators.has(socketID)) { - const user = state.collaborators.get(socketID)!; - user.pointer = pointerCoords; - state.collaborators.set(socketID, user); - return state; + if (!state.collaborators.has(socketID)) { + state.collaborators.set(socketID, {}); } - return null; + const user = state.collaborators.get(socketID)!; + user.pointer = pointerCoords; + state.collaborators.set(socketID, user); + return state; }); break; } @@ -428,24 +420,16 @@ export class App extends React.Component { }; private broadcastSceneUpdate = () => { - const deletedIds = { ...this.state.deletedIds }; - const _elements = elements.filter(element => { - if (element.id in deletedIds) { - delete deletedIds[element.id]; - } - return element.id !== this.state.editingElement?.id; - }); const data: SocketUpdateDataSource["SCENE_UPDATE"] = { type: "SCENE_UPDATE", payload: { - elements: _elements, - appState: { - viewBackgroundColor: this.state.viewBackgroundColor, - name: this.state.name, - deletedIds, - }, + elements: getSyncableElements(elements), }, }; + this.lastBroadcastedOrReceivedSceneVersion = Math.max( + this.lastBroadcastedOrReceivedSceneVersion, + getDrawingVersion(elements), + ); return this._broadcastSocketData( data as typeof data & { _brand: "socketUpdateData" }, ); @@ -840,7 +824,7 @@ export class App extends React.Component { action: () => this.pasteFromClipboard(null), }, probablySupportsClipboardBlob && - elements.length > 0 && { + hasNonDeletedElements(elements) && { label: t("labels.copyAsPng"), action: this.copyToClipboardAsPng, }, @@ -1102,6 +1086,7 @@ export class App extends React.Component { const pnt = points[points.length - 1]; pnt[0] = x - originX; pnt[1] = y - originY; + mutateElement(multiElement); invalidateShapeForElement(multiElement); this.setState({}); return; @@ -1485,6 +1470,7 @@ export class App extends React.Component { }, })); multiElement.points.push([x - rx, y - ry]); + mutateElement(multiElement); invalidateShapeForElement(multiElement); } else { this.setState(prevState => ({ @@ -1494,6 +1480,7 @@ export class App extends React.Component { }, })); element.points.push([0, 0]); + mutateElement(element); invalidateShapeForElement(element); elements = [...elements, element]; this.setState({ @@ -1548,20 +1535,19 @@ export class App extends React.Component { const dx = element.x + width + p1[0]; const dy = element.y + height + p1[1]; + p1[0] = absPx - element.x; + p1[1] = absPy - element.y; mutateElement(element, { x: dx, y: dy, }); - p1[0] = absPx - element.x; - p1[1] = absPy - element.y; } else { + p1[0] -= deltaX; + p1[1] -= deltaY; mutateElement(element, { x: element.x + deltaX, y: element.y + deltaY, }); - - p1[0] -= deltaX; - p1[1] -= deltaY; } }; @@ -1586,6 +1572,7 @@ export class App extends React.Component { p1[0] += deltaX; p1[1] += deltaY; } + mutateElement(element); }; const onPointerMove = (event: PointerEvent) => { @@ -1925,6 +1912,8 @@ export class App extends React.Component { pnt[0] = dx; pnt[1] = dy; } + + mutateElement(draggingElement, { points }); } else { if (event.shiftKey) { ({ width, height } = getPerfectElementSize( @@ -2005,6 +1994,7 @@ export class App extends React.Component { x - draggingElement.x, y - draggingElement.y, ]); + mutateElement(draggingElement); invalidateShapeForElement(draggingElement); this.setState({ multiElement: this.state.draggingElement, @@ -2263,13 +2253,20 @@ export class App extends React.Component { if (scrollBars) { currentScrollBars = scrollBars; } - const scrolledOutside = !atLeastOneVisibleElement && elements.length > 0; + const scrolledOutside = + !atLeastOneVisibleElement && hasNonDeletedElements(elements); if (this.state.scrolledOutside !== scrolledOutside) { this.setState({ scrolledOutside: scrolledOutside }); } this.saveDebounced(); - if (history.isRecording()) { + + if ( + getDrawingVersion(elements) > this.lastBroadcastedOrReceivedSceneVersion + ) { this.broadcastSceneUpdate(); + } + + if (history.isRecording()) { history.pushEntry(this.state, elements); history.skipRecording(); } diff --git a/src/data/index.ts b/src/data/index.ts index c6011414..b4b5ce0e 100644 --- a/src/data/index.ts +++ b/src/data/index.ts @@ -13,6 +13,7 @@ import { serializeAsJSON } from "./json"; import { ExportType } from "../scene/types"; import { restore } from "./restore"; import { restoreFromLocalStorage } from "./localStorage"; +import { hasNonDeletedElements } from "../element"; export { loadFromBlob } from "./blob"; export { saveAsJSON, loadFromJSON } from "./json"; @@ -35,7 +36,6 @@ export type SocketUpdateDataSource = { type: "SCENE_UPDATE"; payload: { elements: readonly ExcalidrawElement[]; - appState: Pick; }; }; MOUSE_LOCATION: { @@ -288,7 +288,7 @@ export async function exportCanvas( scale?: number; }, ) { - if (!elements.length) { + if (!hasNonDeletedElements(elements)) { return window.alert(t("alerts.cannotExportEmptyCanvas")); } // calculate smallest area to fit the contents in diff --git a/src/data/restore.ts b/src/data/restore.ts index 499366c1..1abd61ef 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -52,7 +52,8 @@ export function restore( return { ...element, - version: element.version || 0, + // all elements must have version > 0 so getDrawingVersion() will pick up newly added elements + version: element.version || 1, id: element.id || nanoid(), fillStyle: element.fillStyle || "hachure", strokeWidth: element.strokeWidth || 1, diff --git a/src/element/index.ts b/src/element/index.ts index 1b71b2c7..1f60778c 100644 --- a/src/element/index.ts +++ b/src/element/index.ts @@ -1,3 +1,6 @@ +import { ExcalidrawElement } from "./types"; +import { isInvisiblySmallElement } from "./sizeHelpers"; + export { newElement, newTextElement, duplicateElement } from "./newElement"; export { getElementAbsoluteCoords, @@ -24,3 +27,28 @@ export { normalizeDimensions, } from "./sizeHelpers"; export { showSelectedShapeActions } from "./showSelectedShapeActions"; + +export function getSyncableElements(elements: readonly ExcalidrawElement[]) { + // There are places in Excalidraw where synthetic invisibly small elements are added and removed. + // It's probably best to keep those local otherwise there might be a race condition that + // gets the app into an invalid state. I've never seen it happen but I'm worried about it :) + return elements.filter(el => !isInvisiblySmallElement(el)); +} + +export function getElementMap(elements: readonly ExcalidrawElement[]) { + return getSyncableElements(elements).reduce( + (acc: { [key: string]: ExcalidrawElement }, element: ExcalidrawElement) => { + acc[element.id] = element; + return acc; + }, + {}, + ); +} + +export function getDrawingVersion(elements: readonly ExcalidrawElement[]) { + return elements.reduce((acc, el) => acc + el.version, 0); +} + +export function hasNonDeletedElements(elements: readonly ExcalidrawElement[]) { + return elements.some(element => !element.isDeleted); +} diff --git a/src/element/mutateElement.ts b/src/element/mutateElement.ts index 70c3652e..9410039f 100644 --- a/src/element/mutateElement.ts +++ b/src/element/mutateElement.ts @@ -1,4 +1,5 @@ import { ExcalidrawElement, ExcalidrawTextElement } from "./types"; +import { randomSeed } from "roughjs/bin/math"; type ElementUpdate = Omit< Partial, @@ -10,17 +11,25 @@ type ElementUpdate = Omit< // the same drawing. export function mutateElement( element: ExcalidrawElement, - updates: ElementUpdate, + updates?: ElementUpdate, ) { - Object.assign(element, updates); + if (updates) { + Object.assign(element, updates); + } (element as any).version++; + (element as any).versionNonce = randomSeed(); } export function newElementWith( element: ExcalidrawElement, updates: ElementUpdate, ): ExcalidrawElement { - return { ...element, ...updates, version: element.version + 1 }; + return { + ...element, + ...updates, + version: element.version + 1, + versionNonce: randomSeed(), + }; } // This function tracks updates of text elements for the purposes for collaboration. @@ -32,11 +41,17 @@ export function mutateTextElement( ): void { Object.assign(element, updates); (element as any).version++; + (element as any).versionNonce = randomSeed(); } export function newTextElementWith( element: ExcalidrawTextElement, updates: ElementUpdate, ): ExcalidrawTextElement { - return { ...element, ...updates, version: element.version + 1 }; + return { + ...element, + ...updates, + version: element.version + 1, + versionNonce: randomSeed(), + }; } diff --git a/src/element/newElement.ts b/src/element/newElement.ts index 2e981eec..2b27571b 100644 --- a/src/element/newElement.ts +++ b/src/element/newElement.ts @@ -34,6 +34,8 @@ export function newElement( seed: randomSeed(), points: [] as Point[], version: 1, + versionNonce: 0, + isDeleted: false, }; return element; } diff --git a/src/history.ts b/src/history.ts index 28e9b001..b7aaab58 100644 --- a/src/history.ts +++ b/src/history.ts @@ -13,6 +13,11 @@ export class SceneHistory { private stateHistory: string[] = []; private redoStack: string[] = []; + clear() { + this.stateHistory.length = 0; + this.redoStack.length = 0; + } + private generateEntry( appState: AppState, elements: readonly ExcalidrawElement[], diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index 46c417ff..efdf6090 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -24,7 +24,7 @@ function colorForClientId(clientId: string) { } export function renderScene( - elements: readonly ExcalidrawElement[], + allElements: readonly ExcalidrawElement[], appState: AppState, selectionElement: ExcalidrawElement | null, scale: number, @@ -49,6 +49,8 @@ export function renderScene( return { atLeastOneVisibleElement: false }; } + const elements = allElements.filter(element => !element.isDeleted); + const context = canvas.getContext("2d")!; // When doing calculations based on canvas width we should used normalized one diff --git a/src/scene/comparisons.ts b/src/scene/comparisons.ts index 1cac33c9..da4683c9 100644 --- a/src/scene/comparisons.ts +++ b/src/scene/comparisons.ts @@ -25,6 +25,9 @@ export function getElementAtPosition( let hitElement = null; // We need to to hit testing from front (end of the array) to back (beginning of the array) for (let i = elements.length - 1; i >= 0; --i) { + if (elements[i].isDeleted) { + continue; + } if (hitTest(elements[i], appState, x, y, zoom)) { hitElement = elements[i]; break; @@ -42,6 +45,9 @@ export function getElementContainingPosition( let hitElement = null; // We need to to hit testing from front (end of the array) to back (beginning of the array) for (let i = elements.length - 1; i >= 0; --i) { + if (elements[i].isDeleted) { + continue; + } const [x1, y1, x2, y2] = getElementAbsoluteCoords(elements[i]); if (x1 < x && x < x2 && y1 < y && y < y2) { hitElement = elements[i]; diff --git a/src/scene/selection.ts b/src/scene/selection.ts index 25706864..3dfd37e8 100644 --- a/src/scene/selection.ts +++ b/src/scene/selection.ts @@ -1,6 +1,7 @@ import { ExcalidrawElement } from "../element/types"; import { getElementAbsoluteCoords } from "../element"; import { AppState } from "../types"; +import { newElementWith } from "../element/mutateElement"; export function getElementsWithinSelection( elements: readonly ExcalidrawElement[], @@ -34,24 +35,16 @@ export function deleteSelectedElements( elements: readonly ExcalidrawElement[], appState: AppState, ) { - const deletedIds: AppState["deletedIds"] = {}; return { - elements: elements.filter(el => { + elements: elements.map(el => { if (appState.selectedElementIds[el.id]) { - deletedIds[el.id] = { - version: el.version, - }; - return false; + return newElementWith(el, { isDeleted: true }); } - return true; + return el; }), appState: { ...appState, selectedElementIds: {}, - deletedIds: { - ...appState.deletedIds, - ...deletedIds, - }, }, }; } diff --git a/src/types.ts b/src/types.ts index 3eeebbde..6e96b833 100644 --- a/src/types.ts +++ b/src/types.ts @@ -34,7 +34,6 @@ export type AppState = { openMenu: "canvas" | "shape" | null; lastPointerDownWith: PointerType; selectedElementIds: { [id: string]: boolean }; - deletedIds: { [id: string]: { version: ExcalidrawElement["version"] } }; collaborators: Map; };