Abort saving if a file cannot be saved #16900
Conversation
|
Thanks for making a pull request to jupyterlab! |
packages/docregistry/src/context.ts
Outdated
| if (!this.canSave) { | ||
| // File cannot be saved. | ||
| return Promise.reject(); | ||
| } |
There was a problem hiding this comment.
Just for fun, I asked Claude to review this and it reverted with this suggestion:
if (!this.canSave) {
// File cannot be saved.
return Promise.reject(new Error('File cannot be saved')); // Add error message
}
Feel free to ignore Claude! Else it all LGTM.
|
Thanks @JasonWeill for looking into this 👍 The test failures appear to be related though: |
packages/docregistry/src/context.ts
Outdated
| if (!this.canSave) { | ||
| // File cannot be saved. The "save" command is disabled in the UI, | ||
| // but if the user tries to save anyway, act as though it succeeded. | ||
| this._saveState.emit('completed'); | ||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
It would be good to consider what effect it has on the logic for updating the model hash (which is needed to prevent spurious "File Changed" dialogs). This condition prevents updating contentHash as it is performed in _maybeSave method and this condition prevents ever reaching it. I hope this would not be a problem for RTC because it now uses fileChanged signal to propagate new contentHash since:
- Update contents model on file change due to save from RTC #16695
- Fix spurious "File Changed" dialog in JupyterLab 4.1+/Notebook 7.1+ jupyter-collaboration#337
I also think that it would not make the situation worse for other drives either as in that case they would not update the hash as they could not save the content.
I think that all we need to do is document the side-effects when saving and when saving is skipped, especially if we are going to emit saveState even if the save did not happen.
88ca556 to
8a08b6d
Compare
|
It might be good to document what happens on save (what events are emitted depending on whether save is supported or not) in https://jupyterlab.readthedocs.io/en/latest/extension/documents.html |
|
I think the tests failing might be related to jupyterlab/packages/docregistry/src/context.ts Lines 225 to 230 in 8a08b6d |
8a08b6d to
e78d30d
Compare
|
@krassowski Tests are passing now! Could you please take another look when you get the chance? |
|
@meeseeksdev please backport to 4.3.x |
Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>
|
@meeseeksdev please backport to 4.2.x |
* Revert "Alternate description for disabled filters" This reverts commit 059fb7e. * Revert "Revert "Alternate description for disabled filters"" This reverts commit 27ed1d0. * Aborts saving if the file cannot be saved * Treat save as successful if canSave is false * Emit a "completed" event when saving an unsaveable file * Modifies save check to use collaborative flag
References
Fixes jupyterlab/jupyter-collaboration#364.
Code changes
Aborts saving if the
canSaveproperty of the context isfalse. This isfalsewhen the context's model is in collaborative mode.User-facing changes
When the user presses CTRL+S or ⌘+S with Jupyter Collaboration installed, the save command does nothing, and does not cause an error dialog to appear. Without Jupyter Collaboration installed, the command works as it did before.
Backwards-incompatible changes
None.