Keyboard shortcut fixes by Davidy22 · Pull Request #1941 · Guake/guake

Davidy22 · 2021-10-20T14:08:26Z

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

Fixes #1939

mlouielu · 2021-10-20T23:59:24Z

Can you also verify this issue #1942

mlouielu · 2021-10-21T00:05:40Z

Yes, this PR also fixed #1942

mlouielu · 2021-10-21T00:08:38Z

        </key>
        <key name="split-tab-vertical" type="s">
-            <default>'&lt;Super&gt;less'</default>
+            <default>'&lt;Super&gt;comma'</default>


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

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.

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 · 2021-10-21T00:11:54Z

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

Davidy22 · 2021-10-21T04:20:16Z

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 · 2021-10-22T06:11:00Z

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

Davidy22 · 2021-10-22T06:51:55Z

@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 · 2021-10-22T08:05:59Z

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

Davidy22 · 2021-10-22T08:09:48Z

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 · 2021-10-22T08:34:24Z

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 · 2021-10-22T09:00:37Z

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 · 2021-10-22T10:36:14Z

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 · 2021-10-23T02:01:40Z

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 · 2021-10-24T13:58:22Z

        </key>
        <key name="split-tab-vertical" type="s">
-            <default>'&lt;Super&gt;less'</default>
+            <default>'&lt;Super&gt;comma'</default>


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 · 2021-10-24T14:12:17Z

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.

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

Davidy22 · 2021-10-24T14:48:51Z

Alright, split shortcut changed to shift+comma.

mlouielu

lgtm

Davidy22 · 2021-10-24T15:41:37Z

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

cmacht · 2021-10-24T16:24:44Z

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 · 2021-10-24T16:41:03Z

@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! 👍

Davidy22 requested a review from mlouielu October 20, 2021 14:08

Davidy22 force-pushed the Keybinding branch from 4b40cc0 to 651e36c Compare October 20, 2021 14:47

mlouielu mentioned this pull request Oct 21, 2021

CTRL not working anymore as modifier in "Tab Management" #1942

Closed

mlouielu added this to the 3.8.1 milestone Oct 21, 2021

mlouielu reviewed Oct 21, 2021

View reviewed changes

Davidy22 mentioned this pull request Oct 21, 2021

Keyboard Shortcut "Go to first Tab" goes to second tab #1943

Closed

Davidy22 requested review from mlouielu and removed request for mlouielu October 21, 2021 08:45

Davidy22 mentioned this pull request Oct 22, 2021

feat:Add support for tabs in keyboard shortcuts #1902

Merged

Davidy22 mentioned this pull request Oct 24, 2021

Please, return ctrl+pgup/pgdn as default next/previous tab shortcuts or allow them alongside ctrl+[shift+]tab #1948

Closed

mlouielu requested changes Oct 24, 2021

View reviewed changes

Keyboard shortcut fixes

7c4d380

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

Davidy22 force-pushed the Keybinding branch from 651e36c to 7c4d380 Compare October 24, 2021 14:48

mlouielu self-requested a review October 24, 2021 14:54

mlouielu approved these changes Oct 24, 2021

View reviewed changes

mlouielu merged commit 1062014 into Guake:master Oct 24, 2021

Davidy22 deleted the Keybinding branch October 24, 2021 15:16

Pysis868 mentioned this pull request Dec 21, 2021

Unable to bind any hotkey under GNOME/Wayland #1642

Closed

Uh oh!

Conversation

Davidy22 commented Oct 20, 2021

Uh oh!

mlouielu commented Oct 20, 2021

Uh oh!

mlouielu commented Oct 21, 2021

Uh oh!

mlouielu Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Davidy22 Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

mlouielu Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

mlouielu commented Oct 21, 2021

Uh oh!

Davidy22 commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpribyl commented Oct 22, 2021

Uh oh!

Davidy22 commented Oct 22, 2021

Uh oh!

vantu5z commented Oct 22, 2021

Uh oh!

Davidy22 commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vantu5z commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Davidy22 commented Oct 22, 2021

Uh oh!

vantu5z commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Davidy22 commented Oct 23, 2021

Uh oh!

mlouielu left a comment

Choose a reason for hiding this comment

Uh oh!

mlouielu Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

mlouielu commented Oct 24, 2021

Uh oh!

Davidy22 commented Oct 24, 2021

Uh oh!

mlouielu left a comment

Choose a reason for hiding this comment

Uh oh!

Davidy22 commented Oct 24, 2021

Uh oh!

cmacht commented Oct 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

besworks commented Oct 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Davidy22 commented Oct 21, 2021 •

edited

Loading

Davidy22 commented Oct 22, 2021 •

edited

Loading

vantu5z commented Oct 22, 2021 •

edited

Loading

vantu5z commented Oct 22, 2021 •

edited

Loading

cmacht commented Oct 24, 2021 •

edited

Loading

besworks commented Oct 24, 2021 •

edited

Loading