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.

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

feat: resave to png/svg with metadata if you loaded your scene from a png/svg file #3645

Merged
merged 6 commits into from
Jul 15, 2021

Conversation

alsuren
Copy link
Contributor

@alsuren alsuren commented May 26, 2021

This is a cleaned-up version of #3516, and fixes #2261. It doesn't attempt anywhere near the number of refactors that I outlined in that PR, because I realised that many of the functions I wanted to change were already part of the public API.

  • It is based on top of feat: save exportScale in AppState #3580 (which is required to save scale as appState.exportScale), so you can ignore everything before add saveType to AppState when reviewing, if you have already reviewed that PR.
  • it adds appState.saveType (png or svg for images with embedded scene, or null for the default .excalidraw json format)
  • It adds a new resaveAsImageWithScene() helper, as shown in the diagram below, which calls out to the exportScene machinery when you cmd+S.

plan-2

Steps to test:

  • Download the above diagram (which is a png file that contains an excalidraw scene).
  • In an excalidraw from this branch/pr build:
    • open up the png image that you just downloaded
    • make an edit
    • cmd+s to save (on macos. Other operating systems are available)
    • verify that your changes have been persisted to the png image.

Things that have not changed:

  • If you export as an image with embedded scene, it will not set fileHandle or saveType, so:
    • cmd+s will still continue to save to your .exalidraw file rather than your image file.
    • If you want cmd+s to resave to the image, you will need to open up the image file after exporting.

@vercel
Copy link

vercel bot commented May 26, 2021

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/BB1zn4SE2RE19pDiRtaqA95L7Fcp
✅ Preview: https://excalidraw-git-fork-alsuren-resave-attempt-2-excalidraw.vercel.app

@alsuren
Copy link
Contributor Author

alsuren commented Jun 14, 2021

Force-pushed to resolve conflicts.

Entirely imports, I think. Next time this happens, I might try putting my imports higher up in the bottom of the block, to reduce future conflicts.

@dwelle
Copy link
Member

dwelle commented Jul 15, 2021

Hey. So checking the PR, do we need to store saveType to appState?

@dwelle
Copy link
Member

dwelle commented Jul 15, 2021

I've pushed a version without storing saveType → less complexity, less state. Works fine. Lemme know if I've introduced any regression and/or the saveType should be serving a specific purpose in the future.

Also fixed a case of drag&dropping an image on canvas which previously wasn't persisting fileHandle.

@dwelle
Copy link
Member

dwelle commented Jul 15, 2021

Also fixed not persisting fileHandle on image export 🚀

@alsuren
Copy link
Contributor Author

alsuren commented Jul 15, 2021

I've pushed a version without storing saveType → less complexity, less state. Works fine. Lemme know if I've introduced any regression and/or the saveType should be serving a specific purpose in the future.

Sweet. I had just assumed that fileHandle was opaque because "handle"s are usually opaque in other languages that I used. That looks much better 😀

@alsuren
Copy link
Contributor Author

alsuren commented Jul 15, 2021

Also fixed not persisting fileHandle on image export 🚀

I had done this as a separate commit on a different PR because I wasn't sure of the UX requirements : f5a248b

@dwelle
Copy link
Member

dwelle commented Jul 15, 2021

I had done this as a separate commit on a different PR because I wasn't sure of the UX requirements

I see. Well, it's done now so let's ship it :)

@dwelle dwelle merged commit 685abac into excalidraw:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embedded scene in PNG/SVG should reuse fileHandle
2 participants