At the moment there's a problem with keybinding creation and processing. When a keybinding is accessed using shift such as the zoom in keybinding `Ctrl-+` which on English keyboard requires pressing `<Ctrl><Shift>=`, the `<Shift>` key is preserved right now. This is visible when changing keybindings - pressing the above key combination leads to `<Primary><Shift>+`. This is incorrect - the keybinding should either be `<Primary><Shift>=` or `<Primary>+` but in `<Primary><Shift>+` there's extra `<Shift>` which leads to problems.
On linux the problems aren't so much visible - the problem is visible when changing keybindings as mentioned above. Even though this particular keybinding isn't executed through on_key_press_event() because the combination `<Primary><Shift>+` doesn't correspond to the default `<Primary>+` value, it's executed by GTK as an accelerator so the problem isn't directly visible.
This might however break when a user modifies this keybinding to the wrong value `<Primary><Shift>+` and when the user is using multiple keyboards such as English where + has to be accessed over shift and German where typing + doesn't require shift. The keybinding will stop working on the German keyboard in this case because of the extra shift.
On MacOS the accelerator isn't executed for some reason and non- letter keybindings don't work when shift is used.
To fix the issue we can use gdk_keymap_translate_keyboard_state() which reports "consumed" modifiers - that is those which lead to typing the secondary character. However, we don't want this to happen when typing alphabetical characters because it's easier to notice the use of shift in `<Ctrl><Shift>r` than in `<Ctrl>R`. Modifier consumption is undone in this case.
Should also fix https://github.com/geany/geany/issues/694 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1633
-- Commit Summary --
* Normalise key events when creating/processing keybindings * Eliminate keybindings_check_event()
-- File Changes --
M src/keybindings.c (44) M src/keybindings.h (2) M src/prefs.c (16)
-- Patch Links --
https://github.com/geany/geany/pull/1633.patch https://github.com/geany/geany/pull/1633.diff
Just investigated why accelerators are not used on Mac and I completely forgot about my own code
https://github.com/geany/geany/blob/master/src/osx.c#L107
and the corresponding explanation in
https://github.com/geany/geany/commit/ada4595264def0f96d40e223f41243b46580fc...
b4n requested changes on this pull request.
This looks great, and actually fixes some things on Linux as well, as e.g. default keybindings like <kbd><Primary>1</kbd> for custom commands didn't work before on the French layouts where `1` is <kbd><Shift>&</kbd> (so one had to use <kbd><Primary><Shift>1</kbd>). Nice! I'm not very confident, but it might also help fixing some problems we had with Russian layouts on Windows? I'm afraid I remember something a lot more complicated had to be done, though.
However, we need an upgrade path for current keybindings from *keybindings.conf*, because right now it breaks any user-defined keybinding that is affected by the change – e.g. in my case <kbd><Primary><Shift>1</kbd> (which would now be <kbd><Primary>1</kbd>), <kbd><Primary><Shift>greater</kbd> (<kbd><Primary>greater</kbd>), etc.
{
- guint state, keyval; + GdkModifierType consumed; + GdkKeymap *keymap = gdk_keymap_get_default();
ideally would use `gdk_keymap_get_for_display()`, probably on `gdk_window_get_display(ev->window)`
@techee pushed 3 commits.
9612666 Correctly handle cases when caps lock is on 5ea0e08 Make sure we get GtkKeymap from the correct display f240a30 Make sure previously defined keybindings with extra mosifiers still work
techee commented on this pull request.
{
- guint state, keyval; + GdkModifierType consumed; + GdkKeymap *keymap = gdk_keymap_get_default();
Good point, done.
However, we need an upgrade path for current keybindings from keybindings.conf
Fixed in the last patch (hopefully - haven't tested that so please check if it works for you).
I noticed one more problem - we shouldn't treat caps lock as if shift was pressed but just lowercase the key. Should be fixed now.
I pushed the fixes separately so they are easier to review but can squash them for the final merge.
techee commented on this pull request.
@@ -1633,8 +1633,14 @@ static gboolean prefs_dialog_key_press_response_cb(GtkWidget *dialog, GdkEventKe
gpointer data) { GeanyKeyBinding *kb = keybindings_lookup_item(GEANY_KEY_GROUP_HELP, GEANY_KEYS_HELP_HELP); + guint state, keyval; + + if (event->keyval == 0) + return FALSE; + + keybindings_get_normalised_event(event, &state, &keyval);
One should probably check for the "legacy" keybindings with extra modifiers here too but I'm not sure the code should be uglyfied because of that. It's just a single keybinding used in a single specific case so I think it's not worth it.
Couldn't the legacy keybindings be translated at startup somehow?
Well maybe we could feed them somehow through gdk_keymap_translate_keyboard_state(). But this could cause problems when users are switching between two keyboard layouts because the re-mapping will be specific to the keyboard used at Geany start.
Switching between keyboards is really common for me for instance because the Czech keyboard has +ěščřžýáíé in the top row instead of numbers, numbers are accessible through shift and you are missing many characters needed for programming. So when coding one basically has to use the English layout.
Oh dear this is sounding even more like the Cyrillic layouts problem that @b4n referred to above #1368 and the GTK bug report referred to in that.
@b4n Is this pull request waiting for something? There was this https://github.com/geany/geany-osx/issues/8 issue reported against geany-osx and this pull request could fix some of the points.
github-comments@lists.geany.org