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 Reload from Disk command and menu item. #4615

Merged
merged 3 commits into from May 26, 2018

Conversation

@kcramer
Copy link
Contributor

@kcramer kcramer commented May 21, 2018

Issue #4154 looked straightforward so I added the reload ability as specified. However, there is an existing Revert command (named restoreCheckpoint in the code) which calls restoreCheckpoint() and revert(). Is there any concern the two options could be confusing?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

Thanks @kcramer, this is looking really good. I agree that it is potentially kind of confusing to have both a "Reload from Disk" and a "Revert to Checkpoint" command, but they still should both be useful (I think I personally would use "Reload" much more than checkpointing). What do you think @jasongrout?

Perhaps the best thing for now would be to add a sentence or two in the "Files" documentation about what the distinction is?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 22, 2018

I think both are good, and having docs would be great too. I might finally understand checkpoints!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

Hah, likewise.

@kcramer
Copy link
Contributor Author

@kcramer kcramer commented May 22, 2018

The Revert option currently is "Revert {type} to Saved". Perhaps changing it to something like "Revert {type} to Checkpoint" or "Revert {type} to Last Checkpoint" would be clearer?

I can add some documentation on the two although, as far as I can tell by searching the code base, currently checkpoints are only created when saving a file. Am I missing something or is the plan to add more checkpointing options in the future?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

Hm, they are subtly different in how how-out-of-band changes to the contents are handled, but it certainly is confusing. I think your suggestion is a good one: "Revert {type} to Last Checkpoint" and "Reload {type} from Disk" are clearer.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 22, 2018

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented May 22, 2018

Hm, they are subtly different in how how-out-of-band changes to the contents are handled, but it certainly is confusing.

Do we have a sense of when reverting to the last checkpoint would make sense over reverting from disk? We could just remove that option if it's too confusing. I don't think many users will have an intuition for when the last checkpoint was.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

I think it's largely a feature-parity issue (though the classic notebook has a full submenu with different labeled checkpoints). I don't have much knowledge of how people like to use the checkpointing system.

@kcramer
Copy link
Contributor Author

@kcramer kcramer commented May 22, 2018

With the classic notebook, the checkpoints were handling what is now done with auto save. Is the plan to add more features around checkpoints?

For now I could update the "Revert {type} to Saved" to be "Revert {type} to Checkpoint".

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 22, 2018

Are checkpoints used for anything other than notebooks? I thought they were very specific to notebooks.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

I'm pretty sure we use them for all documents.

@vidartf
Copy link
Member

@vidartf vidartf commented May 23, 2018

The implementation of checkpoints can be extended on the server side to be more powerful (history of checkpoint, etc. etc.). I know that some people do this (I think it might be a closed solution?). Checkpoints can be used for any file.

@kcramer
Copy link
Contributor Author

@kcramer kcramer commented May 23, 2018

I updated the Revert menu item to specify checkpoint and not saved, as discussed.

I tried to write out a short paragraph that could be added in the documentation but realize it isn't easy to explain it briefly.

Here is my best attempt to explain it:

If you want to discard your changes, you have two options.  Reload will 
discard your current changes and load the version of the file on the disk.  
Revert will restore the last checkpoint.  Checkpoints are separate versions 
of the document that are written when you explicitly save the document or
possibly at other times if you use a custom checkpoint extension.

I believe that is technically true although may not fully explain checkpoints, especially the customization aspect. The other complication is that autosave is the one place I have seen where saving and checkpointing can be out of sync. Whether you want reload or revert could depend on your autosave setting and your goal.

@ian-r-rose
Copy link
Member

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

Thanks @kcramer, this looks great! We can improve the documentation, as well as maybe adding a checkpoing submenu, in some follow-up work.

@ian-r-rose ian-r-rose merged commit 37ad483 into jupyterlab:master May 26, 2018
2 checks passed
@kcramer kcramer deleted the reload-from-disk branch May 26, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Jun 12, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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.

None yet

5 participants