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

@ian-r-rose ian-r-rose commented Jan 26, 2018

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

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jan 26, 2018

Loading

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jan 26, 2018

Loading

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jan 26, 2018

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

Loading

@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

@afshin afshin Jan 28, 2018

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?

Loading

Copy link
Member Author

@ian-r-rose ian-r-rose Jan 29, 2018

Choose a reason for hiding this comment

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

Good point. Changing now.

Loading

@afshin
Copy link
Member

@afshin afshin commented Jan 28, 2018

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

Loading

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jan 29, 2018

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?

Loading

@afshin
Copy link
Member

@afshin 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.

Loading

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jan 29, 2018

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

Loading

afshin
afshin approved these changes Jan 29, 2018
Copy link
Member

@afshin afshin left a comment

👍

Loading

@afshin
Copy link
Member

@afshin 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.

Loading

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jan 29, 2018

No objection here 😃

Loading

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

@fperez fperez commented Feb 2, 2018

Greatly appreciated folks, many thanks!!

Loading

@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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants