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

Fix logic for changing keybindings in shortcut editor #16214

Merged

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Apr 20, 2024

References

Fixes #16211

Code changes

  • Adds a test case
  • Adds missing check for keybinding keys equality with iterated shortcut
  • 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

@krassowski krassowski added the bug label Apr 20, 2024
@krassowski krassowski added this to the 4.2.0 milestone Apr 20, 2024
Copy link

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

The data for use by commands were set on level of individual keys
rather than on the level of individual keybindings which let to context
menu working on a different item than one would expect (based on key
number, not keybinding number).
@kolibril13
Copy link

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.

Screen.Recording.2024-04-20.at.21.37.40.mov

@krassowski
Copy link
Member Author

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
Copy link

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
Copy link
Contributor

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.

image

@krassowski
Copy link
Member Author

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.

image

This is a separate issue tracked in #16233

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit f3867b0 into jupyterlab:main Apr 26, 2024
82 checks passed
gderocher pushed a commit to gderocher/jupyterlab that referenced this pull request Apr 26, 2024
* Fix keybinding logic

* Fix shortcut/keybinding being set at wrong level

The data for use by commands were set on level of individual keys
rather than on the level of individual keybindings which let to context
menu working on a different item than one would expect (based on key
number, not keybinding number).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing a second shortcut changes the first one in shortcuts editor
4 participants