Fix logic for changing keybindings in shortcut editor

jupyterlab / jupyterlab

JupyterLab computational environment.

https://jupyterlab.readthedocs.io/

Other

13.81k stars 3.12k forks source link

Fix logic for changing keybindings in shortcut editor #16214

Closed krassowski closed 2 weeks ago

krassowski commented 3 weeks ago

References

Fixes #16211

Code changes

[x] Adds a test case
[x] Adds missing check for keybinding keys equality with iterated shortcut
[x] Fix shortcut/keybinding data being set at wrong level

User-facing changes

Repalcing shortcuts in settings work when a mix of default and non-default shortcuts is used, and when multiple keybindings are used.

Backwards-incompatible changes

None

jupyterlab-probot[bot] commented 3 weeks ago

Thanks for making a pull request to jupyterlab! To try out this branch on binder, follow this link:

kolibril13 commented 3 weeks ago

Hi Mike, thanks a lot for taking a look at this! I've just tried this at https://mybinder.org/v2/gh/krassowski/jupyterlab/fix-changing-second-default-shortcut?urlpath=lab In firefox, everything works fine, but safari looks quite broken. There is a big magnifying glass, text is overlapping, and the second shortcut is not assigned correctly.

https://github.com/jupyterlab/jupyterlab/assets/44469195/8e957edb-6540-49ee-b2d4-35e644dd1443

krassowski commented 3 weeks ago

fine, but safari looks quite broken

From the recording, it appears that in Safari you end up clicking on the "Default" text because the accept button is moved to under the "Default". It appears that latest Safari switched to a new (and apparently not quite ready) inline layout implementation - is this the Safari version you are using?

There is a big magnifying glass, text is overlapping, and the second shortcut is not assigned correctly.

Can you open a dedicated issue for magnifying glass? I think replacing the search component with jupyter-ui-toolkit one would solve the issue. Can you also check if this is happening in released versions already (in that issue).

kolibril13 commented 3 weeks ago

It appears that latest Safari switched to a new (and apparently not quite ready) inline layout implementation - is this the Safari version you are using?

Yep, I'm using that new version -> Safari 17.4.1.

Can you open a dedicated issue for magnifying glass? I think replacing the search component with jupyter-ui-toolkit one would solve the issue. Can you also check if this is happening in released versions already (in that issue).

I'll do that! :)

JasonWeill commented 2 weeks ago

In Firefox 124.0.2 on macOS, with this code change, I see the expected behavior.

In Safari 17.4.1 on macOS, I see the UI component for a new shortcut overlap some other elements. After I click ✔️ , the new shortcut is not saved.

krassowski commented 2 weeks ago

In Safari 17.4.1 on macOS, I see the UI component for a new shortcut overlap some other elements. After I click ✔️ , the new shortcut is not saved.

This is a separate issue tracked in https://github.com/jupyterlab/jupyterlab/issues/16233