From 79b181bcdcbff6166dbfa7b8b11118ddbd6c9833 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Tue, 29 Oct 2024 22:40:24 +0100 Subject: [PATCH] fix: restore svg image DataURL dimensions (#8730) --- excalidraw-app/collab/Collab.tsx | 29 ++++++++- excalidraw-app/data/FileManager.ts | 71 ++++++++++++++++------- excalidraw-app/data/LocalData.ts | 8 +-- excalidraw-app/data/firebase.ts | 8 +-- packages/excalidraw/components/App.tsx | 66 +++++++++++++++------ packages/excalidraw/data/blob.ts | 22 +++++-- packages/excalidraw/data/encode.ts | 53 +++++++++-------- packages/excalidraw/data/image.ts | 16 ++--- packages/excalidraw/element/image.ts | 2 +- packages/excalidraw/scene/export.ts | 4 +- packages/excalidraw/tests/export.test.tsx | 4 +- packages/excalidraw/types.ts | 5 ++ packages/utils/utils.unmocked.test.ts | 2 +- 13 files changed, 196 insertions(+), 94 deletions(-) diff --git a/excalidraw-app/collab/Collab.tsx b/excalidraw-app/collab/Collab.tsx index 7059a67c5..944c0deb0 100644 --- a/excalidraw-app/collab/Collab.tsx +++ b/excalidraw-app/collab/Collab.tsx @@ -1,6 +1,7 @@ import throttle from "lodash.throttle"; import { PureComponent } from "react"; import type { + BinaryFileData, ExcalidrawImperativeAPI, SocketId, } from "../../packages/excalidraw/types"; @@ -9,6 +10,7 @@ import { APP_NAME, ENV, EVENT } from "../../packages/excalidraw/constants"; import type { ImportedDataState } from "../../packages/excalidraw/data/types"; import type { ExcalidrawElement, + FileId, InitializedExcalidrawImageElement, OrderedExcalidrawElement, } from "../../packages/excalidraw/element/types"; @@ -157,7 +159,7 @@ class Collab extends PureComponent { throw new AbortError(); } - return saveFilesToFirebase({ + const { savedFiles, erroredFiles } = await saveFilesToFirebase({ prefix: `${FIREBASE_STORAGE_PREFIXES.collabFiles}/${roomId}`, files: await encodeFilesForUpload({ files: addedFiles, @@ -165,6 +167,29 @@ class Collab extends PureComponent { maxBytes: FILE_UPLOAD_MAX_BYTES, }), }); + + return { + savedFiles: savedFiles.reduce( + (acc: Map, id) => { + const fileData = addedFiles.get(id); + if (fileData) { + acc.set(id, fileData); + } + return acc; + }, + new Map(), + ), + erroredFiles: erroredFiles.reduce( + (acc: Map, id) => { + const fileData = addedFiles.get(id); + if (fileData) { + acc.set(id, fileData); + } + return acc; + }, + new Map(), + ), + }; }, }); this.excalidrawAPI = props.excalidrawAPI; @@ -394,7 +419,7 @@ class Collab extends PureComponent { .filter((element) => { return ( isInitializedImageElement(element) && - !this.fileManager.isFileHandled(element.fileId) && + !this.fileManager.isFileTracked(element.fileId) && !element.isDeleted && (opts.forceFetchFiles ? element.status !== "pending" || diff --git a/excalidraw-app/data/FileManager.ts b/excalidraw-app/data/FileManager.ts index e9b460247..56f832b68 100644 --- a/excalidraw-app/data/FileManager.ts +++ b/excalidraw-app/data/FileManager.ts @@ -16,14 +16,26 @@ import type { BinaryFiles, } from "../../packages/excalidraw/types"; +type FileVersion = Required["version"]; + export class FileManager { /** files being fetched */ private fetchingFiles = new Map(); + private erroredFiles_fetch = new Map< + ExcalidrawImageElement["fileId"], + true + >(); /** files being saved */ - private savingFiles = new Map(); + private savingFiles = new Map< + ExcalidrawImageElement["fileId"], + FileVersion + >(); /* files already saved to persistent storage */ - private savedFiles = new Map(); - private erroredFiles = new Map(); + private savedFiles = new Map(); + private erroredFiles_save = new Map< + ExcalidrawImageElement["fileId"], + FileVersion + >(); private _getFiles; private _saveFiles; @@ -37,8 +49,8 @@ export class FileManager { erroredFiles: Map; }>; saveFiles: (data: { addedFiles: Map }) => Promise<{ - savedFiles: Map; - erroredFiles: Map; + savedFiles: Map; + erroredFiles: Map; }>; }) { this._getFiles = getFiles; @@ -46,19 +58,28 @@ export class FileManager { } /** - * returns whether file is already saved or being processed + * returns whether file is saved/errored, or being processed */ - isFileHandled = (id: FileId) => { + isFileTracked = (id: FileId) => { return ( this.savedFiles.has(id) || - this.fetchingFiles.has(id) || this.savingFiles.has(id) || - this.erroredFiles.has(id) + this.fetchingFiles.has(id) || + this.erroredFiles_fetch.has(id) || + this.erroredFiles_save.has(id) + ); + }; + + isFileSavedOrBeingSaved = (file: BinaryFileData) => { + const fileVersion = this.getFileVersion(file); + return ( + this.savedFiles.get(file.id) === fileVersion || + this.savingFiles.get(file.id) === fileVersion ); }; - isFileSaved = (id: FileId) => { - return this.savedFiles.has(id); + getFileVersion = (file: BinaryFileData) => { + return file.version ?? 1; }; saveFiles = async ({ @@ -71,13 +92,16 @@ export class FileManager { const addedFiles: Map = new Map(); for (const element of elements) { + const fileData = + isInitializedImageElement(element) && files[element.fileId]; + if ( - isInitializedImageElement(element) && - files[element.fileId] && - !this.isFileHandled(element.fileId) + fileData && + // NOTE if errored during save, won't retry due to this check + !this.isFileSavedOrBeingSaved(fileData) ) { addedFiles.set(element.fileId, files[element.fileId]); - this.savingFiles.set(element.fileId, true); + this.savingFiles.set(element.fileId, this.getFileVersion(fileData)); } } @@ -86,8 +110,12 @@ export class FileManager { addedFiles, }); - for (const [fileId] of savedFiles) { - this.savedFiles.set(fileId, true); + for (const [fileId, fileData] of savedFiles) { + this.savedFiles.set(fileId, this.getFileVersion(fileData)); + } + + for (const [fileId, fileData] of erroredFiles) { + this.erroredFiles_save.set(fileId, this.getFileVersion(fileData)); } return { @@ -121,10 +149,10 @@ export class FileManager { const { loadedFiles, erroredFiles } = await this._getFiles(ids); for (const file of loadedFiles) { - this.savedFiles.set(file.id, true); + this.savedFiles.set(file.id, this.getFileVersion(file)); } for (const [fileId] of erroredFiles) { - this.erroredFiles.set(fileId, true); + this.erroredFiles_fetch.set(fileId, true); } return { loadedFiles, erroredFiles }; @@ -160,7 +188,7 @@ export class FileManager { ): element is InitializedExcalidrawImageElement => { return ( isInitializedImageElement(element) && - this.isFileSaved(element.fileId) && + this.savedFiles.has(element.fileId) && element.status === "pending" ); }; @@ -169,7 +197,8 @@ export class FileManager { this.fetchingFiles.clear(); this.savingFiles.clear(); this.savedFiles.clear(); - this.erroredFiles.clear(); + this.erroredFiles_fetch.clear(); + this.erroredFiles_save.clear(); } } diff --git a/excalidraw-app/data/LocalData.ts b/excalidraw-app/data/LocalData.ts index c8ac5b19a..2e524522c 100644 --- a/excalidraw-app/data/LocalData.ts +++ b/excalidraw-app/data/LocalData.ts @@ -183,8 +183,8 @@ export class LocalData { ); }, async saveFiles({ addedFiles }) { - const savedFiles = new Map(); - const erroredFiles = new Map(); + const savedFiles = new Map(); + const erroredFiles = new Map(); // before we use `storage` event synchronization, let's update the flag // optimistically. Hopefully nothing fails, and an IDB read executed @@ -195,10 +195,10 @@ export class LocalData { [...addedFiles].map(async ([id, fileData]) => { try { await set(id, fileData, filesStore); - savedFiles.set(id, true); + savedFiles.set(id, fileData); } catch (error: any) { console.error(error); - erroredFiles.set(id, true); + erroredFiles.set(id, fileData); } }), ); diff --git a/excalidraw-app/data/firebase.ts b/excalidraw-app/data/firebase.ts index c73018acf..299f3ee98 100644 --- a/excalidraw-app/data/firebase.ts +++ b/excalidraw-app/data/firebase.ts @@ -177,8 +177,8 @@ export const saveFilesToFirebase = async ({ }) => { const firebase = await loadFirebaseStorage(); - const erroredFiles = new Map(); - const savedFiles = new Map(); + const erroredFiles: FileId[] = []; + const savedFiles: FileId[] = []; await Promise.all( files.map(async ({ id, buffer }) => { @@ -194,9 +194,9 @@ export const saveFilesToFirebase = async ({ cacheControl: `public, max-age=${FILE_CACHE_MAX_AGE_SEC}`, }, ); - savedFiles.set(id, true); + savedFiles.push(id); } catch (error: any) { - erroredFiles.set(id, true); + erroredFiles.push(id); } }), ); diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index fb0c24d91..6b6022794 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -304,8 +304,10 @@ import { Toast } from "./Toast"; import { actionToggleViewMode } from "../actions/actionToggleViewMode"; import { dataURLToFile, + dataURLToString, generateIdFromFile, getDataURL, + getDataURL_sync, getFileFromEvent, ImageURLToFile, isImageFileHandle, @@ -2122,9 +2124,7 @@ class App extends React.Component { } if (actionResult.files) { - this.files = actionResult.replaceFiles - ? actionResult.files - : { ...this.files, ...actionResult.files }; + this.addMissingFiles(actionResult.files, actionResult.replaceFiles); this.addNewImagesToImageCache(); } @@ -3237,7 +3237,7 @@ class App extends React.Component { }); if (opts.files) { - this.files = { ...this.files, ...opts.files }; + this.addMissingFiles(opts.files); } this.store.shouldCaptureIncrement(); @@ -3746,23 +3746,56 @@ class App extends React.Component { } }; - /** adds supplied files to existing files in the appState */ + /** + * adds supplied files to existing files in the appState. + * NOTE if file already exists in editor state, the file data is not updated + * */ public addFiles: ExcalidrawImperativeAPI["addFiles"] = withBatchedUpdates( (files) => { - const filesMap = files.reduce((acc, fileData) => { - acc.set(fileData.id, fileData); - return acc; - }, new Map()); - - this.files = { ...this.files, ...Object.fromEntries(filesMap) }; + const { addedFiles } = this.addMissingFiles(files); - this.clearImageShapeCache(Object.fromEntries(filesMap)); + this.clearImageShapeCache(addedFiles); this.scene.triggerUpdate(); this.addNewImagesToImageCache(); }, ); + private addMissingFiles = ( + files: BinaryFiles | BinaryFileData[], + replace = false, + ) => { + const nextFiles = replace ? {} : { ...this.files }; + const addedFiles: BinaryFiles = {}; + + const _files = Array.isArray(files) ? files : Object.values(files); + + for (const fileData of _files) { + if (nextFiles[fileData.id]) { + continue; + } + + addedFiles[fileData.id] = fileData; + nextFiles[fileData.id] = fileData; + + if (fileData.mimeType === MIME_TYPES.svg) { + const restoredDataURL = getDataURL_sync( + normalizeSVG(dataURLToString(fileData.dataURL)), + MIME_TYPES.svg, + ); + if (fileData.dataURL !== restoredDataURL) { + // bump version so persistence layer can update the store + fileData.version = (fileData.version ?? 1) + 1; + fileData.dataURL = restoredDataURL; + } + } + } + + this.files = nextFiles; + + return { addedFiles }; + }; + public updateScene = withBatchedUpdates( (sceneData: { elements?: SceneData["elements"]; @@ -9285,7 +9318,7 @@ class App extends React.Component { if (mimeType === MIME_TYPES.svg) { try { imageFile = SVGStringToFile( - await normalizeSVG(await imageFile.text()), + normalizeSVG(await imageFile.text()), imageFile.name, ); } catch (error: any) { @@ -9353,16 +9386,15 @@ class App extends React.Component { return new Promise>( async (resolve, reject) => { try { - this.files = { - ...this.files, - [fileId]: { + this.addMissingFiles([ + { mimeType, id: fileId, dataURL, created: Date.now(), lastRetrieved: Date.now(), }, - }; + ]); const cachedImageData = this.imageCache.get(fileId); if (!cachedImageData) { this.addNewImagesToImageCache(); diff --git a/packages/excalidraw/data/blob.ts b/packages/excalidraw/data/blob.ts index 1eb1e1bed..69a62cda5 100644 --- a/packages/excalidraw/data/blob.ts +++ b/packages/excalidraw/data/blob.ts @@ -8,13 +8,14 @@ import { calculateScrollCenter } from "../scene"; import type { AppState, DataURL, LibraryItem } from "../types"; import type { ValueOf } from "../utility-types"; import { bytesToHexString, isPromiseLike } from "../utils"; +import { base64ToString, stringToBase64, toByteString } from "./encode"; import type { FileSystemHandle } from "./filesystem"; import { nativeFileSystemSupported } from "./filesystem"; import { isValidExcalidrawData, isValidLibrary } from "./json"; import { restore, restoreLibraryItems } from "./restore"; import type { ImportedLibraryData } from "./types"; -const parseFileContents = async (blob: Blob | File) => { +const parseFileContents = async (blob: Blob | File): Promise => { let contents: string; if (blob.type === MIME_TYPES.png) { @@ -46,9 +47,7 @@ const parseFileContents = async (blob: Blob | File) => { } if (blob.type === MIME_TYPES.svg) { try { - return await ( - await import("./image") - ).decodeSvgMetadata({ + return (await import("./image")).decodeSvgMetadata({ svg: contents, }); } catch (error: any) { @@ -249,6 +248,7 @@ export const generateIdFromFile = async (file: File): Promise => { } }; +/** async. For sync variant, use getDataURL_sync */ export const getDataURL = async (file: Blob | File): Promise => { return new Promise((resolve, reject) => { const reader = new FileReader(); @@ -261,6 +261,16 @@ export const getDataURL = async (file: Blob | File): Promise => { }); }; +export const getDataURL_sync = ( + data: string | Uint8Array | ArrayBuffer, + mimeType: ValueOf, +): DataURL => { + return `data:${mimeType};base64,${stringToBase64( + toByteString(data), + true, + )}` as DataURL; +}; + export const dataURLToFile = (dataURL: DataURL, filename = "") => { const dataIndexStart = dataURL.indexOf(","); const byteString = atob(dataURL.slice(dataIndexStart + 1)); @@ -274,6 +284,10 @@ export const dataURLToFile = (dataURL: DataURL, filename = "") => { return new File([ab], filename, { type: mimeType }); }; +export const dataURLToString = (dataURL: DataURL) => { + return base64ToString(dataURL.slice(dataURL.indexOf(",") + 1)); +}; + export const resizeImageFile = async ( file: File, opts: { diff --git a/packages/excalidraw/data/encode.ts b/packages/excalidraw/data/encode.ts index 104ab1ca8..44e6b9974 100644 --- a/packages/excalidraw/data/encode.ts +++ b/packages/excalidraw/data/encode.ts @@ -5,24 +5,23 @@ import { encryptData, decryptData } from "./encryption"; // byte (binary) strings // ----------------------------------------------------------------------------- -// fast, Buffer-compatible implem -export const toByteString = ( - data: string | Uint8Array | ArrayBuffer, -): Promise => { - return new Promise((resolve, reject) => { - const blob = - typeof data === "string" - ? new Blob([new TextEncoder().encode(data)]) - : new Blob([data instanceof Uint8Array ? data : new Uint8Array(data)]); - const reader = new FileReader(); - reader.onload = (event) => { - if (!event.target || typeof event.target.result !== "string") { - return reject(new Error("couldn't convert to byte string")); - } - resolve(event.target.result); - }; - reader.readAsBinaryString(blob); - }); +// Buffer-compatible implem. +// +// Note that in V8, spreading the uint8array (by chunks) into +// `String.fromCharCode(...uint8array)` tends to be faster for large +// strings/buffers, in case perf is needed in the future. +export const toByteString = (data: string | Uint8Array | ArrayBuffer) => { + const bytes = + typeof data === "string" + ? new TextEncoder().encode(data) + : data instanceof Uint8Array + ? data + : new Uint8Array(data); + let bstring = ""; + for (const byte of bytes) { + bstring += String.fromCharCode(byte); + } + return bstring; }; const byteStringToArrayBuffer = (byteString: string) => { @@ -46,12 +45,12 @@ const byteStringToString = (byteString: string) => { * @param isByteString set to true if already byte string to prevent bloat * due to reencoding */ -export const stringToBase64 = async (str: string, isByteString = false) => { - return isByteString ? window.btoa(str) : window.btoa(await toByteString(str)); +export const stringToBase64 = (str: string, isByteString = false) => { + return isByteString ? window.btoa(str) : window.btoa(toByteString(str)); }; // async to align with stringToBase64 -export const base64ToString = async (base64: string, isByteString = false) => { +export const base64ToString = (base64: string, isByteString = false) => { return isByteString ? window.atob(base64) : byteStringToString(window.atob(base64)); @@ -82,18 +81,18 @@ type EncodedData = { /** * Encodes (and potentially compresses via zlib) text to byte string */ -export const encode = async ({ +export const encode = ({ text, compress, }: { text: string; /** defaults to `true`. If compression fails, falls back to bstring alone. */ compress?: boolean; -}): Promise => { +}): EncodedData => { let deflated!: string; if (compress !== false) { try { - deflated = await toByteString(deflate(text)); + deflated = toByteString(deflate(text)); } catch (error: any) { console.error("encode: cannot deflate", error); } @@ -102,11 +101,11 @@ export const encode = async ({ version: "1", encoding: "bstring", compressed: !!deflated, - encoded: deflated || (await toByteString(text)), + encoded: deflated || toByteString(text), }; }; -export const decode = async (data: EncodedData): Promise => { +export const decode = (data: EncodedData): string => { let decoded: string; switch (data.encoding) { @@ -114,7 +113,7 @@ export const decode = async (data: EncodedData): Promise => { // if compressed, do not double decode the bstring decoded = data.compressed ? data.encoded - : await byteStringToString(data.encoded); + : byteStringToString(data.encoded); break; default: throw new Error(`decode: unknown encoding "${data.encoding}"`); diff --git a/packages/excalidraw/data/image.ts b/packages/excalidraw/data/image.ts index 4c5ef39dc..02d059fd3 100644 --- a/packages/excalidraw/data/image.ts +++ b/packages/excalidraw/data/image.ts @@ -32,7 +32,7 @@ export const encodePngMetadata = async ({ const metadataChunk = tEXt.encode( MIME_TYPES.excalidraw, JSON.stringify( - await encode({ + encode({ text: metadata, compress: true, }), @@ -59,7 +59,7 @@ export const decodePngMetadata = async (blob: Blob) => { } throw new Error("FAILED"); } - return await decode(encodedData); + return decode(encodedData); } catch (error: any) { console.error(error); throw new Error("FAILED"); @@ -72,9 +72,9 @@ export const decodePngMetadata = async (blob: Blob) => { // SVG // ----------------------------------------------------------------------------- -export const encodeSvgMetadata = async ({ text }: { text: string }) => { - const base64 = await stringToBase64( - JSON.stringify(await encode({ text })), +export const encodeSvgMetadata = ({ text }: { text: string }) => { + const base64 = stringToBase64( + JSON.stringify(encode({ text })), true /* is already byte string */, ); @@ -87,7 +87,7 @@ export const encodeSvgMetadata = async ({ text }: { text: string }) => { return metadata; }; -export const decodeSvgMetadata = async ({ svg }: { svg: string }) => { +export const decodeSvgMetadata = ({ svg }: { svg: string }) => { if (svg.includes(`payload-type:${MIME_TYPES.excalidraw}`)) { const match = svg.match( /\s*(.+?)\s*/, @@ -100,7 +100,7 @@ export const decodeSvgMetadata = async ({ svg }: { svg: string }) => { const isByteString = version !== "1"; try { - const json = await base64ToString(match[1], isByteString); + const json = base64ToString(match[1], isByteString); const encodedData = JSON.parse(json); if (!("encoded" in encodedData)) { // legacy, un-encoded scene JSON @@ -112,7 +112,7 @@ export const decodeSvgMetadata = async ({ svg }: { svg: string }) => { } throw new Error("FAILED"); } - return await decode(encodedData); + return decode(encodedData); } catch (error: any) { console.error(error); throw new Error("FAILED"); diff --git a/packages/excalidraw/element/image.ts b/packages/excalidraw/element/image.ts index 33c585269..7088e301f 100644 --- a/packages/excalidraw/element/image.ts +++ b/packages/excalidraw/element/image.ts @@ -94,7 +94,7 @@ export const isHTMLSVGElement = (node: Node | null): node is SVGElement => { return node?.nodeName.toLowerCase() === "svg"; }; -export const normalizeSVG = async (SVGString: string) => { +export const normalizeSVG = (SVGString: string) => { const doc = new DOMParser().parseFromString(SVGString, MIME_TYPES.svg); const svg = doc.querySelector("svg"); const errorNode = doc.querySelector("parsererror"); diff --git a/packages/excalidraw/scene/export.ts b/packages/excalidraw/scene/export.ts index a311e4404..43e737be5 100644 --- a/packages/excalidraw/scene/export.ts +++ b/packages/excalidraw/scene/export.ts @@ -319,9 +319,7 @@ export const exportToSvg = async ( // the tempScene hack which duplicates and regenerates ids if (exportEmbedScene) { try { - metadata = await ( - await import("../data/image") - ).encodeSvgMetadata({ + metadata = (await import("../data/image")).encodeSvgMetadata({ // when embedding scene, we want to embed the origionally supplied // elements which don't contain the temp frame labels. // But it also requires that the exportToSvg is being supplied with diff --git a/packages/excalidraw/tests/export.test.tsx b/packages/excalidraw/tests/export.test.tsx index 35429d9b4..65b399dbb 100644 --- a/packages/excalidraw/tests/export.test.tsx +++ b/packages/excalidraw/tests/export.test.tsx @@ -62,10 +62,10 @@ describe("export", () => { }); it("test encoding/decoding scene for SVG export", async () => { - const encoded = await encodeSvgMetadata({ + const encoded = encodeSvgMetadata({ text: serializeAsJSON(testElements, h.state, {}, "local"), }); - const decoded = JSON.parse(await decodeSvgMetadata({ svg: encoded })); + const decoded = JSON.parse(decodeSvgMetadata({ svg: encoded })); expect(decoded.elements).toEqual([ expect.objectContaining({ type: "text", text: "😀" }), ]); diff --git a/packages/excalidraw/types.ts b/packages/excalidraw/types.ts index b843dfacb..453851a0e 100644 --- a/packages/excalidraw/types.ts +++ b/packages/excalidraw/types.ts @@ -106,6 +106,11 @@ export type BinaryFileData = { * Epoch timestamp in milliseconds. */ lastRetrieved?: number; + /** + * indicates the version of the file. This can be used to determine whether + * the file dataURL has changed e.g. as part of restore due to schema update. + */ + version?: number; }; export type BinaryFileMetadata = Omit; diff --git a/packages/utils/utils.unmocked.test.ts b/packages/utils/utils.unmocked.test.ts index 0cf0bb8a4..7c892cb59 100644 --- a/packages/utils/utils.unmocked.test.ts +++ b/packages/utils/utils.unmocked.test.ts @@ -27,7 +27,7 @@ describe("embedding scene data", () => { const svg = svgNode.outerHTML; - const parsedString = await decodeSvgMetadata({ svg }); + const parsedString = decodeSvgMetadata({ svg }); const importedData: ImportedDataState = JSON.parse(parsedString); expect(sourceElements.map((x) => x.id)).toEqual(