input/keyboard: fix bindsym --to-code failing on duplicates

[sahinf]

The libxkbcommon api does not prohibit multiple keycodes mapping to a single keysym. There is no reason to treat it as an error. With these changes, all of the matched keycodes will be bound the keysym.

Error variable is to track whether malloc failed while iterating. Can potentially keep calling a failing malloc but it will get logged and aborted once xkb_keymap_key_for_each returns control flow to sway. The only way to avoid malloc within the iterator is by preallocating enough memory to hold possible every keycode in the keymap, then copying over those matches->keycodes into the binding->keycodes. But that feels even more problematic.

Retains cleanup semantics of blame-commit a09c144

[sahinf]

Hey I forgot to use a separate branch and accidentally closed the original PR #8323 by force pushing my undo.
These changes bind all the matching keycodes as discussed.

[emersion]

Thanks for updating your patch!

Does this actually work? I would expect this to require all keycodes matching a keysym to be pressed. So if a single keysym can be produced by either keycode 42 and 43, I would expect this patch to require both to be pressed.

[sahinf]

I thought it was working for me but I tested again and no it doesn't, oops.
Looks like this would require the get_active_binding() to additionally handle this specific case.

I left an issue on the libxkbcommon gh asking for clarification regarding duplicate keysymbols. Still not sure whether that's a bug or not. In the case that it is intentional, and multiple keycodes CAN be mapped to a unique keysym, then we should remove bindsym's --to-code flag because it's not possible to guarantee that mapping.

[emersion]

It's not a libxkbcommon bug, it's an expected feature to be able to map multiple physical keys to a single keysym.

Looks like this would require the get_active_binding() to additionally handle this specific case.

Another way to implement this would be to duplicate the binding as many times as we find keycode combinations.

The annoying thing here is handling the combinatorial explosion, e.g. if keycodes (42, 43) both trigger keysym A and keycodes (45, 46) both trigger keysym B, then for an A+B binding we'd need to create 4 keycode bindings.

[sahinf]

Thanks, yeah I got confirmation that the duplicate mapping is correct.

The annoying thing here is handling the combinatorial explosion, e.g. if keycodes (42, 43) both trigger keysym A and keycodes (45, 46) both trigger keysym B, then for an A+B binding we'd need to create 4 keycode bindings.

This seems clever, thanks. Can you specify what you mean by "trigger keysym"? I'm under the impression that keysyms aren't a factor when handling events, just keycodes. So the keycodes would trigger the command.

[emersion]

Can you specify what you mean by "trigger keysym"?

By "trigger", I mean "keycode is mapped to keysym". IOW, pressing on a physical key will emit one or more keysyms.

Reworded: if keycodes (42, 43) both are mapped to keysym A and keycodes (45, 46) both are mapped to keysym B, then for an A+B binding we'd need to create 4 keycode bindings.

[sahinf]

@emersion Marking as ready for review. It introduces new functions, removes an existing function, and modifies some functions, but I have reasoning for everything. Please let me know if I left out any reasoning in the commit message or within comments.

[kennylevinsen]

For clarity of discussion, this is the retranslation logic that this (and my WIP) patch does not currently handle correctly. This serves to redo the --to-code conversion if the primary keymap changes, i.e., if you go from us,ru to ru,us or if you first set your non-standard keymap after all the bindyms. --to-code is not locked at the time of binding, but instead respect the configured primary keymap rather than the current keymap.

[sahinf]

I was already testing by switching between us,ru and it seemed to work in both cases. Like I would toggle the layout

### Input configuration
input 1:1:AT_Translated_Set_2_keyboard {
    xkb_layout us,ru
    xkb_options ctrl:swapcaps,grp:alt_shift_toggle
}

Which input did you test with and failed? So I can try on my machine too.

[kennylevinsen]

swaymsg 'input * xkb_keymap us'
swaymsg 'bindsym --to-code mod1+a exec notify-send hi'
# test that alt+a sends hi according to a US QWERTY layout
swaymsg 'input * xkb_keymap fr'
# Test that alt+q now sends hi, as the French AZERTY layout is the new
# "first configured layout" that `--to-code` should respect

[sahinf]

That worked for me.

[sahinf]

Test that alt+q now sends hi

Hold up I just saw this. That seems like a weird use case to me. If I bind something to code, I expect those keys to not change positions regardless of the layout.

[sahinf]

If bindcode and bindsym --to-code do not stick with the layout that my physical keyboard is configured in, that makes both commands obsolete. What you really want to do in that example is bindsym.

[kennylevinsen]

bindsym --to-code is not the same as bindcode. Because it uses symbols and depends on a layout, it is implemented so that it makes the binding to match the keycodes of the at any time first configured layout.

Quoting the manpage:

	Bindings to keysyms are layout-dependent. This can be changed with the
	_--to-code_ flag. In this case, the keysyms will be translated into the
	corresponding keycodes in the first configured layout.

This means that when you have a keyboard configured for xkb_layout us,fr, a bindsym --to-code will use the sequence according to the US keyboard layout even if you switch to French using xkb_switch_layout (man 5 sway-input) or an XKB-builtin layout switch toggle.

If the translation only happened once, then it would mean that the result depended on the order of bindsym and xkb_layout in your config file, which we do not want to be the case.

[kennylevinsen]

If bindcode and bindsym --to-code do not stick with the layout that my physical keyboard is configured in, that makes both commands obsolete. What you really want to do in that example is bindsym.

This is how the functionality is defined and documented. The PR and issue is just about adding support for symbols emitted by more than one keycode.

[kennylevinsen]

What's the "original PR" btw?

#3058

[sahinf]

bindsym --to-code is not the same as bindcode

Thanks.

[sahinf]

@emersion @kennylevinsen I think adding multiple bindings per each found duplicate would require cleaning up each of those duplicates when re-translating, which seems excessively complicated given the problem I was trying to fix.

But I realized someone may have already thought of this back in 2018 in the commit a056419ad which introduced state_keysyms_translated.

This get_active_binding tries to utilize it, but I don't believe we actually ever fill it:

0$ rg state_keysyms_translated
include/sway/input/keyboard.h
64:     struct sway_shortcut_state state_keysyms_translated;

sway/input/keyboard.c
373:            update_shortcut_state(&keyboard->state_keysyms_translated,
436:    get_active_binding(&keyboard->state_keysyms_translated,
468:            get_active_binding(&keyboard->state_keysyms_translated,

And this comment for pressed_keycodes says it's specifically for when a keycode generates multiple key ids. I assume that is referring to when a keycode is associated with multiple keysyms (I've never seen "key id" lingo in libxkbcommon)

struct sway_shortcut_state {
	/**
	 * A list of pressed key ids (either keysyms or keycodes),
	 * including duplicates when different keycodes produce the same key id.
	 *
	 * Each key id is associated with the keycode (in `pressed_keycodes`)
	 * whose press generated it, so that the key id can be removed on
	 * keycode release without recalculating the transient link between
	 * keycode and key id at the time of the key press.
	 */
	uint32_t pressed_keys[SWAY_KEYBOARD_PRESSED_KEYS_CAP];
	/**
	 * The list of keycodes associated to currently pressed key ids,
	 * including duplicates when a keycode generates multiple key ids.
	 */
	uint32_t pressed_keycodes[SWAY_KEYBOARD_PRESSED_KEYS_CAP];
	uint32_t last_keycode;
	uint32_t last_raw_modifiers;
	size_t npressed;
	uint32_t current_key;
};

Do ya'll think it's worthwhile to try and handle duplicates through this? Might require a lot less changes.

EDIT: Would also need to undo 30931ad

[kennylevinsen]

@emersion @kennylevinsen I think adding multiple bindings per each found duplicate would require cleaning up each of those duplicates when re-translating,

Yeah, that's what I meant by:

I suspect that the easiest solution is to find and remove all "derived" bindings and retranslate them from their original description (which will either have to be extracted and deduplicated, or stored separately).

I don't think it's particularly hard, but it is somewhat annoying. It would require being able to recognize synthetic/derived/translated bindings as opposed to "real" ones to make them easy to remove, and would require storing the original binding in an inert way for reference for retranslation. That could be done with a parent pointer: Normal bindings don't have a parent, synthetic bindings point to the binding they were generated from. The parent is then stored in a different list not considered for actual execution of keybindings.

But I realized someone may have already thought of this back in 2018 in the commit a056419ad which introduced state_keysyms_translated.

Everything there is about the keyboard state in the currently active keymap, so that would not be immediately useful.