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

Superkey shortcuts being hijacked/blocked. #55

Closed
the-mentor opened this issue May 22, 2017 · 39 comments
Closed

Superkey shortcuts being hijacked/blocked. #55

the-mentor opened this issue May 22, 2017 · 39 comments
Assignees

Comments

@the-mentor
Copy link

the-mentor commented May 22, 2017

Dear developer,
With the recent update to brisk menu 0.4.0 the super key shortcut is being hijacked and other shortcuts that use the super key dont work anymore for example i have super+space configured for "albert" launcher and it doesn't work since the latest brisk update.

Edit: this seems to only affect the left super key the right super key works fine.

Also another issue is super key opens the brisk menu it would be nice if hitting the supper key once the menu is open would close the menu.

thank you and keep up the good work.

@the-mentor the-mentor changed the title Superkey being hijacked issue. Superkey shortcuts being hijacked. May 22, 2017
@the-mentor the-mentor changed the title Superkey shortcuts being hijacked. Superkey shortcuts being hijacked/blocked. May 22, 2017
@ikeydoherty
Copy link
Member

If other things are hijacking it then you can change your keyboard shortcut to not use Super_L.
Given your hijack issue its probably why it's not closing again after.

@the-mentor
Copy link
Author

the-mentor commented May 22, 2017

@ikeydoherty thanks for the quick reply.
the problem is that brisk is hijacking the shortcuts of other programs.
if you want to reproduce the issue install "albert" launcher set the shortcut to super+space and then try to launch albert using the super+space key.
you'll see that it doesn't work.
if you use the right super key + space it works.
this seems to be caused by the latest update to brisk.

edit: i also use a bunch of other shortcuts that use the left super key that don't work.
for example i use left super + right to snap a window to the right and that doesn't work.
its a real bummer since this update made my existing workflow unusable.

@ikeydoherty
Copy link
Member

Gotcha.

@ikeydoherty
Copy link
Member

Is Brisk showing when you release the button or before?

@ikeydoherty
Copy link
Member

Confirmed that the hungry bastid is eating it

@the-mentor
Copy link
Author

Brisk shows when i release the button.
also I'm using Solus MATE if that changes anything.
i have no idea what is bastid but i'm happy you confirmed whats causing the issue.

@flexiondotorg
Copy link
Contributor

@ikeydoherty I tested the original pull request for Super support it was working correctly and not swallowing Super for other applications.

@flexiondotorg
Copy link
Contributor

@vkareh Can you take a look?

@vkareh
Copy link
Contributor

vkareh commented May 31, 2017

Of course! Just tested the latest from master and I can experience the following behavior:

  • When the menu is closed, other Super bindings get swallowed sporadically (I can move certain windows, but not others, using Super+Left/Right/etc). Sometimes it works once or twice, then stops working until I open and close the menu.

  • When the menu is open, pressing on the Super_L key closes the menu, and other Super bindings (called within the same key-press) work as expected. I don't have a Super_R key, but I suspect it shouldn't ever be an issue, since combo shortcuts use <Mod4>+Something, whereas brisk-menu uses <Super_L> specifically.

  • Closing the menu seems to reset whatever state the keyboard binding is reading, which gives me the impression that the issue might be in the logic within the brisk_key_binder_filter function, or thereabouts...

I'll play with the key-binder code throughout the day and see what I can figure out. I'll report back as soon as I have something.

@vkareh
Copy link
Contributor

vkareh commented Jun 1, 2017

Okay, here are some of my findings:

  • When doing a Super+something shortcut, the brisk-menu code sends the event (using XSendEvent) to a NULL window, the reasoning here was that by not setting a window, it will propagate the event up the window hierarchy until it found a window that was grabbing it, or disappear into the æther... Seems like I was wrong, and setting the window to be default root window instead does the trick for shortcuts that involve windows (Super+Left/Right/etc). I can have a pull request ready for this at any point today, but...

  • Second, if I set Super-based shortcuts from the MATE shortcuts combination app, they don't work. However, if I set the same shortcuts from the Compiz configuration, they work just fine. I can test whether using Marco would make a difference in this (but I'm currently at work and messing around with my wm right now seems like a ripe time for a client to request an impromptu demo of something...). I will test with Marco later today and report back.

These two items seem related somehow - fixing the XSendEvent window seems to allow Compiz to finally allow shortcuts to happen on its windows. I don't understand enough about the interplay between MATE and Compiz/Marco to make a more educated guess than that, but they seem related to me...

Thoughts, @flexiondotorg ?

In fact, in testing mate-menu, which uses the exact same technique, I can get the same behavior of Compiz super-based shortcuts work well, but MATE super-based shortcuts don't... now I'm eager to test out the wm theory for both menu apps. I'll report back later today, hopefully

@vkareh
Copy link
Contributor

vkareh commented Jun 1, 2017

More findings:

  • I couldn't wait and went ahead with changing back and forth between Compiz and Marco - no luck. Regardless of the window manager, whenever brisk-menu (or mate-menu for that matter) is running (with the Super key), MATE shortcuts that use the Super key do not work. When using Compiz, Compiz shortcuts work just fine (once I apply my fix for brisk-menu, of course).

  • I'm not ready to blame MATE shortcuts yet, though - I want to run through a few more scenarios, but I can't guarantee that I won't clone the code and hack at it at some point ;)

  • I think it makes sense for me to send in a pull request with the fix, even if some shortcuts are still swallowed.

vkareh added a commit to vkareh/brisk-menu that referenced this issue Jun 1, 2017
This fixes an issue where shortcuts that use the Super key are being
swallowed by brisk-menu. This branch does not fix _all_ instances of the
issue, but a large subset of them seem to be fixed by it.

A workaround is to set any desired shortcuts that use the Super key
directly in the Compiz configuration (assuming you use Compiz).

This partially addresses the issues in solus-project#55.
@vkareh
Copy link
Contributor

vkareh commented Jun 2, 2017

okay, #59 fixes part of this issue by allowing the correct XEvent behavior during the keybinding filter. It's very much compatible with the way that the mate-settings-daemon does it (although I have issues with mate executing the bound command on KeyPress, rather than on KeyRelease, but whatever, it doesn't seem to interfere for now).

I think the way brisk-menu does the initial key grabbing during the binding phase is where the issue lies... will keep experimenting with different key-grabbing techniques. mate-settings-daemon has a grab_key_unsafe function that I'll have to explore in more detail. So far it seems promising.

@ikeydoherty - After applying #59, does the issue persist in Budgie? Or is it a MATE-only thing?

@ikeydoherty
Copy link
Member

Budgie locks out the windows key from grabs at the compositor level so its impossible to test there

@vkareh
Copy link
Contributor

vkareh commented Jun 6, 2017

Summary time. This is my experience so far:

  • Marco and Compiz <Mod4> shortcuts work just fine, at all times, regardless of any other shortcut settings (assuming Fix Super key shortcuts being swallowed #59 has been applied).
  • mate-settings-daemon (which handles bindings for the MATE Keyboard Shortcuts app) <Mod4> shortcuts do not work whenever there is another application with a shortcut bound to the Super_L key.
  • An application's <Mod4> shortcuts do not work whenever there is another application's shortcut bound to the Super_L key.
  • Super_L shortcuts take precedence over <Mod4> shortcuts.

Apart from the first item above, this is no longer a brisk-menu issue. It's a MATE issue.

It seems that whatever higher entity is handling global keybindings in MATE has an issue with the Super_L/<Mod4> pair. Since all key grabs are done directly through XLib, it all points to mate-settings-daemon, unless there's yet another entity handling global keybindings, @flexiondotorg?

I would try to change mate-settings-daemon to handle shortcuts on KeyRelease, rather than on KeyPress to see if that would solve the problem, but I have no idea how to even start compiling and testing it without messing up my computer... VM time?

@vkareh
Copy link
Contributor

vkareh commented Jun 7, 2017

Welp, it seems I was wrong... I recompiled mate-settings-daemon without the key grabs, and the behavior persists between brisk-menu and mate-menu. Seems like any program that attempts global key grabs, if one of them is using Super_L, all others using <Mod4> don't grab it anymore. I'm not aware of any other programs that do global key grabs, so I'm not testing with anything else at the moment.

  • <Mod4> key grabs work well as long as Super_L is not bound
  • When Super_L is bound, <Mod4> shortcuts don't even grab (it's not even a filtering logic issue - it just plain does not grab it)

I still refuse to blame XLib for this, so I'm going to focus on the different permutations of modifier masks set during the grab phase. Maybe something along the lines of "if the bound key is Super_L, remove/add Mod4 masks to the XGrabKey call", who knows... I'm just hoping that I don't have to go the route of coding "if any other program has a bound Super_L, and this one is Mod4, then change this and that when calling XGrabKey". Not sure if I'm explaining this right, I'm running out of ideas.

Research and tinkering continues...

@vkareh
Copy link
Contributor

vkareh commented Jun 8, 2017

w00t! That was some seriously painful spelunking, but I submitted a pull request to the mate-settings-daemon project that fixes this issue for brisk menu.

From what I can see, any program that expects to register global keybindings that may have similar issues (mainly using the <Mod4> key), should implement a similar fix. I'll try to do the same for brisk-menu.

@vkareh
Copy link
Contributor

vkareh commented Jun 8, 2017

hmmm - it's probably not necessary for brisk-menu to implement this, as it seems low probability that someone has brisk menu running, mapped to a <Mod4> shortcut, and have a different app (most likely another menu) mapped to Super_L.

I'm saying this in part because this change would mean that brisk-menu needs to request event reports from the window hierarchy of every screen, which seems both unnecessary and very likely a bad idea. The MATE settings daemon, on the other hand, is already aware of all the windows by definition, so it's relatively benign to expect it to be aware of keybindings sent to application windows.

I think mate-desktop/mate-settings-daemon#179 should take care of this issue at a general level, and any other menu/launcher that feels this is necessary can make that call independently.
@flexiondotorg, @ikeydoherty - thoughts?

@ikeydoherty
Copy link
Member

OK so - does this fix the original issue with Albert?

@vkareh
Copy link
Contributor

vkareh commented Jun 8, 2017

@ikeydoherty - it does not, as far as I know.

I think Albert needs to implement the fix for it to work. I'm curious as to why it used to work and now it doesn't. Maybe it has to do with Brisk checking for KeyRelease rather than KeyPress, but I haven't been able to get Brisk to work consistently with just the KeyPress event...

@ikeydoherty
Copy link
Member

Yeah I ran into the same issues and then the Async KeyPress buggered up KeyRelease..

@vkareh
Copy link
Contributor

vkareh commented Jun 8, 2017

I can keep trying - since Brisk moved to Async key grabs, a lot has changed in terms of interoperability between X keybindings and Brisk. I will give it another shot and see where it leads.

@vkareh
Copy link
Contributor

vkareh commented Jun 8, 2017

yeah, it doesn't work... all I can manage is to get brisk to show up once, after that the menu entry gets highlighted in the panel, but the menu itself doesn't open (if I squint the right way, it seems like it opens and closes in quick succession). I'm really out of ideas now - I've been trying to get this thing to work with just KeyPress for a few days now thinking it might help, but nothing...

Sorry, @the-mentor 😢

@ikeydoherty
Copy link
Member

We could maybe hack it. Make Brisk add an idle/timeout callback to then toggle the menu and cancel rapid successions ?

@the-mentor
Copy link
Author

the-mentor commented Jun 8, 2017

@vkareh don't be sorry i know you've spent a bunch of time on it and i really appreciate it!
I wonder how other menus do it.
Also if there was a way to change the hotkey that would also be good since i would probably chance it to something else since i use the Super_L key for more things just that menu.

Edit: I guess i really love my launcher and the Super_L + Spacebar key combo :D

@vkareh
Copy link
Contributor

vkareh commented Jun 9, 2017

@ikeydoherty that seems like a good idea - a debounce is far from a hack, even though Xlib would probably disagree.

@the-mentor, thanks for your patience and understanding. The only other menu in MATE that is by default bound to the Super key is the Advanced MATE Menu, and it has the exact same issue as Brisk.

To change the hotkey, open dconf and go to "com / solus-project / brisk-menu / hot-key" (or something along those lines, I'm on my phone and going by memory. I use Ubuntu, so it might be different for you). Also, I totally get the love for a specific hotkey, it's literally the reason I got suckered into this project in the first place :)

@vkareh
Copy link
Contributor

vkareh commented Jun 9, 2017

@ikeydoherty - just realized that part of the reason that it's necessary to check for both KeyPress and KeyRelease is because we're using the Super_L key - since Super is both a key and a modifier, we need to make sure that a user pressing Super did not intend to press Mod4 instead, therefore we check for both KeyPress and KeyRelease to make sure the user did not press another key in between (in which case it should be treated as Mod4, rather than Super)

In playing with this some more, if we do only KeyPress or only KeyRelease, Brisk gets triggered for both Super_L and <Mod4>, which means that the menu will open when I'm, say, snapping windows with the keyboard or other such action...

To clarify with an example, say I want to open a terminal, mapped to <Mod4>t in MATE - here's how it currently happens:

  1. KeyPress Super
  2. KeyPress t
  3. terminal opens (mate-settings-daemon only looks at KeyPress)
  4. KeyRelease t
  5. KeyRelease Super

Unless we check for both KeyPress and KeyRelease, Brisk would open at either 1 or 5, along with the terminal at 3. So in Brisk we check that nothing else was Pressed/Release in between the Super key: https://github.com/solus-project/brisk-menu/blob/master/src/lib/key-binder.c#L185-L192

None of this would be necessary with other key combinations, but to support the Super_L key, we're stuck doing it this way. I'm assuming using other Mod keys (Ctrl, Alt, Shift, etc) as triggers would have similar issues, but Super seems to be the favorite. Other key combinations do not have any of these issues... Also, MATE does not allow you to set a single modifier key as a shortcut - to support that would be a nightmare!

Does that make sense?

What this means is that we're currently stuck keeping the code as is, hope mate-settings-daemon gets their pull request in to fix swallowed keys within MATE, and let other launchers implement their own logic however they choose. Then some time in the next decade we'll finally replace X with something more modern 😝

@ikeydoherty
Copy link
Member

Alternatively we could have MSD provide the global key handlers for all MATE apps over dbus, similar to GSD. Otherwise, yes, this is the best path right now.

@vkareh
Copy link
Contributor

vkareh commented Jun 9, 2017

MSD taking care of this would be lovely, still, they don't support single-modifier shortcuts :-/

Another alternative could be to have the different launchers/menus (Brisk, Albert, etc) provide an executable to just open its program window, so Brisk could have a brisk-menu-open program that triggers the menu being open from the panel, and then use MATE to create shortcuts for it - not very elegant, though...

I'll take a look at gnome and see how they do it... I'm already way too deep in this rabbit hole, what's a few more!

@vkareh
Copy link
Contributor

vkareh commented Jun 9, 2017

To recap:

@vkareh
Copy link
Contributor

vkareh commented Jun 9, 2017

@flexiondotorg
Copy link
Contributor

Thank you such a detailed analysis of the issue. Sorry I've been quiet on this, real life and such. I'll take care of the PR in MATE.

@flexiondotorg
Copy link
Contributor

I've tested mate-desktop/mate-settings-daemon#179 and release a patched version of mate-settings-daemon 1.18.1 to the Ubuntu 17.10 archive.

@flexiondotorg
Copy link
Contributor

@vkareh: Please see ubuntu-mate/mate-dock-applet#91 and I'm sorting our some funding for all your hard work, it is very much appreciated.

@flexiondotorg
Copy link
Contributor

@vkareh @ikeydoherty I think this can be closed, right? Patches landed in mate-settings-daemon, marco, mate-dock-applet, etc.

@vkareh
Copy link
Contributor

vkareh commented Aug 16, 2017

@flexiondotorg - for MATE, this is resolved. However, the original report was for albertlauncher/albert. I submitted a fix for it a few months ago but it hasn't been merged...

@the-mentor
Copy link
Author

@flexiondotorg @vkareh @ikeydoherty the issues with MATE R_Super key shortcuts issues still exists in my Solus MATE install.
I'm not refering to the albertluncher issues i'm refering to the MATE desktop shortcuts.
Do you know if these fixes were pushed to Solus ?
My Solus is completely up to date (solus3).

Thank you very much for all your support !!

@flexiondotorg
Copy link
Contributor

flexiondotorg commented Jan 26, 2018

@ikeydoherty @vkareh This issue has been resolved in Ubuntu MATE for many months. Any reason this issue can't be closed? The fact the Albert devs haven't merged a pull request is unfortunate but no reason to keep this issue open here.

@vkareh
Copy link
Contributor

vkareh commented Jan 26, 2018

@flexiondotorg - Good question! I think the original issue referenced the Albert launcher. I sent a patch to the project, but it never got merged. To be honest, I don't even know if it's necessary anymore, we've pretty much addressed all the keybinding blocks in m-s-d and marco at this point.

@the-mentor
Copy link
Author

@flexiondotorg @vkareh @ikeydoherty i'm fine with the existing resolution.
feel free to close this and thank you very much for all the help !!

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

No branches or pull requests

4 participants