Rewrite of how we match key bindings #380

Manually merged
dnkl merged 10 commits from consume-modifiers-for-key-bindings into master 9 months ago
dnkl commented 9 months ago
Owner

Bindings are matched in one out of three ways:

  • By translated (by XKB) symbols
  • By untranslated symbols
  • By raw key codes

A translated symbol is affected by pressed modifiers, some of which can be “consumed”. Consumed modifiers to not partake in the comparison with the binding’s modifiers. In this mode, ctrl+shift+2 maps to Control+@ on a US layout.

Untranslated symbols, or un-shifted symbols refer to the “base” symbol of the pressed key, i.e. it’s unaffected by modifiers. In this mode, consumed modifiers do partake in the comparison with the binding’s modifiers, and ctrl+shift+2 maps to Control+Shift+2 on a US layout.

More examples: ctrl+shift+u maps to Control+U in the translated lookup, while ctrl+shift+u maps to Control+Shift+u in the untranslated lookup.

Finally, we also match raw key codes. This allows our bindings to work using the same physical keys when the user switches between latin and non-latin layouts.

This means key bindings in foot.ini must not include both shift and a shifted key. I.e. Control+Shift+U is not a valid combo as it cannot be triggered. Unfortunately, this was how you were supposed to write bindings up until now... so, we try to detect such bindings, log a deprecation warning and then “fix” the binding for the user.

When specifying bindings in foot.ini, both Control+U and Control+Shift+u are valid, and will work. The latter is preferred though, since we cannot detect the raw key code for the former variant. Personally, I also prefer the latter one because it is more explicit; it’s more obvious which keys are involved.

However, in some cases it makes more sense to use the other variant. Typically for non-letter combos.

Closes #376

@lechner
@craigbarnes

Note this PR is based on #378, but should be easy to rebase if we decide not to merge #378.

Bindings are matched in one out of three ways: * By translated (by XKB) symbols * By untranslated symbols * By raw key codes A translated symbol is affected by pressed modifiers, some of which can be “consumed”. Consumed modifiers to not partake in the comparison with the binding’s modifiers. In this mode, <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>2</kbd> maps to `Control+@` on a US layout. Untranslated symbols, or un-shifted symbols refer to the “base” symbol of the pressed key, i.e. it’s unaffected by modifiers. In this mode, consumed modifiers *do* partake in the comparison with the binding’s modifiers, and <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>2</kbd> maps to `Control+Shift+2` on a US layout. More examples: <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>u</kbd> maps to `Control+U` in the translated lookup, while <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>u</kbd> maps to `Control+Shift+u` in the untranslated lookup. Finally, we also match raw key codes. This allows our bindings to work using the same physical keys when the user switches between latin and non-latin layouts. This means key bindings in `foot.ini` **must** not include both `shift` and a **shifted** key. I.e. `Control+Shift+U` is not a valid combo as it cannot be triggered. Unfortunately, this was how you were supposed to write bindings up until now... so, we try to detect such bindings, log a deprecation warning and then “fix” the binding for the user. When specifying bindings in `foot.ini`, both `Control+U` and `Control+Shift+u` are valid, and will work. The latter is preferred though, since we cannot detect the raw key code for the former variant. Personally, I also prefer the latter one because it is more explicit; it’s more obvious which keys are involved. However, in some cases it makes more sense to use the other variant. Typically for non-letter combos. Closes #376 @lechner @craigbarnes **Note** this PR is based on https://codeberg.org/dnkl/foot/pulls/378, but should be easy to rebase if we decide not to merge #378.
Poster
Owner

While this should mirror Sway's matching logic (minus matching raw key codes by default), I haven't yet verified this is the case. I will try to do so soon.

While this _should_ mirror Sway's matching logic (minus matching raw key codes by default), I haven't yet verified this is the case. I will try to do so soon.
Poster
Owner

@lechner this should fix your issue with Control+0.

@lechner this _should_ fix your issue with `Control+0`.
Collaborator

This definitely looks like the right approach to fixing the consumed modifiers issue. I have a couple of questions about the details, but the patch is mostly looking good so far.

This means key bindings in foot.ini must not include both shift and a shifted key. I.e. Control+Shift+U is not a valid combo as it cannot be triggered. Unfortunately, this was how you were supposed to write bindings up until now... so, we try to detect such bindings, log a deprecation warning and then “fix” the binding for the user.

If these bindings aren't possible to trigger and can also be automatically "fixed", is deprecating them even necessary? Can't they just continue to be "fixed" indefinitely (and the warning be removed)?

From an implementation point of view, I understand why they can be thought of as "deprecated", but a normal user may just think in terms of what they see printed on their key caps—in which case Control+Shift+U seems like it should be perfectly valid.

I also noticed this PR seems to change the escape sequences sent to the client for Shift+Tab and Ctrl+Shift+Tab. The sequences are usually CSI Z and CSI 27;6;9~, but with this branch it behaves as if the Shift key wasn't pressed (i.e. the sequences are \t and CSI 27;5;9~).

This definitely looks like the right approach to fixing the consumed modifiers issue. I have a couple of questions about the details, but the patch is mostly looking good so far. > This means key bindings in `foot.ini` **must** not include both `shift` and a **shifted** key. I.e. `Control+Shift+U` is not a valid combo as it cannot be triggered. Unfortunately, this was how you were supposed to write bindings up until now... so, we try to detect such bindings, log a deprecation warning and then “fix” the binding for the user. If these bindings aren't possible to trigger and can also be automatically "fixed", is deprecating them even necessary? Can't they just continue to be "fixed" indefinitely (and the warning be removed)? From an implementation point of view, I understand why they can be thought of as "deprecated", but a normal user may just think in terms of what they see printed on their key caps—in which case `Control+Shift+U` seems like it should be perfectly valid. I also noticed this PR seems to change the escape sequences sent to the client for <kbd>Shift</kbd>+<kbd>Tab</kbd> and <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>Tab</kbd>. The sequences are usually `CSI Z` and `CSI 27;6;9~`, but with this branch it behaves as if the <kbd>Shift</kbd> key wasn't pressed (i.e. the sequences are `\t` and `CSI 27;5;9~`).
craigbarnes reviewed 9 months ago
config.c Outdated
"shifted keys not supported with explicit \033[1mShift\033[m "
"modifier",
path, lineno, section, option, combo);
*key = tolower(*key);
Poster
Collaborator

It may be a good idea to avoid using locale-dependant <ctype.h> functions for things like this. For example; tolower('I') returns 'I' in the glibc tr_TR.UTF-8 locale.

In this case it's possibly not worth caring about, but in some other cases the implications might be worse.

It may be a good idea to avoid using locale-dependant `<ctype.h>` functions for things like this. For example; `tolower('I')` returns `'I'` in the glibc `tr_TR.UTF-8` locale. In this case it's possibly not worth caring about, but in some other cases the implications might be worse.
dnkl commented 9 months ago
Poster
Owner

Since the range we're checking is well known, we can drop to tolower() call and do the conversion ourselves.

Since the range we're checking is well known, we can drop to `tolower()` call and do the conversion ourselves.
dnkl marked this conversation as resolved
Poster
Owner

From an implementation point of view, I understand why they can be thought of as "deprecated", but a normal user may just think in terms of what they see printed on their key caps—in which case Control+Shift+U seems like it should be perfectly valid.

@lechner brought up this question on IRC:

dnkl: as for detecting "bad" combos, what do you do if someone remapped the alphabet to uppercase, and uses Shift to get lowercase? (although I never saw that)

I'm in favor of dropping the deprecation warning, if we choose to repair the bindings. Question is, should we, or should we not repair them?

A layout with inverted shift logic is rare (CapsLock doesn't count here - it changes the current shift level, but doesn't actually invert it). But, if we repair bindings in foot we would make these inverted bindings impossible to trigger.

I still think we should repair bindings, but wanted to at least bring it up.

> From an implementation point of view, I understand why they can be thought of as "deprecated", but a normal user may just think in terms of what they see printed on their key caps—in which case Control+Shift+U seems like it should be perfectly valid. @lechner brought up this question on IRC: ``` dnkl: as for detecting "bad" combos, what do you do if someone remapped the alphabet to uppercase, and uses Shift to get lowercase? (although I never saw that) ``` I'm in favor of dropping the deprecation warning, if we choose to repair the bindings. Question is, should we, or should we not repair them? A layout with inverted shift logic is rare (CapsLock doesn't count here - it _changes_ the current shift level, but doesn't actually invert it). But, if we repair bindings in foot we would make these inverted bindings impossible to trigger. I still think we should repair bindings, but wanted to at least bring it up.
Poster
Owner

I also noticed this PR seems to change the escape sequences sent to the client for Shift+Tab and Ctrl+Shift+Tab. The sequences are usually CSI Z and CSI 27;6;9~, but with this branch it behaves as if the Shift key wasn't pressed (i.e. the sequences are \t and CSI 27;5;9~).

This is from the #378 part of the PR, and has to do with Shift+Tab being "shifted" by XKB to a different symbol.

I'll update #378 and then re-base this PR.

> I also noticed this PR seems to change the escape sequences sent to the client for Shift+Tab and Ctrl+Shift+Tab. The sequences are usually CSI Z and CSI 27;6;9~, but with this branch it behaves as if the Shift key wasn't pressed (i.e. the sequences are \t and CSI 27;5;9~). This is from the https://codeberg.org/dnkl/foot/pulls/378 part of the PR, and has to do with <kbd>Shift</kbd>+<kbd>Tab</kbd> being "shifted" by XKB to a different symbol. I'll update #378 and then re-base this PR.
Poster
Owner

I'll update #378 and then re-base this PR.

d5479a9973

> I'll update #378 and then re-base this PR. https://codeberg.org/dnkl/foot/commit/d5479a9973eaea5d064f42ab1da572bbac32209d
dnkl force-pushed consume-modifiers-for-key-bindings from 9b8ed90873 to b5d84e9b67 9 months ago

this should fix your issue with Control+0.

Maybe I'm getting confused. Control+0 works here. I believe my issue is with Control+Shift+u after applying #378.

I think I should apply both branches before testing. Does that make sense to you? Thanks!

> this _should_ fix your issue with `Control+0`. Maybe I'm getting confused. `Control+0` works here. I believe my issue is with `Control+Shift+u` after applying https://codeberg.org/dnkl/foot/issues/378. I think I should apply both branches before testing. Does that make sense to you? Thanks!
Poster
Owner

I think I should apply both branches before testing. Does that make sense to you? Thanks!

This PR includes #378, so you shouldn't have to apply that one first. Furthermore, that one only changes the escapes we sent to the client application. Control+Shift+u is a foot key binding which is interpreted by foot directly, and never sent to the application.

Unless you've manually changed the show-urls-launch key binding in foot.ini, Ctrl+Shift+u should work. If not, check with e.g. xev, or similar - do the keys produce the expected symbols and modifiers?

> I think I should apply both branches before testing. Does that make sense to you? Thanks! This PR **includes** #378, so you shouldn't have to apply that one first. Furthermore, that one only changes the escapes we sent to the client application. `Control+Shift+u` is a foot key binding which is interpreted by foot directly, and never sent to the application. Unless you've manually changed the `show-urls-launch` key binding in foot.ini, <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>u</kbd> should work. If not, check with e.g. `xev`, or similar - do the keys produce the expected symbols and modifiers?

Ctrl+Shift+u should work

Should work in what I currently have, i.e. only with #378 applied?

> <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>u</kbd> should work Should work in what I currently have, i.e. only with https://codeberg.org/dnkl/foot/issues/378 applied?
Poster
Owner

@lechner #378 is irrelevant to all the issues you have been seeing.

ctrl+shift+u works in foot master, right? But not ctrl+0?

What I'm hoping, is that both ctrl+0, and ctrl+shift+u works with this PR.

@lechner https://codeberg.org/dnkl/foot/pulls/378 is irrelevant to all the issues you have been seeing. ctrl+shift+u works in foot master, right? But not ctrl+0? What I'm hoping, is that both ctrl+0, and ctrl+shift+u works with **this** PR.

What I'm hoping, is that both ctrl+0, and ctrl+shift+u works with this PR.

They do. Thank you!

> What I'm hoping, is that both ctrl+0, and ctrl+shift+u works with **this** PR. They do. Thank you!
Poster
Owner

@lechner awesome!

Then all that's left to do is decide what to do with "broken" key combos ala Control+Shift+U in foot.ini.

@lechner awesome! Then all that's left to do is decide what to do with "broken" key combos ala `Control+Shift+U` in foot.ini.
Poster
Owner

@craigbarnes @lechner regarding "repairing" seemingly broken combos; if we all agree that we can skip the deprecation log message, then the repair step can be deferred until we decode the combos when we receive a layout from the compositor.

Once we have a layout, I think we'll be able to check all symbols and see if they correspond to an unshifted symbol or not. If we detect a combo with both the shift modifier, and a shifted symbol, we can ask XKB for the unshifted symbol and use that instead.

@craigbarnes @lechner regarding "repairing" seemingly broken combos; if we all agree that we can skip the deprecation log message, then the repair step can be deferred until we decode the combos when we receive a layout from the compositor. Once we have a layout, I think we'll be able to check all symbols and see if they correspond to an unshifted symbol or not. If we detect a combo with both the shift modifier, and a shifted symbol, we can ask XKB for the unshifted symbol and use that instead.
Poster
Owner

@lechner @craigbarnes I've pushed a commit that tries to detect key combinations containing modifiers that will be consumed by XKB when producing the symbol in the key combo. In other words, combinations that we'll never be able to trigger.

Example: Control+Shift+U.

I've tried to solve this in a generic way. I.e. not just checking for Shift or matching ASCII characters.

Instead, we use XKB's API to do reverse symbol lookup. I haven't found a way to get a list of key codes that produces a specific symbol. What we do instead, is loop all key codes. For each key code, get all its shifted symbols.

If one of the shifted symbols matches the symbol from the key combo, get the sets of modifiers that produce the shift level where we found the shifted symbol. Remember, each key may have multiple shift levels - it's not as simple as either being un-shifted or shifted. Put another way, keys may have more than two symbols.

For each modifier set, check if it contains any of the modifiers from the key combo. If at least one modifier set contains at least one common modifier, we replace the shifted symbol in the key combo with the un-shifted symbol.

I've tested this with Control+U, Control+Shift+u and Control+Shift+U. The last one is correctly detected as invalid, and correctly repaired. All three "work", i.e. the key binding can be triggered.

All default bindings are considered "valid". The only custom key bindings I have are Control+Shift+Page_Up/Down, and they are (correctly) being considered "valid".

I would appreciate if you could test this. Invalid key combos are logged as warnings on stderr, but there is no output in the foot window:

warn: input.c:472: Shift+Control+U: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with u

Btw, the log message isn't really meant to be understood by "normal" users. That said, if we can improve the wording in the log message, I'm open for suggestions.

@lechner @craigbarnes I've pushed a commit that tries to detect key combinations containing modifiers that will be consumed by XKB when producing the symbol in the key combo. In other words, combinations that we'll never be able to trigger. Example: `Control+Shift+U`. I've tried to solve this in a generic way. I.e. not just checking for <kbd>Shift</kbd> or matching ASCII characters. Instead, we use XKB's API to do reverse symbol lookup. I haven't found a way to get a list of key codes that produces a specific symbol. What we do instead, is loop all key codes. For each key code, get all its **shifted** symbols. If one of the shifted symbols matches the symbol from the key combo, get the sets of modifiers that produce the shift **level** where we found the shifted symbol. Remember, each key may have multiple shift levels - it's not as simple as either being un-shifted or shifted. Put another way, keys may have more than two symbols. For each modifier set, check if it contains _any_ of the modifiers from the key combo. If at least one modifier set contains at least one common modifier, we replace the shifted symbol in the key combo with the un-shifted symbol. I've tested this with `Control+U`, `Control+Shift+u` and `Control+Shift+U`. The last one is correctly detected as invalid, and correctly repaired. All three "work", i.e. the key binding can be triggered. All default bindings are considered "valid". The only custom key bindings I have are `Control+Shift+Page_Up/Down`, and they are (correctly) being considered "valid". I would appreciate if you could test this. Invalid key combos are logged as warnings on stderr, but there is **no** output in the foot window: ``` warn: input.c:472: Shift+Control+U: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with u ``` Btw, the log message isn't really meant to be understood by "normal" users. That said, if we can improve the wording in the log message, I'm open for suggestions.
Collaborator

@dnkl The latest commit seems to works fine with my existing config:

warn: input.c:472: Shift+Control+F: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with f
warn: input.c:472: Shift+Control+plus: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with equal
warn: input.c:472: Shift+Control+underscore: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with minus
warn: input.c:472: Shift+Control+parenright: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with 0
warn: input.c:472: Shift+Control+U: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with u
warn: input.c:472: Shift+Control+L: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with l

..all the key combos that produce warnings can be triggered just as before.

I've tried to solve this in a generic way. I.e. not just checking for Shift or matching ASCII characters.

Instead, we use XKB's API to do reverse symbol lookup.

This new approach seems like the ideal way to solve the problem, even for cases involving unusual keymaps. 👍

@dnkl The latest commit seems to works fine with my existing config: ``` warn: input.c:472: Shift+Control+F: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with f warn: input.c:472: Shift+Control+plus: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with equal warn: input.c:472: Shift+Control+underscore: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with minus warn: input.c:472: Shift+Control+parenright: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with 0 warn: input.c:472: Shift+Control+U: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with u warn: input.c:472: Shift+Control+L: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with l ``` ..all the key combos that produce warnings can be triggered just as before. > I've tried to solve this in a generic way. I.e. not just checking for Shift or matching ASCII characters. > Instead, we use XKB's API to do reverse symbol lookup. This new approach seems like the ideal way to solve the problem, even for cases involving unusual keymaps. 👍

@dnkl @craigbarnes Is there anything I can test—perhaps around the inverted Shift on the number keys or any reassigned modifiers?

@dnkl @craigbarnes Is there anything I can test—perhaps around the inverted <kbd>Shift</kbd> on the number keys or [any reassigned modifiers](https://salsa.debian.org/lechner/rocket-layout/-/blob/master/README.md#L4-11)?
Poster
Owner

@lechner first and foremost, check all your key bindings are still working. Second, check foot's stderr output and check if it's logging any warnings about repaired key bindings. If there are - are they expected? Correct?

Anything "unusual" is worth testing. That includes the inverted shift keys, and re-assigned modifiers.

Thanks for helping out!

@lechner first and foremost, check all your key bindings are still working. Second, check foot's stderr output and check if it's logging any warnings about repaired key bindings. If there are - are they expected? Correct? Anything "unusual" is worth testing. That includes the inverted shift keys, and re-assigned modifiers. Thanks for helping out!

@dnkl This branch, when rebased on master, did everything I was looking for!

I did not see any warnings whatsoever, except when I uncommented those two lines:

clipboard-copy=Control+Shift+C
clipboard-paste=Control+Shift+V

Then, foot generated the following warnings:

warn: input.c:472: Shift+Control+C: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with c
warn: input.c:472: Shift+Control+V: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with v

I think they are expected and wanted. Great job! Thanks so much!

@dnkl This branch, when rebased on `master`, did everything I was looking for! I did not see any warnings whatsoever, except when I uncommented those two lines: ``` clipboard-copy=Control+Shift+C clipboard-paste=Control+Shift+V ``` Then, `foot` generated the following warnings: ``` warn: input.c:472: Shift+Control+C: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with c warn: input.c:472: Shift+Control+V: combo with both explicit modifier and shifted symbol (level=1, mod-mask=0x00000001), replacing with v ``` I think they are expected and wanted. Great job! Thanks so much!
Poster
Owner

I did not see any warnings whatsoever, except when I uncommented those two lines:

I had forgotten to update the example foot.ini to use lower-case keys.

Given the combos you uncommented where using upper-case keys means the warnings you saw were expected, and correct.

Thanks!

> I did not see any warnings whatsoever, except when I uncommented those two lines: I had forgotten to update the example `foot.ini` to use lower-case keys. Given the combos you uncommented where using upper-case keys means the warnings you saw were expected, and correct. Thanks!
dnkl force-pushed consume-modifiers-for-key-bindings from 197b8ec0d6 to 93b02cf2b8 9 months ago
Poster
Owner

I've now re-based this PR against the master branch.

I've now re-based this PR against the master branch.
dnkl changed title from WIP: rewrite of how we match key bindings to Rewrite of how we match key bindings 9 months ago
dnkl force-pushed consume-modifiers-for-key-bindings from f760c0e5be to b796965c3d 9 months ago
dnkl merged commit e8a8d122f0 into master manually 9 months ago
The pull request has been manually merged as e8a8d122f0.
Sign in to join this conversation.
Loading…
There is no content yet.