fix(xworkspaces): Assign desktops to correct viewport

Before the module would just try to evenly distribute desktops
(workspaces) among the viewports.

But since `_NET_DESKTOP_VIEWPORT` actually maps desktops to viewports,
we can use that information to assign workspaces to the right viewport.

Fixes #1849
Fixes #1764

include/x11/extensions/randr.hpp

@@ -7,6 +7,7 @@
 #endif
 
 #include <xcb/randr.h>
+
 #include <xpp/proto/randr.hpp>
 
 #include "common.hpp"
@@ -19,7 +20,7 @@ struct position;
 namespace evt {
   using randr_notify = xpp::randr::event::notify<connection&>;
   using randr_screen_change_notify = xpp::randr::event::screen_change_notify<connection&>;
-}
+}  // namespace evt
 
 struct backlight_values {
   unsigned int atom{0};
@@ -40,6 +41,8 @@ struct randr_output {
 
   bool match(const string& o, bool exact = true) const;
   bool match(const position& p) const;
+
+  bool contains(const position& p) const;
 };
 
 using monitor_t = shared_ptr<randr_output>;
@@ -49,13 +52,14 @@ namespace randr_util {
 
   bool check_monitor_support();
 
-  monitor_t make_monitor(xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x, short int y,
-      bool primary);
-  vector<monitor_t> get_monitors(connection& conn, xcb_window_t root, bool connected_only = false, bool realloc = false);
+  monitor_t make_monitor(xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x,
+      short int y, bool primary);
+  vector<monitor_t> get_monitors(
+      connection& conn, xcb_window_t root, bool connected_only = false, bool realloc = false);
   monitor_t match_monitor(vector<monitor_t> monitors, const string& name, bool exact_match);
 
   void get_backlight_range(connection& conn, const monitor_t& mon, backlight_values& dst);
   void get_backlight_value(connection& conn, const monitor_t& mon, backlight_values& dst);
-}
+}  // namespace randr_util
 
 POLYBAR_NS_END

src/modules/xworkspaces.cpp

@@ -1,17 +1,17 @@
 #include "modules/xworkspaces.hpp"
 
 #include <algorithm>
+#include <set>
 #include <utility>
 
 #include "drawtypes/iconset.hpp"
 #include "drawtypes/label.hpp"
+#include "modules/meta/base.inl"
 #include "utils/factory.hpp"
 #include "utils/math.hpp"
 #include "x11/atoms.hpp"
 #include "x11/connection.hpp"
 
-#include "modules/meta/base.inl"
-
 POLYBAR_NS
 
 namespace {
@@ -20,6 +20,15 @@ namespace {
   }
 }  // namespace
 
+/**
+ * Defines a lexicographical order on position:
+ *
+ * (x1, y1) < (x2, y2) <==> (x1 == x2 && y1 < y2) || (x1 < x2)
+ */
+bool operator<(const position& a, const position& b) {
+  return a.x < b.x || (a.x == b.x && a.y < b.y);
+}
+
 namespace modules {
   template class module<xworkspaces_module>;
 
@@ -147,37 +156,62 @@ namespace modules {
 
   /**
    * Rebuild the desktop tree
+   *
+   * This requires m_desktop_names to have an up-to-date value
    */
   void xworkspaces_module::rebuild_desktops() {
     m_viewports.clear();
 
-    auto bounds = [&] {
-      if (m_monitorsupport) {
-        return ewmh_util::get_desktop_viewports();
-      } else {
-        vector<position> b;
-        std::fill_n(std::back_inserter(b), m_desktop_names.size(), position{m_bar.monitor->x, m_bar.monitor->y});
-        return b;
-      }
-    }();
-
-    bounds.erase(std::unique(bounds.begin(), bounds.end(), [](auto& a, auto& b) { return a == b; }), bounds.end());
+    /*
+     * Stores the _NET_DESKTOP_VIEWPORT hint
+     *
+     * For WMs that don't support that hint, we store an empty vector
+     *
+     * If the length of the vector is less than _NET_NUMBER_OF_DESKTOPS
+     * all desktops which aren't explicitly assigned a postion will be
+     * assigned (0, 0)
+     *
+     * We use this to map workspaces to viewports, desktop i is at position
+     * ws_positions[i].
+     */
+    vector<position> ws_positions;
+    if (m_monitorsupport) {
+      ws_positions = ewmh_util::get_desktop_viewports();
+    }
 
-    unsigned int step;
-    if (bounds.size() > 0) {
-      step = m_desktop_names.size() / bounds.size();
-    } else {
-      step = 1;
+    /*
+     * Not all desktops were assigned a viewport, add (0, 0) for all missing
+     * desktops.
+     */
+    if (ws_positions.size() < m_desktop_names.size()) {
+      auto num_insert = m_desktop_names.size() - ws_positions.size();
+      ws_positions.reserve(num_insert);
+      std::fill_n(std::back_inserter(ws_positions), num_insert, position{0, 0});
     }
-    unsigned int offset = 0;
-    for (unsigned int i = 0; i < bounds.size(); i++) {
-      if (!m_pinworkspaces || m_bar.monitor->match(bounds[i])) {
+
+    /*
+     * The list of viewports is the set of unique positions in ws_positions
+     * Using a set automatically removes duplicates.
+     *
+     * TODO is it possible to have two monitors but only a single workspace? In
+     * that case, this would only give us a single viewport.
+     */
+    std::set<position> viewports(ws_positions.begin(), ws_positions.end());
+
+    for (auto&& viewport_pos : viewports) {
+      /*
+       * If pin-workspaces is set, we only add the viewport if it's in the
+       * monitor the bar is on.
+       * Generally viewport_pos is the same as the top-left coordinate of the
+       * monitor but we use `contains` here as a safety in case it isn't exactly.
+       */
+      if (!m_pinworkspaces || m_bar.monitor->contains(viewport_pos)) {
         auto viewport = make_unique<struct viewport>();
         viewport->state = viewport_state::UNFOCUSED;
-        viewport->pos = bounds[i];
+        viewport->pos = viewport_pos;
 
         for (auto&& m : m_monitors) {
-          if (m->match(viewport->pos)) {
+          if (m->contains(viewport->pos)) {
             viewport->name = m->name;
             viewport->state = viewport_state::FOCUSED;
           }
@@ -193,13 +227,15 @@ namespace modules {
           return label;
         }();
 
-        for (unsigned int index = offset; index < offset + step; index++) {
-          viewport->desktops.emplace_back(make_unique<struct desktop>(index, offset, desktop_state::EMPTY, label_t{}));
+        for (size_t i = 0; i < ws_positions.size(); i++) {
+          auto&& ws_pos = ws_positions[i];
+          if (ws_pos == viewport_pos) {
+            viewport->desktops.emplace_back(make_unique<struct desktop>(i, 0, desktop_state::EMPTY, label_t{}));
+          }
         }
 
         m_viewports.emplace_back(move(viewport));
       }
-      offset += step;
     }
   }
 
@@ -214,7 +250,7 @@ namespace modules {
 
     for (auto&& v : m_viewports) {
       for (auto&& d : v->desktops) {
-        if (m_desktop_names[d->index] == m_current_desktop_name) {
+        if (d->index == m_current_desktop) {
           d->state = desktop_state::ACTIVE;
         } else if (occupied_desks.count(d->index) > 0) {
           d->state = desktop_state::OCCUPIED;

src/x11/extensions/randr.cpp

@@ -1,3 +1,5 @@
+#include "x11/extensions/randr.hpp"
+
 #include <algorithm>
 #include <utility>
 
@@ -7,7 +9,6 @@
 #include "utils/string.hpp"
 #include "x11/atoms.hpp"
 #include "x11/connection.hpp"
-#include "x11/extensions/randr.hpp"
 
 POLYBAR_NS
 
@@ -32,6 +33,13 @@ bool randr_output::match(const position& p) const {
   return p.x == x && p.y == y;
 }
 
+/**
+ * Checks if `this` contains the position p
+ */
+bool randr_output::contains(const position& p) const {
+  return x <= p.x && y <= p.y && (x + w) > p.x && (y + h) > p.y;
+}
+
 namespace randr_util {
   /**
    * XRandR version
@@ -63,9 +71,8 @@ namespace randr_util {
   /**
    * Define monitor
    */
-  monitor_t make_monitor(
-      xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x, short int y,
-      bool primary) {
+  monitor_t make_monitor(xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x,
+      short int y, bool primary) {
     monitor_t mon{new monitor_t::element_type{}};
     mon->output = randr;
     mon->name = move(name);
@@ -192,9 +199,9 @@ namespace randr_util {
    */
   monitor_t match_monitor(vector<monitor_t> monitors, const string& name, bool exact_match) {
     monitor_t result{};
-    for(auto&& monitor : monitors) {
+    for (auto&& monitor : monitors) {
       // If we can do an exact match, we have found our result
-      if(monitor->match(name, true)) {
+      if (monitor->match(name, true)) {
         result = move(monitor);
         break;
       }
@@ -205,7 +212,7 @@ namespace randr_util {
        * Note: If exact_match == true, we don't need to run this because it
        * would be the exact same check as above
        */
-      if(!exact_match && monitor->match(name, false)) {
+      if (!exact_match && monitor->match(name, false)) {
         result = move(monitor);
       }
     }
@@ -254,6 +261,6 @@ namespace randr_util {
       dst.val = static_cast<double>(value);
     }
   }
-}
+}  // namespace randr_util
 
 POLYBAR_NS_END