From a34e3ccc86e180df182f4aa48a9b2af599cfb27e Mon Sep 17 00:00:00 2001 From: yangyingchao Date: Fri, 5 Jan 2024 13:49:11 +0800 Subject: [PATCH 1/2] Improvements for Hyprland workspace 1. Utilize `m_mutex` to safeguard member fields of `hyprland::Workspaces` as they are modified by multiple threads, including the event listener thread and UI thread. This applies to all member fields, not just `m_workspacesToCreate`. 2. Tidy up the create/remove workspace code. --- include/modules/hyprland/workspaces.hpp | 2 ++ src/modules/hyprland/workspaces.cpp | 40 +++++++++++-------------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/include/modules/hyprland/workspaces.hpp b/include/modules/hyprland/workspaces.hpp index a17c2db4..0109149e 100644 --- a/include/modules/hyprland/workspaces.hpp +++ b/include/modules/hyprland/workspaces.hpp @@ -161,6 +161,8 @@ class Workspaces : public AModule, public EventHandler { int windowRewritePriorityFunction(std::string const& window_rule); + void doUpdate(); + bool m_allOutputs = false; bool m_showSpecial = false; bool m_activeOnly = false; diff --git a/src/modules/hyprland/workspaces.cpp b/src/modules/hyprland/workspaces.cpp index 74a09f80..3d8a5932 100644 --- a/src/modules/hyprland/workspaces.cpp +++ b/src/modules/hyprland/workspaces.cpp @@ -4,11 +4,8 @@ #include #include -#include #include -#include #include -#include #include #include @@ -16,8 +13,6 @@ namespace waybar::modules::hyprland { -std::shared_mutex workspaceCreateSmtx; - int Workspaces::windowRewritePriorityFunction(std::string const &window_rule) { // Rules that match against title are prioritized // Rules that don't specify if they're matching against either title or class are deprioritized @@ -153,27 +148,26 @@ auto Workspaces::registerIpc() -> void { } } -auto Workspaces::update() -> void { +/** + * Workspaces::doUpdate - update workspaces in UI thread. + * + * Note: some memberfields are modified by both UI thread and event listener thread, use m_mutex to + * protect these member fields, and lock should released before calling AModule::update(). + */ +void Workspaces::doUpdate() { + std::unique_lock lock(m_mutex); + // remove workspaces that wait to be removed - unsigned int currentRemoveWorkspaceNum = 0; - for (const std::string &workspaceToRemove : m_workspacesToRemove) { - removeWorkspace(workspaceToRemove); - currentRemoveWorkspaceNum++; - } - for (unsigned int i = 0; i < currentRemoveWorkspaceNum; i++) { - m_workspacesToRemove.erase(m_workspacesToRemove.begin()); + for (auto &elem : m_workspacesToRemove) { + removeWorkspace(elem); } + m_workspacesToRemove.clear(); // add workspaces that wait to be created - std::shared_lock workspaceCreateShareLock(workspaceCreateSmtx); - unsigned int currentCreateWorkspaceNum = 0; - for (Json::Value const &workspaceToCreate : m_workspacesToCreate) { - createWorkspace(workspaceToCreate); - currentCreateWorkspaceNum++; - } - for (unsigned int i = 0; i < currentCreateWorkspaceNum; i++) { - m_workspacesToCreate.erase(m_workspacesToCreate.begin()); + for (auto &elem : m_workspacesToCreate) { + createWorkspace(elem); } + m_workspacesToCreate.clear(); // get all active workspaces auto monitors = gIPC->getSocket1JsonReply("monitors"); @@ -231,7 +225,10 @@ auto Workspaces::update() -> void { m_windowsToCreate.clear(); m_windowsToCreate = notCreated; +} +auto Workspaces::update() -> void { + doUpdate(); AModule::update(); } @@ -305,7 +302,6 @@ void Workspaces::onWorkspaceCreated(std::string const &payload) { if (name == payload && (allOutputs() || m_bar.output->name == workspaceJson["monitor"].asString()) && (showSpecial() || !name.starts_with("special")) && !isDoubleSpecial(payload)) { - std::unique_lock workspaceCreateUniqueLock(workspaceCreateSmtx); m_workspacesToCreate.push_back(workspaceJson); break; } From bdd7271da9aa30aecde9ef703a38a9195ec284c5 Mon Sep 17 00:00:00 2001 From: yangyingchao Date: Sat, 6 Jan 2024 12:57:27 +0800 Subject: [PATCH 2/2] Improvements for Hyprland backend 1. Fix warnings reported by clang tidy 2. Use unique lock instead of manully lock/unlock on mutex. The RAII style locking makes sure mutex is unlocked when exceptions are thrown --- include/modules/hyprland/backend.hpp | 10 ++-- src/modules/hyprland/backend.cpp | 79 +++++++++++----------------- 2 files changed, 35 insertions(+), 54 deletions(-) diff --git a/include/modules/hyprland/backend.hpp b/include/modules/hyprland/backend.hpp index 7d97b553..d197df3a 100644 --- a/include/modules/hyprland/backend.hpp +++ b/include/modules/hyprland/backend.hpp @@ -1,11 +1,9 @@ #pragma once -#include #include #include #include #include -#include #include #include "util/json.hpp" @@ -25,16 +23,16 @@ class IPC { void registerForIPC(const std::string&, EventHandler*); void unregisterForIPC(EventHandler*); - std::string getSocket1Reply(const std::string& rq); + static std::string getSocket1Reply(const std::string& rq); Json::Value getSocket1JsonReply(const std::string& rq); private: void startIPC(); void parseIPC(const std::string&); - std::mutex callbackMutex; - util::JsonParser parser_; - std::list> callbacks; + std::mutex m_callbackMutex; + util::JsonParser m_parser; + std::list> m_callbacks; }; inline std::unique_ptr gIPC; diff --git a/src/modules/hyprland/backend.cpp b/src/modules/hyprland/backend.cpp index 5a48d369..3c8313fc 100644 --- a/src/modules/hyprland/backend.cpp +++ b/src/modules/hyprland/backend.cpp @@ -1,21 +1,16 @@ #include "modules/hyprland/backend.hpp" -#include #include #include #include -#include -#include -#include #include #include #include #include #include -#include -#include #include +#include namespace waybar::modules::hyprland { @@ -24,9 +19,9 @@ void IPC::startIPC() { std::thread([&]() { // check for hyprland - const char* HIS = getenv("HYPRLAND_INSTANCE_SIGNATURE"); + const char* his = getenv("HYPRLAND_INSTANCE_SIGNATURE"); - if (!HIS) { + if (his == nullptr) { spdlog::warn("Hyprland is not running, Hyprland IPC will not be available."); return; } @@ -45,8 +40,8 @@ void IPC::startIPC() { addr.sun_family = AF_UNIX; - // socket path - std::string socketPath = "/tmp/hypr/" + std::string(HIS) + "/.socket2.sock"; + // socket path, specified by EventManager of Hyprland + std::string socketPath = "/tmp/hypr/" + std::string(his) + "/.socket2.sock"; strncpy(addr.sun_path, socketPath.c_str(), sizeof(addr.sun_path) - 1); @@ -61,10 +56,9 @@ void IPC::startIPC() { auto file = fdopen(socketfd, "r"); - while (1) { - // read - + while (true) { char buffer[1024]; // Hyprland socket2 events are max 1024 bytes + auto recievedCharPtr = fgets(buffer, 1024, file); if (!recievedCharPtr) { @@ -72,28 +66,21 @@ void IPC::startIPC() { continue; } - callbackMutex.lock(); - std::string messageRecieved(buffer); - messageRecieved = messageRecieved.substr(0, messageRecieved.find_first_of('\n')); - spdlog::debug("hyprland IPC received {}", messageRecieved); - parseIPC(messageRecieved); - callbackMutex.unlock(); - std::this_thread::sleep_for(std::chrono::milliseconds(1)); } }).detach(); } void IPC::parseIPC(const std::string& ev) { - // todo std::string request = ev.substr(0, ev.find_first_of('>')); + std::unique_lock lock(m_callbackMutex); - for (auto& [eventname, handler] : callbacks) { + for (auto& [eventname, handler] : m_callbacks) { if (eventname == request) { handler->onEvent(ev); } @@ -104,11 +91,9 @@ void IPC::registerForIPC(const std::string& ev, EventHandler* ev_handler) { if (!ev_handler) { return; } - callbackMutex.lock(); - callbacks.emplace_back(std::make_pair(ev, ev_handler)); - - callbackMutex.unlock(); + std::unique_lock lock(m_callbackMutex); + m_callbacks.emplace_back(ev, ev_handler); } void IPC::unregisterForIPC(EventHandler* ev_handler) { @@ -116,37 +101,35 @@ void IPC::unregisterForIPC(EventHandler* ev_handler) { return; } - callbackMutex.lock(); + std::unique_lock lock(m_callbackMutex); - for (auto it = callbacks.begin(); it != callbacks.end();) { - auto it_current = it; - it++; - auto& [eventname, handler] = *it_current; + for (auto it = m_callbacks.begin(); it != m_callbacks.end();) { + auto& [eventname, handler] = *it; if (handler == ev_handler) { - callbacks.erase(it_current); + m_callbacks.erase(it++); + } else { + ++it; } } - - callbackMutex.unlock(); } std::string IPC::getSocket1Reply(const std::string& rq) { // basically hyprctl - struct addrinfo ai_hints; - struct addrinfo* ai_res = NULL; - const auto SERVERSOCKET = socket(AF_UNIX, SOCK_STREAM, 0); + struct addrinfo aiHints; + struct addrinfo* aiRes = nullptr; + const auto serverSocket = socket(AF_UNIX, SOCK_STREAM, 0); - if (SERVERSOCKET < 0) { + if (serverSocket < 0) { spdlog::error("Hyprland IPC: Couldn't open a socket (1)"); return ""; } - memset(&ai_hints, 0, sizeof(struct addrinfo)); - ai_hints.ai_family = AF_UNSPEC; - ai_hints.ai_socktype = SOCK_STREAM; + memset(&aiHints, 0, sizeof(struct addrinfo)); + aiHints.ai_family = AF_UNSPEC; + aiHints.ai_socktype = SOCK_STREAM; - if (getaddrinfo("localhost", NULL, &ai_hints, &ai_res) != 0) { + if (getaddrinfo("localhost", nullptr, &aiHints, &aiRes) != 0) { spdlog::error("Hyprland IPC: Couldn't get host (2)"); return ""; } @@ -173,13 +156,13 @@ std::string IPC::getSocket1Reply(const std::string& rq) { return ""; } - if (connect(SERVERSOCKET, reinterpret_cast(&serverAddress), sizeof(serverAddress)) < + if (connect(serverSocket, reinterpret_cast(&serverAddress), sizeof(serverAddress)) < 0) { spdlog::error("Hyprland IPC: Couldn't connect to " + socketPath + ". (3)"); return ""; } - auto sizeWritten = write(SERVERSOCKET, rq.c_str(), rq.length()); + auto sizeWritten = write(serverSocket, rq.c_str(), rq.length()); if (sizeWritten < 0) { spdlog::error("Hyprland IPC: Couldn't write (4)"); @@ -190,22 +173,22 @@ std::string IPC::getSocket1Reply(const std::string& rq) { std::string response; do { - sizeWritten = read(SERVERSOCKET, buffer, 8192); + sizeWritten = read(serverSocket, buffer, 8192); if (sizeWritten < 0) { spdlog::error("Hyprland IPC: Couldn't read (5)"); - close(SERVERSOCKET); + close(serverSocket); return ""; } response.append(buffer, sizeWritten); } while (sizeWritten > 0); - close(SERVERSOCKET); + close(serverSocket); return response; } Json::Value IPC::getSocket1JsonReply(const std::string& rq) { - return parser_.parse(getSocket1Reply("j/" + rq)); + return m_parser.parse(getSocket1Reply("j/" + rq)); } } // namespace waybar::modules::hyprland