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

Use keycode for keybindings #1953

Merged
merged 3 commits into from
Oct 26, 2021
Merged

Use keycode for keybindings #1953

merged 3 commits into from
Oct 26, 2021

Conversation

vantu5z
Copy link
Contributor

@vantu5z vantu5z commented Oct 25, 2021

Try to fix #1946.

@Davidy22
Copy link
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
Copy link
Contributor Author

vantu5z commented Oct 26, 2021

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).

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

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
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