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] Use File System Access API to choose file saving locations #39

Merged
merged 4 commits into from
Jan 9, 2021

Conversation

brad
Copy link
Contributor

@brad brad commented Dec 7, 2020

@zalo Here's what I've come up with for #30. The changes:

  • Save the project to a file of your choosing
  • Implement the Ctrl+S keyboard shortcut to save the project. If we have already saved, this automatically re-saves to the same file as before
  • Change the title of the code editor to the filename, with a * if the code has changed since the last save
  • Automatically Evaluate the code on save, just like OpenSCAD does
  • There were some conflicts with the "Make Main Project" feature so I removed it since it seems unecessary at this point
  • Keep track of the file handler when loading a project, and subsequent saves will save to the same file (NOTE: The first time you save after loading you will be greeted with a permission dialog. This is demonstrated in the final GIF below

NOTE: One caveat on this. There is a little weirdness when using this in a browser tab and reloading the page with ?code in the URL since we lose the file handle but retain the code. I'm not sure how to fix this. I prefer to use the app in an installed fashion so this didn't bother me.

Saving the project

cs-saving

Saving other file types

cs-saving-other-files

Loading then saving

cs-loading-then-saving

@vercel
Copy link

vercel bot commented Dec 7, 2020

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/zalo/cascade-studio/mejkc3jb3
✅ Preview: https://cascade-studio-git-fork-brad-better-saving.zalo.vercel.app

@zalo
Copy link
Owner

zalo commented Dec 7, 2020

Wow; this looks great! Thank you for adding .gifs! I’ll try to review this in the next few days.

The functionality/statefulness around the “MainProject” is a bit squirrelly (I was trying to emulate a bit of paper.js’s Sketch IDE auto-saving functionality, but probably didn’t hit the mark completely); hopefully I can push towards internal consistency there a bit more...

Thank you!

@zalo
Copy link
Owner

zalo commented Dec 8, 2020

Thoughts-in-progress (I'll be reviewing more and probably submitting changes over the week):

  • The saving and loading flow seems to work well; a big improvement over what was there before!

  • Hrmm usually loading a project file refreshes the page, but since this isn't, it's making trouble with the typescript editor thinking variables are getting redeclared in the global scope.

  • This also (incompletely) removes the local storage-saving aspect, which I'd be a little sad to see go, but I understand why it's confusing.

@brad
Copy link
Contributor Author

brad commented Dec 8, 2020

Wow; this looks great! Thank you for adding .gifs! I’ll try to review this in the next few days.

👍 No problem! It's the least I can do to contribute a little back to your awesome application

Hrmm usually loading a project file refreshes the page, but since this isn't, it's making trouble with the typescript editor thinking variables are getting redeclared in the global scope.

Shoot, I didn't notice that. I changed things to avoid refreshing the page in order to retain the file handle. I couldn't come up with any way to keep the file handle through a refresh, I don't think it can be serialized to localstorage for instance.

@zalo
Copy link
Owner

zalo commented Dec 8, 2020

No worries there; when I have time, I’ll come back through and figure out how to reset Monaco’s internal model.

It’s saying goodbye to the MainProject/automatic local storage-side of things that I’m going to need a bit more time to come to terms with...

This commit cleans up some of the dangling functionality left over from aggressively saving models to local storage.
@zalo
Copy link
Owner

zalo commented Dec 18, 2020

Sorry this took me so long to get to; I've added a bit of state cleanup (the initialize function didn't properly kill all the old state of the window on its second call before). This should also fix the intellisense throwing a fit every time a new project is loaded.

I'm still going to need a bit of time to think through the consequences of getting rid of local storage and the possible redundancy/conflict of URL sharing...

@brad
Copy link
Contributor Author

brad commented Dec 23, 2020

@zalo Thanks for the updates. I'll test it out over the holidays and maybe I'll be lucky and have a brilliant insight for how to resolve the conflict

@zalo zalo changed the title use File System Access API to choose file saving locations [FEAT] Use File System Access API to choose file saving locations Jan 9, 2021
@zalo
Copy link
Owner

zalo commented Jan 9, 2021

I think I'm going to merge this now and face the consequences later...

@zalo zalo merged commit d6cad5d into zalo:master Jan 9, 2021
@brad
Copy link
Contributor Author

brad commented Jan 9, 2021

Sounds good! Sorry, I forgot to report back but I did not have any insights 🤷

@brad brad deleted the better-saving branch January 9, 2021 01:03
@Irev-Dev
Copy link

Irev-Dev commented Jan 23, 2021

Sorry, I'm late to this. I'm a bit concerned that we've just nuked some cross-browser compatibility with this change.

Compatibility for Window.showOpenFilePicker is pretty low.

I mean chrome (and by extension edge) have the lion's share of browser users, but making exports incompatible with Firefox plus the default browser on mac and all phones seem like a bit of a shame. Maybe we could put a fall back in place for non-chrome browsers?

Thoughts @zalo, @brad?

Another thought. Instead of adding our own fallback, surely this problem of cross-browser file picking has been solved before, might there be an npm package that does this for us? (I haven't checked to see if there's anything good).

@brad
Copy link
Contributor Author

brad commented Jan 25, 2021

Sorry @zalo that was a rookie mistake not to check support on that feature. Looks like there are a couple ponyfills that could smooth out support:
https://github.com/GoogleChromeLabs/browser-nativefs
https://github.com/jimmywarting/native-file-system-adapter/

But as I understand it, it's not super easy to incorporate new dependencies at this point?

@Irev-Dev
Copy link

Polyfills nice @brad,

I've actually got a PR up to convert the project to modules with webpack in #39, I'm not sure how likely that is to be merged but that should make adding those polyfills much easier.

@ephetic
Copy link

ephetic commented Dec 13, 2022

FWIW, this isn't supported in Brave either, even though it's Chromium based.

@zalo
Copy link
Owner

zalo commented Dec 13, 2022

That stinks that this feature is taking so long to be taken up by other browsers...

I generally just use Chrome, but if there's a PR to add the old file support back, I'll take a look.

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.

4 participants