From dc462515717a7b7dd4333470d440ac6dd30c8167 Mon Sep 17 00:00:00 2001 From: patrick96 Date: Tue, 22 Feb 2022 17:51:11 +0100 Subject: [PATCH] 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 | 2 + include/tags/action_context.hpp | 18 +++- include/tags/dispatch.hpp | 3 +- src/tags/action_context.cpp | 25 +++++- src/tags/dispatch.cpp | 18 +++- tests/unit_tests/tags/dispatch.cpp | 134 ++++++++++++++++++++++++++++- 6 files changed, 193 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40363f2e..4664eb3e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/include/tags/action_context.hpp b/include/tags/action_context.hpp index 504e4209..533316ed 100644 --- a/include/tags/action_context.hpp +++ b/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_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 get_actions(int x) const; @@ -115,6 +131,6 @@ namespace tags { {alignment::NONE, 0}, {alignment::LEFT, 0}, {alignment::CENTER, 0}, {alignment::RIGHT, 0}}; }; -} // namespace tags +} // namespace tags POLYBAR_NS_END diff --git a/include/tags/dispatch.hpp b/include/tags/dispatch.hpp index 2ceccbaf..3e9453b5 100644 --- a/include/tags/dispatch.hpp +++ b/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: @@ -39,6 +40,6 @@ namespace tags { unique_ptr m_ctxt; action_context& m_action_ctxt; }; -} // namespace tags +} // namespace tags POLYBAR_NS_END diff --git a/src/tags/action_context.cpp b/src/tags/action_context.cpp index d6993080..88479a0d 100644 --- a/src/tags/action_context.cpp +++ b/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) { @@ -104,6 +125,6 @@ namespace tags { const std::vector& action_context::get_blocks() const { return m_action_blocks; } -} // namespace tags +} // namespace tags POLYBAR_NS_END diff --git a/src/tags/dispatch.cpp b/src/tags/dispatch.cpp index 5a12bd8f..0a1a3f43 100644 --- a/src/tags/dispatch.cpp +++ b/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: @@ -146,6 +160,6 @@ namespace tags { } } -} // namespace tags +} // namespace tags POLYBAR_NS_END diff --git a/tests/unit_tests/tags/dispatch.cpp b/tests/unit_tests/tags/dispatch.cpp index b8d4738b..309e4002 100644 --- a/tests/unit_tests/tags/dispatch.cpp +++ b/tests/unit_tests/tags/dispatch.cpp @@ -19,7 +19,7 @@ namespace polybar { inline bool operator==(const extent_val& a, const extent_val& b) { return a.type == b.type && a.value == b.value; } -} // namespace polybar +} // namespace polybar class MockRenderer : public renderer_interface { public: @@ -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); +}