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

Add an option to toggle autosave for the document manager. #3734

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

ian-r-rose
Copy link
Member

Fixes #3728
This adds a new toggleable menu item and setting to disable document autosaving:
image

@ellisonbg
Copy link
Contributor

ellisonbg commented Jan 26, 2018 via email

@ellisonbg
Copy link
Contributor

ellisonbg commented Jan 26, 2018 via email

@ian-r-rose
Copy link
Member Author

Yes, it is a toggle. A configurable interval would also be nice. I have not done that here, as the specific auto-save procedure is a little subtle, and not strictly a single interval: https://github.com/ipython/ipython/wiki/IPEP-15:-Autosaving-the-IPython-Notebook

@ian-r-rose ian-r-rose added this to the Beta 2 milestone Jan 27, 2018
}
set autosave(value: boolean) {
this._autosave = value;
each(toArray(this._contexts), context => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._contexts is already an array, so you can just do this._contexts.forEach(...).

This pattern in the file probably exists as a a holdover from an earlier version where it was an iterator and not an array. Would you mind changing this spot and the other toArray(...) places that do this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Changing now.

@afshin
Copy link
Member

afshin commented Jan 28, 2018

👍 on the feature and implementation, I think it's great.

@ian-r-rose
Copy link
Member Author

A possible alternative implementation: rather than toggling autosave as true/false, we could have the minimum autosave interval be settable as a number, where Infinity is a valid interval, indicating no autosaving. Thoughts?

@afshin
Copy link
Member

afshin commented Jan 29, 2018

@ian-r-rose Unfortunately, Infinity is not a valid JSON value. We could do something like save interval in milliseconds where:

  • interval <= 0 is treated as: do not save
  • 0 < interval < 50 is treated as 50ms
  • interval >= 50 is treated as intervalms

Since this can be documented in the JSON schema it seems like an okay strategy. But for now, I think the boolean approach you're taking is good. Any interval functionality we add in the future could be implemented in a backward-compatible way, I imagine.

@ian-r-rose
Copy link
Member Author

Okay, that's good to know. Lets keep it simple for now, then.

Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@afshin
Copy link
Member

afshin commented Jan 29, 2018

Even though the milestone is Beta 2, since it is a backward-compatible API change, I'm happy merging it in now unless there's a reason to wait.

@ian-r-rose
Copy link
Member Author

No objection here 😃

@afshin afshin merged commit ec22756 into jupyterlab:master Jan 29, 2018
@fperez
Copy link
Contributor

fperez commented Feb 2, 2018

Greatly appreciated folks, many thanks!!

@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement pkg:docmanager status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UI controls to toggle autosave?
4 participants