Piskel falsely claims file is saved when using keyboard shortcut

[lostfictions]

Describe the bug

When adding a sprite and editing it with Piskel, pressing Ctrl+S yields a notification that the sprite was successfully saved. However, on closing the Piskel modal, all changes are lost.

To Reproduce

Steps to reproduce the behavior:

1. Create a new blank project

2. Right click the scene backdrop and pick "Insert new..."

3. Pick "Sprite" from the menu. The Properties pane for the new sprite is shown. Click the "Add an animation" button. Optionally, select an existing image.

4. Click "Edit with Piskel" to begin drawing the animation. Add multiple frames if you'd like.

5. Hit Ctrl-S. A "Successfully saved!" toast is shown in the bottom-right corner of the Piskel editor.

   

6. Close the Piskel window. No confirmation about closing with unsaved changes is shown.

7. If you started from scratch, the animation shown in the Properties plane is blank. If you started from an existing sprite, it will be shown in its original state before you made edits in Piskel. In both cases, the animation was not actually saved.

   

---

To actually save the sprite, it looks like you have to open the "..." menu in the top right corner and select "Save". This immediately dismisses the editor. The workflow here is extremely confusing -- I'm really not sure why Ctrl+S claims to work but doesn't actually do anything.

Other details

• OS: Linux, using GDevelop Flatpak 5.0.140
• Project file type is set to "Multiple files", since I saw this was recommended for collaboration/source control. I'm unsure whether this makes a difference.

[AlexandreSi]

Hi, thanks for reporting this!
I'm currently following your steps and I wonder what action you actually do to "Close the Piskel window".
On my end, to close it, I either have to click save or cancel so I cannot leave Piskel with unsaved changes.

Either way, this Ctrl+S false shortcut should be removed.

[AlexandreSi]

Just realized that this Ctrl+S actually does something: it stores the current work to a browser-based storage.

You should be able to find the current work saved in:

Then:

And finally:

This feature could be interesting but it's hard to wrap our head around different "save slots".

It must be the same place where Piskel saves color palettes.

I'll flag this issue differently then.

[lostfictions]

Hi, thanks for reporting this! I'm currently following your steps and I wonder what action you actually do to "Close the Piskel window". On my end, to close it, I either have to click save or cancel so I cannot leave Piskel with unsaved changes.

That sounds like a second bug then -- for me, the Piskel modal has a close button, and it doesn't give me any confirmation prompt when I try to close it with unsaved changes:

simplescreenrecorder-2022-08-30_12.38.20.mp4

Just realized that this Ctrl+S actually does something: it stores the current work to a browser-based storage.

Thanks for pointing this out, I was wondering if it was doing something like that! I understand what's going on as a developer who sometimes uses Electron -- but as a user of GDevelop who might not have any knowledge of any of this, having this second semi-transient place where things can be "saved" (but not to the filesystem!) which can only be accessed through a completely different UI flow that solely exists within this drawing tool modal is extremely confusing.

Personally I think it would probably be much less confusing to remove this "feature" and UI entirely. I agree that backups could be cool, but it might make sense for them to be more holistically integrated with GDevelop as a whole and stored somewhere they could easily be located, rather that being an obscure feature of a specific sub-tool that only lives within the Electron instance's LocalStorage or IndexedDB.

[AlexandreSi]

I agree with all you're saying.

The thing is that, at the moment, we don't really maintain the version of Piskel we use. There has been discussions about replacing it/taking over maintenance of Piskel (See #3670).

An immediate thing I can think of is looking into removing the possibility to close the window without clicking either on Save or Cancel like in my experience, but I'm not sure it's feasible with Electron. I'll look into it.

[AlexandreSi]

After a bit of work, here is what I ended up to:
Our users have a different experience with external editors:

• Windows and Linux users see the top bar of the detached window. So, they can:
  • close said window with the cross button;
  • move the window around, which is convenient.
• Mac users only see the content of the window and have to choose between Save and Cancel buttons to close the window.

So what I'm suggesting is to prevent the window to be closed via the cross button. Here is what we could do:

When closing an Electron BrowserWindow with the cross button, a few events are fired in this order:

• close
• beforeunload
• unload
• closed

The same path is taken when closing the window programmatically with windows.close(), but if we use window.destroy() (See https://www.electronjs.org/docs/latest/api/browser-window), only the closed event is fired.

So we could:

• Add event listeners on the close events that displays an alert that reads "It is unsafe to close the window, please choose Save or Cancel at the top of the window." and calls event.preventDefault() to prevent the window to be closed.
• We are not using the events close, unload and beforeunload so we could use window.destroy() instead of windows.close() so that the preventDefault above wouldn't prevent to close the window programmatically.

I'm on Mac so I cannot really make this change and make sure it doesn't break anything. Maybe @D8H or @ClementPasteau if you have the dev environment installed on a Windows.