jupyterlab / jupyter-collaboration

A Jupyter Server Extension Providing Support for Y Documents
https://jupyterlab-realtime-collaboration.readthedocs.io/en/latest/
Other
163 stars 31 forks source link

Ctrl-S is not disabled #364

Open davidbrochart opened 1 month ago

davidbrochart commented 1 month ago

Description

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

https://github.com/user-attachments/assets/bab1dde1-a957-423f-bbf3-4a2c3956cba1

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

krassowski commented 1 month ago

Are you testing with JupyterLab 4.3.0b1 or with 4.2? It should be fixed by https://github.com/jupyterlab/jupyterlab/pull/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

With which version of collaboration packages? This was fixed by https://github.com/jupyterlab/jupyter-collaboration/pull/337 on this side. It could be a regression due to newer changes.

davidbrochart commented 1 month ago

The issue appears with jupyter-collaboration main.

gogakoreli commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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

jasongrout commented 1 week ago

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 commented 1 week ago

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 https://github.com/jupyterlab/jupyterlab/pull/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 https://github.com/jupyterlab/jupyterlab/pull/16695

JasonWeill commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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?