Keyboard shortcut fixes

[Davidy22]

Fix keyboard shortcuts without the shift key, switch tab mappings and update split tab vertical keybind default to new keybinding code.

Fixes #1939

[mlouielu]

Can you also verify this issue #1942

[mlouielu]

Yes, this PR also fixed #1942

[mlouielu]

Is there a conflict on this so changing this default binding?

[Davidy22]

I discovered during testing that the previous keybinding doesn't work in the new code because the key combination generates the event with a comma instead of the less than sign. Potentially might have been super+shift+comma before, but split tab horizontal doesn't need the shift key so I went with cutting the shift key for symmetry.

[mlouielu]

It seems legit for me, super + < is super + shift + , because shift + , = < on the keyboard.

As we can test it with binding shortcut as < will need to input with shift + comma.

I would like to not change this, too.

[mlouielu]

I do want to know why this PR also fixed #1942

[Davidy22]

Fixes all lettered keyboard shortcuts without the shift key, as it turns out the only keyboard shortcut I used that didn't have a shift in it was CTRL+tab, which was unaffected, so I just never noticed this issue. 1942 deals with keyboard shortcuts that only have CTRL in them, so they are also affected by this.

[jpribyl]

oof, thank you for doing this - thought I was going crazy at first

[Davidy22]

@mlouielu Do you have anything else that you want clarified? This resolves a fairly major issue, it'd be nice if this finished getting through the review process

[vantu5z]

Is it works for not EN layout?
Maybe we should use hardware keycodes (for example event.get_scancode()) for keybinds.

[Davidy22]

If you have a non-us keyboard, are you able to confirm whether this patch works or not on a non-us keyboard?

EDIT: From quick testing, event.get_scancode() returns 0 for all keys, so there may be additional arguments to this I need to read up on but I think this specifically isn't the way to go

[vantu5z]

Maybe this is another issue but keybinds with layout sensitive keys (letters and other) doesn't work.
If it metter I use Arch Linux.

For example copy keys (Ctrl+Shift+C)
For US layout

keyval:  67
mod:  <flags GDK_SHIFT_MASK | GDK_CONTROL_MASK | GDK_MOD2_MASK of type Gdk.ModifierType>
scancode:  54

For RU layout

keyval:  1779
mod:  <flags GDK_SHIFT_MASK | GDK_CONTROL_MASK | GDK_MOD2_MASK | GDK_MODIFIER_RESERVED_13_MASK of type Gdk.ModifierType>
scancode:  54

[Davidy22]

Yeah, should probably be in its own issue, this fix doesn't need more reasons to get delayed. I'll note though that we're using the event keyval, not keycode, which do give different values on cursory testing. Can try see why get_scancode() only spits out 0s for me, in the meantime does rebinding the shortcuts at least work on the russian layout?

[vantu5z]

should probably be in its own issue, this fix doesn't need more reasons to get delayed

Yes, I opened separate issue #1946.

does rebinding the shortcuts at least work on the russian layout?

Yes, I can rebind keys for russian layout and it works, but when I switch to english it will not work.

get_scancode() only spits out 0s for me

Do you try get_keycode() insted of get_scancode()?

In Guake 3.7.0 keybindings works on both layouts, I think because there used Gtk.AccelGroup.
This was changed in commit 6af0a95

[Davidy22]

There is a get_keycode(), can try it and make a branch for it in another pr. Also, @mlouielu, is there anything still holding this PR up? I'm fine if you have comments to add, but radio silence for two days on an issue that you marked as high priority doesn't help us get this fixed.

[mlouielu]

I would like not to change the default binding. This may just be GTK preference of showing shift binding.

[mlouielu]

It seems legit for me, super + < is super + shift + , because shift + , = < on the keyboard.

As we can test it with binding shortcut as < will need to input with shift + comma.

I would like to not change this, too.

[mlouielu]

But yes, as @vantu5z point out, different keyboard layout give different result here
When I use RU layout, binding Super+< will be Shift+Super+<.

I think we can put this in another issue or PR to discuss how to handle this.

[Davidy22]

Alright, split shortcut changed to shift+comma.

[mlouielu]

lgtm

[Davidy22]

Late today, I'll write the release announcement in the morning

[cmacht]

Sorry to budge in here again, but this was amazing to me: I updated guake on Friday, it broke my shortcuts. I considered using a different drop-down terminal. Half-heartedly, I sat down to write a bug report. In a matter of hours @Davidy22 jumped in and explained and linked my issues to his PR and now, two days later, within a weekend, where everyone should really enjoy their free time at leisure, this is merged and might just become available in my updates now.

In only 3 days! Forgive my excitement, this is the first time I witnessed Open Source being this awesome. @Davidy22, @mlouielu you guys rock! 🥳

[besworks]

@cmacht, same kind of experience here. Guake is an integral part of my daily workflow and it's very nice to see this issue resolved quickly. By the way, this fix is not available in pacman yet but I have flagged the package as out of date and requested that it be updated.

For anyone interested, the Arch Linux Package has been updated. My keyboard shortcuts are working perfectly again. Thanks to everyone who got this taken care of! 👍