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

Editor - Prompt warning when overwriting a file that is modified on disk #2783

Merged
merged 7 commits into from Feb 12, 2018

Conversation

@unnamedplay-r
Copy link
Contributor

@unnamedplay-r unnamedplay-r commented Aug 19, 2017

Closes #2726.

Adds functionality to issue the user a warning and prompts them to overwrite the file currently modified on disk.

For any reviewer, I have a question:

  1. Do you have any suggestions on how I might make the 'overwrite modal' modular? Can we add this into base/js/dialog.js and modify the notebook and the editor to use the same function? There is a lot of overlapping functionality involved in this prompt, but I can also understand if we want to keep the features isolated from another.

TODO

  1. Review questions and material posed in personal notes
  2. Add comments and documentation to save function in format of notebook.js
  3. Learn how to create tests for this function!

Personal Notes

Some notes I need to ponder about next time I take this on:

  • When issuing events such as {Editor}.events.trigger('file_renamed.Editor', model), what assumptions do these events come with? Should we be modifying the {Editor}.last_modified property after, or before these events?
  • Look for more areas where I need to update the last modified variable.
  • What does the _clean_state function do, and what is the codemirror?
  • Why is there a variable _last_modified in savewidget.js for ensuring the renaming of the file, but isn't found in the main editor class? Can we use this instead of creating another variable within the Editor class? Also, why is it used to display information about the last save event to the user in base/js/notebook.js (using this file to model the functionality)?
  • Learn a bit more about i18n and what purpose it serves (so far, it looks like an API that enforces semantics to the user)
  • Test logic for function(error) on the promise when saving
  • Am I using ‘that’ too much? Where can I switch it for ‘this’ to properly address the right object?
  • Should the editor not address file renames as ‘last modified’ and let it write into the file if the file is renamed? (look into how base/js/notebook.js handles this)
@minrk minrk added this to the 5.2 milestone Aug 21, 2017
@unnamedplay-r
Copy link
Contributor Author

@unnamedplay-r unnamedplay-r commented Aug 22, 2017

Hi @minrk, @takluyver, can one of you look over this pull request? Suggestions, and criticism welcome.

The left over personal tasks for me are to remove some of the personal comments, and set up tests for this.

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Aug 23, 2017

Thanks! We're just about to release notebook 5.1, and most of the team are currently at Jupytercon, so it might be a week or two before we can look at this properly. I can probably have a quick look today, but I'd like others to have a look too.

Loading


var _save = function () {
// What does this event do? Does this always need to happen,
// even if the file can't be saved? What is dependent on it?
Copy link
Member

@takluyver takluyver Aug 23, 2017

Choose a reason for hiding this comment

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

It appears that nothing in our code currently listens for it, but it parallels an event notebook_saving.Notebook which is used to show a 'Saving notebook' notification. It's possible that we'll want something similar in the future, and I don't think it hurts to have an event.

There should probably be a corresponding file_save_failed.Editor event to fire on failure, but that needn't be in this PR.

Loading

Copy link
Contributor Author

@unnamedplay-r unnamedplay-r Aug 25, 2017

Choose a reason for hiding this comment

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

Gotcha, I'll hold off adding in functionality for this. @takluyver, is there a backlog where we can place tasks and items, or does the issues tab function as this?

Loading

Copy link
Member

@takluyver takluyver Aug 25, 2017

Choose a reason for hiding this comment

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

Yup, it's the issues. Thanks!

Loading

// record change generation for isClean
// (I don't know what this implies for the editor)
Copy link
Member

@takluyver takluyver Aug 23, 2017

Choose a reason for hiding this comment

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

The 'generation' is a number Codemirror uses to tell whether the editor is 'clean' - whether the current text matches what is saved. This gets the current generation when we save the file. Elsewhere in the code, we pass the number to isClean to ask Codemirror if the file has changed since the last save.

This means that if you save, type something, and Ctrl-z to undo, we know that you're back at the saved state.

So this should probably only happen when we're actually about to save. If the users cancels the dialog, this.generation shouldn't have changed.

Loading

Copy link
Member

@takluyver takluyver Aug 23, 2017

Choose a reason for hiding this comment

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

I meant to add this link to the Codemirror docs: http://codemirror.net/doc/manual.html#changeGeneration

Loading

Copy link
Contributor Author

@unnamedplay-r unnamedplay-r Aug 25, 2017

Choose a reason for hiding this comment

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

Ah, fascinating! Thanks for the explanation and link. I'll also pull the changeGeneration method within the promise 'then' statement that validates the save.

Loading

@gnestor
Copy link
Contributor

@gnestor gnestor commented Oct 9, 2017

We are about to release 5.2, so I'm going to mark this for 5.3. Let's try to get this merged soon! 👍

Loading

@gnestor gnestor removed this from the 5.2 milestone Oct 9, 2017
@gnestor gnestor added this to the 5.3 milestone Oct 9, 2017
@unnamedplay-r
Copy link
Contributor Author

@unnamedplay-r unnamedplay-r commented Jan 4, 2018

@gnestor, this should be all set for a merge. I've reverted the changes I've made to the test suite that were impacting the build. I was trying to create a test for this patch but came under time constraints from other duties. Let me know if you have any suggestions for my PR, or if I could be of anymore assistance for this bug.

Loading

@gnestor gnestor removed this from the 5.3 milestone Jan 5, 2018
@gnestor gnestor added this to the 5.4 milestone Jan 5, 2018
@unnamedplay-r unnamedplay-r changed the title [WIP] Editor - Prompt warning when overwriting a file that is modified on disk Editor - Prompt warning when overwriting a file that is modified on disk Jan 9, 2018
@gnestor
Copy link
Contributor

@gnestor gnestor commented Jan 19, 2018

@unnamedplay-r I checked this out and tested it and it appears to be working but not as I would expect.

Master:

  • Open the same text file in two different tabs
  • Edit the file in one tab
  • Save
    • Save successful
  • Switch to the other tab
  • Save
    • Save successful
    • Overwrites file without warning

This PR:

  • Open the same text file in two different tabs
  • Edit the file in one tab
  • Save
    • File is saved to disk but the normal checkmark feedback is not displayed and the unsaved changes "*" in the tab is not cleared
  • Save again
    • Prompted to overwrite, click overwrite
      • The prompt shouldn't be displayed in this case because the file hasn't changed elsewhere, it changed here
    • File is saved to disk but the normal checkmark feedback is not displayed and the unsaved changes "*" in the tab is not cleared
  • Switch to the other tab
  • Save
    • Prompted to overwrite, click overwrite
    • Save successful

Loading

@unnamedplay-r
Copy link
Contributor Author

@unnamedplay-r unnamedplay-r commented Jan 20, 2018

@gnestor, when I perform the steps you provided, I don't experience any of the behavior you're describing. This has been tested on Mac's common browsers: Safari, Chrome, Firefox with cache cleared, and also done in private/incognito modes.

When I save a modified file, then re-save it, I'm not presented the overwrite modal. The modal appears only until I modify the second file in the other tab and save.

Two questions:

  1. Is there a way I can mimic your environment to see what's happening here?
  2. Are you testing this from the merged branch of the current master and this branch? (I don't see any major differences between the master and what's proposed here besides additional key bindings)

Loading

@takluyver takluyver removed this from the 5.4 milestone Jan 27, 2018
@takluyver takluyver added this to the 5.5 milestone Jan 27, 2018
@takluyver
Copy link
Member

@takluyver takluyver commented Feb 8, 2018

I tested this in Firefox and saw the same behaviour that @gnestor described, and I noticed an error message in the JS console about "this is undefined". With the tiny fix I just pushed, it behaves as expected for me.

@unnamedplay-r I'm not sure why it was working for you but not for us. As I could see the error, I hope you don't mind me fixing it directly!

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Feb 9, 2018

cc @gnestor - if you're happy with this, I think it's ready to merge.

Loading

@unnamedplay-r
Copy link
Contributor Author

@unnamedplay-r unnamedplay-r commented Feb 11, 2018

Thanks for digging into the error and fixing it @takluyver. It's highly appreciated!

Loading

// We want to check last_modified (disk) > that.last_modified (our last save)
// In some cases the filesystem reports an inconsistent time,
// so we allow 0.5 seconds difference before complaining.
if ((last_modified.getTime() - that.last_modified.getTime()) > 500) { // 500 ms
Copy link
Member

@takluyver takluyver Feb 12, 2018

Choose a reason for hiding this comment

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

We recently merged a pull request to make this 0.5s difference configurable in the notebook - #3273 . Could you have a look at that and copy the functionality here? Thanks!

Loading

@gnestor
Copy link
Contributor

@gnestor gnestor commented Feb 12, 2018

@takluyver I should've caught that console error 😳

@unnamedplay-r It's working great! Thank you!! 🙏

Loading

@gnestor gnestor merged commit 331a7a2 into jupyter:master Feb 12, 2018
4 checks passed
Loading
@cancan101
Copy link
Contributor

@cancan101 cancan101 commented Feb 21, 2018

Will this change apply also to jupyterlab or should I file a similar issues there as well?

Loading

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 21, 2018

This behavior already exists in JupyterLab, so no further action is necessary.

Loading

@cancan101
Copy link
Contributor

@cancan101 cancan101 commented Feb 21, 2018

I'm reasonable sure that I am not seeing this behavior. e.g I have an open editor and do a git checkout but don't see a warning etc

Loading

@cancan101
Copy link
Contributor

@cancan101 cancan101 commented Feb 21, 2018

Ah okay, it doesn't notify until you try to save which is a little annoying. Might be nice for it to poll / check on some basis or events.

Loading

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.

6 participants