fix: Handling for actions with negative offsets

If any action block contains a negative offset, it can cause text to be
theoretically be rendered outside of the block, making that text not
clickable.
To fix this, we ensure that an action block starts at the lowest
observed position while the block is open and ends at the highest
observed position while it is open.

Fixes #1814

CHANGELOG.md

@@ -224,6 +224,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Some modules stop updating when system time moves backwards. ([`#857`](https://github.com/polybar/polybar/issues/857), [`#1932`](https://github.com/polybar/polybar/issues/1932))
 - Broken positioning in Openbox when the bar is hidden and shown again
   ([`#2021`](https://github.com/polybar/polybar/issues/2021))
+- Handling of action blocks that contain negative offsets
+  ([`#1814`](https://github.com/polybar/polybar/issues/1814))
 
 ## [3.5.7] - 2021-09-21
 ### Fixed

include/tags/action_context.hpp

@@ -40,6 +40,8 @@ namespace tags {
 
     /**
      * End position of the action block (exclusive), relative to the alignment.
+     *
+     * May be incrementally updated while the action block is still open
      */
     double end_x{0};
     mousebtn button;
@@ -80,6 +82,20 @@ namespace tags {
     action_t action_open(mousebtn btn, const string&& cmd, alignment align, double x);
     std::pair<action_t, mousebtn> action_close(mousebtn btn, alignment align, double x);
 
+    /**
+     * Compensate for the current position moving backwards in the renderer.
+     *
+     * This can happen if negative offsets are used.
+     *
+     * The policy is that an action block's start is the smallest x position observed while the block is open.
+     * And its end is the largest x position observed while it is open.
+     *
+     * @param a The current alignment
+     * @param old_x The x position before the backwards move
+     * @param new_x The x position after the backwards move new_x < old_x
+     */
+    void compensate_for_negative_move(alignment a, double old_x, double new_x);
+
     void set_alignment_start(const alignment a, const double x);
 
     std::map<mousebtn, tags::action_t> get_actions(int x) const;

include/tags/dispatch.hpp

@@ -31,6 +31,7 @@ namespace tags {
    protected:
     void handle_text(renderer_interface& renderer, string&& data);
     void handle_action(renderer_interface& renderer, mousebtn btn, bool closing, const string&& cmd);
+    void handle_offset(renderer_interface& renderer, extent_val offset);
     void handle_control(controltag ctrl);
 
    private:

src/tags/action_context.cpp

@@ -37,7 +37,28 @@ namespace tags {
   }
 
   void action_context::set_end(action_t id, double x) {
-    m_action_blocks[id].end_x = x;
+    /*
+     * Only ever increase the end position.
+     * A larger end position may have been set before.
+     */
+    m_action_blocks[id].end_x = std::max(m_action_blocks[id].end_x, x);
+  }
+
+  void action_context::compensate_for_negative_move(alignment a, double old_x, double new_x) {
+    assert(new_x < old_x);
+    for (auto& block : m_action_blocks) {
+      if (block.is_open && block.align == a) {
+        // Move back the start position if a smaller position is observed
+        if (block.start_x > new_x) {
+          block.start_x = new_x;
+        }
+
+        // Move forward the end position if a larger position is observed
+        if (old_x > block.end_x) {
+          block.end_x = old_x;
+        }
+      }
+    }
   }
 
   void action_context::set_alignment_start(const alignment a, const double x) {

src/tags/dispatch.cpp

@@ -42,6 +42,9 @@ namespace tags {
         continue;
       }
 
+      alignment old_alignment = m_ctxt->get_alignment();
+      double old_x = old_alignment == alignment::NONE ? 0 : renderer.get_x(*m_ctxt);
+
       if (el.is_tag) {
         switch (el.tag_data.type) {
           case tags::tag_type::FORMAT:
@@ -59,7 +62,7 @@ namespace tags {
                 m_ctxt->apply_font(el.tag_data.font);
                 break;
               case tags::syntaxtag::O:
-                renderer.render_offset(*m_ctxt, el.tag_data.offset);
+                handle_offset(renderer, el.tag_data.offset);
                 break;
               case tags::syntaxtag::R:
                 m_ctxt->apply_reverse();
@@ -97,6 +100,13 @@ namespace tags {
       } else {
         handle_text(renderer, std::move(el.data));
       }
+
+      if (old_alignment == m_ctxt->get_alignment()) {
+        double new_x = renderer.get_x(*m_ctxt);
+        if (new_x < old_x) {
+          m_action_ctxt.compensate_for_negative_move(old_alignment, old_x, new_x);
+        }
+      }
     }
 
     /*
@@ -136,6 +146,10 @@ namespace tags {
     }
   }
 
+  void dispatch::handle_offset(renderer_interface& renderer, extent_val offset) {
+    renderer.render_offset(*m_ctxt, offset);
+  }
+
   void dispatch::handle_control(controltag ctrl) {
     switch (ctrl) {
       case controltag::R:

tests/unit_tests/tags/dispatch.cpp

@@ -135,7 +135,19 @@ TEST_F(DispatchTest, unclosedActions) {
 TEST_F(DispatchTest, actions) {
   {
     InSequence seq;
+    // Called for 'foo'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
     EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    // Called for '%{A1:cmd:}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    // Called for 'bar'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(6));
+    // Called for '%{A}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(6));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(6));
     EXPECT_CALL(r, get_x(_)).WillOnce(Return(6));
   }
 
@@ -154,3 +166,123 @@ TEST_F(DispatchTest, actions) {
   EXPECT_EQ(mousebtn::LEFT, blk.button);
   EXPECT_EQ("cmd", blk.cmd);
 }
+
+TEST_F(DispatchTest, actionOffsetStart) {
+  {
+    InSequence seq;
+    // Called for 'a'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{A1:cmd:}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{O-1}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
+    // Called for 'bar'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    // Called for '%{A}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(3));
+  }
+
+  bar_settings settings;
+  m_dispatch->parse(settings, r, "%{l}a%{A1:cmd:}%{O-1}bar%{A}");
+
+  const auto& actions = m_action_ctxt->get_blocks();
+
+  ASSERT_EQ(1, actions.size());
+
+  const auto& blk = actions[0];
+
+  EXPECT_EQ(0, blk.start_x);
+  EXPECT_EQ(3, blk.end_x);
+  EXPECT_EQ(alignment::LEFT, blk.align);
+  EXPECT_EQ(mousebtn::LEFT, blk.button);
+  EXPECT_EQ("cmd", blk.cmd);
+}
+
+TEST_F(DispatchTest, actionOffsetEnd) {
+  {
+    InSequence seq;
+    // Called for 'a'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{A1:cmd:}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for 'bar'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(4));
+    // Called for '%{O-3}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(4));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{A}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+  }
+
+  bar_settings settings;
+  m_dispatch->parse(settings, r, "%{l}a%{A1:cmd:}bar%{O-3}%{A}");
+
+  const auto& actions = m_action_ctxt->get_blocks();
+
+  ASSERT_EQ(1, actions.size());
+
+  const auto& blk = actions[0];
+
+  EXPECT_EQ(1, blk.start_x);
+  EXPECT_EQ(4, blk.end_x);
+  EXPECT_EQ(alignment::LEFT, blk.align);
+  EXPECT_EQ(mousebtn::LEFT, blk.button);
+  EXPECT_EQ("cmd", blk.cmd);
+}
+
+TEST_F(DispatchTest, actionOffsetBefore) {
+  {
+    InSequence seq;
+    // Called for 'a'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(0));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{O100}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(101));
+    // Called for '%{O-100}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(101));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{A1:cmd:}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for 'bar'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(4));
+    // Called for '%{O-3}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(4));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    // Called for '%{A}'
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+    EXPECT_CALL(r, get_x(_)).WillOnce(Return(1));
+  }
+
+  bar_settings settings;
+  m_dispatch->parse(settings, r, "%{l}%{O100 O-100}a%{A1:cmd:}bar%{O-3}%{A}");
+
+  const auto& actions = m_action_ctxt->get_blocks();
+
+  ASSERT_EQ(1, actions.size());
+
+  const auto& blk = actions[0];
+
+  EXPECT_EQ(1, blk.start_x);
+  EXPECT_EQ(4, blk.end_x);
+  EXPECT_EQ(alignment::LEFT, blk.align);
+  EXPECT_EQ(mousebtn::LEFT, blk.button);
+  EXPECT_EQ("cmd", blk.cmd);
+}