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: Fix some mappings involving shift #2018

Merged
merged 3 commits into from Sep 23, 2023

Conversation

fredizzimo
Copy link
Member

@fredizzimo fredizzimo commented Sep 9, 2023

NOTE: this needs careful testing before merging, so that something else does not break,

What kind of change does this PR introduce?

This fixes #2017 - "can't map <c-s-v>". This is done by sending shfit when it's combined with CTRL and an ascii alpha character. See the corresponding logic here https://github.com/neovim/neovim/blob/c422722b2e94b94d7f9374dbae12f17580cd1d41/src/nvim/keycodes.c#L764-L799

It also partially fixes the following issue #1237 - "CMD + SHIFT works as CMD only". Partially because it's likely not dealing with all the cases. The real issue is in Winit. The fix is to normalize shifted asicii alpha characters already on the Neovide side. This is always done one the Neovim side anyway, so it should be a safe thing to do.

Note that only the following form of mapping work: <D-A>. There appears to be a bug in Neovim neovim/neovim#25068, preventing the alternative forms <D-S-A> and <D-S-a> from working, but once that's fixed all forms should work. The <D-A> form was chosen because that's the most basic form, for example <M-S-a> also simplifies down to <M-A> in Neovide, so it's consistent with that. Furthermore, this format of mapping will work, even after the Winit bug rust-windowing/winit#3078 is fixed.

The same non <S-> form should also always be use for non-alpha mappings for all modifiers. The only special case is CTRL + SHIFT + alpha, which should be mapped like <C-S-A> due to legacy reasons.
From https://neovim.io/doc/user/intro.html (the notation section)

CTRL-{char} {char} typed as a control character; that is, typing {char}
while holding the CTRL key down. The case of {char} is
ignored; thus CTRL-A and CTRL-a are equivalent. But in
some terminals and environments, using the SHIFT key will
produce a distinct code (e.g. CTRL-SHIFT-a); in these
environments using the SHIFT key will not trigger commands
such as CTRL-A

Did this PR introduce a breaking change?

No

This was referenced Sep 9, 2023
@MultisampledNight MultisampledNight linked an issue Sep 9, 2023 that may be closed by this pull request
@JamesWidman
Copy link
Contributor

Note that only the following form of mapping work: <D-A>. There appears to be a bug in Neovim neovim/neovim#25068, preventing the alternative forms <D-S-A> and <D-S-a> from working, but once that's fixed all forms should work.

What about <S-D-a>? (In neovide 0.10.4 and in the patch that i mentioned on issue 2017, this is what neovide/nvim inserts for me when i type <ctrl-v> followed by cmd + shift + a, and my cmd + shift mappings have been working fine with it.)

@fredizzimo
Copy link
Member Author

What about ? (In neovide 0.10.4 and in the patch that i mentioned #2017 (comment), this is what neovide/nvim inserts for me when i type followed by cmd + shift + a, and my cmd + shift mappings have been working fine with it.)

<S-D-a> and <D-S-a> are the same, it's just the order that's different.

When neovim/neovim#25068, is fixed, it will also be the same as <D-A>, and <D-S-A>, but currently those two are different from <D-S-a> inside Neovim.

The reason for not staying backwards compatible with 0.10.4 is simply because reporting <S-D-a> is caused by a winit bug. Furthermore, if that Winit bug is fixed, it will break all user mappings, until additionally neovim/neovim#25068 is fixed. We don't want to do braking changes like that ,0.11.0 already had the breaking change of breaking it completely.

the only two reasonable ways of reporting it is <D-A> or <D-S-A>, but since the normalized format inside Neovim will be <D-A>, that's the way I chose to report it here as well.

@JamesWidman
Copy link
Contributor

Also: on a windows machine, with a keyboard with the windows-logo key, wouldn't shift + win + a produce the same behavior as what we see on macOS for shift + cmd + a? I mean, cmd and win both map to super in winit, right?

(i guess the window manager might intercept it, but hopefully microsoft didn't define any mappings that aren't documented in their list of shortcuts)

@fredizzimo
Copy link
Member Author

For the windows key, there are very few keys that are available and I'm not sure if the ones that are forwarded from Winit, during my quick testing it seems like they are not. But if it's available and winit forwards it, then it should be mappable.

Usually, you need to register an actual hotkey to capture the windows key events, and some are not even overridable by using global hotkeys.

@JamesWidman
Copy link
Contributor

fwiw, LGTM (though i'm not sure if this matters since i'm not a regular contributor)

@technicalpickles
Copy link

It also partially fixes the following issue #1237 - "CMD + SHIFT works as CMD only".

Pulled this branch and tested locally. Can confirm that this PR fixes this issue

@fredizzimo
Copy link
Member Author

@technicalpickles, Can you test with some special characters as well, like <D-%> for example? I think that we still need rust-windowing/winit#3078, for the complete fix.

@MultisampledNight
Copy link
Contributor

Works perfect from what I tested on Wayland with Bone as layout.

What was important for me that most characters were indeed mapped as the resulting characters. For example, + on QWERTY results in ´ as compose, but Shift + + results in ~ as compose. So mapping <C-~> is triggered by both Caps Lock + v (which types a literal ~) and Shift + + hit twice — which is completely fine imo. Shift (and any other modifier) + Enter still results in <S-Enter> (or the respective modifier), which works fine, too.

@technicalpickles
Copy link

@fredizzimo Just tested, and that one does not seem to be working. Using these mappings:

vim.api.nvim_set_keymap("n", "<D-5>", "<cmd>echo 'D-5'<CR>", { noremap = true })
vim.api.nvim_set_keymap("n", "<D-%>", "<cmd>echo 'D-%'<CR>", { noremap = true })

vim.api.nvim_set_keymap("n", "<D-i>", "<cmd>echo 'D-i'<CR>", { noremap = true })
vim.api.nvim_set_keymap("n", "<D-I>", "<cmd>echo 'D-I'<CR>", { noremap = true })

These all work except for <D-%>

@fredizzimo
Copy link
Member Author

@technicalpickles, thanks, we will not close #1237 just yet then.

@JamesWidman JamesWidman mentioned this pull request Sep 23, 2023
@fredizzimo fredizzimo merged commit 24f6a78 into neovide:main Sep 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't map <c-s-v>
4 participants