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

Ctrl-S is not disabled #364

Open
davidbrochart opened this issue Sep 24, 2024 · 14 comments · May be fixed by jupyterlab/jupyterlab#16900
Open

Ctrl-S is not disabled #364

davidbrochart opened this issue Sep 24, 2024 · 14 comments · May be fixed by jupyterlab/jupyterlab#16900
Labels
bug Something isn't working

Comments

@davidbrochart
Copy link
Collaborator

Description

Hitting Ctrl-S in a document that is "dirty" (not yet saved to disk) shows the following warning:

ctrl-s.mp4

Reproduce

  1. Create a new text file.
  2. Enter some text and type Ctrl-S before auto-save (one second by default).
  3. See the warning.

Expected behavior

Ctrl-S should probably be disabled, as is the save button in a notebook:
image

@davidbrochart davidbrochart added the bug Something isn't working label Sep 24, 2024
@krassowski
Copy link
Member

Are you testing with JupyterLab 4.3.0b1 or with 4.2? It should be fixed by jupyterlab/jupyterlab#16695.

Ctrl-S should probably be disabled, as is the save button in a notebook:

I don't agree here. Users most often want to have this button just for reassurance. What happens on click is a separate question. They also want to be able to press Ctrl + S. What happens is again a separate question.

@davidbrochart
Copy link
Collaborator Author

I'm testing with JupyterLab v4.2.5, good to know that it was already fixed in JupyterLab!

Users most often want to have this button just for reassurance.

So do you think this button should be restored (not greyed out) in jupyter-collaboration? AFAIK there is no such button in Google Docs, and Ctrl-S does't have any effect there.

@davidbrochart
Copy link
Collaborator Author

I just tested with JupyterLab v4.3.0b1 and the problem is still present. There might still be a problem when we get the model, because the date didn't change after saving?

@krassowski
Copy link
Member

I just tested with JupyterLab v4.3.0b1 and the problem is still present

With which version of collaboration packages? This was fixed by #337 on this side. It could be a regression due to newer changes.

@davidbrochart
Copy link
Collaborator Author

The issue appears with jupyter-collaboration main.

@gogakoreli
Copy link

Also RTC: prefix should not be added by itself to the name of the file, notice that file browser doesn't show this prefix but everywhere else it has.
image

@JasonWeill
Copy link
Contributor

Microsoft OneNote doesn't have a "save" command, since it only auto-saves, but CTRL+S or CMD+S remains reserved, not bound to anything. If collaboration is enabled in JupyterLab, the Accel S shortcut should not cause a save command to run. Whether this should be done by changing the save command to be aware of RTC status, by unbinding the shortcut, or something else, is not clear to me.

@krassowski
Copy link
Member

RTC extension could add a custom shortcut mapped to ctrl + s which would have a selector with higher specificity (hence run first) and:
a) ask for rename of a new file if user has not opted out, otherwise
b) show little self-disappearing notification bubble saying that saving is done automatically, e.g. "Last synced with server 2s ago" with button options [save as] [download copy].

@krassowski
Copy link
Member

I think RTC could even add "disabled" true" to the default shortcut but no 100% i that would work

@jasongrout
Copy link
Contributor

RTC extension could add a custom shortcut mapped to ctrl + s which would have a selector with higher specificity (hence run first) and:

Somewhat naive question, since I don't fully understand how the rtc framework is integrated into the document system, but: how would the save menu item work in this case? It seems that we would need to address this at the command level, rather than the shortcut level, to get consistent behavior across all the ways to invoke the command.

@krassowski
Copy link
Member

Good point!

It seems that we would need to address this at the command level, rather than the shortcut level, to get consistent behavior across all the ways to invoke the command.

We could go even deeper and make this a part of the content provider API discussed in jupyterlab/jupyterlab#16744 - a field/callable saying whether a document can be saved or if it is saved on the server-side might do it.

A separate but related idea was special-casing server-side saves related to jupyterlab/jupyterlab#16695

@JasonWeill
Copy link
Contributor

@jasongrout Does RTC have a "force sync" command? If so, it might be a good substitute for CTRL+S, since "sync" in RTC is similar enough in functionality to "save" without RTC, and at least in English, both start with S. If it doesn't, consider this a vote to add one.

@JasonWeill
Copy link
Contributor

In this package, in packages/docprovider-extension/package.json, I've added a "schemaDir" key under jupyterlab, which is set to "schema".

Creating a schema/plugin.json file with the following contents, I'm trying to disable the Accel S shortcut for "save":

{
    "title": "Document Manager",
    "description": "Document Manager settings for real-time collaboration.",
    "jupyter.lab.setting-icon": "ui-components:file",
    "jupyter.lab.setting-icon-label": "Document Manager",
    "jupyter.lab.transform": true,
    "jupyter.lab.shortcuts": [
      {
        "args": {},
        "command": "docmanager:save",
        "keys": ["Accel S"],
        "selector": "body",
        "disabled": true
      }
    ],
    "additionalProperties": false,
    "type": "object"
  }

After saving all files, then reinstalling jupyter-collaboration from local source, Accel S is still bound to "save", and shows up as such in the settings editor.

@JasonWeill
Copy link
Contributor

I've noticed in the JupyterLab document:save command, we check the value of the boolean context.model.collaborative to see whether we're in a collaborative environment. (context is the current widget's context.) However, when I try to save a document when jupyter-collaboration is enabled, context.model.collaborative is false. It looks like this value might depend on the collaborative command-line flag, which is now deprecated and slated for removal in Lab 5. Is there another way we can modify JupyterLab's model to have collaborative be true when we're using Jupyter Collaboration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants