From 52f062331503449236c320cefaf40dc499846def Mon Sep 17 00:00:00 2001 From: Matthias Viehweger Date: Mon, 21 Oct 2019 10:00:38 +0200 Subject: [PATCH] feat(xworkspaces): Support occupied workspaces (#882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A workspace is occupied if it is not active and there is at least one window managed by the WM (`_NET_CLIENT_LIST`) that has set `_NET_WM_DESKTOP` to that workspace. The behavior when `_NET_WM_DESKTOP` is not set is not yet clear but this is unlikely to happen since most WMs will position windows on some desktop. Closes #874 Fixes #1444 Fixes #1033 * Set Desktop OCCUPIED if a window moves there This covers more of an edge-case. I did this first by accident, it might vanish later on. * Replace tracking change of WS with currently used WS * Untrack occupied workspaces * Track windows and their desktops in pairs * Match type of occupied_desktops with current_desktop Because the index needs to be matched later on, type mismatches would be non-ideal. * Recreate the occupied desktops everytime and remove duplicates * Readd support for moving windows to other desktops * Use less characters to empty the vector * Rename variable storing the desktops * Recount windows on every occasion This alone simplifies the management and the lookup for occupation of a workspace * Keep track of number of windows in every workspace * Add debugging output that shall be removed before merging * Remove obsolete TODO * m_client_list should always be diff'd, since the desktop may change Therefore we update the desktop-count tally every time the client_list changes. It may just be a desktop-change without a change of clients.size()... * Add more logging-spam to understand window/desktop lifecycle * Lock event-handler to serialize handling of events * Fix occupied workspace counting and change to bool array Also, performance improvements when diffing new and old client lists * Fix crash when all clients are removed * Conform to linter and styleguide * Shorten conditional as it is standard enough Since this only guards against 0-divisions, it can be shortened without risking too much confusion down the road. * Guard against multiple threads accessing and modifying data Fixes #1444 Modification of internal data happens through the handle-method, while the build-method tries to access the data structures for display. Since some modifications clear e.g. the m_viewports, references may become invalid between looping over them an accessing them. The mutex should guard against this simultanuous access. * Do not 'adopt_lock', because calls come from very different threads To my understanding, adopt_lock has some dependency on the mutex-ownership. Since the lock is once called from the inside (in handle) and once from the outside (in build), there might be a problem. After brief testing, the segfaults happened fewer times. See #1444 * Also listen to _NET_WM_DESKTOP In order to move a window from one desktop to another, it is sufficient to set the desktop-property of that window. xmonad fires a lot of events in the case of moving a window, herbstluftwm only updates the _NET_WM_DESKTOP-atom of the window. This change reloads the clientlist in order to correctly set the desktop state "occupied". * Describe need and use of mutex It might be possible to relieve the guard in xworkspaces_module::handle, but I am unsure about this. Since xmonad emits a lot of events on almost every minor change, I would let the guard keep its post, avoiding race-conditions in event-handling. * Give temporary variables better names * Clarify purpose of loop About 80% of this comment are taken from https://github.com/jaagr/polybar/pull/882#discussion_r255317363 * Remove merge-remainder * Use a simpler method to list occupied desktops. Co-authored-by: Jérôme Boulmier * Document m_clients field --- include/modules/xworkspaces.hpp | 16 ++++++-- src/modules/xworkspaces.cpp | 71 ++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/include/modules/xworkspaces.hpp b/include/modules/xworkspaces.hpp index cd6522e7..495ed8ee 100644 --- a/include/modules/xworkspaces.hpp +++ b/include/modules/xworkspaces.hpp @@ -1,6 +1,8 @@ #pragma once #include +#include +#include #include "components/config.hpp" #include "components/types.hpp" @@ -69,9 +71,10 @@ namespace modules { void set_desktop_urgent(xcb_window_t window); bool input(string&& cmd); - vector get_desktop_names(); private: + static vector get_desktop_names(); + static constexpr const char* DEFAULT_ICON{"icon-default"}; static constexpr const char* DEFAULT_LABEL_STATE{"%icon% %name%"}; static constexpr const char* DEFAULT_LABEL_MONITOR{"%name%"}; @@ -94,7 +97,10 @@ namespace modules { unsigned int m_current_desktop; string m_current_desktop_name; - vector m_clientlist; + /** + * Maps an xcb window to its desktop number + */ + map m_clients; vector> m_viewports; map m_labels; label_t m_monitorlabel; @@ -104,8 +110,12 @@ namespace modules { bool m_scroll{true}; size_t m_index{0}; + // The following mutex is here to protect the data of this modules. + // This can't be achieved using m_buildlock since we "CRTP override" get_output(). + mutable mutex m_workspace_mutex; + event_timer m_timer{0L, 25L}; }; -} +} // namespace modules POLYBAR_NS_END diff --git a/src/modules/xworkspaces.cpp b/src/modules/xworkspaces.cpp index da94264b..1515ca4e 100644 --- a/src/modules/xworkspaces.cpp +++ b/src/modules/xworkspaces.cpp @@ -1,9 +1,10 @@ +#include "modules/xworkspaces.hpp" + #include #include #include "drawtypes/iconset.hpp" #include "drawtypes/label.hpp" -#include "modules/xworkspaces.hpp" #include "utils/factory.hpp" #include "utils/math.hpp" #include "x11/atoms.hpp" @@ -17,7 +18,7 @@ namespace { inline bool operator==(const position& a, const position& b) { return a.x + a.y == b.x + b.y; } -} +} // namespace namespace modules { template class module; @@ -86,21 +87,25 @@ namespace modules { m_current_desktop_name = m_desktop_names[m_current_desktop]; rebuild_desktops(); - rebuild_desktop_states(); // Get _NET_CLIENT_LIST rebuild_clientlist(); + rebuild_desktop_states(); } /** * Handler for XCB_PROPERTY_NOTIFY events */ void xworkspaces_module::handle(const evt::property_notify& evt) { - if (evt->atom == m_ewmh->_NET_CLIENT_LIST) { + std::lock_guard lock(m_workspace_mutex); + + if (evt->atom == m_ewmh->_NET_CLIENT_LIST || evt->atom == m_ewmh->_NET_WM_DESKTOP) { rebuild_clientlist(); + rebuild_desktop_states(); } else if (evt->atom == m_ewmh->_NET_DESKTOP_NAMES || evt->atom == m_ewmh->_NET_NUMBER_OF_DESKTOPS) { m_desktop_names = get_desktop_names(); rebuild_desktops(); + rebuild_clientlist(); rebuild_desktop_states(); } else if (evt->atom == m_ewmh->_NET_CURRENT_DESKTOP) { m_current_desktop = ewmh_util::get_current_desktop(); @@ -123,28 +128,21 @@ namespace modules { * Rebuild the list of managed clients */ void xworkspaces_module::rebuild_clientlist() { - vector clients = ewmh_util::get_client_list(); - vector diff; - std::sort(clients.begin(), clients.end()); - std::sort(m_clientlist.begin(), m_clientlist.end()); + vector newclients = ewmh_util::get_client_list(); + std::sort(newclients.begin(), newclients.end()); - if (m_clientlist.size() > clients.size()) { - std::set_difference( - m_clientlist.begin(), m_clientlist.end(), clients.begin(), clients.end(), back_inserter(diff)); - for (auto&& win : diff) { - // untrack window - m_clientlist.erase(std::remove(m_clientlist.begin(), m_clientlist.end(), win), m_clientlist.end()); - } - } else { - std::set_difference( - clients.begin(), clients.end(), m_clientlist.begin(), m_clientlist.end(), back_inserter(diff)); - for (auto&& win : diff) { - // listen for wm_hint (urgency) changes - m_connection.ensure_event_mask(win, XCB_EVENT_MASK_PROPERTY_CHANGE); - // track window - m_clientlist.emplace_back(win); + for (auto&& client : newclients) { + if (m_clients.count(client) == 0) { + // new client: listen for changes (wm_hint or desktop) + m_connection.ensure_event_mask(client, XCB_EVENT_MASK_PROPERTY_CHANGE); } } + + // rebuild entire mapping of clients to desktops + m_clients.clear(); + for (auto&& client : newclients) { + m_clients[client] = ewmh_util::get_desktop_from_window(client); + } } /** @@ -209,13 +207,20 @@ namespace modules { * Update active state of current desktops */ void xworkspaces_module::rebuild_desktop_states() { + std::set occupied_desks; + for (auto&& c : m_clients) { + occupied_desks.insert(c.second); + } + for (auto&& v : m_viewports) { for (auto&& d : v->desktops) { if (m_desktop_names[d->index] == m_current_desktop_name) { d->state = desktop_state::ACTIVE; + } else if (occupied_desks.count(d->index) > 0) { + d->state = desktop_state::OCCUPIED; } else { d->state = desktop_state::EMPTY; - } + } d->label = m_labels.at(d->state)->clone(); d->label->reset_tokens(); @@ -226,14 +231,13 @@ namespace modules { } } - vector xworkspaces_module::get_desktop_names(){ + vector xworkspaces_module::get_desktop_names() { vector names = ewmh_util::get_desktop_names(); unsigned int desktops_number = ewmh_util::get_number_of_desktops(); - if(desktops_number == names.size()) { + if (desktops_number == names.size()) { return names; - } - else if(desktops_number < names.size()) { - names.erase(names.begin()+desktops_number, names.end()); + } else if (desktops_number < names.size()) { + names.erase(names.begin() + desktops_number, names.end()); return names; } for (unsigned int i = names.size(); i < desktops_number + 1; i++) { @@ -247,7 +251,7 @@ namespace modules { */ void xworkspaces_module::set_desktop_urgent(xcb_window_t window) { auto desk = ewmh_util::get_desktop_from_window(window); - if(desk == m_current_desktop) + if (desk == m_current_desktop) // ignore if current desktop is urgent return; for (auto&& v : m_viewports) { @@ -264,7 +268,6 @@ namespace modules { } } } - } /** @@ -276,6 +279,8 @@ namespace modules { * Generate module output */ string xworkspaces_module::get_output() { + std::unique_lock lock(m_workspace_mutex); + // Get the module output early so that // the format prefix/suffix also gets wrapped // with the cmd handlers @@ -335,6 +340,8 @@ namespace modules { * Handle user input event */ bool xworkspaces_module::input(string&& cmd) { + std::lock_guard lock(m_workspace_mutex); + size_t len{strlen(EVENT_PREFIX)}; if (cmd.compare(0, len, EVENT_PREFIX) != 0) { return false; @@ -372,6 +379,6 @@ namespace modules { return true; } -} +} // namespace modules POLYBAR_NS_END