Stop tracking action blocks in renderer

Dispatch can now directly get the current position from the renderer and
create action blocks with the proper coordinates.

include/components/renderer.hpp

@@ -53,8 +53,9 @@ class renderer : public renderer_interface,
 
   void change_alignment(const tags::context& ctxt) override;
 
-  void action_open(const tags::context& ctxt, tags::action_t id) override;
+  double get_x(const tags::context& ctxt) const override;
-  void action_close(const tags::context& ctxt, tags::action_t id) override;
+
+  double get_alignment_start(const alignment align) const override;
 
  protected:
   void fill_background();

include/components/renderer_interface.hpp

@@ -7,23 +7,36 @@ POLYBAR_NS
 
 class renderer_interface {
  public:
-  renderer_interface(tags::action_context& action_ctxt) : m_action_ctxt(action_ctxt){};
+  renderer_interface(const tags::action_context& action_ctxt) : m_action_ctxt(action_ctxt){};
 
   virtual void render_offset(const tags::context& ctxt, int pixels) = 0;
   virtual void render_text(const tags::context& ctxt, const string&& str) = 0;
   virtual void change_alignment(const tags::context& ctxt) = 0;
 
-  virtual void action_open(const tags::context& ctxt, tags::action_t id) = 0;
+  /**
-  virtual void action_close(const tags::context& ctxt, tags::action_t id) = 0;
+   * Get the current x-coordinate of the renderer.
+   *
+   * This position denotes the coordinate where the next thing will be rendered.
+   * It is relative to the start of the current alignment because the absolute
+   * positions may not be known until after the renderer has finished.
+   */
+  virtual double get_x(const tags::context& ctxt) const = 0;
+
+  /**
+   * Get the absolute x-position of the start of an alignment block.
+   *
+   * The position is absolute in terms of the bar window.
+   *
+   * Only call this after all the rendering is finished as these values change
+   * when new things are rendered.
+   */
+  virtual double get_alignment_start(const alignment align) const = 0;
 
  protected:
   /**
-   * Stores information about actions in the current action cycle.
+   * Stores information about actions in the current render cycle.
-   *
-   * The renderer is only responsible for updating the start and end positions
-   * of each action block.
    */
-  tags::action_context& m_action_ctxt;
+  const tags::action_context& m_action_ctxt;
 };
 
 POLYBAR_NS_END

include/tags/context.hpp

@@ -98,23 +98,47 @@ namespace tags {
 
   static constexpr action_t NO_ACTION = -1;
 
+  /**
+   * Defines a clickable (or scrollable) action block.
+   *
+   * An action block is an area on the bar that executes some command when clicked.
+   */
   struct action_block {
     action_block(const string&& cmd, mousebtn button, alignment align, bool is_open)
         : cmd(std::move(cmd)), button(button), align(align), is_open(is_open){};
 
     string cmd;
+    /**
+     * Start position of the action block (inclusive), relative to the alignment.
+     */
     double start_x{0};
+
+    /**
+     * End position of the action block (exclusive), relative to the alignment.
+     */
     double end_x{0};
     mousebtn button;
     alignment align;
+    /**
+     * Tracks whether this block is still open or whether it already has a
+     * corresponding closing tag.
+     *
+     * After rendering, all action blocks should be closed.
+     */
     bool is_open;
 
     unsigned int width() const {
       return static_cast<unsigned int>(end_x - start_x + 0.5);
     }
 
-    bool test(int point) const {
+    /**
-      return static_cast<int>(start_x) <= point && static_cast<int>(end_x) > point;
+     * Tests whether a given point is inside this block.
+     *
+     * This additionally needs the position of the start of the alignment
+     * because the given point is relative to the bar window.
+     */
+    bool test(double align_start, int point) const {
+      return static_cast<int>(start_x + align_start) <= point && static_cast<int>(end_x + align_start) > point;
     }
   };
 
@@ -122,11 +146,10 @@ namespace tags {
    public:
     void reset();
 
-    action_t action_open(mousebtn btn, const string&& cmd, alignment align);
+    action_t action_open(mousebtn btn, const string&& cmd, alignment align, double x);
-    std::pair<action_t, mousebtn> action_close(mousebtn btn, alignment align);
+    std::pair<action_t, mousebtn> action_close(mousebtn btn, alignment align, double x);
 
-    void set_start(action_t id, double x);
+    void set_alignmnent_start(const alignment a, const double x);
-    void set_end(action_t id, double x);
 
     std::map<mousebtn, tags::action_t> get_actions(int x) const;
     action_t has_action(mousebtn btn, int x) const;
@@ -137,15 +160,28 @@ namespace tags {
     size_t num_actions() const;
 
     // TODO provide better interface for adjusting the start positions of actions
-    std::vector<action_block>& get_blocks();
+    const std::vector<action_block>& get_blocks() const;
 
    protected:
+    void set_start(action_t id, double x);
+    void set_end(action_t id, double x);
+
     /**
      * Stores all currently known action blocks.
      *
-     * The action_t type is meant as an index into this vector.
+     * The action_t type is an index into this vector.
      */
     std::vector<action_block> m_action_blocks;
+
+    /**
+     * Stores the x-coordinate for the start of all the alignment blocks.
+     *
+     * This is needed because the action block coordinates are relative to the
+     * alignment blocks and thus need the alignment block coordinates for
+     * intersection tests.
+     */
+    std::map<alignment, double> m_align_start{
+        {alignment::NONE, 0}, {alignment::LEFT, 0}, {alignment::CENTER, 0}, {alignment::RIGHT, 0}};
   };
 
 }  // namespace tags

include/tags/dispatch.hpp

@@ -34,7 +34,6 @@ namespace tags {
     void handle_control(controltag ctrl);
 
    private:
-    vector<mousebtn> m_actions;
     const logger& m_log;
 
     unique_ptr<context> m_ctxt;

src/components/renderer.cpp

@@ -251,19 +251,6 @@ void renderer::begin(xcb_rectangle_t rect) {
 void renderer::end() {
   m_log.trace_x("renderer: end");
 
-  /*
-   * Finalize the positions of the action blocks.
-   * Up until this point, the positions were relative to the start of the
-   * alignment block the action was located in.
-   * Here we add the start position of the block as well as the start position
-   * of the bar itself (without borders or tray) to create positions relative to
-   * the bar window.
-   */
-  for (auto& a : m_action_ctxt.get_blocks()) {
-    a.start_x += block_x(a.align) + m_rect.x;
-    a.end_x += block_x(a.align) + m_rect.x;
-  }
-
   if (m_align != alignment::NONE) {
     m_log.trace_x("renderer: pop(%i)", static_cast<int>(m_align));
     m_context->pop(&m_blocks[m_align].pattern);
@@ -706,12 +693,12 @@ void renderer::change_alignment(const tags::context& ctxt) {
   }
 }
 
-void renderer::action_open(const tags::context&, tags::action_t id) {
+double renderer::get_x(const tags::context& ctxt) const {
-  m_action_ctxt.set_start(id, m_blocks.at(m_align).x);
+  return m_blocks.at(ctxt.get_alignment()).x;
 }
 
-void renderer::action_close(const tags::context&, tags::action_t id) {
+double renderer::get_alignment_start(const alignment align) const {
-  m_action_ctxt.set_end(id, m_blocks.at(m_align).x);
+  return block_x(align) + m_rect.x;
 }
 
 /**

src/tags/context.cpp

@@ -118,19 +118,22 @@ namespace tags {
     m_action_blocks.clear();
   }
 
-  action_t action_context::action_open(mousebtn btn, const string&& cmd, alignment align) {
+  action_t action_context::action_open(mousebtn btn, const string&& cmd, alignment align, double x) {
     action_t id = m_action_blocks.size();
     m_action_blocks.emplace_back(std::move(cmd), btn, align, true);
+    set_start(id, x);
     return id;
   }
 
-  std::pair<action_t, mousebtn> action_context::action_close(mousebtn btn, alignment align) {
+  std::pair<action_t, mousebtn> action_context::action_close(mousebtn btn, alignment align, double x) {
     for (auto it = m_action_blocks.rbegin(); it != m_action_blocks.rend(); it++) {
       if (it->is_open && it->align == align && (btn == mousebtn::NONE || it->button == btn)) {
         it->is_open = false;
 
         // Converts a reverse iterator into an index
-        return {std::distance(m_action_blocks.begin(), it.base()) - 1, it->button};
+        action_t id = std::distance(m_action_blocks.begin(), it.base()) - 1;
+        set_end(id, x);
+        return {id, it->button};
       }
     }
 
@@ -145,6 +148,10 @@ namespace tags {
     m_action_blocks[id].end_x = x;
   }
 
+  void action_context::set_alignmnent_start(const alignment a, const double x) {
+    m_align_start[a] = x;
+  }
+
   std::map<mousebtn, tags::action_t> action_context::get_actions(int x) const {
     std::map<mousebtn, tags::action_t> buttons;
 
@@ -157,7 +164,7 @@ namespace tags {
       mousebtn btn = action.button;
 
       // Higher IDs are higher in the action stack.
-      if (id > buttons[btn] && action.test(x)) {
+      if (id > buttons[btn] && action.test(m_align_start.at(action.align), x)) {
         buttons[action.button] = id;
       }
     }
@@ -191,7 +198,7 @@ namespace tags {
     return m_action_blocks.size();
   }
 
-  std::vector<action_block>& action_context::get_blocks() {
+  const std::vector<action_block>& action_context::get_blocks() const {
     return m_action_blocks;
   }
 

src/tags/dispatch.cpp

@@ -100,6 +100,10 @@ namespace tags {
       }
     }
 
+    for (auto a : {alignment::LEFT, alignment::CENTER, alignment::RIGHT}) {
+      m_action_ctxt.set_alignmnent_start(a, renderer.get_alignment_start(a));
+    }
+
     // TODO handle unclosed action tags in ctxt
     /* if (!m_actions.empty()) { */
     /*   throw runtime_error(to_string(m_actions.size()) + " unclosed action block(s)"); */
@@ -122,12 +126,9 @@ namespace tags {
 
   void dispatch::handle_action(renderer_interface& renderer, mousebtn btn, bool closing, const string&& cmd) {
     if (closing) {
-      action_t id;
+      m_action_ctxt.action_close(btn, m_ctxt->get_alignment(), renderer.get_x(*m_ctxt));
-      std::tie(id, btn) = m_action_ctxt.action_close(btn, m_ctxt->get_alignment());
-      renderer.action_close(*m_ctxt, id);
     } else {
-      action_t id = m_action_ctxt.action_open(btn, std::move(cmd), m_ctxt->get_alignment());
+      m_action_ctxt.action_open(btn, std::move(cmd), m_ctxt->get_alignment(), renderer.get_x(*m_ctxt));
-      renderer.action_open(*m_ctxt, id);
     }
   }
 

tests/unit_tests/tags/action_context.cpp

@@ -1,7 +1,6 @@
-#include "tags/context.hpp"
-
 #include "common/test.hpp"
 #include "components/logger.hpp"
+#include "tags/context.hpp"
 
 using namespace polybar;
 using namespace std;
@@ -10,20 +9,20 @@ using namespace tags;
 TEST(ActionCtxtTest, dblClick) {
   action_context ctxt;
 
-  ctxt.action_open(mousebtn::DOUBLE_LEFT, "", alignment::LEFT);
+  ctxt.action_open(mousebtn::DOUBLE_LEFT, "", alignment::LEFT, 0);
-  ctxt.action_close(mousebtn::DOUBLE_LEFT, alignment::LEFT);
+  ctxt.action_close(mousebtn::DOUBLE_LEFT, alignment::LEFT, 1);
 
   ASSERT_TRUE(ctxt.has_double_click());
 
   ctxt.reset();
 
-  ctxt.action_open(mousebtn::DOUBLE_MIDDLE, "", alignment::LEFT);
+  ctxt.action_open(mousebtn::DOUBLE_MIDDLE, "", alignment::LEFT, 0);
-  ctxt.action_close(mousebtn::DOUBLE_MIDDLE, alignment::LEFT);
+  ctxt.action_close(mousebtn::DOUBLE_MIDDLE, alignment::LEFT, 1);
 
   ASSERT_TRUE(ctxt.has_double_click());
 
-  ctxt.action_open(mousebtn::DOUBLE_RIGHT, "", alignment::LEFT);
+  ctxt.action_open(mousebtn::DOUBLE_RIGHT, "", alignment::LEFT, 0);
-  ctxt.action_close(mousebtn::DOUBLE_RIGHT, alignment::LEFT);
+  ctxt.action_close(mousebtn::DOUBLE_RIGHT, alignment::LEFT, 1);
 
   ASSERT_TRUE(ctxt.has_double_click());
 }
@@ -31,20 +30,20 @@ TEST(ActionCtxtTest, dblClick) {
 TEST(ActionCtxtTest, closing) {
   action_context ctxt;
 
-  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT);
+  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT, 0);
-  auto id2 = ctxt.action_open(mousebtn::RIGHT, "", alignment::CENTER);
+  auto id2 = ctxt.action_open(mousebtn::RIGHT, "", alignment::CENTER, 0);
-  auto id3 = ctxt.action_open(mousebtn::RIGHT, "", alignment::LEFT);
+  auto id3 = ctxt.action_open(mousebtn::RIGHT, "", alignment::LEFT, 0);
-  auto id4 = ctxt.action_open(mousebtn::MIDDLE, "", alignment::LEFT);
+  auto id4 = ctxt.action_open(mousebtn::MIDDLE, "", alignment::LEFT, 0);
 
   EXPECT_NE(NO_ACTION, id1);
   EXPECT_NE(NO_ACTION, id2);
   EXPECT_NE(NO_ACTION, id3);
   EXPECT_NE(NO_ACTION, id4);
 
-  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::LEFT, alignment::LEFT));
+  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::LEFT, alignment::LEFT, 1));
-  EXPECT_EQ(make_pair(id4, mousebtn::MIDDLE), ctxt.action_close(mousebtn::NONE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id4, mousebtn::MIDDLE), ctxt.action_close(mousebtn::NONE, alignment::LEFT, 1));
-  EXPECT_EQ(make_pair(id3, mousebtn::RIGHT), ctxt.action_close(mousebtn::NONE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id3, mousebtn::RIGHT), ctxt.action_close(mousebtn::NONE, alignment::LEFT, 1));
-  EXPECT_EQ(make_pair(id2, mousebtn::RIGHT), ctxt.action_close(mousebtn::NONE, alignment::CENTER));
+  EXPECT_EQ(make_pair(id2, mousebtn::RIGHT), ctxt.action_close(mousebtn::NONE, alignment::CENTER, 1));
 
   EXPECT_EQ(4, ctxt.num_actions());
 }
@@ -63,19 +62,12 @@ TEST(ActionCtxtTest, overlapping) {
    * clang-format on
    */
 
-  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT);
+  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT, 0);
-  auto id2 = ctxt.action_open(mousebtn::MIDDLE, "", alignment::LEFT);
+  auto id2 = ctxt.action_open(mousebtn::MIDDLE, "", alignment::LEFT, 1);
-  auto id3 = ctxt.action_open(mousebtn::RIGHT, "", alignment::LEFT);
+  auto id3 = ctxt.action_open(mousebtn::RIGHT, "", alignment::LEFT, 2);
-  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::LEFT, alignment::LEFT));
+  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::LEFT, alignment::LEFT, 3));
-  EXPECT_EQ(make_pair(id3, mousebtn::RIGHT), ctxt.action_close(mousebtn::RIGHT, alignment::LEFT));
+  EXPECT_EQ(make_pair(id3, mousebtn::RIGHT), ctxt.action_close(mousebtn::RIGHT, alignment::LEFT, 6));
-  EXPECT_EQ(make_pair(id2, mousebtn::MIDDLE), ctxt.action_close(mousebtn::MIDDLE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id2, mousebtn::MIDDLE), ctxt.action_close(mousebtn::MIDDLE, alignment::LEFT, 5));
-
-  ctxt.set_start(id1, 0);
-  ctxt.set_end(id1, 3);
-  ctxt.set_start(id2, 1);
-  ctxt.set_end(id2, 6);
-  ctxt.set_start(id3, 2);
-  ctxt.set_end(id3, 5);
 
   auto actions = ctxt.get_actions(2);
 
@@ -100,19 +92,12 @@ TEST(ActionCtxtTest, stacking) {
    * clang-format on
    */
 
-  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT);
+  auto id1 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT, 0);
-  auto id2 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT);
+  auto id2 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT, 1);
-  auto id3 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT);
+  auto id3 = ctxt.action_open(mousebtn::LEFT, "", alignment::LEFT, 3);
-  EXPECT_EQ(make_pair(id3, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id3, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT, 6));
-  EXPECT_EQ(make_pair(id2, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id2, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT, 7));
-  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT));
+  EXPECT_EQ(make_pair(id1, mousebtn::LEFT), ctxt.action_close(mousebtn::NONE, alignment::LEFT, 8));
-
-  ctxt.set_start(id1, 0);
-  ctxt.set_end(id1, 8);
-  ctxt.set_start(id2, 1);
-  ctxt.set_end(id2, 7);
-  ctxt.set_start(id3, 3);
-  ctxt.set_end(id3, 6);
 
   EXPECT_EQ(id1, ctxt.has_action(mousebtn::LEFT, 0));
   EXPECT_EQ(id2, ctxt.has_action(mousebtn::LEFT, 1));
@@ -131,7 +116,7 @@ TEST(ActionCtxtTest, cmd) {
 
   string cmd = "foobar";
 
-  auto id = ctxt.action_open(mousebtn::DOUBLE_RIGHT, cmd.substr(), alignment::RIGHT);
+  auto id = ctxt.action_open(mousebtn::DOUBLE_RIGHT, cmd.substr(), alignment::RIGHT, 0);
 
   ASSERT_EQ(cmd, ctxt.get_action(id));
 }

tests/unit_tests/tags/dispatch.cpp

@@ -8,8 +8,8 @@ using namespace polybar;
 using namespace std;
 using namespace tags;
 
-using ::testing::InSequence;
 using ::testing::_;
+using ::testing::InSequence;
 
 class MockRenderer : public renderer_interface {
  public:
@@ -18,8 +18,8 @@ class MockRenderer : public renderer_interface {
   MOCK_METHOD(void, render_offset, (const context& ctxt, int pixels), (override));
   MOCK_METHOD(void, render_text, (const context& ctxt, const string&& str), (override));
   MOCK_METHOD(void, change_alignment, (const context& ctxt), (override));
-  MOCK_METHOD(void, action_open, (const context& ctxt, action_t id), (override));
+  MOCK_METHOD(double, get_x, (const context& ctxt), (const, override));
-  MOCK_METHOD(void, action_close, (const context& ctxt, action_t id), (override));
+  MOCK_METHOD(double, get_alignment_start, (const alignment align), (const, override));
 };
 
 class TestableDispatch : public dispatch {};