Refactoring session 2 #233

antis81 · 2021-09-13T12:44:29Z

Description

Registering a shortcut action on one of the "Super" keys (aka "Windows" or "Cmd") does not completely work and th lxqt-globalkeys the KeyPress event is not "forwarded" to the window manager.

For example:

assign Super+D to the window manager (e.g. openbox, kwin,… or other non-LXQt globalkeys handler)
assign Super_L or Super_R to an lxqt-globalkeys action (e.g. "show/hide main menu")

Result:
The third-party key handler (openbox, kwin,…) no longer gets a KeyPress event and does not trigger.

Debugging/Fix

For debugging this it is important to understand:
As long as no BaseAction is registered on the "Super" keys no XEvent is generated at all. (Why?)

Notes

To make the code maintable this PR also implies refactoring (Session 2):

turn common const strings into const members
move shortcut state machine to dedicated function (-> RAII)

antis81 · 2021-09-13T18:02:29Z

@palinek Would you help with testing this again? I plan on implementing the full Super/Meta behaviour now.

At this point it's still refactoring. Also I haven't touched the "grabber" code path yet - because it works. I plan on this in the last step after everything works.

palinek

Working as expected. Even assigning the "plain" Meta to an action and Meta + <something> to another is working. The only difference now is that Meta +<something> is triggered upon the <something> keypress and the plain Meta is triggered upon keyrelease.

I plan on implementing the full Super/Meta behaviour now.

Do you just intend to unify the behavior to trigger "everything" on keyrelease?

palinek · 2021-09-14T06:24:17Z

daemon/core.h

@@ -146,6 +146,7 @@ class Core : public QThread, public LogTarget
    void grabShortcut(const uint &timeout, QString &shortcut, bool &failed, bool &cancelled, bool &timedout, const QDBusMessage &message);
    void cancelShortcutGrab();

+    void on_shortcut(const Ids& ids) const;


Is there a reason for not following the naming style here?

Because Qt vs STL signal/slot cardinality. 😺 (Actually onShortcutGrabbed signal is the "black sheep" in this context.)
However it doesn't matter, just feels weird seeing a Qt signal named "onXYZ".

EDIT: See 68e4f73 (that's maybe another reason)

palinek · 2021-09-14T06:25:48Z

daemon/core.cpp

+    case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST:
+    {
+        auto lastIds = ids.end();
+        for (auto idi = ids.begin(); idi != lastIds; ++idi)


Can we use range loop in this?

See the MULTIPLE_ACTIONS_BEHAVIOUR_LAST case below (reverse iteration). Just thought it being better to understand this way.

antis81 · 2021-09-14T19:05:02Z

@palinek Added more dbugging output -> makes it actually visible now why Super_L fails. Would you have a look? (To enable log output search for #if 0 line.)

antis81 · 2021-09-16T08:16:13Z

@palinek I have rewritten the PR description (the assumption was wrong -> the log messages show what is happening). Could you please re-read? Why is no XEvent generated?

antis81 · 2021-09-17T16:46:16Z

@palinek Interesting: XFCE uses xcape, which remaps e.g. Super_L -> Alt+F1. However this is just a workaround and we should have a look inside the source and see how the XEvent is re-emitted to other processes.

antis81 · 2021-09-20T17:43:02Z

@tsujan Would you have an idea how we could "pass through" XEvents when unhandled by globalkeysd?

Background:
None of the modifier keys will create KeyPress/-Release events. Events are usually created when pressing/releasing the second/third/… key (then it sets the "state" flags). However if we register an Action (BaseAction) on a standalone modifier key (aka Super) an XEvent will be created on both key press & release of that key. Plus it will no longer be "passed" to other applications. Hence e.g. openbox will no longer receive key events containing the Super key.

tsujan · 2021-09-20T18:19:03Z

Not being an expert in XEvents, I think it's logical to expect that, if pressing of a modifier key is treated as a shortcut, that key couldn't be used as a modifier anywhere. However, there may be a different story about releasing it.

antis81 · 2021-09-20T18:33:02Z

However, there may be a different story about releasing it.

Exactly: Super_L/R should not trigger a KeyPress event - but it does. What is unclear to me is why do we only get this event, when an action is registered? (If you like to see it for yourself please enable the #if 0 line).

tsujan · 2021-09-20T19:15:43Z

Since I don't have time to read the code for now, ignore my comment if it isn't relevant but shortcuts work everywhere when the last key is pressed, not when it's released. That's why a modifier can't be a usual shortcut. To make a global shortcut out of a modifying key forcefully, one may need to trigger the action only when it's released. Even that might have side effects, I think.

antis81 · 2021-09-20T21:43:57Z

@tsujan Actually no need to read the code. Just change the #if 0 to #if 1, compile & run. With this we can now actually see what is going on. Code is mergable in this state (carries quite some goodies). It will not fix the Super keys though.

Similar to what you state:
For fixing the Super keys we'd need to register an action that only creates an XEvent with type KeyRelease. Practically this will not have side-effects with other applications. X11 cleanly sends the KeySym for Super_L / Super_R. If you are concerned about lxqt-globalkeys -> it already works with Meta+<key> AND Super_L. Try it and you will see!

tsujan · 2021-09-21T08:44:47Z

Actually no need to read the code. Just change the #if 0 to #if 1, compile & run.

Will do when I become a little less busy. Keep up the good work!

Still refactoring! Note that the "continue" is no longer needed and replaced by the update function's "return" (aka "early exit").

1. When debugging we do not want filtered information in the output. 2. Also logs the key state on release before any filter is applied.

Refactoring notes: - "signal" parameter is passed by reference (-> instead of char*) - the "return;" line replaces the "while-break" (throw instead?)

Actually the pointer is unused in XyzAction classes.

Makes it easier to follow the code flow.

palinek reviewed Sep 14, 2021

View reviewed changes

antis81 force-pushed the patch-0 branch from ffe849a to 9520f11 Compare September 17, 2021 20:21

antis81 changed the title ~~enable Super_L/R key shortcut~~ Fully support Super_L/R key shortcut Oct 4, 2021

antis81 mentioned this pull request Dec 20, 2021

Shortcuts Ignored After Upgrade #247

Closed

antis81 added 16 commits December 21, 2021 11:30

move shortcut state machine to dedicated function

234e946

Still refactoring! Note that the "continue" is no longer needed and replaced by the update function's "return" (aka "early exit").

move shortcut action creation into on_shortcut() function

993c4f3

log unfiltered key states one key press & release events

8fc27fc

1. When debugging we do not want filtered information in the output. 2. Also logs the key state on release before any filter is applied.

scope brackets

f317187

signal/slot naming

ee2ad88

move pending event handling into dedicated funcion

88b3d18

Refactoring notes: - "signal" parameter is passed by reference (-> instead of char*) - the "return;" line replaces the "while-break" (throw instead?)

add detailed logging for debugging

51518b0

change comment to what it actually does

bead912

add debugging log option

659629e

replace another switch-case default

7824cd9

some minor readability improvements

8d403ba

fix vtable warnings for LogTarget

e6a98f6

Actually the pointer is unused in XyzAction classes.

use static_cast for BaseAction type conversion

9a7e25b

simplify modifier for-loops

a7d5036

add template methods for reading/writing X11 pipe requests/responses

ab589c8

utilize read/write X11 pipe methods in handlePendingEvents

ec6523a

Makes it easier to follow the code flow.

antis81 force-pushed the patch-0 branch from fc02136 to ec6523a Compare December 21, 2021 10:30

antis81 changed the title ~~Fully support Super_L/R key shortcut~~ Refactoring session 2 Dec 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring session 2 #233

Refactoring session 2 #233

antis81 commented Sep 13, 2021 •

edited

Loading

antis81 commented Sep 13, 2021

palinek left a comment

palinek Sep 14, 2021

antis81 Sep 14, 2021 •

edited

Loading

palinek Sep 14, 2021

antis81 Sep 14, 2021

antis81 commented Sep 14, 2021

antis81 commented Sep 16, 2021

antis81 commented Sep 17, 2021 •

edited

Loading

antis81 commented Sep 20, 2021

tsujan commented Sep 20, 2021

antis81 commented Sep 20, 2021

tsujan commented Sep 20, 2021

antis81 commented Sep 20, 2021

tsujan commented Sep 21, 2021

Refactoring session 2 #233

Are you sure you want to change the base?

Refactoring session 2 #233

Conversation

antis81 commented Sep 13, 2021 • edited Loading

Description

Debugging/Fix

Notes

antis81 commented Sep 13, 2021

palinek left a comment

Choose a reason for hiding this comment

palinek Sep 14, 2021

Choose a reason for hiding this comment

antis81 Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

palinek Sep 14, 2021

Choose a reason for hiding this comment

antis81 Sep 14, 2021

Choose a reason for hiding this comment

antis81 commented Sep 14, 2021

antis81 commented Sep 16, 2021

antis81 commented Sep 17, 2021 • edited Loading

antis81 commented Sep 20, 2021

tsujan commented Sep 20, 2021

antis81 commented Sep 20, 2021

tsujan commented Sep 20, 2021

antis81 commented Sep 20, 2021

tsujan commented Sep 21, 2021

antis81 commented Sep 13, 2021 •

edited

Loading

antis81 Sep 14, 2021 •

edited

Loading

antis81 commented Sep 17, 2021 •

edited

Loading