From 234e946221c85b717c299ed41df4d78cb2d3a408 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Mon, 13 Sep 2021 14:11:30 +0200 Subject: [PATCH 01/16] move shortcut state machine to dedicated function Still refactoring! Note that the "continue" is no longer needed and replaced by the update function's "return" (aka "early exit"). --- daemon/core.cpp | 163 ++++++++++++++++++++++++------------------------ daemon/core.h | 4 ++ 2 files changed, 87 insertions(+), 80 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index ce2240f..95daeb3 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1016,8 +1016,6 @@ void Core::runEventLoop(Window rootWindow) { XEvent event; bool keyReleaseExpected = false; - const QString superLeft = QString::fromUtf8(XKeysymToString(XK_Super_L)); - const QString superRight = QString::fromUtf8(XKeysymToString(XK_Super_R)); QSet allModifiers; unsigned int allShifts = ShiftMask | ControlMask | AltMask | MetaMask | Level3Mask | Level5Mask; unsigned int ignoreMask = 0xff ^ allShifts; @@ -1233,84 +1231,7 @@ void Core::runEventLoop(Window rootWindow) } else { - if (event.type == KeyRelease) - { - event.xkey.state &= ~allShifts; // Modifier keys must not use shift states. - } - - X11Shortcut shortcutKey = qMakePair(static_cast(event.xkey.keycode), event.xkey.state & allShifts); - ShortcutByX11::const_iterator shortcutIt = mShortcutByX11.constFind(shortcutKey); - if (shortcutIt == mShortcutByX11.constEnd()) - { - continue; - } - const QString& shortcut = shortcutIt.value(); - - if (event.type == KeyPress) - { - if ((shortcut == superLeft) || (shortcut == superRight)) - { - keyReleaseExpected = true; - continue; - } - log(LOG_DEBUG, "KeyPress %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); - } - else - { - log(LOG_DEBUG, "KeyRelease %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); - } - - IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); - if (idsByShortcut != mIdsByShortcut.end()) - { - Ids &ids = idsByShortcut.value(); - switch (mMultipleActionsBehaviour) - { - case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST: - { - Ids::iterator lastIds = ids.end(); - for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) - if (mShortcutAndActionById[*idi].second->call()) - { - break; - } - } - break; - - case MULTIPLE_ACTIONS_BEHAVIOUR_LAST: - { - Ids::iterator firstIds = ids.begin(); - for (Ids::iterator idi = ids.end(); idi != firstIds;) - { - --idi; - if (mShortcutAndActionById[*idi].second->call()) - { - break; - } - } - } - break; - - case MULTIPLE_ACTIONS_BEHAVIOUR_NONE: - if (ids.size() == 1) - { - mShortcutAndActionById[*(ids.begin())].second->call(); - } - break; - - case MULTIPLE_ACTIONS_BEHAVIOUR_ALL: - { - Ids::iterator lastIds = ids.end(); - for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) - { - mShortcutAndActionById[*idi].second->call(); - } - } - break; - - case MULTIPLE_ACTIONS_BEHAVIOUR__COUNT: break; // just a counter - } - } + updateShortcutState(event, keyReleaseExpected, allShifts); } } @@ -1594,6 +1515,88 @@ void Core::runEventLoop(Window rootWindow) } } +void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned int allShifts) +{ + if (event.type == KeyRelease) + { + event.xkey.state &= ~allShifts; // Modifier keys must not use shift states. + } + + X11Shortcut shortcutKey = qMakePair(static_cast(event.xkey.keycode), event.xkey.state & allShifts); + ShortcutByX11::const_iterator shortcutIt = mShortcutByX11.constFind(shortcutKey); + if (shortcutIt == mShortcutByX11.constEnd()) + { + return; + } + const QString& shortcut = shortcutIt.value(); + + if (event.type == KeyPress) + { + if ((shortcut == superLeft) || (shortcut == superRight)) + { + keyReleaseExpected = true; + return; + } + log(LOG_DEBUG, "KeyPress %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); + } + else + { + log(LOG_DEBUG, "KeyRelease %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); + } + + IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); + if (idsByShortcut != mIdsByShortcut.end()) + { + Ids &ids = idsByShortcut.value(); + switch (mMultipleActionsBehaviour) + { + case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST: + { + Ids::iterator lastIds = ids.end(); + for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) + if (mShortcutAndActionById[*idi].second->call()) + { + break; + } + } + break; + + case MULTIPLE_ACTIONS_BEHAVIOUR_LAST: + { + Ids::iterator firstIds = ids.begin(); + for (Ids::iterator idi = ids.end(); idi != firstIds;) + { + --idi; + if (mShortcutAndActionById[*idi].second->call()) + { + break; + } + } + } + break; + + case MULTIPLE_ACTIONS_BEHAVIOUR_NONE: + if (ids.size() == 1) + { + mShortcutAndActionById[*(ids.begin())].second->call(); + } + break; + + case MULTIPLE_ACTIONS_BEHAVIOUR_ALL: + { + Ids::iterator lastIds = ids.end(); + for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) + { + mShortcutAndActionById[*idi].second->call(); + } + } + break; + + case MULTIPLE_ACTIONS_BEHAVIOUR__COUNT: break; // just a counter + } + } +} + void Core::serviceDisappeared(const QString &sender) { log(LOG_DEBUG, "serviceDisappeared '%s'", qPrintable(sender)); diff --git a/daemon/core.h b/daemon/core.h index 9bf9da7..c5d2d2a 100644 --- a/daemon/core.h +++ b/daemon/core.h @@ -171,6 +171,7 @@ class Core : public QThread, public LogTarget void run() override; void runEventLoop(Window rootWindow); + void updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned int allShifts); KeyCode remoteStringToKeycode(const QString &str); QString remoteKeycodeToString(KeyCode keyCode); @@ -434,6 +435,9 @@ class Core : public QThread, public LogTarget const unsigned int Level3Mask = Mod5Mask; // note: mask swapped const unsigned int Level5Mask = Mod3Mask; // note: mask swapped + const QString superLeft = QString::fromUtf8(XKeysymToString(XK_Super_L)); + const QString superRight = QString::fromUtf8(XKeysymToString(XK_Super_R)); + MultipleActionsBehaviour mMultipleActionsBehaviour; bool mAllowGrabLocks; From 993c4f3664f362d4fe6bfa28e93c96720debebbb Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Mon, 13 Sep 2021 19:43:07 +0200 Subject: [PATCH 02/16] move shortcut action creation into on_shortcut() function --- daemon/core.cpp | 76 ++++++++++++++++++++++++++----------------------- daemon/core.h | 1 + 2 files changed, 41 insertions(+), 36 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 95daeb3..205156c 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1528,8 +1528,8 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned { return; } - const QString& shortcut = shortcutIt.value(); + const QString& shortcut = shortcutIt.value(); if (event.type == KeyPress) { if ((shortcut == superLeft) || (shortcut == superRight)) @@ -1547,54 +1547,58 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); if (idsByShortcut != mIdsByShortcut.end()) { - Ids &ids = idsByShortcut.value(); - switch (mMultipleActionsBehaviour) - { - case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST: - { - Ids::iterator lastIds = ids.end(); - for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) - if (mShortcutAndActionById[*idi].second->call()) - { - break; - } - } - break; + on_shortcut(idsByShortcut.value()); + } +} - case MULTIPLE_ACTIONS_BEHAVIOUR_LAST: - { - Ids::iterator firstIds = ids.begin(); - for (Ids::iterator idi = ids.end(); idi != firstIds;) +void Core::on_shortcut(const Ids& ids) const +{ + switch (mMultipleActionsBehaviour) + { + case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST: + { + auto lastIds = ids.end(); + for (auto idi = ids.begin(); idi != lastIds; ++idi) + if (mShortcutAndActionById[*idi].second->call()) { - --idi; - if (mShortcutAndActionById[*idi].second->call()) - { - break; - } + break; } - } - break; + } + break; - case MULTIPLE_ACTIONS_BEHAVIOUR_NONE: - if (ids.size() == 1) + case MULTIPLE_ACTIONS_BEHAVIOUR_LAST: + { + auto firstIds = ids.begin(); + for (auto idi = ids.end(); idi != firstIds;) + { + --idi; + if (mShortcutAndActionById[*idi].second->call()) { - mShortcutAndActionById[*(ids.begin())].second->call(); + break; } - break; + } + } + break; - case MULTIPLE_ACTIONS_BEHAVIOUR_ALL: + case MULTIPLE_ACTIONS_BEHAVIOUR_NONE: + if (ids.size() == 1) { - Ids::iterator lastIds = ids.end(); - for (Ids::iterator idi = ids.begin(); idi != lastIds; ++idi) - { - mShortcutAndActionById[*idi].second->call(); - } + mShortcutAndActionById[*(ids.begin())].second->call(); } break; - case MULTIPLE_ACTIONS_BEHAVIOUR__COUNT: break; // just a counter + case MULTIPLE_ACTIONS_BEHAVIOUR_ALL: + { + auto lastIds = ids.end(); + for (auto idi = ids.begin(); idi != lastIds; ++idi) + { + mShortcutAndActionById[*idi].second->call(); } } + break; + + case MULTIPLE_ACTIONS_BEHAVIOUR__COUNT: break; // just a counter + } } void Core::serviceDisappeared(const QString &sender) diff --git a/daemon/core.h b/daemon/core.h index c5d2d2a..52f753a 100644 --- a/daemon/core.h +++ b/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; void shortcutGrabbed(); void shortcutGrabTimedout(); From 8fc27fcd734458f583cb7a3b47297b96a21c44c9 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Mon, 13 Sep 2021 19:47:59 +0200 Subject: [PATCH 03/16] log unfiltered key states one key press & release events 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. --- daemon/core.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 205156c..894e2fd 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1519,6 +1519,7 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned { if (event.type == KeyRelease) { + log(LOG_DEBUG, "KeyRelease 1 -> %08x %08x", event.xkey.state, event.xkey.keycode); event.xkey.state &= ~allShifts; // Modifier keys must not use shift states. } @@ -1537,11 +1538,11 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned keyReleaseExpected = true; return; } - log(LOG_DEBUG, "KeyPress %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); + log(LOG_DEBUG, "KeyPress %08x %08x %s", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); } else { - log(LOG_DEBUG, "KeyRelease %08x %08x %s", event.xkey.state & allShifts, event.xkey.keycode, qPrintable(shortcut)); + log(LOG_DEBUG, "KeyRelease 2 -> %08x %08x %s", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); } IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); From f317187ffbf5e61520be05ac420d6af0eb664e90 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Mon, 13 Sep 2021 21:04:03 +0200 Subject: [PATCH 04/16] scope brackets --- daemon/core.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/daemon/core.cpp b/daemon/core.cpp index 894e2fd..c69b52f 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1560,10 +1560,12 @@ void Core::on_shortcut(const Ids& ids) const { auto lastIds = ids.end(); for (auto idi = ids.begin(); idi != lastIds; ++idi) + { if (mShortcutAndActionById[*idi].second->call()) { break; } + } } break; From ee2ad88850551d3d25cd149576a8287f1272ac9f Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Tue, 14 Sep 2021 12:25:35 +0200 Subject: [PATCH 05/16] signal/slot naming --- daemon/core.cpp | 4 ++-- daemon/core.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index c69b52f..89a1039 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1548,11 +1548,11 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); if (idsByShortcut != mIdsByShortcut.end()) { - on_shortcut(idsByShortcut.value()); + this->shortcut(idsByShortcut.value()); } } -void Core::on_shortcut(const Ids& ids) const +void Core::shortcut(const Ids& ids) const { switch (mMultipleActionsBehaviour) { diff --git a/daemon/core.h b/daemon/core.h index 52f753a..ccbc9b1 100644 --- a/daemon/core.h +++ b/daemon/core.h @@ -146,7 +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; + void shortcut(const Ids& ids) const; void shortcutGrabbed(); void shortcutGrabTimedout(); From 88b3d187d568f60d55434686fcc39252a9f67fff Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Tue, 14 Sep 2021 14:13:35 +0200 Subject: [PATCH 06/16] move pending event handling into dedicated funcion Refactoring notes: - "signal" parameter is passed by reference (-> instead of char*) - the "return;" line replaces the "while-break" (throw instead?) --- daemon/core.cpp | 504 ++++++++++++++++++++++++------------------------ daemon/core.h | 1 + 2 files changed, 256 insertions(+), 249 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 89a1039..79c7a7e 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1236,281 +1236,287 @@ void Core::runEventLoop(Window rootWindow) } else - // check for pending pipe requests from other thread { - if ((event.type != KeyPress) && (event.type != KeyRelease)) { - XNextEvent(mDisplay, &event); - } + // check for pending pipe requests from other thread + handlePendingEvents(event, rootWindow, signal, allModifiers); + } + } + } +} - pollfd fds[1]; - fds[0].fd = mX11RequestPipe[STDIN_FILENO]; - fds[0].events = POLLIN | POLLERR | POLLHUP; - if (poll(fds, 1, 0) >= 0) - { - if (fds[0].revents & POLLIN) - { - size_t X11Operation; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11Operation, sizeof(X11Operation))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } +/** check for pending pipe requests from other thread */ +void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, const QSet& allModifiers) +{ + if ((event.type != KeyPress) && (event.type != KeyRelease)) { + XNextEvent(mDisplay, &event); + } + + pollfd fds[1]; + fds[0].fd = mX11RequestPipe[STDIN_FILENO]; + fds[0].events = POLLIN | POLLERR | POLLHUP; + if (poll(fds, 1, 0) >= 0) + { + if (fds[0].revents & POLLIN) + { + size_t X11Operation; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11Operation, sizeof(X11Operation))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + return; + } // log(LOG_DEBUG, "X11Operation: %d", X11Operation); - switch (X11Operation) - { - case X11_OP_StringToKeycode: - { - bool x11Error = false; - KeyCode keyCode = 0; - size_t length; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &length, sizeof(length))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - if (length) - { - QScopedArrayPointer str(new char[length + 1]); - str[length] = '\0'; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], str.data(), length)) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - KeySym keySym = XStringToKeysym(str.data()); - lockX11Error(); - keyCode = XKeysymToKeycode(mDisplay, keySym); - x11Error = checkX11Error(); - } + switch (X11Operation) + { + case X11_OP_StringToKeycode: + { + bool x11Error = false; + KeyCode keyCode = 0; + size_t length; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &length, sizeof(length))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + if (length) + { + QScopedArrayPointer str(new char[length + 1]); + str[length] = '\0'; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], str.data(), length)) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + KeySym keySym = XStringToKeysym(str.data()); + lockX11Error(); + keyCode = XKeysymToKeycode(mDisplay, keySym); + x11Error = checkX11Error(); + } - signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } + signal = x11Error ? 1 : 0; + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } - if (!x11Error) - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &keyCode, sizeof(keyCode))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - } + if (!x11Error) + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &keyCode, sizeof(keyCode))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; break; + } + } + break; - case X11_OP_KeycodeToString: - { - KeyCode keyCode; - bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &keyCode, sizeof(keyCode))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - int keysymsPerKeycode; - lockX11Error(); - KeySym *keySyms = XGetKeyboardMapping(mDisplay, keyCode, 1, &keysymsPerKeycode); - x11Error = checkX11Error(); - char *str = nullptr; + case X11_OP_KeycodeToString: + { + KeyCode keyCode; + bool x11Error = false; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &keyCode, sizeof(keyCode))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + int keysymsPerKeycode; + lockX11Error(); + KeySym *keySyms = XGetKeyboardMapping(mDisplay, keyCode, 1, &keysymsPerKeycode); + x11Error = checkX11Error(); + char *str = nullptr; - if (!x11Error) - { - KeySym keySym = 0; - if ((keysymsPerKeycode >= 2) && keySyms[1] && (keySyms[0] >= XK_a) && (keySyms[0] <= XK_z)) - { - keySym = keySyms[1]; - } - else if (keysymsPerKeycode >= 1) - { - keySym = keySyms[0]; - } + if (!x11Error) + { + KeySym keySym = 0; + if ((keysymsPerKeycode >= 2) && keySyms[1] && (keySyms[0] >= XK_a) && (keySyms[0] <= XK_z)) + { + keySym = keySyms[1]; + } + else if (keysymsPerKeycode >= 1) + { + keySym = keySyms[0]; + } - if (keySym) - { - str = XKeysymToString(keySym); - } - } + if (keySym) + { + str = XKeysymToString(keySym); + } + } - signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } + signal = x11Error ? 1 : 0; + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } - if (!x11Error) - { - size_t length = 0; - if (str) - { - length = strlen(str); - } - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &length, sizeof(length))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - if (length) - { - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], str, length)) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - } - } - } + if (!x11Error) + { + size_t length = 0; + if (str) + { + length = strlen(str); + } + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &length, sizeof(length))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; break; - - case X11_OP_XGrabKey: + } + if (length) + { + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], str, length)) { - X11Shortcut X11shortcut; - bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - - QSet::const_iterator lastAllModifiers = allModifiers.cend(); - for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) - { - lockX11Error(); - XGrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow, False, GrabModeAsync, GrabModeAsync); - bool x11e = checkX11Error(); - if (x11e) - { - log(LOG_DEBUG, "XGrabKey: %02x + %02x", X11shortcut.first, X11shortcut.second | *modifiers); - } - x11Error |= x11e; - } - - signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; } - break; + } + } + } + break; - case X11_OP_XUngrabKey: - { - X11Shortcut X11shortcut; - bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) - { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } + case X11_OP_XGrabKey: + { + X11Shortcut X11shortcut; + bool x11Error = false; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } - lockX11Error(); - QSet::const_iterator lastAllModifiers = allModifiers.cend(); - for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) - { - XUngrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow); - } - x11Error = checkX11Error(); + QSet::const_iterator lastAllModifiers = allModifiers.cend(); + for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) + { + lockX11Error(); + XGrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow, False, GrabModeAsync, GrabModeAsync); + bool x11e = checkX11Error(); + if (x11e) + { + log(LOG_DEBUG, "XGrabKey: %02x + %02x", X11shortcut.first, X11shortcut.second | *modifiers); + } + x11Error |= x11e; + } - signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - } - break; + signal = x11Error ? 1 : 0; + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + } + break; - case X11_OP_XGrabKeyboard: - { - lockX11Error(); - int result = XGrabKeyboard(mDisplay, rootWindow, False, GrabModeAsync, GrabModeAsync, CurrentTime); - bool x11Error = checkX11Error(); - if (!result && x11Error) - { - result = -1; - } + case X11_OP_XUngrabKey: + { + X11Shortcut X11shortcut; + bool x11Error = false; + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) + { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &result, sizeof(result))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } - mDataMutex.lock(); - mGrabbingShortcut = true; - mDataMutex.unlock(); - } - break; + lockX11Error(); + QSet::const_iterator lastAllModifiers = allModifiers.cend(); + for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) + { + XUngrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow); + } + x11Error = checkX11Error(); - case X11_OP_XUngrabKeyboard: - { - lockX11Error(); - XUngrabKeyboard(mDisplay, CurrentTime); - bool x11Error = checkX11Error(); + signal = x11Error ? 1 : 0; + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + } + break; - signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); - mX11EventLoopActive = false; - break; - } + case X11_OP_XGrabKeyboard: + { + lockX11Error(); + int result = XGrabKeyboard(mDisplay, rootWindow, False, GrabModeAsync, GrabModeAsync, CurrentTime); + bool x11Error = checkX11Error(); + if (!result && x11Error) + { + result = -1; + } - mDataMutex.lock(); - mGrabbingShortcut = false; - mDataMutex.unlock(); - } - break; - } // end of switch-case + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &result, sizeof(result))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; + } + mDataMutex.lock(); + mGrabbingShortcut = true; + mDataMutex.unlock(); + } + break; - } + case X11_OP_XUngrabKeyboard: + { + lockX11Error(); + XUngrabKeyboard(mDisplay, CurrentTime); + bool x11Error = checkX11Error(); + + signal = x11Error ? 1 : 0; + if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + mX11EventLoopActive = false; + break; } + + mDataMutex.lock(); + mGrabbingShortcut = false; + mDataMutex.unlock(); } + break; + } // end of switch-case + } } } diff --git a/daemon/core.h b/daemon/core.h index ccbc9b1..c5da90a 100644 --- a/daemon/core.h +++ b/daemon/core.h @@ -172,6 +172,7 @@ class Core : public QThread, public LogTarget void run() override; void runEventLoop(Window rootWindow); + void handlePendingEvents(XEvent& event, Window rootWindow, char& signal, const QSet& allModifiers); void updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned int allShifts); KeyCode remoteStringToKeycode(const QString &str); From 51518b055e5687300fe91bf8743355e62bcbf56e Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Tue, 14 Sep 2021 19:30:49 +0200 Subject: [PATCH 07/16] add detailed logging for debugging --- daemon/core.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 79c7a7e..2e7a66c 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1036,7 +1036,9 @@ void Core::runEventLoop(Window rootWindow) if ((event.type == KeyRelease) && !keyReleaseExpected) { // pop event from the x11 queue and do nothing + log(LOG_DEBUG, "Ignored KeyRelease 1 -> %08x %08x", event.xkey.state, event.xkey.keycode); XNextEvent(mDisplay, &event); + log(LOG_DEBUG, "Ignored KeyRelease 2 -> %08x %08x", event.xkey.state, event.xkey.keycode); continue; } keyReleaseExpected = false; // Close time window for accepting meta keys. @@ -1047,6 +1049,7 @@ void Core::runEventLoop(Window rootWindow) // pop event from the x11 queue and process it XNextEvent(mDisplay, &event); + log(LOG_DEBUG, "Handling %s -> %08x %08x", event.type == KeyPress ? "KeyPress" : "KeyRelease", event.xkey.state, event.xkey.keycode); if (mGrabbingShortcut) { @@ -1527,6 +1530,7 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned { log(LOG_DEBUG, "KeyRelease 1 -> %08x %08x", event.xkey.state, event.xkey.keycode); event.xkey.state &= ~allShifts; // Modifier keys must not use shift states. + log(LOG_DEBUG, "KeyRelease 2 -> %08x %08x", event.xkey.state, event.xkey.keycode); } X11Shortcut shortcutKey = qMakePair(static_cast(event.xkey.keycode), event.xkey.state & allShifts); @@ -1539,16 +1543,16 @@ void Core::updateShortcutState(XEvent& event, bool& keyReleaseExpected, unsigned const QString& shortcut = shortcutIt.value(); if (event.type == KeyPress) { + log(LOG_DEBUG, "KeyPress %08x %08x %s ", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); if ((shortcut == superLeft) || (shortcut == superRight)) { keyReleaseExpected = true; return; } - log(LOG_DEBUG, "KeyPress %08x %08x %s", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); } else { - log(LOG_DEBUG, "KeyRelease 2 -> %08x %08x %s", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); + log(LOG_DEBUG, "KeyRelease 3 -> %08x %08x %s", event.xkey.state, event.xkey.keycode, qPrintable(shortcut)); } IdsByShortcut::iterator idsByShortcut = mIdsByShortcut.find(shortcut); From bead9126d1bf7364e4834971d47892fdda2082a8 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Tue, 14 Sep 2021 19:31:47 +0200 Subject: [PATCH 08/16] change comment to what it actually does --- daemon/core.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 2e7a66c..acef6c0 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1041,7 +1041,7 @@ void Core::runEventLoop(Window rootWindow) log(LOG_DEBUG, "Ignored KeyRelease 2 -> %08x %08x", event.xkey.state, event.xkey.keycode); continue; } - keyReleaseExpected = false; // Close time window for accepting meta keys. + keyReleaseExpected = false; // initialize for next event if (((event.type == KeyPress) || (event.type == KeyRelease)) && mDataMutex.tryLock(0)) { From 659629e7f2abb2e86d7b53e0b067b0e4343e26f2 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Tue, 14 Sep 2021 20:43:21 +0200 Subject: [PATCH 09/16] add debugging log option --- daemon/core.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/daemon/core.cpp b/daemon/core.cpp index acef6c0..98ba882 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -401,6 +401,12 @@ Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringLi , mShortcutGrabTimeout(new QTimer(this)) , mShortcutGrabRequested(false) { +#if 0 + // debugging + mUseSyslog = false; + mMinLogLevel = 7; +#endif + s_Core = this; initBothPipeEnds(mX11ErrorPipe); From 7824cd97ff5f14a7dd902611dd8980816d1cc94b Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Wed, 15 Sep 2021 00:48:42 +0200 Subject: [PATCH 10/16] replace another switch-case default --- daemon/core.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 98ba882..ecfce3e 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -700,8 +700,7 @@ Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringLi log(LOG_DEBUG, "MultipleActionsBehaviour: none"); break; - default: - ; + case MULTIPLE_ACTIONS_BEHAVIOUR__COUNT: break; // just a counter } log(LOG_DEBUG, "AllowGrabLocks: %s", mAllowGrabLocks ? "true" : "false"); log(LOG_DEBUG, "AllowGrabBaseSpecial: %s", mAllowGrabBaseSpecial ? "true" : "false"); From 8d403bab334b47091fe27413fe8bb11e6fc2281b Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Wed, 15 Sep 2021 00:51:30 +0200 Subject: [PATCH 11/16] some minor readability improvements --- daemon/core.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index ecfce3e..36a7414 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -2031,7 +2031,6 @@ QString Core::checkShortcut(const QString &shortcut, X11Shortcut &X11shortcut) QPair Core::addOrRegisterClientAction(const QString &shortcut, const QDBusObjectPath &path, const QString &description, const QString &sender) { X11Shortcut X11shortcut; - QString newShortcut = checkShortcut(shortcut, X11shortcut); // if (newShortcut.isEmpty()) // { @@ -2054,13 +2053,13 @@ QPair Core::addOrRegisterClientAction(const QString &shortc mIdsByShortcut[newShortcut].insert(id); } - dynamic_cast(shortcutAndAction.second)->appeared(QDBusConnection::sessionBus(), sender); + auto action = static_cast(shortcutAndAction.second); + action->appeared(QDBusConnection::sessionBus(), sender); return qMakePair(newShortcut, id); } qulonglong id = ++mLastId; - if (!sender.isEmpty() && !newShortcut.isEmpty()) { newShortcut = grabOrReuseKey(X11shortcut, newShortcut); @@ -2068,11 +2067,10 @@ QPair Core::addOrRegisterClientAction(const QString &shortc } mIdByClientPath[path] = id; - ClientAction *clientAction = sender.isEmpty() ? new ClientAction(this, path, description) : new ClientAction(this, QDBusConnection::sessionBus(), sender, path, description); + auto clientAction = sender.isEmpty() ? new ClientAction(this, path, description) : new ClientAction(this, QDBusConnection::sessionBus(), sender, path, description); mShortcutAndActionById[id] = qMakePair(newShortcut, clientAction); log(LOG_INFO, "addClientAction shortcut:'%s' id:%llu", qPrintable(newShortcut), id); - return qMakePair(newShortcut, id); } @@ -2125,7 +2123,8 @@ qulonglong Core::registerClientAction(const QString &shortcut, const QDBusObject QMutexLocker lock(&mDataMutex); - return addOrRegisterClientAction(shortcut, path, description, QString()).second; + auto action = addOrRegisterClientAction(shortcut, path, description, QString()); + return action.second; } void Core::addMethodAction(QPair &result, const QString &shortcut, const QString &service, const QDBusObjectPath &path, const QString &interface, const QString &method, const QString &description) From e6a98f6c91431b9635d6d0835787c3d21df55d2b Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Fri, 17 Sep 2021 20:32:10 +0200 Subject: [PATCH 12/16] fix vtable warnings for LogTarget Actually the pointer is unused in XyzAction classes. --- daemon/core.cpp | 111 ++++++++---------------------------------- daemon/core.h | 14 ++++-- daemon/log_target.cpp | 26 ++++++++-- daemon/log_target.h | 60 +++++++++++++++++++---- 4 files changed, 101 insertions(+), 110 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 36a7414..9460baf 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -343,46 +343,9 @@ const char *x11opcodeToString(unsigned char opcode) return ""; } -static const char *strLevel(int level) -{ - switch (level) - { - case LOG_EMERG: - return "Emergency"; - - case LOG_ALERT: - return "Alert"; - - case LOG_CRIT: - return "Critical"; - - case LOG_ERR: - return "Error"; - - case LOG_WARNING: - return "Warning"; - - case LOG_NOTICE: - return "Notice"; - - case LOG_INFO: - return "Info"; - - case LOG_DEBUG: - return "Debug"; - - default: - return ""; - } -} - - Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringList &configFiles, bool multipleActionsBehaviourSet, MultipleActionsBehaviour multipleActionsBehaviour, QObject *parent) : QThread(parent) - , LogTarget() , mReady(false) - , mUseSyslog(useSyslog) - , mMinLogLevel(minLogLevel) , mDisplay(nullptr) , mInterClientCommunicationWindow(0) , mServiceWatcher{new QDBusServiceWatcher{this}} @@ -403,8 +366,11 @@ Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringLi { #if 0 // debugging - mUseSyslog = false; - mMinLogLevel = 7; + Q_UNUSED(minLogLevel); + Q_UNUSED(useSyslog); + mCoreLogger = std::make_unique(LOG_DEBUG, false); +#else + mCoreLogger = std::make_unique(minLogLevel, useSyslog); #endif s_Core = this; @@ -487,33 +453,16 @@ Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringLi if (!minLogLevelSet) { iniValue = settings.value(/* General/ */QStringLiteral("LogLevel")).toString(); - if (!iniValue.isEmpty()) + auto lvl = LogTarget::levelFromStr(qPrintable(iniValue)); + if (lvl >= 0) { + mCoreLogger->mMinLogLevel = lvl; minLogLevelSet = true; - if (iniValue == QLatin1String("error")) - { - mMinLogLevel = LOG_ERR; - } - else if (iniValue == QLatin1String("warning")) - { - mMinLogLevel = LOG_WARNING; - } - else if (iniValue == QLatin1String("notice")) - { - mMinLogLevel = LOG_NOTICE; - } - else if (iniValue == QLatin1String("info")) - { - mMinLogLevel = LOG_INFO; - } - else if (iniValue == QLatin1String("debug")) - { - mMinLogLevel = LOG_DEBUG; - } - else - { - minLogLevelSet = false; - } + } + else + { + auto lvlStr = LogTarget::strLevel(mCoreLogger->mMinLogLevel); + qInfo("no loglevel configured in %s (result = %d); current loglevel: %s", qUtf8Printable(settings.fileName()), lvl, lvlStr); } } @@ -681,7 +630,7 @@ Core::Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringLi log(LOG_DEBUG, "Config file: %s", qPrintable(configs[0])); } - log(LOG_DEBUG, "MinLogLevel: %s", strLevel(mMinLogLevel)); + log(LOG_DEBUG, "MinLogLevel: %s", LogTarget::strLevel(mCoreLogger->mMinLogLevel)); switch (mMultipleActionsBehaviour) { case MULTIPLE_ACTIONS_BEHAVIOUR_FIRST: @@ -882,28 +831,6 @@ void Core::unixSignalHandler(int signalNumber) qApp->quit(); } -void Core::log(int level, const char *format, ...) const -{ - if (level > mMinLogLevel) - { - return; - } - - va_list ap; - va_start(ap, format); - if (mUseSyslog) - { - vsyslog(LOG_MAKEPRI(LOG_USER, level), format, ap); - } - else - { - fprintf(stderr, "[%s] ", strLevel(level)); - vfprintf(stderr, format, ap); - fprintf(stderr, "\n"); - } - va_end(ap); -} - int Core::x11ErrorHandler(Display */*display*/, XErrorEvent *errorEvent) { if (error_t error = writeAll(mX11ErrorPipe[STDOUT_FILENO], errorEvent, sizeof(XErrorEvent))) @@ -2067,7 +1994,7 @@ QPair Core::addOrRegisterClientAction(const QString &shortc } mIdByClientPath[path] = id; - auto clientAction = sender.isEmpty() ? new ClientAction(this, path, description) : new ClientAction(this, QDBusConnection::sessionBus(), sender, path, description); + auto clientAction = sender.isEmpty() ? new ClientAction(mCoreLogger.get(), path, description) : new ClientAction(mCoreLogger.get(), QDBusConnection::sessionBus(), sender, path, description); mShortcutAndActionById[id] = qMakePair(newShortcut, clientAction); log(LOG_INFO, "addClientAction shortcut:'%s' id:%llu", qPrintable(newShortcut), id); @@ -2152,7 +2079,7 @@ void Core::addMethodAction(QPair &result, const QString &sh qulonglong id = ++mLastId; mIdsByShortcut[newShortcut].insert(id); - mShortcutAndActionById[id] = qMakePair(newShortcut, new MethodAction(this, QDBusConnection::sessionBus(), service, path, interface, method, description)); + mShortcutAndActionById[id] = qMakePair(newShortcut, new MethodAction(mCoreLogger.get(), QDBusConnection::sessionBus(), service, path, interface, method, description)); log(LOG_INFO, "addMethodAction shortcut:'%s' id:%llu", qPrintable(newShortcut), id); @@ -2193,7 +2120,7 @@ void Core::addCommandAction(QPair &result, const QString &s qulonglong id = ++mLastId; mIdsByShortcut[newShortcut].insert(id); - mShortcutAndActionById[id] = qMakePair(newShortcut, new CommandAction(this, command, arguments, description)); + mShortcutAndActionById[id] = qMakePair(newShortcut, new CommandAction(mCoreLogger.get(), command, arguments, description)); log(LOG_INFO, "addCommandAction shortcut:'%s' id:%llu", qPrintable(newShortcut), id); @@ -2304,7 +2231,7 @@ void Core::modifyMethodAction(bool &result, const qulonglong &id, const QString bool isEnabled = action->isEnabled(); delete action; - MethodAction *newAction = new MethodAction(this, QDBusConnection::sessionBus(), service, path, interface, method, description); + auto newAction = new MethodAction(mCoreLogger.get(), QDBusConnection::sessionBus(), service, path, interface, method, description); newAction->setEnabled(isEnabled); shortcutAndActionById.value().second = newAction; @@ -2338,7 +2265,7 @@ void Core::modifyCommandAction(bool &result, const qulonglong &id, const QString bool isEnabled = action->isEnabled(); delete action; - CommandAction *newAction = new CommandAction(this, command, arguments, description); + auto newAction = new CommandAction(mCoreLogger.get(), command, arguments, description); newAction->setEnabled(isEnabled); shortcutAndActionById.value().second = newAction; diff --git a/daemon/core.h b/daemon/core.h index c5da90a..6108aee 100644 --- a/daemon/core.h +++ b/daemon/core.h @@ -68,16 +68,21 @@ class QOrderedSet : public QMap } }; -class Core : public QThread, public LogTarget +class Core : public QThread { Q_OBJECT + public: Core(bool useSyslog, bool minLogLevelSet, int minLogLevel, const QStringList &configFiles, bool multipleActionsBehaviourSet, MultipleActionsBehaviour multipleActionsBehaviour, QObject *parent = nullptr); ~Core() override; bool ready() const { return mReady; } - void log(int level, const char *format, ...) const override; + template + void log(int level, const char *format, Args&&... args) const { + // NOTE: If the logger is unassigned the SEGFAULT is intentional! + mCoreLogger->log(level, format, std::forward(args)...); + } signals: void onShortcutGrabbed(); @@ -399,10 +404,9 @@ class Core : public QThread, public LogTarget bool waitForX11Error(int level, uint timeout); private: - bool mReady; - bool mUseSyslog; - int mMinLogLevel; + std::unique_ptr mCoreLogger; + bool mReady; int mX11ErrorPipe[2]; int mX11RequestPipe[2]; int mX11ResponsePipe[2]; diff --git a/daemon/log_target.cpp b/daemon/log_target.cpp index 656b110..71051a6 100644 --- a/daemon/log_target.cpp +++ b/daemon/log_target.cpp @@ -26,12 +26,32 @@ * END_COMMON_COPYRIGHT_HEADER */ #include "log_target.h" +#include - -LogTarget::LogTarget() +LogTarget::LogTarget(int minLogLevel, bool useSyslog) { + mMinLogLevel = minLogLevel; + mUseSyslog = useSyslog; } -LogTarget::~LogTarget() +void LogTarget::log(int level, const char *format, ...) { + if (level > mMinLogLevel) + { + return; + } + + va_list ap; + va_start(ap, format); + if (mUseSyslog) + { + vsyslog(LOG_MAKEPRI(LOG_USER, level), format, ap); + } + else + { + fprintf(stderr, "[%s] ", strLevel(level)); + vfprintf(stderr, format, ap); + fprintf(stderr, "\n"); + } + va_end(ap); } diff --git a/daemon/log_target.h b/daemon/log_target.h index 6ca4590..f292d30 100644 --- a/daemon/log_target.h +++ b/daemon/log_target.h @@ -25,20 +25,60 @@ * * END_COMMON_COPYRIGHT_HEADER */ -#ifndef GLOBAL_ACTION_DAEMON__LOG_TARGET__INCLUDED -#define GLOBAL_ACTION_DAEMON__LOG_TARGET__INCLUDED - +#pragma once #include +#include +class LogTarget { +public: + int mMinLogLevel = LOG_INFO; + bool mUseSyslog = false; -class LogTarget -{ public: - LogTarget(); - virtual ~LogTarget(); + LogTarget(int minLogLevel = LOG_INFO, bool useSyslog = false); - virtual void log(int level, const char *format, ...) const = 0; -}; + void log(int level, const char *format, ...); + + static constexpr auto strLevel(int level) + { + switch (level) { + case LOG_EMERG: return "Emergency"; + case LOG_ALERT: return "Alert"; + case LOG_CRIT: return "Critical"; + case LOG_ERR: return "Error"; + case LOG_WARNING: return "Warning"; + case LOG_NOTICE: return "Notice"; + case LOG_INFO: return "Info"; + case LOG_DEBUG: return "Debug"; + } -#endif // GLOBAL_ACTION_DAEMON__LOG_TARGET__INCLUDED + return ""; // fallthrough + } + + static constexpr auto levelFromStr(const char* str) { + if (str == nullptr) { + // error: null string + return -2; + } + + if (std::strcmp(str, "error") == 0) { + return LOG_ERR; + } + if (std::strcmp(str, "warning") == 0) { + return LOG_WARNING; + } + if (std::strcmp(str, "notice") == 0) { + return LOG_NOTICE; + } + if (std::strcmp(str, "info") == 0) { + return LOG_INFO; + } + if (std::strcmp(str, "debug") == 0) { + return LOG_DEBUG; + } + + // error: unknown log level string + return -1; + } +}; From 9a7e25bd64a2a3df8da1fda3803d5d5b2626b92e Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Sat, 2 Oct 2021 22:53:37 +0200 Subject: [PATCH 13/16] use static_cast for BaseAction type conversion --- daemon/core.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 9460baf..70b5ac8 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -804,12 +804,12 @@ void Core::saveConfig() if (!strcmp(action->type(), CommandAction::id())) { - const CommandAction *commandAction = dynamic_cast(action); + auto commandAction = static_cast(action); settings.setValue(ExecKey, QVariant(QStringList() << commandAction->command() += commandAction->args())); } else if (!strcmp(action->type(), MethodAction::id())) { - const MethodAction *methodAction = dynamic_cast(action); + auto methodAction = static_cast(action); settings.setValue(serviceKey, methodAction->service()); settings.setValue(pathKey, methodAction->path().path()); settings.setValue(interfaceKey, methodAction->interface()); @@ -817,7 +817,7 @@ void Core::saveConfig() } else if (!strcmp(action->type(), ClientAction::id())) { - const ClientAction *clientAction = dynamic_cast(action); + auto clientAction = static_cast(action); settings.setValue(pathKey, clientAction->path().path()); } From a7d5036cdbd16b2af5687687ec47fbd1b902b49f Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Sat, 2 Oct 2021 23:08:12 +0200 Subject: [PATCH 14/16] simplify modifier for-loops --- daemon/core.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 70b5ac8..93ca0e8 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1345,15 +1345,14 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c break; } - QSet::const_iterator lastAllModifiers = allModifiers.cend(); - for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) + for (auto modifier : qAsConst(allModifiers)) { lockX11Error(); - XGrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow, False, GrabModeAsync, GrabModeAsync); + XGrabKey(mDisplay, X11shortcut.first, X11shortcut.second | modifier, rootWindow, False, GrabModeAsync, GrabModeAsync); bool x11e = checkX11Error(); if (x11e) { - log(LOG_DEBUG, "XGrabKey: %02x + %02x", X11shortcut.first, X11shortcut.second | *modifiers); + log(LOG_DEBUG, "XGrabKey: %02x + %02x", X11shortcut.first, X11shortcut.second | modifier); } x11Error |= x11e; } @@ -1389,10 +1388,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c } lockX11Error(); - QSet::const_iterator lastAllModifiers = allModifiers.cend(); - for (QSet::const_iterator modifiers = allModifiers.cbegin(); modifiers != lastAllModifiers; ++modifiers) + for (auto modifier : qAsConst(allModifiers)) { - XUngrabKey(mDisplay, X11shortcut.first, X11shortcut.second | *modifiers, rootWindow); + XUngrabKey(mDisplay, X11shortcut.first, X11shortcut.second | modifier, rootWindow); } x11Error = checkX11Error(); From ab589c8e4ad5331d66b7f6ccd9e602dee9ee3991 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Sat, 2 Oct 2021 23:14:43 +0200 Subject: [PATCH 15/16] add template methods for reading/writing X11 pipe requests/responses --- daemon/core.cpp | 2 -- daemon/core.h | 22 ++++++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index 93ca0e8..d9cb16f 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -37,7 +37,6 @@ #include #include -#include #include #include #include @@ -48,7 +47,6 @@ #include -#include "pipe_utils.h" #include "string_utils.h" #include "daemon_adaptor.h" #include "native_adaptor.h" diff --git a/daemon/core.h b/daemon/core.h index 6108aee..de0761b 100644 --- a/daemon/core.h +++ b/daemon/core.h @@ -40,6 +40,7 @@ #include "meta_types.h" #include "log_target.h" +#include "pipe_utils.h" extern "C" { #include @@ -49,6 +50,7 @@ extern "C" { #undef Bool } +#include class QTimer; class DaemonAdaptor; @@ -185,6 +187,26 @@ class Core : public QThread bool remoteXGrabKey(const X11Shortcut &X11shortcut); bool remoteXUngrabKey(const X11Shortcut &X11shortcut); + template + error_t readX11PipeRequest(T* ptr, size_t len) { + error_t error = readAll(mX11RequestPipe[STDIN_FILENO], ptr, len); + if (error) { + log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); + close(mX11ResponsePipe[STDIN_FILENO]); + } + return error; + } + + template + error_t writeX11PipeResponse(T* ptr, size_t len) { + error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], ptr, len); + if (error) { + log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); + close(mX11RequestPipe[STDIN_FILENO]); + } + return error; + } + QString grabOrReuseKey(const X11Shortcut &X11shortcut, const QString &shortcut); QString checkShortcut(const QString &shortcut, X11Shortcut &X11shortcut); From ec6523aaf21353e7e9c2e947007b48d3c98ffd27 Mon Sep 17 00:00:00 2001 From: Nils Fenner Date: Sat, 2 Oct 2021 23:23:11 +0200 Subject: [PATCH 16/16] utilize read/write X11 pipe methods in handlePendingEvents Makes it easier to follow the code flow. --- daemon/core.cpp | 88 ++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 53 deletions(-) diff --git a/daemon/core.cpp b/daemon/core.cpp index d9cb16f..8aa0edd 100644 --- a/daemon/core.cpp +++ b/daemon/core.cpp @@ -1192,10 +1192,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c if (fds[0].revents & POLLIN) { size_t X11Operation; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11Operation, sizeof(X11Operation))) + if (readX11PipeRequest(&X11Operation, sizeof(X11Operation))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; return; } @@ -1208,10 +1207,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c bool x11Error = false; KeyCode keyCode = 0; size_t length; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &length, sizeof(length))) + if (readX11PipeRequest(&length, sizeof(length))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1219,10 +1217,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c { QScopedArrayPointer str(new char[length + 1]); str[length] = '\0'; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], str.data(), length)) + if (readX11PipeRequest(str.data(), length)) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1233,19 +1230,17 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c } signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + if (writeX11PipeResponse(&signal, sizeof(signal))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } if (!x11Error) - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &keyCode, sizeof(keyCode))) + if (writeX11PipeResponse(&keyCode, sizeof(keyCode))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1256,10 +1251,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c { KeyCode keyCode; bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &keyCode, sizeof(keyCode))) + if (readX11PipeRequest(&keyCode, sizeof(keyCode))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1288,10 +1282,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c } signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + if (writeX11PipeResponse(&signal, sizeof(signal))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1303,19 +1296,17 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c { length = strlen(str); } - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &length, sizeof(length))) + if (writeX11PipeResponse(&length, sizeof(length))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } if (length) { - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], str, length)) + if (writeX11PipeResponse(str, length)) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1328,17 +1319,15 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c { X11Shortcut X11shortcut; bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) + if (readX11PipeRequest(&X11shortcut.first, sizeof(X11shortcut.first))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) + if (readX11PipeRequest(&X11shortcut.second, sizeof(X11shortcut.second))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1356,10 +1345,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c } signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + if (writeX11PipeResponse(&signal, sizeof(signal))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1370,17 +1358,15 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c { X11Shortcut X11shortcut; bool x11Error = false; - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.first, sizeof(X11shortcut.first))) + if (readX11PipeRequest(&X11shortcut.first, sizeof(X11shortcut.first))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } - if (error_t error = readAll(mX11RequestPipe[STDIN_FILENO], &X11shortcut.second, sizeof(X11shortcut.second))) + if (readX11PipeRequest(&X11shortcut.second, sizeof(X11shortcut.second))) { - log(LOG_CRIT, "Cannot read from X11 request pipe: %s", strerror(error)); - close(mX11ResponsePipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1393,10 +1379,9 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c x11Error = checkX11Error(); signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) + if (writeX11PipeResponse(&signal, sizeof(signal))) { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + // pipe error mX11EventLoopActive = false; break; } @@ -1413,13 +1398,12 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c result = -1; } - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &result, sizeof(result))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + if (writeX11PipeResponse(&result, sizeof(result))) { + // pipe error mX11EventLoopActive = false; break; } + mDataMutex.lock(); mGrabbingShortcut = true; mDataMutex.unlock(); @@ -1433,10 +1417,8 @@ void Core::handlePendingEvents(XEvent& event, Window rootWindow, char& signal, c bool x11Error = checkX11Error(); signal = x11Error ? 1 : 0; - if (error_t error = writeAll(mX11ResponsePipe[STDOUT_FILENO], &signal, sizeof(signal))) - { - log(LOG_CRIT, "Cannot write to X11 response pipe: %s", strerror(error)); - close(mX11RequestPipe[STDIN_FILENO]); + if (writeX11PipeResponse(&signal, sizeof(signal))) { + // pipe error mX11EventLoopActive = false; break; }