feat: Add auto save support when working on an existing file

[kbariotis]

For #2733

Auto save will be saving changes as they happen. The debounce will put these changes in order so they don't override each other.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/HzpMc3AYwXdKwV94JNkz7hhFkXwY
✅ Preview: https://excalidraw-git-kb-auto-save-support-excalidraw.vercel.app

[lipis]

Maybe instead of a timer.. when the scene is changed?

[kbariotis]

@lipis are you referring to the debounce function? it rather serves as a queue in this instance to avoid doing multiple calls at the same time. Although in this case, I'm not 100% sure how much better it is.

[dwelle]

Thanks for working on this @kbariotis.

lipis are you referring to the debounce function? it rather serves as a queue in this instance to avoid doing multiple calls at the same time. Although in this case, I'm not 100% sure how much better it is.

Let's keep the debounce.

We should also only attempt to save if there's an active file handle, and even in that case we should listen for errors and not attempt to autosave multiple times if the previous few tries failed. This can happen either due to some error, or when the user decides to cancel the "save changes" dialog.

I've gotten into a bad cycle when it tried save to a file without having a file handle, which was opening the save-file dialog ad infinitum.

Given that, let's only display the autosave option if the browser supports it (the new Filesystem API).

As for where to put the toggle, maybe the export dialog is a better place? Because we'll also want some tooltip with more information detailing in which cases the autosave will apply to.

[kbariotis]

OK, I think its looking much better now. :)

@dwelle

Given that, let's only display the autosave option if the browser supports it (the new Filesystem API).

The browser-fs-access is a "ponyfill" so should always work, am I thinking right?

[tomayac]

This should indeed only be done with the new File System Access API, else, we litter the Downloads folder of the user.

Is there a way to only save when there’s a change? A naive implementation could be to MD5 the stringified Excalidraw elements array and only save when there’s a difference.

[dwelle]

Is there a way to only save when there’s a change? A naive implementation could be to MD5 the stringified Excalidraw elements array and only save when there’s a difference.

IMO this optimization isn't needed at this point (and it's also not easy, see #3185). The save is debounced so it shouldn't cause perf issues.

But yes, to answer @kbariotis question — the fs-browser-access cannot polyfill the FS API's file handle (and saving back to the same file), so we can't use autosave for legacy FS.

[tomayac]

My brain is in weekend mode and I may miss things, but if all we need to know is if things have changed since the last save, hashing the array should work, no? It could happen in line 916 in App.tsx. Again in weekend mode and on mobile, but is this autosaving every 500ms? If so, is this what apps like Word do? It seems wasteful at first sight, but I have to be honest no idea what’s a usual value here.

[kbariotis]

hehe no worries @tomayac, no it doesn't save every X time, it saves when the component updates, so when there is a change. The timeout is for debouncing subsequent updates so they don't override each other. :)

[kbariotis]

@tomayac is it worth exporting this helper function to use with this PR? I'm happy to do a PR. :)

[tomayac]

The timeout is for debouncing subsequent updates so they don't override each other. :)

Phew, seeing it now. Sorry for the noise.

@tomayac is it worth exporting this helper function to use with this PR? I'm happy to do a PR. :)

Might be worth it, indeed. We have another “supported” check somewhere, so yes!

[dwelle]

Yea, we have several of these checks hardcoded across the app already so we may as well factor them into a single API.

[kbariotis]

Awesome, if we do merge the PR in browser-fs-access we can replace these checks.

[tomayac]

Awesome, if we do merge the PR in browser-fs-access we can replace these checks.

I’ll have a look on Monday (since this is a work project) and merge this. Thanks already for the PR!

[tomayac]

Awesome, if we do merge the PR in browser-fs-access we can replace these checks.

This has happened in #3303.

[tomayac]

(#3303 has been merged now.)

I'm not sure the export dialog is the right place for the autosave checkbox. It's not very discoverable.

Is it time for a proper settings dialog? What else could go in there would be the dark mode toggle and the language selector. It could be a cog icon in the lower right corner, where currently the language selection is.

[dwelle]

@ad1992 thoughts on how to make it obvious this doesn't relate to server persistence (not just, but mainly for host apps that autosave to server)?

[ad1992]

@ad1992 thoughts on how to make it obvious this doesn't relate to server persistence (not just, but mainly for host apps that autosave to server)?

May be instead of just saying Autosave we can say something like Autosave to the existing file to make it explicit that its only for file system autosave ? Though we are adding a tooltip but if the label is itself more explicit then it won't cause any confusion I think

[ad1992]

Played with this and it looks great @kbariotis 🎉

[ad1992]

suggestion: if else might be more readable here instead of nested ternary operators.

[dwelle]

Would require 3 if/else branches and duplicating the setState data which will be error-prone.

[ad1992]

I meant something like 👇

let toastMessage = t("toast.autosaveFailed");
if (error.name === "error.name === "NotAllowedError"") {
  toastMessage = t("toast.autosaveFailed_notAllowed");
}
else if (error.name === "NotFoundError") { 
  toastMessage = t("toast.autosaveFailed");
}
this.setState({ autosave: false, toastMessage});

[ad1992]

instead of checking here can we render this comp only when supported ?

[dwelle]

After discussion, here's current status:

• There's a problem where the autosave is triggered on any change to app state, even one that's unrelated to the file. This results in saving too early and unnecessarily prompting the user for permissions. Most notably, the users are prompted immediately after importing a file. This needs to be solved — a naive fix would be to diff last saved state to the current one. If no change, don't attempt to save.
• There's a lack of consensus on where the setting should be displayed, how automated it should be, what the default should be. One thing we agree on is that we shouldn't compromise on Excalidraw's simplicity principle.
• Regardless, I think we need to track two states: currentFileAutosaveStatus & autosavePreference. This is so that users don't have to re-enable the setting every time there's a problem and we disable autosave for that session/file.

That said, first we need to reevaluate whether the benefit of this feature outweighs the costs — something we should have done at the beginning, but lots of the specs weren't clear from the start.

For example, the fact you need to separately give permissions to each file (and in each session) adds a lot of friction (this could in theory be partly alleviated by caching file handles into IDB so that you don't have to give permissions to the same file across sessions — provided the IDB flow would work like this).

[tomayac]

• There's a problem where the autosave is triggered on any change to app state, even one that's unrelated to the file. This results in saving too early and unnecessarily prompting the user for permissions. Most notably, the users are prompted immediately after importing a file. This needs to be solved — a naive fix would be to diff last saved state to the current one. If no change, don't attempt to save.

The naive fix sounds workable. Else a flag that tells if the actual (future) file contents have changed.

• There's a lack of consensus on where the setting should be displayed, how automated it should be, what the default should be. One thing we agree on is that we shouldn't compromise on Excalidraw's simplicity principle.

It could also just be auto-on, Glitch style or Google Docs style (which, to be fair, work in the cloud, not the local file system).

• Regardless, I think we need to track two states: currentFileAutosaveStatus & autosavePreference. This is so that users don't have to re-enable the setting every time there's a problem and we disable autosave for that session/file.

The goal of course would be to make it so that there are no problems… :-)

For example, the fact you need to separately give permissions to each file (and in each session) adds a lot of friction (this could in theory be partly alleviated by caching file handles into IDB so that you don't have to give permissions to the same file across sessions — provided the IDB flow would work like this).

File handles can be serialized, so this would work. Also, some of the permission friction may go away in the future, we're still experimenting with what the right threshold is. For example, see WICG/file-system-access#288 or WICG/file-system-access#89.

[dwelle]

It could also just be auto-on, Glitch style or Google Docs style (which, to be fair, work in the cloud, not the local file system).

I still think it should be opt-in, user initiated. Otherwise every time users will import a file and edit it'll prompt them for write perms and they'll have no idea what's going on.

AFAIK only cloud-based apps do it. No desktop graphics editor I know has autosave enabled by default.

The goal of course would be to make it so that there are no problems… :-)

The "problems" here include user rejecting the permission prompt :).

[soulsam480]

Is it coming ?

[soulsam480]

Ok I'm using a very shitty hack. Open a file from fs and do a normal save using the button. Then paste this to js console

(() => {
  const elements = document.getElementsByClassName(
    'ToolIcon_type_button ToolIcon_size_medium ToolIcon_type_button--show ToolIcon ToolIcon--plain',
  );

  if (elements === null || elements.length == 0) return;

  setInterval(() => {
    elements[1].click();
  }, 60000);
})();

I know, I know this is bad. But I'm doing this as a hack for some help.

[artemanufrij]

Hey guys, what ist the status of this PR? I would be very helpful to have an auto save option. I lost a lot of my work yesterday, because I opened a other board for quick check... 😢

[sojusnik]

Would also love to see this implemented soon, because loosing your work due to an unsaved file is no fun.