From 058918f8e5406d036fff76bab9bbb2b5281547c9 Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Sun, 15 Jun 2025 23:43:14 +0200 Subject: [PATCH] feat: capture images after they initialize (#9643) Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com> --- packages/element/src/store.ts | 14 +- packages/excalidraw/components/App.tsx | 250 +++++++----------- .../tests/__snapshots__/history.test.tsx.snap | 12 +- packages/excalidraw/tests/flip.test.tsx | 24 ++ packages/excalidraw/tests/history.test.tsx | 25 ++ 5 files changed, 164 insertions(+), 161 deletions(-) diff --git a/packages/element/src/store.ts b/packages/element/src/store.ts index da7352e3e..0f5933422 100644 --- a/packages/element/src/store.ts +++ b/packages/element/src/store.ts @@ -23,6 +23,8 @@ import { syncInvalidIndicesImmutable, hashElementsVersion, hashString, + isInitializedImageElement, + isImageElement, } from "./index"; import type { @@ -137,6 +139,8 @@ export class Store { } else { // immediately create an immutable change of the scheduled updates, // compared to the current state, so that they won't mutate later on during batching + // also, we have to compare against the current state, + // as comparing against the snapshot might include yet uncomitted changes (i.e. async freedraw / text / image, etc.) const currentSnapshot = StoreSnapshot.create( this.app.scene.getElementsMapIncludingDeleted(), this.app.state, @@ -869,7 +873,7 @@ export class StoreSnapshot { } /** - * Detect if there any changed elements. + * Detect if there are any changed elements. */ private detectChangedElements( nextElements: SceneElementsMap, @@ -904,6 +908,14 @@ export class StoreSnapshot { !prevElement || // element was added prevElement.version < nextElement.version // element was updated ) { + if ( + isImageElement(nextElement) && + !isInitializedImageElement(nextElement) + ) { + // ignore any updates on uninitialized image elements + continue; + } + changedElements.set(nextElement.id, nextElement); } } diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 9b3fe0d8b..dd362ecc2 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -3063,18 +3063,7 @@ class App extends React.Component { return; } - const imageElement = this.createImageElement({ sceneX, sceneY }); - this.insertImageElement(imageElement, file); - this.initializeImageDimensions(imageElement); - this.store.scheduleCapture(); - this.setState({ - selectedElementIds: makeNextSelectedElementIds( - { - [imageElement.id]: true, - }, - this.state, - ), - }); + this.createImageElement({ sceneX, sceneY, imageFile: file }); return; } @@ -3380,15 +3369,12 @@ class App extends React.Component { const nextSelectedIds: Record = {}; for (const response of responses) { if (response.file) { - const imageElement = this.createImageElement({ + const initializedImageElement = await this.createImageElement({ sceneX, sceneY: y, + imageFile: response.file, }); - const initializedImageElement = await this.insertImageElement( - imageElement, - response.file, - ); if (initializedImageElement) { // vertically center first image in the batch if (!firstImageYOffsetDone) { @@ -3403,9 +3389,9 @@ class App extends React.Component { { informMutation: false, isDragging: false }, ); - y = imageElement.y + imageElement.height + 25; + y = initializedImageElement.y + initializedImageElement.height + 25; - nextSelectedIds[imageElement.id] = true; + nextSelectedIds[initializedImageElement.id] = true; } } } @@ -7628,14 +7614,16 @@ class App extends React.Component { return element; }; - private createImageElement = ({ + private createImageElement = async ({ sceneX, sceneY, addToFrameUnderCursor = true, + imageFile, }: { sceneX: number; sceneY: number; addToFrameUnderCursor?: boolean; + imageFile: File; }) => { const [gridX, gridY] = getGridPoint( sceneX, @@ -7652,10 +7640,10 @@ class App extends React.Component { }) : null; - const element = newImageElement({ + const placeholderSize = 100 / this.state.zoom.value; + + const placeholderImageElement = newImageElement({ type: "image", - x: gridX, - y: gridY, strokeColor: this.state.currentItemStrokeColor, backgroundColor: this.state.currentItemBackgroundColor, fillStyle: this.state.currentItemFillStyle, @@ -7666,9 +7654,18 @@ class App extends React.Component { opacity: this.state.currentItemOpacity, locked: false, frameId: topLayerFrame ? topLayerFrame.id : null, + x: gridX - placeholderSize / 2, + y: gridY - placeholderSize / 2, + width: placeholderSize, + height: placeholderSize, }); - return element; + const initializedImageElement = await this.insertImageElement( + placeholderImageElement, + imageFile, + ); + + return initializedImageElement; }; private handleLinearElementOnPointerDown = ( @@ -9092,32 +9089,6 @@ class App extends React.Component { return; } - if (isImageElement(newElement)) { - const imageElement = newElement; - try { - this.initializeImageDimensions(imageElement); - this.setState( - { - selectedElementIds: makeNextSelectedElementIds( - { [imageElement.id]: true }, - this.state, - ), - }, - () => { - this.actionManager.executeAction(actionFinalize); - }, - ); - } catch (error: any) { - console.error(error); - this.scene.replaceAllElements( - this.scene - .getElementsIncludingDeleted() - .filter((el) => el.id !== imageElement.id), - ); - this.actionManager.executeAction(actionFinalize); - } - return; - } if (isLinearElement(newElement)) { if (newElement!.points.length > 1) { @@ -9829,13 +9800,10 @@ class App extends React.Component { } }; - private initializeImage = async ({ - imageFile, - imageElement: _imageElement, - }: { - imageFile: File; - imageElement: ExcalidrawImageElement; - }) => { + private initializeImage = async ( + placeholderImageElement: ExcalidrawImageElement, + imageFile: File, + ) => { // at this point this should be guaranteed image file, but we do this check // to satisfy TS down the line if (!isSupportedImageFile(imageFile)) { @@ -9895,13 +9863,14 @@ class App extends React.Component { const dataURL = this.files[fileId]?.dataURL || (await getDataURL(imageFile)); - let imageElement = newElementWith(_imageElement, { - fileId, - }) as NonDeleted; - return new Promise>( async (resolve, reject) => { try { + let initializedImageElement = this.getLatestInitializedImageElement( + placeholderImageElement, + fileId, + ); + this.addMissingFiles([ { mimeType, @@ -9912,34 +9881,39 @@ class App extends React.Component { }, ]); - let cachedImageData = this.imageCache.get(fileId); - - if (!cachedImageData) { + if (!this.imageCache.get(fileId)) { this.addNewImagesToImageCache(); - const { updatedFiles } = await this.updateImageCache([ - imageElement, + const { erroredFiles } = await this.updateImageCache([ + initializedImageElement, ]); - if (updatedFiles.size) { - ShapeCache.delete(_imageElement); + if (erroredFiles.size) { + throw new Error("Image cache update resulted with an error."); } - - cachedImageData = this.imageCache.get(fileId); } - const imageHTML = await cachedImageData?.image; + const imageHTML = await this.imageCache.get(fileId)?.image; + + if ( + imageHTML && + this.state.newElement?.id !== initializedImageElement.id + ) { + initializedImageElement = this.getLatestInitializedImageElement( + placeholderImageElement, + fileId, + ); - if (imageHTML && this.state.newElement?.id !== imageElement.id) { const naturalDimensions = this.getImageNaturalDimensions( - imageElement, + initializedImageElement, imageHTML, ); - imageElement = newElementWith(imageElement, naturalDimensions); + // no need to create a new instance anymore, just assign the natural dimensions + Object.assign(initializedImageElement, naturalDimensions); } - resolve(imageElement); + resolve(initializedImageElement); } catch (error: any) { console.error(error); reject(new Error(t("errors.imageInsertError"))); @@ -9948,11 +9922,31 @@ class App extends React.Component { ); }; + /** + * use during async image initialization, + * when the placeholder image could have been modified in the meantime, + * and when you don't want to loose those modifications + */ + private getLatestInitializedImageElement = ( + imagePlaceholder: ExcalidrawImageElement, + fileId: FileId, + ) => { + const latestImageElement = + this.scene.getElement(imagePlaceholder.id) ?? imagePlaceholder; + + return newElementWith( + latestImageElement as InitializedExcalidrawImageElement, + { + fileId, + }, + ); + }; + /** * inserts image into elements array and rerenders */ - insertImageElement = async ( - imageElement: ExcalidrawImageElement, + private insertImageElement = async ( + placeholderImageElement: ExcalidrawImageElement, imageFile: File, ) => { // we should be handling all cases upstream, but in case we forget to handle @@ -9962,34 +9956,39 @@ class App extends React.Component { return; } - this.scene.insertElement(imageElement); + this.scene.insertElement(placeholderImageElement); try { - const image = await this.initializeImage({ + const initializedImageElement = await this.initializeImage( + placeholderImageElement, imageFile, - imageElement, - }); + ); const nextElements = this.scene .getElementsIncludingDeleted() .map((element) => { - if (element.id === image.id) { - return image; + if (element.id === initializedImageElement.id) { + return initializedImageElement; } return element; }); - // schedules an immediate micro action, which will update snapshot, - // but won't be undoable, which is what we want! this.updateScene({ - captureUpdate: CaptureUpdateAction.NEVER, + captureUpdate: CaptureUpdateAction.IMMEDIATELY, elements: nextElements, + appState: { + selectedElementIds: makeNextSelectedElementIds( + { [initializedImageElement.id]: true }, + this.state, + ), + }, }); - return image; + return initializedImageElement; } catch (error: any) { - this.scene.mutateElement(imageElement, { + this.store.scheduleAction(CaptureUpdateAction.NEVER); + this.scene.mutateElement(placeholderImageElement, { isDeleted: true, }); this.actionManager.executeAction(actionFinalize); @@ -10017,26 +10016,17 @@ class App extends React.Component { ) as (keyof typeof IMAGE_MIME_TYPES)[], }); - const imageElement = this.createImageElement({ + await this.createImageElement({ sceneX: x, sceneY: y, addToFrameUnderCursor: false, + imageFile, }); - this.insertImageElement(imageElement, imageFile); - this.initializeImageDimensions(imageElement); - this.store.scheduleCapture(); - this.setState( - { - selectedElementIds: makeNextSelectedElementIds( - { [imageElement.id]: true }, - this.state, - ), - }, - () => { - this.actionManager.executeAction(actionFinalize); - }, - ); + // avoid being batched (just in case) + this.setState({}, () => { + this.actionManager.executeAction(actionFinalize); + }); } catch (error: any) { if (error.name !== "AbortError") { console.error(error); @@ -10055,45 +10045,6 @@ class App extends React.Component { } }; - initializeImageDimensions = (imageElement: ExcalidrawImageElement) => { - const imageHTML = - isInitializedImageElement(imageElement) && - this.imageCache.get(imageElement.fileId)?.image; - - if (!imageHTML || imageHTML instanceof Promise) { - if ( - imageElement.width < DRAGGING_THRESHOLD / this.state.zoom.value && - imageElement.height < DRAGGING_THRESHOLD / this.state.zoom.value - ) { - const placeholderSize = 100 / this.state.zoom.value; - - this.scene.mutateElement(imageElement, { - x: imageElement.x - placeholderSize / 2, - y: imageElement.y - placeholderSize / 2, - width: placeholderSize, - height: placeholderSize, - }); - } - - return; - } - - // if user-created bounding box is below threshold, assume the - // intention was to click instead of drag, and use the image's - // intrinsic size - if ( - imageElement.width < DRAGGING_THRESHOLD / this.state.zoom.value && - imageElement.height < DRAGGING_THRESHOLD / this.state.zoom.value - ) { - const naturalDimensions = this.getImageNaturalDimensions( - imageElement, - imageHTML, - ); - - this.scene.mutateElement(imageElement, naturalDimensions); - } - }; - private getImageNaturalDimensions = ( imageElement: ExcalidrawImageElement, imageHTML: HTMLImageElement, @@ -10135,8 +10086,9 @@ class App extends React.Component { }); if (erroredFiles.size) { + this.store.scheduleAction(CaptureUpdateAction.NEVER); this.scene.replaceAllElements( - this.scene.getElementsIncludingDeleted().map((element) => { + elements.map((element) => { if ( isInitializedImageElement(element) && erroredFiles.has(element.fileId) @@ -10357,17 +10309,7 @@ class App extends React.Component { // if no scene is embedded or we fail for whatever reason, fall back // to importing as regular image // --------------------------------------------------------------------- - - const imageElement = this.createImageElement({ sceneX, sceneY }); - this.insertImageElement(imageElement, file); - this.initializeImageDimensions(imageElement); - this.store.scheduleCapture(); - this.setState({ - selectedElementIds: makeNextSelectedElementIds( - { [imageElement.id]: true }, - this.state, - ), - }); + this.createImageElement({ sceneX, sceneY, imageFile: file }); return; } diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 22b8519dc..9a2c62131 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -12514,7 +12514,7 @@ exports[`history > singleplayer undo/redo > should create new history entry on i "strokeWidth": 2, "type": "image", "updated": 1, - "version": 7, + "version": 5, "width": 318, "x": -159, "y": "-167.50000", @@ -12573,14 +12573,14 @@ exports[`history > singleplayer undo/redo > should create new history entry on i "strokeStyle": "solid", "strokeWidth": 2, "type": "image", - "version": 7, + "version": 5, "width": 318, "x": -159, "y": "-167.50000", }, "inserted": { "isDeleted": true, - "version": 6, + "version": 4, }, }, }, @@ -12737,7 +12737,7 @@ exports[`history > singleplayer undo/redo > should create new history entry on i "strokeWidth": 2, "type": "image", "updated": 1, - "version": 7, + "version": 5, "width": 56, "x": -28, "y": "-38.50000", @@ -12796,14 +12796,14 @@ exports[`history > singleplayer undo/redo > should create new history entry on i "strokeStyle": "solid", "strokeWidth": 2, "type": "image", - "version": 7, + "version": 5, "width": 56, "x": -28, "y": "-38.50000", }, "inserted": { "isDeleted": true, - "version": 6, + "version": 4, }, }, }, diff --git a/packages/excalidraw/tests/flip.test.tsx b/packages/excalidraw/tests/flip.test.tsx index 79a935068..e965a0068 100644 --- a/packages/excalidraw/tests/flip.test.tsx +++ b/packages/excalidraw/tests/flip.test.tsx @@ -38,6 +38,8 @@ import { import { getTextEditor } from "./queries/dom"; +import { mockHTMLImageElement } from "./helpers/mocks"; + import type { NormalizedZoomValue } from "../types"; const { h } = window; @@ -742,6 +744,28 @@ describe("freedraw", () => { //image //TODO: currently there is no test for pixel colors at flipped positions. describe("image", () => { + const smileyImageDimensions = { + width: 56, + height: 77, + }; + + beforeEach(() => { + // it's necessary to specify the height in order to calculate natural dimensions of the image + h.state.height = 1000; + }); + + beforeAll(() => { + mockHTMLImageElement( + smileyImageDimensions.width, + smileyImageDimensions.height, + ); + }); + + afterAll(() => { + vi.unstubAllGlobals(); + h.state.height = 0; + }); + const createImage = async () => { const sendPasteEvent = (file?: File) => { const clipboardEvent = createPasteEvent({ files: file ? [file] : [] }); diff --git a/packages/excalidraw/tests/history.test.tsx b/packages/excalidraw/tests/history.test.tsx index be7c5821f..ba013e29d 100644 --- a/packages/excalidraw/tests/history.test.tsx +++ b/packages/excalidraw/tests/history.test.tsx @@ -642,6 +642,19 @@ describe("history", () => { ...deerImageDimensions, }), ]); + + // need to check that delta actually contains initialized image element (with fileId & natural dimensions) + expect( + Object.values(h.history.undoStack[0].elements.removed)[0].deleted, + ).toEqual( + expect.objectContaining({ + type: "image", + fileId: expect.any(String), + x: expect.toBeNonNaNNumber(), + y: expect.toBeNonNaNNumber(), + ...deerImageDimensions, + }), + ); }); Keyboard.undo(); @@ -753,6 +766,18 @@ describe("history", () => { ...smileyImageDimensions, }), ]); + // need to check that delta actually contains initialized image element (with fileId & natural dimensions) + expect( + Object.values(h.history.undoStack[0].elements.removed)[0].deleted, + ).toEqual( + expect.objectContaining({ + type: "image", + fileId: expect.any(String), + x: expect.toBeNonNaNNumber(), + y: expect.toBeNonNaNNumber(), + ...smileyImageDimensions, + }), + ); }); Keyboard.undo();