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

Implement layout-independent keysym bindings #3058

Merged
merged 7 commits into from
Apr 26, 2019

Conversation

kupospelov
Copy link
Contributor

Fixes #2999.

The idea is to have an instance of default xkb_keymap and try to resolve bindsyms into bindcodes while binding.

If the conversion is successful, we just treat the binding as a bindcode. If the conversion has failed, we do not change the binding, so it will be processed as a normal bindsym.

In my configuration, all of the 80 bindsyms have been converted into bindcodes. I have tested the following and it worked in any keyboard layout:

  • Default bindings
  • Bindings with function keys
  • Uppercase keysyms
  • Special ones like XF86AudioRaiseVolume, XF86AudioMute, XF86MonBrightnessUp

I also checked a keysym with no simple keycode translation (the question mark, keysym question) and it worked as a keysym binding, however this kind of bindings still depends on the current keyboard layout.

This makes shortcut behavior more consistent with that of i3 and other programs (e.g. terminal emulators).

@ddevault
Copy link
Contributor

ddevault commented Nov 3, 2018

@emersion
Copy link
Member

emersion commented Nov 4, 2018

I'm not sure this is a good idea. If you have a keyboard with multiple keys producing the same keysym, it'll only work with one of them.

@kupospelov
Copy link
Contributor Author

I would say that in one layout keyboards should only produce one unique keysym per key most of the time. From what I see, even numpad keys (digits, enter, insert, etc) produce keysyms which are different from the other keys' keysyms.

In the rare case when the same keysym is producible from different keys, we could just skip the conversion. In other words, we will only convert keysyms which can be replaced with a single keycode. This should still fix bindings with Latin characters, since it is hard to imagine a keyboard with several keys to type in the same letter in one layout, and keysyms which cannot be safely replaced won't be affected.

@progandy
Copy link
Contributor

progandy commented Nov 4, 2018

Maybe this should be optional, let bindsym detect a parameter like --code to switch the behaviour? Or do the reverse with bindcode --fromsym

(If you use two layouts with different positions for the latin letters (e.g. de and us) I personally prefer the binding follows the new layout.)
Edit: Tried it again, and it seems muscle memory trumps my preference. I quickly switch z and y, but for the shortcuts I keep pressing the original position.

@kupospelov
Copy link
Contributor Author

it seems muscle memory trumps my preference. I quickly switch z and y, but for the shortcuts I keep pressing the original position.

This is exactly what I experience from time to time (except that in my case almost all shortcuts just stop working). People do not normally think about the current layout all the time, so this inconsistency is annoying.

Maybe this should be optional, let bindsym detect a parameter like --code to switch the behaviour? Or do the reverse with bindcode --fromsym

Making the bindsym conversion optional would mean that there are cases when people use some shortcuts in one keyboard layout and do not use in another. I cannot say for everyone, but to me it seems weird.

In my opinion, it is convenient to use keysyms in the config file, but they should not be interpreted literally. Instead, bindings should be bound to proper hardware keys and just work consistently in any layout.

This is not exactly what this pull request does, though. This change fixes the problem, but does not eliminate layout-dependent keysym comparisons altogether. I do not think I can do this until there is an agreement to drop support for layout-dependent keysym bindings.

Copy link
Contributor

@Hi-Angel Hi-Angel left a comment

Choose a reason for hiding this comment

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

This commit probably better to just merge with the prev. one, and then force-push the changes.

@Hi-Angel
Copy link
Contributor

Hi-Angel commented Nov 25, 2018

Hmm this is odd as the View changes link refers not to the commit I reviewed. I meant, it would probably make sense to merge the commit with style fixes into the prev. one.

@kupospelov
Copy link
Contributor Author

Squashed the style fixes and rebased the changes.

@emersion
Copy link
Member

I would say that in one layout keyboards should only produce one unique keysym per key most of the time. From what I see, even numpad keys (digits, enter, insert, etc) produce keysyms which are different from the other keys' keysyms.

In the rare case when the same keysym is producible from different keys, we could just skip the conversion. In other words, we will only convert keysyms which can be replaced with a single keycode. This should still fix bindings with Latin characters, since it is hard to imagine a keyboard with several keys to type in the same letter in one layout, and keysyms which cannot be safely replaced won't be affected.

Let's say keycodes Shift+1 gives the keysym 1 in one of my layouts (e.g. fr), and that just pressing the keycode 1 gives the keysym 1 in the other (e.g. en). What happens with this patch applied?

@kupospelov
Copy link
Contributor Author

kupospelov commented Dec 16, 2018

Let's say keycodes Shift+1 gives the keysym 1 in one of my layouts (e.g. fr), and that just pressing the keycode 1 gives the keysym 1 in the other (e.g. en). What happens with this patch applied?

It depends on how you have configured your layouts. The layout which comes first is used to resolve the bindsyms.

For XKB_DEFAULT_LAYOUT='us,fr':

  1. $mod+1 will switch to workspace 1 in both layouts.
  2. $mod+w will work in both layouts as if you used us.

For XKB_DEFAULT_LAYOUT='fr,us':

  1. 1 cannot be resolved into a bindcode, so it will be layout-dependent: you will have to use $mod+1 in us and $mod+Shift+1 in fr.
  2. $mod+w will work in both layouts as if you used fr.

@chebykinn
Copy link
Contributor

Any updates on this? This is currently one of the biggest pain points with sway for me.

@emersion
Copy link
Member

I'm really not a fan of converting keysyms into keycodes. This makes bindings depend on the first layout, and breaks other layouts (set via xkb_layout a,b,c or when changed via the IPC).

@emersion
Copy link
Member

Investigating what i3 does would be interesting.

@kupospelov
Copy link
Contributor Author

I'm really not a fan of converting keysyms into keycodes. This makes bindings depend on the first layout, and breaks other layouts (set via xkb_layout a,b,c or when changed via the IPC).

The issue with keysyms is that they depend on the current layout and this is a pain. The simplest way to fix that is to use only one layout for bindings, which sounds like a reasonable requirement.

As for IPC, there are options:

  • Do not allow changing the layout configuration using the IPC
  • Re-translate the keysyms every time there is a change in the layout configuration

Investigating what i3 does would be interesting.

This approach is actually inspired by what i3 does. You can take a look at i3/bindings.c, function translate_keysyms. You can also look into function get_binding to make sure that there are no direct keysym comparisons. The behavior is also similar: the first configured layout is used to resolve keysyms.

@mstoeckl
Copy link
Contributor

i3 has another feature: keybindings can be restricted to a single layout with the Group1, Group2, Group3, Group4 modifiers. See i3/i3#1857 and the discussion in i3/i3#1835.

@kupospelov
Copy link
Contributor Author

@emersion, could you please make your point clearer:

  1. Do you generally agree that layout-dependent shortcuts is a problem that should be addressed in one way or another?
  2. If you do agree, but do not like the keycode translation, do you know any better ways to achieve that?
  3. If, for example, the keysym translation was performed on every change of layout configuration via the IPC and it worked with the xkb_layout command, how would this approach look?

@emersion
Copy link
Member

@kupospelov, thanks for bumping this, and sorry for not clearly replying. I'll try to fully understand the issue, because I'm missing some pieces of the problem and haven't spent enough time thinking about it.

Do you generally agree that layout-dependent shortcuts is a problem that should be addressed in one way or another?

Excuse me, but an you please re-explain why it's an issue with Russian layouts?

I only know the French issue: with the current design, if you bind $mod+1 you'll need to press $mod+Shift+à (but I don't think this feature will change that, unless you set your layout to en,fr). Also, keybindings won't be the same across layouts (more on that below).

If you do agree, but do not like the keycode translation, do you know any better ways to achieve that?

I should really read i3's code to understand in detail what they do.

The thing that makes me skeptical about this feature is that as a user, I really don't expect $mod+1 to be bound to a completely different keysym combination when switching layouts. If I bind a keysym, then I don't expect to trigger it when pressing keys that don't match keysyms on another layout.

I think it would make sense to have an option for this.

It would be nice when implementing this to try to keep the code as simple and straightforward as possible (disclaimer: I haven't looked at the code yet).

If, for example, the keysym translation was performed on every change of layout configuration via the IPC and it worked with the xkb_layout command, how would this approach look?

Yes, I think we need to re-translate from keysyms each time the layout is changed via the xkb_layout command.

@ojab
Copy link

ojab commented Jan 25, 2019

Excuse me, but an you please re-explain why it's an issue with Russian layouts?

Because pressing $mod+x , for instance, with ru layout (the physical key, that is actually a ч keysym) doesn't work like pressing $mod+x in en layout.
It's an issue since shortcuts are generally working in any layout (in Qt & GTK+, for example, Ctrl+q [Ctrl+й] in ru closes the program).
Right now $mod-[a-z] is not working at all in ru layout and that's a PITA: switching to the container on the right while using ru layout leads to $mod+д, $mod+д, $mod+д, why the fuck it doesn't work, oh, change layout, $mod+l, change layout, there it is.

@RedBorg
Copy link

RedBorg commented Jan 26, 2019

This could work like variables, you add an option to bindcode and then specify the layout to use it on, and when the file is processed, you convert it to a normal bindcode.

Example of implementation:
bindcode --sym [layout]:[variant] [keybind] [command]

This:
bindcode --sym us:us Mod4+e reload

Would get read as:
bindcode Mod4+44 reload

@YaLTeR
Copy link
Contributor

YaLTeR commented Jan 26, 2019

Excuse me, but an you please re-explain why it's an issue with Russian layouts?

To better demonstrate the issue, here's an example Russian keyboard:

image

It's expected to be used with the US layout interchangeably (so you toggle between the two). When you switch the Russian layout on, keys don't really move around (it's not like 1 becomes shift+something or anything like that) so it's expected that US bindings continue to work the same regardless of the currently active layout. For the keys that do move (some punctuation) I would say it's still expected that they work as in the US layout rather than as in the Russian layout.

@kupospelov
Copy link
Contributor Author

Excuse me, but an you please re-explain why it's an issue with Russian layouts?

Folks in this thread were quite eloquent at describing issues, it is hard to add anything else.

Generally speaking, the main issue is that there is no [a-z] keysyms in the Russian layout. When layout changes, every such key changes its keysym to something else. The [a-z] and [а-я] keysym sets do not overlap (e.g. keysym for а is Cyrilic_a, not a). Because of that most of the default keysym bindings just stop working until the layout is switched back to us.

I am also pretty sure that the same issue exists for a number of other layouts (Ukrainian and Greek for example, should be more).

I should really read i3's code to understand in detail what they do.

Please take a look when you have a chance. The most interesting file there is src/binding.c.

From what I see the meaning of bindsym is different in i3. It basically says "bind the command to all the keys the keysym can be produced from in layout N". By default N = 1, but as @mstoeckl mentioned, it is also possible to specify which layout to use with Group[1-4].

The thing that makes me skeptical about this feature is that as a user, I really don't expect $mod+1 to be bound to a completely different keysym combination when switching layouts. If I bind a keysym, then I don't expect to trigger it when pressing keys that don't match keysyms on another layout.

This sounds reasonable, but there is another expectation: any binding should consistently work. As a result, in case of two or more layouts, only keys producing the same keysyms in all of the layouts are actually usable, and the others are the source of pain and despair.

It would be nice when implementing this to try to keep the code as simple and straightforward as possible (disclaimer: I haven't looked at the code yet).

Please consider the current implementation a prototype. It has worked for me since October, but some pieces of code will need to be changed or added to support dynamic re-translation.

It seems that the key decision that needs to be made is whether sway must be consistent with i3 in how bindings work.

In terms of implementation, I tend to believe that approach used in i3 ("translate every keysym into keycodes and then only use keycodes") is simplest possible, just because there would basically be only one type of bindings instead of two and the code to process keysyms might be dropped.

@meadofpoetry
Copy link

This works perfectly in i3 and doesn't work with sway, causing a lot of pain when using non-latin keymap. Does 1.0 release imply feature parity with i3?

@ddevault
Copy link
Contributor

Tone down the entitlement. This project is run by volunteers who are just doing their best in their spare time.

I suppose a verdict of some kind is necessary here. Unless @emersion disagrees, I think that this behavior (translating keysyms to keycodes) should only be done with a flag, e.g. bindsym --to-code. Since sway supports more general subcommands, like this:

bindsym --to-code {
    Mod1+f foobar
    Mod1+b foobaz
}

This shouldn't be too much of a pain to configure.

@Hi-Angel
Copy link
Contributor

I think that this behavior (translating keysyms to keycodes) should only be done with a flag

Why not default? What would be downsides? FWIW, this is how keybindings work in pretty much any software. The example with Ctrl+q someone put above is a bright example. Not having it as default, I think, a unique "feature" to sway, any reason why?

@ddevault
Copy link
Contributor

Why not default? What would be downsides?

This was already explained earlier in the thread.

@emersion
Copy link
Member

Why not default? What would be downsides?

  1. Setup xkb_layout en,fr
  2. Bind q
  3. Switch to fr
  4. The a key on the keyboard triggers q

@Hi-Angel
Copy link
Contributor

Hi-Angel commented Feb 12, 2019

Thanks @emersion! Please, could you run any Qt application, and tell whether the behavior is as you described when Ctrl+q is pressed (by default it should close the application). It would help us to figure out whether they do something different or not.

@emersion
Copy link
Member

I'm using Firefox right now. In the en layout I need to press Ctrl+Q to quit, but when I switch to the fr layout I need to press Ctrl+A (the physical A key is bound to the Q key in the layout).

@ddevault
Copy link
Contributor

Which seems correct to me... no?

sway/config/input.c Outdated Show resolved Hide resolved
sway/config.c Outdated Show resolved Hide resolved
@kupospelov kupospelov force-pushed the consistent-shortcuts branch 2 times, most recently from 12877ec to a034e53 Compare March 5, 2019 20:09
@emersion
Copy link
Member

I'll try having a look again soon.

Can some other contributor look at the changes too?

@emersion emersion self-requested a review April 9, 2019 05:41
@mstoeckl
Copy link
Contributor

mstoeckl commented Apr 19, 2019

Could you please rebase again? (There is a conflict with the now-merged #3998, which introduced a binding_add function that plays the same role as add_binding. It is reasonably straightforward to fix; see for example mstoeckl@b8a6a0b , the result of my own attempt at resolving the rebase conflicts.)

Also, I have read through the changeset a few times and have no immediate complaints; testing layout switching with xkb_options grp:alt_shift_toggle did not reveal any bugs.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. This version looks much better than the previous one.

Here are a few comments, only about style. The logic looks good to me. After these minor fixes and a rebase, I'll be okay to merge this.


switch (binding->type) {
// a bindsym to translate
case BINDING_KEYSYM:
Copy link
Member

Choose a reason for hiding this comment

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

Style: case should be on the same level as switch (applies to the whole patch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

sway/config.c Outdated
@@ -921,3 +945,55 @@ void config_update_font_height(bool recalculate) {
arrange_root();
}
}

static void translate_bindings(list_t *bindings, list_t *bindsyms,
Copy link
Member

Choose a reason for hiding this comment

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

This is a little confusing, the only difference between translate_bindings and translate_binding is the final "s". Can we rename this one to translate_bindings_list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to translate_binding_list

* `bindsym --to-code` enables keysym to keycode translation.
* If there are no `xkb_layout` commands in the config file, the translation
uses the XKB_DEFAULT_LAYOUT value.
* It there is one or more `xkb_layout` command, the translation uses
the first one.
* If the translation is unsuccessful, a message is logged and the binding
is stored as BINDING_KEYSYM.
* The binding keysyms are stored and re-translated when a change in the input
configuration may affect the translated bindings.
Do not store `xkb_keymap` since it can be retrieved from `xkb_state`.
Replace XKB_DEFAULT_LAYOUT with NULL as the default layout.
@kupospelov
Copy link
Contributor Author

Rebased. binding_add turned out to require more logging context than the translation code could provide, so I created binding_add_translated and shared the code between the two in the binding_upsert function.

@chebykinn
Copy link
Contributor

@emersion Can this be merged before 1.1 release?

@emersion
Copy link
Member

Oh, thanks for the heads-up. That would be nice indeed. Will have a look tomorrow.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM

@emersion emersion merged commit e6fbb3c into swaywm:master Apr 26, 2019
@emersion
Copy link
Member

Thanks!

@eternal-sorrow
Copy link

https://gist.github.com/eternal-sorrow/396d642a7c6be712157cc24376298faf

simple patch so that this behaviour was default (without --to-code)

@alecmev
Copy link

alecmev commented May 18, 2019

DRYing things up without patching:

bindsym --to-code {
  $super+Return exec termite
  $super+Shift+q kill
  # ...
}

@kupospelov kupospelov deleted the consistent-shortcuts branch October 23, 2019 13:08
@kindaro
Copy link

kindaro commented Jan 26, 2020

@kupospelov Thank you, great work. Also helps when moving to optimized layouts, such as Dvorak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or incremental improvement
Development

Successfully merging this pull request may close these issues.

bindsym does not work in some keyboard layouts