Skip to content

Use keycode for keybindings - #1953

Merged
Davidy22 merged 3 commits into
Guake:masterfrom
vantu5z:fix_keybinds
Oct 26, 2021
Merged

Use keycode for keybindings#1953
Davidy22 merged 3 commits into
Guake:masterfrom
vantu5z:fix_keybinds

Conversation

@vantu5z

@vantu5z vantu5z commented Oct 25, 2021

Copy link
Copy Markdown
Contributor

Try to fix #1946.

@Davidy22

Copy link
Copy Markdown
Collaborator

As I don't have an affected keyboard, I can't confirm that this fixes the issue, but upon quick testing it doesn't break my own keybindings so this approach at least seems to operate generally correctly. Has less specific logic too which is nice. I presume as the submitter of the original issue that this works for you?

Once I get some confirmation that this fixes the issue I just need you to do a make reno SLUG=use_keycode_for_keybindings and fill the in relevant sections, delete the irrelevant ones and I'll merge. I can fill in the release notes file later myself if I get the time.

@vantu5z

vantu5z commented Oct 26, 2021

Copy link
Copy Markdown
Contributor Author

I just need you to do a make reno SLUG=use_keycode_for_keybindings and fill the in relevant sections, delete the irrelevant ones

Can you look if I do it right?

I presume as the submitter of the original issue that this works for you?

Yes, it works for me. And it allow to add keybindings using non English layout (language) too and it will work with English.

Anyway I have some doubt about Gtk.accelerator_parse_with_keycode(accelerator) as documentation says that we should not use it with not system-level components, but as we choose key-press-event way for keybindings I think we can use it.

Another place of possible issue - accelerator_parse_with_keycode returns accelerator_codes list. For now I use the first item from list, but I think this list can differ depending on the system configuration. Or list variable used because accelerator_codes try to be parsed by multiple ways and keycodes will be identical in the list (for me is only one item in this list).

@Davidy22 Davidy22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe the first one in the list is the one that would be expressed in a text editor and expected by the user, so the first one should be fine. Release note file looks good, merging.

@Davidy22
Davidy22 merged commit a084a1d into Guake:master Oct 26, 2021
@mlouielu mlouielu added this to the 3.8.2 milestone Nov 16, 2021
@fx-kirin

Copy link
Copy Markdown
Contributor

This change breaks shortcut with custom key mapping through xkb. It would be nicer there is an option to choose the behavior.

This #2058 might be a case too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keybindings does not work with not English layout.

4 participants