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.

Sign up for GitHub

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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
052d905
config/bindings: Extend bindsym --to-code to additionally
sahinf Sep 15, 2024
e464128
formatting
sahinf Sep 15, 2024
98bb2c8
formatting
sahinf Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion include/sway/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ struct sway_binding {
char *input;
uint32_t flags;
list_t *keys; // sorted in ascending order
list_t *syms; // sorted in ascending order; NULL if BINDING_CODE is not set
uint32_t modifiers;
xkb_layout_index_t group;
char *command;
Expand Down
183 changes: 119 additions & 64 deletions sway/commands/bind.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ void free_sway_binding(struct sway_binding *binding) {
if (!binding) {
return;
}

list_free_items_and_destroy(binding->keys);
list_free_items_and_destroy(binding->syms);
free(binding->input);
free(binding->command);
free(binding);
Expand Down Expand Up @@ -282,6 +280,7 @@ static struct cmd_results *binding_add(struct sway_binding *binding,
const char *keycombo, bool warn) {
struct sway_binding *config_binding = binding_upsert(binding, mode_bindings);

binding->order = binding_order++;
if (config_binding) {
sway_log(SWAY_INFO, "Overwriting binding '%s' for device '%s' "
"to `%s` from `%s`", keycombo, binding->input,
Expand Down Expand Up @@ -467,6 +466,8 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
// sort ascending
list_qsort(binding->keys, key_qsort_cmp);

binding->command = join_args(argv + 1, argc - 1);

// translate keysyms into keycodes
if (!translate_binding(binding)) {
sway_log(SWAY_INFO,
Expand All @@ -481,13 +482,10 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
} else {
mode_bindings = config->current_mode->mouse_bindings;
}

if (unbind) {
return binding_remove(binding, mode_bindings, bindtype, argv[0]);
}

binding->command = join_args(argv + 1, argc - 1);
binding->order = binding_order++;
return binding_add(binding, mode_bindings, bindtype, argv[0], warn);
}

Expand Down Expand Up @@ -649,101 +647,157 @@ void seat_execute_command(struct sway_seat *seat, struct sway_binding *binding)
}

/**
* The last found keycode associated with the keysym
* and the total count of matches.
* All of the matching keycodes for the given keysym
*/
struct keycode_matches {
xkb_keysym_t keysym;
xkb_keycode_t keycode;
int count;
xkb_keysym_t *keysym;
list_t *keycodes;
bool error;
};

/**
* Iterate through keycodes in the keymap to find ones matching
* the specified keysym.
* Iterate through keycodes in the keymap and add the matching
* the specified keysym's list of matched keycodes.
*/
static void find_keycode(struct xkb_keymap *keymap,
static void add_matching_keycodes(struct xkb_keymap *keymap,
xkb_keycode_t keycode, void *data) {
xkb_keysym_t keysym = xkb_state_key_get_one_sym(
config->keysym_translation_state, keycode);

if (keysym == XKB_KEY_NoSymbol) {
struct keycode_matches *matches = data;
if (*matches->keysym == keysym) {
xkb_keycode_t *new_keycode = malloc(sizeof(xkb_keycode_t));
if (!new_keycode) {
matches->error = true;
return;
}
*new_keycode = keycode;
list_add(matches->keycodes, new_keycode);
}
}

void cartesian_product_helper(list_t **sets, int n, xkb_keycode_t** current_result, int* curr_size, xkb_keycode_t* current_product, int depth) {
// Conquer
if (depth == n) {
current_result[*curr_size] = malloc(n * sizeof(xkb_keycode_t));
for (int i = 0; i < n; ++i) {
current_result[*curr_size][i] = current_product[i];
}
(*curr_size)++;
return;
}

struct keycode_matches *matches = data;
if (matches->keysym == keysym) {
matches->keycode = keycode;
matches->count++;
// Divide
for (int i = 0; i < sets[depth]->length; ++i) {
current_product[depth] = *(xkb_keycode_t*)sets[depth]->items[i];
cartesian_product_helper(sets, n, current_result, curr_size, current_product, depth + 1);
}
}

/**
* Return the keycode for the specified keysym.
* Compute the calculate the Cartesian product of `n` sets
*/
static struct keycode_matches get_keycode_for_keysym(xkb_keysym_t keysym) {
struct keycode_matches matches = {
.keysym = keysym,
.keycode = XKB_KEYCODE_INVALID,
.count = 0,
};

xkb_keymap_key_for_each(
xkb_state_get_keymap(config->keysym_translation_state),
find_keycode, &matches);
return matches;
xkb_keycode_t **cartesian_product(list_t **sets, int n) {
int total_combinations = 1;
for (int i = 0; i < n; ++i) {
total_combinations *= sets[i]->length;
}

// Allocate memory for the current_result
int result_size = 0;
xkb_keycode_t **result = malloc(total_combinations * sizeof(xkb_keycode_t*));
xkb_keycode_t *current_product = malloc(n * sizeof(xkb_keycode_t));
cartesian_product_helper(sets, n, result, &result_size, current_product, 0);
free(current_product);

return result;
}

/*
* Convert keysyms to keycodes for --to-code. If any keysym does not have at
* least 1 keycode: halt conversion and unset BINDING_CODE. Assumes
* identify_key() read in binding->keys in keysym format vs keycode format
*/
bool translate_binding(struct sway_binding *binding) {
if ((binding->flags & BINDING_CODE) == 0) {
return true;
}

switch (binding->type) {
// a bindsym to translate
case BINDING_KEYSYM:
binding->syms = binding->keys;
binding->keys = create_list();
break;
// a bindsym to re-translate
case BINDING_KEYCODE:
list_free_items_and_destroy(binding->keys);
binding->keys = create_list();
break;
Comment on lines -708 to -712
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That worked for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sahinf sahinf Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

What's the "original PR" btw?

#3058

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Thanks.

default:
if (binding->type != BINDING_KEYSYM ||
(binding->flags & BINDING_CODE) == 0) {
return true;
}

for (int i = 0; i < binding->syms->length; ++i) {
xkb_keysym_t *keysym = binding->syms->items[i];
struct keycode_matches matches = get_keycode_for_keysym(*keysym);

if (matches.count != 1) {
sway_log(SWAY_INFO, "Unable to convert keysym %" PRIu32 " into"
" a single keycode (found %d matches)",
*keysym, matches.count);
goto error;
int num_syms = binding->keys->length;
list_t **sym2code = malloc(num_syms * sizeof(list_t*));
if (!sym2code) {
sway_log(SWAY_ERROR,
"Unable to allocate memory for symbol:keycode map");
}
// Collect all keycodes for all keysyms
for (int i = 0; i < num_syms; i++) {
struct keycode_matches matches = {
.keysym = (xkb_keysym_t*)binding->keys->items[i],
.keycodes = create_list(),
.error = false
};

xkb_keymap_key_for_each(
xkb_state_get_keymap(config->keysym_translation_state),
add_matching_keycodes, &matches);

if (matches.error) {
sway_log(SWAY_ERROR,
"Failed to allocate memory for keycodes while "
"iterating for keysym %" PRIu32,
*matches.keysym);
goto error;
}

xkb_keycode_t *keycode = malloc(sizeof(xkb_keycode_t));
if (!keycode) {
sway_log(SWAY_ERROR, "Unable to allocate memory for a keycode");
goto error;
if (matches.keycodes->length == 0) {
sway_log(SWAY_INFO,
"Unable to convert keysym %" PRIu32 " into"
"any keycodes",
*matches.keysym);
goto error;
}

*keycode = matches.keycode;
list_add(binding->keys, keycode);
sym2code[i] = matches.keycodes;
}

list_qsort(binding->keys, key_qsort_cmp);
binding->type = BINDING_KEYCODE;
// If any keycode maps to more than one keysym, use all combinations.
xkb_keycode_t **combos = cartesian_product(sym2code, num_syms);

for (int i = 0; i < num_syms; i++) {
struct sway_binding *copy_binding = malloc(sizeof(*copy_binding));
binding->type = BINDING_KEYCODE;
*copy_binding = *binding;
sahinf marked this conversation as resolved.
Show resolved Hide resolved
list_t *keys = create_list();

// copy the keys over
for (int j = 0; j < num_syms; j++) {
xkb_keycode_t *key = malloc(sizeof(*key));
if (!key) {
sway_log(SWAY_ERROR,
"Unable to allocate memory for key when translating");
}
*key = combos[i][j];
list_add(keys, key);
}
copy_binding->keys = keys;
list_qsort(copy_binding->keys, key_qsort_cmp);
binding_add_translated(copy_binding, config->current_mode->keycode_bindings);
}
for (int i = 0; i < num_syms; i++) {
list_free_items_and_destroy(sym2code[i]);
}
return true;

// if any key cannot be translated, the binding revert to keysym binding
error:
list_free_items_and_destroy(binding->keys);
for (int i = 0; i < binding->keys->length; i++) {
list_free_items_and_destroy(sym2code[i]);
}
free(sym2code);
binding->type = BINDING_KEYSYM;
binding->keys = binding->syms;
binding->syms = NULL;
return false;
}

Expand All @@ -752,6 +806,7 @@ void binding_add_translated(struct sway_binding *binding,
struct sway_binding *config_binding =
binding_upsert(binding, mode_bindings);

binding->order = binding_order++;
if (config_binding) {
sway_log(SWAY_INFO, "Overwriting binding for device '%s' "
"to `%s` from `%s`", binding->input,
Expand Down
44 changes: 10 additions & 34 deletions sway/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,28 +972,6 @@ void config_update_font_height(void) {
}
}

static void translate_binding_list(list_t *bindings, list_t *bindsyms,
list_t *bindcodes) {
for (int i = 0; i < bindings->length; ++i) {
struct sway_binding *binding = bindings->items[i];
translate_binding(binding);

switch (binding->type) {
case BINDING_KEYSYM:
binding_add_translated(binding, bindsyms);
break;
case BINDING_KEYCODE:
binding_add_translated(binding, bindcodes);
break;
default:
sway_assert(false, "unexpected translated binding type: %d",
binding->type);
break;
}

}
}

void translate_keysyms(struct input_config *input_config) {
keysym_translation_state_destroy(config->keysym_translation_state);

Expand All @@ -1008,18 +986,16 @@ void translate_keysyms(struct input_config *input_config) {

for (int i = 0; i < config->modes->length; ++i) {
struct sway_mode *mode = config->modes->items[i];

list_t *bindsyms = create_list();
list_t *bindcodes = create_list();

translate_binding_list(mode->keysym_bindings, bindsyms, bindcodes);
translate_binding_list(mode->keycode_bindings, bindsyms, bindcodes);

list_free(mode->keysym_bindings);
list_free(mode->keycode_bindings);

mode->keysym_bindings = bindsyms;
mode->keycode_bindings = bindcodes;
for (int j = 0; j < mode->keysym_bindings->length; j++){
struct sway_binding* binding = mode->keysym_bindings->items[j];
if(translate_binding(binding)) {
return;
}
list_t *mode_bindings = binding->type == BINDING_KEYCODE
? config->current_mode->keycode_bindings
: config->current_mode->keysym_bindings;
binding_add_translated(binding, mode_bindings);
}
}

sway_log(SWAY_DEBUG, "Translated keysyms using config for device '%s'",
Expand Down