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};
@@ -41,6 +42,7 @@ struct randr_output {
   bool match(const string& o, bool exact = true) const;
   bool match(const position& p) const;
 
+  bool contains(const position& p) const;
   bool contains(const randr_output& output) const;
 
   bool equals(const randr_output& output) const;
@@ -53,13 +55,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,
+  monitor_t make_monitor(xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x,
-      bool primary);
+      short int y, bool primary);
-  vector<monitor_t> get_monitors(connection& conn, xcb_window_t root, bool connected_only = false, bool purge_clones = true);
+  vector<monitor_t> get_monitors(
+      connection& conn, xcb_window_t root, bool connected_only = false, bool purge_clones = true);
   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,6 +1,7 @@
 #include "modules/xworkspaces.hpp"
 
 #include <algorithm>
+#include <set>
 #include <utility>
 
 #include "drawtypes/iconset.hpp"
@@ -19,6 +20,13 @@ namespace {
   }
 }  // namespace
 
+/**
+ * Defines a lexicographical order on position
+ */
+bool operator<(const position& a, const position& b) {
+  return std::make_tuple(a.x, a.y) < std::make_tuple(b.x, b.y);
+}
+
 namespace modules {
   template class module<xworkspaces_module>;
 
@@ -146,37 +154,59 @@ 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 = [&] {
+    /*
+     * 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) {
-        return ewmh_util::get_desktop_viewports();
+      ws_positions = 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());
+    /*
-
+     * Not all desktops were assigned a viewport, add (0, 0) for all missing
-    unsigned int step;
+     * desktops.
-    if (bounds.size() > 0) {
+     */
-      step = m_desktop_names.size() / bounds.size();
+    if (ws_positions.size() < m_desktop_names.size()) {
-    } else {
+      auto num_insert = m_desktop_names.size() - ws_positions.size();
-      step = 1;
+      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.
+     */
+    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;
           }
@@ -192,13 +222,15 @@ namespace modules {
           return label;
         }();
 
-        for (unsigned int index = offset; index < offset + step; index++) {
+        for (size_t i = 0; i < ws_positions.size(); i++) {
-          viewport->desktops.emplace_back(make_unique<struct desktop>(index, offset, desktop_state::EMPTY, label_t{}));
+          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;
     }
   }
 
@@ -213,7 +245,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,14 +33,20 @@ 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;
+}
+
 /**
  * Checks if inner is contained within `this`
  *
  * Also returns true for outputs that occupy the exact same space
  */
 bool randr_output::contains(const randr_output& inner) const {
-  return inner.x >= x && inner.x + inner.w <= x + w
+  return inner.x >= x && inner.x + inner.w <= x + w && inner.y >= y && inner.y + inner.h <= y + h;
-    && inner.y >= y && inner.y + inner.h <= y + h;
 }
 
 /**
@@ -48,8 +55,7 @@ bool randr_output::contains(const randr_output& inner) const {
  * Looks at xcb_randr_output_t, position, dimension, name and 'primary'
  */
 bool randr_output::equals(const randr_output& o) const {
-  return o.output == output && o.x == x && o.y == y && o.w == w && o.h == h &&
+  return o.output == output && o.x == x && o.y == y && o.w == w && o.h == h && o.primary == primary && o.name == name;
-    o.primary == primary && o.name == name;
 }
 
 namespace randr_util {
@@ -83,9 +89,8 @@ namespace randr_util {
   /**
    * Define monitor
    */
-  monitor_t make_monitor(
+  monitor_t make_monitor(xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x,
-      xcb_randr_output_t randr, string name, unsigned short int w, unsigned short int h, short int x, short int y,
+      short int y, bool primary) {
-      bool primary) {
     monitor_t mon{new monitor_t::element_type{}};
     mon->output = randr;
     mon->name = move(name);
@@ -181,8 +186,7 @@ namespace randr_util {
 
       // Remove all cloned monitors (monitors with 0 width)
       monitors.erase(
-          std::remove_if(monitors.begin(), monitors.end(),
+          std::remove_if(monitors.begin(), monitors.end(), [](const auto& monitor) { return monitor->w == 0; }),
-            [](const auto & monitor) { return monitor->w == 0; }),
           monitors.end());
     }
 
@@ -258,6 +262,6 @@ namespace randr_util {
       dst.val = static_cast<double>(value);
     }
   }
-}
+}  // namespace randr_util
 
 POLYBAR_NS_END