Skip to content
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

Jump to bottom

feat: expose StoreAction in relation to multiplayer history #7898

Merged
merged 3 commits into from
Apr 22, 2024
Merged

feat: expose StoreAction in relation to multiplayer history #7898

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f2cb61
Improved Store API, more resilient handling of actions to eliminate p…
Mrazator Apr 18, 2024
03c9075
Changelog update
Mrazator Apr 18, 2024
0029d0b
Adding default action jsdoc to updateScene
Mrazator Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions excalidraw-app/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ import {
LiveCollaborationTrigger,
TTDDialog,
TTDDialogTrigger,
} from "../packages/excalidraw/index";
StoreAction,
} from "../packages/excalidraw";
import {
AppState,
ExcalidrawImperativeAPI,
Expand Down Expand Up @@ -438,7 +439,7 @@ const ExcalidrawWrapper = () => {
excalidrawAPI.updateScene({
...data.scene,
...restore(data.scene, null, null, { repairBindings: true }),
commitToStore: true,
storeAction: StoreAction.CAPTURE,
});
}
});
Expand Down Expand Up @@ -469,6 +470,7 @@ const ExcalidrawWrapper = () => {
setLangCode(langCode);
excalidrawAPI.updateScene({
...localDataState,
storeAction: StoreAction.UPDATE,
});
LibraryIndexedDBAdapter.load().then((data) => {
if (data) {
Expand Down Expand Up @@ -604,6 +606,7 @@ const ExcalidrawWrapper = () => {
if (didChange) {
excalidrawAPI.updateScene({
elements,
storeAction: StoreAction.UPDATE,
});
}
}
Expand Down
6 changes: 5 additions & 1 deletion excalidraw-app/collab/Collab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ import {
OrderedExcalidrawElement,
} from "../../packages/excalidraw/element/types";
import {
StoreAction,
getSceneVersion,
restoreElements,
zoomToFitBounds,
} from "../../packages/excalidraw/index";
} from "../../packages/excalidraw";
import { Collaborator, Gesture } from "../../packages/excalidraw/types";
import {
assertNever,
Expand Down Expand Up @@ -356,6 +357,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {

this.excalidrawAPI.updateScene({
elements,
storeAction: StoreAction.UPDATE,
});
}
};
Expand Down Expand Up @@ -506,6 +508,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {
// to database even if deleted before creating the room.
this.excalidrawAPI.updateScene({
elements,
storeAction: StoreAction.UPDATE,
});

this.saveCollabRoomToFirebase(getSyncableElements(elements));
Expand Down Expand Up @@ -743,6 +746,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {
) => {
this.excalidrawAPI.updateScene({
elements,
storeAction: StoreAction.UPDATE,
});

this.loadImageFiles();
Expand Down
2 changes: 2 additions & 0 deletions excalidraw-app/collab/Portal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import throttle from "lodash.throttle";
import { newElementWith } from "../../packages/excalidraw/element/mutateElement";
import { encryptData } from "../../packages/excalidraw/data/encryption";
import type { Socket } from "socket.io-client";
import { StoreAction } from "../../packages/excalidraw";

class Portal {
collab: TCollabClass;
Expand Down Expand Up @@ -127,6 +128,7 @@ class Portal {
}
return element;
}),
storeAction: StoreAction.UPDATE,
});
}, FILE_UPLOAD_TIMEOUT);

Expand Down
2 changes: 2 additions & 0 deletions excalidraw-app/data/FileManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { StoreAction } from "../../packages/excalidraw";
import { compressData } from "../../packages/excalidraw/data/encode";
import { newElementWith } from "../../packages/excalidraw/element/mutateElement";
import { isInitializedImageElement } from "../../packages/excalidraw/element/typeChecks";
Expand Down Expand Up @@ -238,5 +239,6 @@ export const updateStaleImageStatuses = (params: {
}
return element;
}),
storeAction: StoreAction.UPDATE,
});
};
1 change: 0 additions & 1 deletion excalidraw-app/data/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ export const loadScene = async (
// in the scene database/localStorage, and instead fetch them async
// from a different database
files: data.files,
commitToStore: false,
};
};

Expand Down
10 changes: 6 additions & 4 deletions excalidraw-app/tests/collab.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
createRedoAction,
createUndoAction,
} from "../../packages/excalidraw/actions/actionHistory";
import { newElementWith } from "../../packages/excalidraw";
import { StoreAction, newElementWith } from "../../packages/excalidraw";

const { h } = window;

Expand Down Expand Up @@ -90,15 +90,15 @@ describe("collaboration", () => {

updateSceneData({
elements: syncInvalidIndices([rect1, rect2]),
commitToStore: true,
storeAction: StoreAction.CAPTURE,
});

updateSceneData({
elements: syncInvalidIndices([
rect1,
newElementWith(h.elements[1], { isDeleted: true }),
]),
commitToStore: true,
storeAction: StoreAction.CAPTURE,
});

await waitFor(() => {
Expand Down Expand Up @@ -145,6 +145,7 @@ describe("collaboration", () => {
// simulate force deleting the element remotely
updateSceneData({
elements: syncInvalidIndices([rect1]),
storeAction: StoreAction.UPDATE,
});

await waitFor(() => {
Expand Down Expand Up @@ -182,7 +183,7 @@ describe("collaboration", () => {
h.elements[0],
newElementWith(h.elements[1], { x: 100 }),
]),
commitToStore: true,
storeAction: StoreAction.CAPTURE,
});

await waitFor(() => {
Expand Down Expand Up @@ -217,6 +218,7 @@ describe("collaboration", () => {
// simulate force deleting the element remotely
updateSceneData({
elements: syncInvalidIndices([rect1]),
storeAction: StoreAction.UPDATE,
});

// snapshot was correctly updated and marked the element as deleted
Expand Down
8 changes: 6 additions & 2 deletions packages/excalidraw/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,13 @@ Please add the latest change on the top under the correct section.

### Breaking Changes

- Renamed required `updatedScene` parameter from `commitToHistory` into `commitToStore` [#7348](https://github.com/excalidraw/excalidraw/pull/7348).
- `updateScene` API has changed due to the added `Store` component as part of the multiplayer undo / redo initiative. Specifically, `sceneData` property `commitToHistory: boolean` was replaced with `storeAction: StoreActionType`. Make sure to update all instances of `updateScene` according to the _before / after_ table below. [#7898](https://github.com/excalidraw/excalidraw/pull/7898)

### Breaking Changes
| | Before `commitToHistory` | After `storeAction` | Notes |
| --- | --- | --- | --- |
| _Immediately undoable_ | `true` | `"capture"` | As before, use for all updates which should be recorded by the store & history. Should be used for the most of the local updates. These updates will _immediately_ make it to the local undo / redo stacks. |
| _Eventually undoable_ | `false` | `"none"` | Similar to before, use for all updates which should not be recorded immediately (likely exceptions which are part of some async multi-step process) or those not meant to be recorded at all (i.e. updates to `collaborators` object, parts of `AppState` which are not observed by the store & history - not `ObservedAppState`).<br/><br/>**IMPORTANT** It's likely you should switch to `"update"` in all the other cases. Otherwise, all such updates would end up being recorded with the next `"capture"` - triggered either by the next `updateScene` or internally by the editor. These updates will _eventually_ make it to the local undo / redo stacks. |
| _Never undoable_ | n/a | `"update"` | **NEW**: previously there was no equivalent for this value. Now, it's recommended to use `"update"` for all remote updates (from the other clients), scene initialization, or those updates, which should not be locally "undoable". These updates will _never_ make it to the local undo / redo stacks. |

- `ExcalidrawEmbeddableElement.validated` was removed and moved to private editor state. This should largely not affect your apps unless you were reading from this attribute. We keep validating embeddable urls internally, and the public [`props.validateEmbeddable`](https://docs.excalidraw.com/docs/@excalidraw/excalidraw/api/props#validateembeddable) still applies. [#7539](https://github.com/excalidraw/excalidraw/pull/7539)

Expand Down
4 changes: 2 additions & 2 deletions packages/excalidraw/actions/actionHistory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { KEYS } from "../keys";
import { arrayToMap } from "../utils";
import { isWindows } from "../constants";
import { SceneElementsMap } from "../element/types";
import { IStore, StoreAction } from "../store";
import { Store, StoreAction } from "../store";
import { useEmitter } from "../hooks/useEmitter";

const writeData = (
Expand Down Expand Up @@ -40,7 +40,7 @@ const writeData = (
return { storeAction: StoreAction.NONE };
};

type ActionCreator = (history: History, store: IStore) => Action;
type ActionCreator = (history: History, store: Store) => Action;

export const createUndoAction: ActionCreator = (history, store) => ({
name: "undo",
Expand Down
4 changes: 2 additions & 2 deletions packages/excalidraw/actions/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
UIAppState,
} from "../types";
import { MarkOptional } from "../utility-types";
import { StoreAction } from "../store";
import { StoreActionType } from "../store";

export type ActionSource =
| "ui"
Expand All @@ -26,7 +26,7 @@ export type ActionResult =
"offsetTop" | "offsetLeft" | "width" | "height"
> | null;
files?: BinaryFiles | null;
storeAction: keyof typeof StoreAction;
storeAction: StoreActionType;
replaceFiles?: boolean;
}
| false;
Expand Down
86 changes: 37 additions & 49 deletions packages/excalidraw/components/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ import {
ExcalidrawIframeElement,
ExcalidrawEmbeddableElement,
Ordered,
OrderedExcalidrawElement,
} from "../element/types";
import { getCenter, getDistance } from "../gesture";
import {
Expand Down Expand Up @@ -412,7 +411,7 @@ import { ElementCanvasButton } from "./MagicButton";
import { MagicIcon, copyIcon, fullscreenIcon } from "./icons";
import { EditorLocalStorage } from "../data/EditorLocalStorage";
import FollowMode from "./FollowMode/FollowMode";
import { IStore, Store, StoreAction } from "../store";
import { Store, StoreAction } from "../store";
import { AnimationFrameHandler } from "../animation-frame-handler";
import { AnimatedTrail } from "../animated-trail";
import { LaserTrails } from "../laser-trails";
Expand Down Expand Up @@ -543,7 +542,7 @@ class App extends React.Component<AppProps, AppState> {
public library: AppClassProperties["library"];
public libraryItemsFromStorage: LibraryItems | undefined;
public id: string;
private store: IStore;
private store: Store;
private history: History;
private excalidrawContainerValue: {
container: HTMLDivElement | null;
Expand Down Expand Up @@ -2123,7 +2122,7 @@ class App extends React.Component<AppProps, AppState> {
}
return el;
}),
commitToStore: true,
storeAction: StoreAction.CAPTURE,
});
}
},
Expand Down Expand Up @@ -2810,7 +2809,7 @@ class App extends React.Component<AppProps, AppState> {
);
}

this.store.capture(elementsMap, this.state);
this.store.commit(elementsMap, this.state);

// Do not notify consumers if we're still loading the scene. Among other
// potential issues, this fixes a case where the tab isn't focused during
Expand Down Expand Up @@ -3683,51 +3682,38 @@ class App extends React.Component<AppProps, AppState> {
elements?: SceneData["elements"];
appState?: Pick<AppState, K> | null;
collaborators?: SceneData["collaborators"];
commitToStore?: SceneData["commitToStore"];
storeAction?: SceneData["storeAction"];
Copy link
Member

@dwelle dwelle Apr 21, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's note the default here? (as a tsdoc /** */ so it shows in intellisense)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, added

}) => {
const nextElements = syncInvalidIndices(sceneData.elements ?? []);

if (sceneData.commitToStore) {
this.store.shouldCaptureIncrement();
}

if (sceneData.elements || sceneData.appState) {
let nextCommittedAppState = this.state;
let nextCommittedElements: Map<string, OrderedExcalidrawElement>;

if (sceneData.appState) {
nextCommittedAppState = {
...this.state,
...sceneData.appState, // Here we expect just partial appState
};
}
if (sceneData.storeAction && sceneData.storeAction !== StoreAction.NONE) {
const prevCommittedAppState = this.store.snapshot.appState;
const prevCommittedElements = this.store.snapshot.elements;

const prevElements = this.scene.getElementsIncludingDeleted();

if (sceneData.elements) {
/**
* We need to schedule a snapshot update, as in case `commitToStore` is false (i.e. remote update),
* as it's essential for computing local changes after the async action is completed (i.e. not to include remote changes in the diff).
*
* This is also a breaking change for all local `updateScene` calls without set `commitToStore` to true,
* as it makes such updates impossible to undo (previously they were undone coincidentally with the switch to the whole snapshot captured by the history).
*
* WARN: be careful here as moving it elsewhere could break the history for remote client without noticing
* - we need to find a way to test two concurrent client updates simultaneously, while having access to both stores & histories.
*/
this.store.shouldUpdateSnapshot();
const nextCommittedAppState = sceneData.appState
? Object.assign({}, prevCommittedAppState, sceneData.appState) // new instance, with partial appstate applied to previously captured one, including hidden prop inside `prevCommittedAppState`
: prevCommittedAppState;

// TODO#7348: deprecate once exchanging just store increments between clients
nextCommittedElements = this.store.ignoreUncomittedElements(
arrayToMap(prevElements),
arrayToMap(nextElements),
const nextCommittedElements = sceneData.elements
? this.store.filterUncomittedElements(
this.scene.getElementsMapIncludingDeleted(), // Only used to detect uncomitted local elements
arrayToMap(nextElements), // We expect all (already reconciled) elements
)
: prevCommittedElements;

// WARN: store action always performs deep clone of changed elements, for ephemeral remote updates (i.e. remote dragging, resizing, drawing) we might consider doing something smarter
// do NOT schedule store actions (execute after re-render), as it might cause unexpected concurrency issues if not handled well
if (sceneData.storeAction === StoreAction.CAPTURE) {
this.store.captureIncrement(
nextCommittedElements,
nextCommittedAppState,
);
} else if (sceneData.storeAction === StoreAction.UPDATE) {
this.store.updateSnapshot(
nextCommittedElements,
nextCommittedAppState,
);
} else {
nextCommittedElements = arrayToMap(prevElements);
}

// WARN: Performs deep clone of changed elements, for ephemeral remote updates (i.e. remote dragging, resizing, drawing) we might consider doing something smarter
this.store.capture(nextCommittedElements, nextCommittedAppState);
}

if (sceneData.appState) {
Expand Down Expand Up @@ -5704,6 +5690,7 @@ class App extends React.Component<AppProps, AppState> {
this.state,
),
},
storeAction: StoreAction.UPDATE,
});
return;
}
Expand Down Expand Up @@ -7993,6 +7980,7 @@ class App extends React.Component<AppProps, AppState> {
appState: {
draggingElement: null,
},
storeAction: StoreAction.UPDATE,
});

return;
Expand Down Expand Up @@ -8165,6 +8153,7 @@ class App extends React.Component<AppProps, AppState> {
elements: this.scene
.getElementsIncludingDeleted()
.filter((el) => el.id !== resizingElement.id),
storeAction: StoreAction.UPDATE,
});
}

Expand Down Expand Up @@ -9228,13 +9217,12 @@ class App extends React.Component<AppProps, AppState> {
}

if (ret.type === MIME_TYPES.excalidraw) {
// Restore the fractional indices by mutating elements and update the
// store snapshot, otherwise we would end up with duplicate indices
// restore the fractional indices by mutating elements
syncInvalidIndices(elements.concat(ret.data.elements));
this.store.snapshot = this.store.snapshot.clone(
arrayToMap(elements),
this.state,
);

// update the store snapshot for old elements, otherwise we would end up with duplicated fractional indices on undo
this.store.updateSnapshot(arrayToMap(elements), this.state);

this.setState({ isLoading: true });
this.syncActionResult({
...ret.data,
Expand Down
4 changes: 2 additions & 2 deletions packages/excalidraw/element/linearElementEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import { getBoundTextElement, handleBindTextResize } from "./textElement";
import { DRAGGING_THRESHOLD } from "../constants";
import { Mutable } from "../utility-types";
import { ShapeCache } from "../scene/ShapeCache";
import { IStore } from "../store";
import { Store } from "../store";

const editorMidPointsCache: {
version: number | null;
Expand Down Expand Up @@ -642,7 +642,7 @@ export class LinearElementEditor {
static handlePointerDown(
event: React.PointerEvent<HTMLElement>,
appState: AppState,
store: IStore,
store: Store,
scenePointer: { x: number; y: number },
linearElementEditor: LinearElementEditor,
app: AppClassProperties,
Expand Down