From fb6e874235466a59be66bbad685f4c58ae8bb1b7 Mon Sep 17 00:00:00 2001
From: infokiller <infokiller@users.noreply.github.com>
Date: Sun, 1 Dec 2019 02:28:41 +0200
Subject: [PATCH] feat(label): Add minlen with alignment (#1546)

* Add label minlen and alignment.

Fix build

* Update src/drawtypes/label.cpp

Co-Authored-By: infokiller <infokiller@users.noreply.github.com>

* Use existing alignment type.

* Remove redundant max_len handling in label::get.

* Fix shadowing.

* Add label alignment tests.

* Handle minlen/maxlen and alignment in same function.

Also add a test for a test case brought up in the PR discussion.

* Format files with clang-format

* Move builder::get_label_text tests into label tests

builder::get_label_text doesn't really do anything anymore

* builder: remove get_label_text

* label: Clean up label::get()

* Fix comment style.

* Set default label alignment to left.

* Update src/drawtypes/label.cpp

Co-Authored-By: Patrick Ziegler <p.ziegler96@gmail.com>

* Update include/drawtypes/label.hpp

Co-Authored-By: Patrick Ziegler <p.ziegler96@gmail.com>
---
 include/components/builder.hpp          |   2 -
 include/drawtypes/label.hpp             |  19 +++--
 src/components/builder.cpp              |  21 +----
 src/drawtypes/label.cpp                 |  70 +++++++++++++--
 tests/CMakeLists.txt                    |   2 +-
 tests/unit_tests/components/builder.cpp |  68 ---------------
 tests/unit_tests/drawtypes/label.cpp    | 109 ++++++++++++++++++++++++
 7 files changed, 186 insertions(+), 105 deletions(-)
 delete mode 100644 tests/unit_tests/components/builder.cpp
 create mode 100644 tests/unit_tests/drawtypes/label.cpp

diff --git a/include/components/builder.hpp b/include/components/builder.hpp
index a14d55f0..38c7e69e 100644
--- a/include/components/builder.hpp
+++ b/include/components/builder.hpp
@@ -55,8 +55,6 @@ class builder {
   string background_hex();
   string foreground_hex();
 
-  string get_label_text(const label_t& label);
-
   void tag_open(syntaxtag tag, const string& value);
   void tag_open(attribute attr);
   void tag_close(syntaxtag tag);
diff --git a/include/drawtypes/label.hpp b/include/drawtypes/label.hpp
index 5859b716..86b2173f 100644
--- a/include/drawtypes/label.hpp
+++ b/include/drawtypes/label.hpp
@@ -25,9 +25,10 @@ namespace drawtypes {
     string m_underline{};
     string m_overline{};
     int m_font{0};
-    side_values m_padding{0U,0U};
-    side_values m_margin{0U,0U};
+    side_values m_padding{0U, 0U};
+    side_values m_margin{0U, 0U};
 
+    size_t m_minlen{0};
     /*
      * If m_ellipsis is true, m_maxlen MUST be larger or equal to the length of
      * the ellipsis (3), everything else is a programming error
@@ -36,12 +37,14 @@ namespace drawtypes {
      * labels in a different way.
      */
     size_t m_maxlen{0_z};
+    alignment m_alignment{alignment::LEFT};
     bool m_ellipsis{true};
 
     explicit label(string text, int font) : m_font(font), m_text(text), m_tokenized(m_text) {}
     explicit label(string text, string foreground = ""s, string background = ""s, string underline = ""s,
-        string overline = ""s, int font = 0, struct side_values padding = {0U,0U}, struct side_values margin = {0U,0U},
-        size_t maxlen = 0_z, bool ellipsis = true, vector<token>&& tokens = {})
+        string overline = ""s, int font = 0, struct side_values padding = {0U, 0U},
+        struct side_values margin = {0U, 0U}, int minlen = 0, size_t maxlen = 0_z,
+        alignment label_alignment = alignment::LEFT, bool ellipsis = true, vector<token>&& tokens = {})
         : m_foreground(foreground)
         , m_background(background)
         , m_underline(underline)
@@ -49,13 +52,15 @@ namespace drawtypes {
         , m_font(font)
         , m_padding(padding)
         , m_margin(margin)
+        , m_minlen(minlen)
         , m_maxlen(maxlen)
+        , m_alignment(label_alignment)
         , m_ellipsis(ellipsis)
         , m_text(text)
         , m_tokenized(m_text)
         , m_tokens(forward<vector<token>>(tokens)) {
-          assert(!m_ellipsis || (m_maxlen == 0 || m_maxlen >= 3));
-        }
+      assert(!m_ellipsis || (m_maxlen == 0 || m_maxlen >= 3));
+    }
 
     string get() const;
     operator bool();
@@ -76,6 +81,6 @@ namespace drawtypes {
 
   label_t load_label(const config& conf, const string& section, string name, bool required = true, string def = ""s);
   label_t load_optional_label(const config& conf, string section, string name, string def = ""s);
-}
+}  // namespace drawtypes
 
 POLYBAR_NS_END
diff --git a/src/components/builder.cpp b/src/components/builder.cpp
index 8d220cc3..56e20d44 100644
--- a/src/components/builder.cpp
+++ b/src/components/builder.cpp
@@ -1,6 +1,7 @@
+#include "components/builder.hpp"
+
 #include <utility>
 
-#include "components/builder.hpp"
 #include "drawtypes/label.hpp"
 #include "utils/color.hpp"
 #include "utils/string.hpp"
@@ -124,7 +125,7 @@ void builder::node(const label_t& label, bool add_space) {
     return;
   }
 
-  auto text = get_label_text(label);
+  auto text = label->get();
 
   if (label->m_margin.left > 0) {
     space(label->m_margin.left);
@@ -479,22 +480,6 @@ string builder::foreground_hex() {
   return m_foreground;
 }
 
-string builder::get_label_text(const label_t& label) {
-  string text{label->get()};
-
-  size_t maxlen = label->m_maxlen;
-
-  if (maxlen > 0 && string_util::char_len(text) > maxlen) {
-    if (label->m_ellipsis) {
-      text = string_util::utf8_truncate(std::move(text), maxlen - 3) + "...";
-    } else {
-      text = string_util::utf8_truncate(std::move(text), maxlen);
-    }
-  }
-
-  return text;
-}
-
 /**
  * Insert directive to change value of given tag
  */
diff --git a/src/drawtypes/label.cpp b/src/drawtypes/label.cpp
index 1fee8725..983c6289 100644
--- a/src/drawtypes/label.cpp
+++ b/src/drawtypes/label.cpp
@@ -1,14 +1,49 @@
+#include "drawtypes/label.hpp"
+
+#include <cmath>
 #include <utility>
 
-#include "drawtypes/label.hpp"
 #include "utils/factory.hpp"
 #include "utils/string.hpp"
 
 POLYBAR_NS
 
 namespace drawtypes {
+  /**
+   * Gets the text from the label as it should be rendered
+   *
+   * Here tokens are replaced with values and minlen and maxlen properties are applied
+   */
   string label::get() const {
-    return m_tokenized;
+    const size_t len = string_util::char_len(m_tokenized);
+    if (len >= m_minlen) {
+      string text = m_tokenized;
+      if (m_maxlen > 0 && len > m_maxlen) {
+        if (m_ellipsis) {
+          text = string_util::utf8_truncate(std::move(text), m_maxlen - 3) + "...";
+        } else {
+          text = string_util::utf8_truncate(std::move(text), m_maxlen);
+        }
+      }
+      return text;
+    }
+
+    const size_t num_fill_chars = m_minlen - len;
+    size_t right_fill_len = 0;
+    size_t left_fill_len = 0;
+    if (m_alignment == alignment::RIGHT) {
+      left_fill_len = num_fill_chars;
+    } else if (m_alignment == alignment::LEFT) {
+      right_fill_len = num_fill_chars;
+    } else {
+      right_fill_len = std::ceil(num_fill_chars / 2.0);
+      left_fill_len = right_fill_len;
+      // The text is positioned one character to the left if we can't perfectly center it
+      if (len + left_fill_len + right_fill_len > m_minlen) {
+        --left_fill_len;
+      }
+    }
+    return string(left_fill_len, ' ') + m_tokenized + string(right_fill_len, ' ');
   }
 
   label::operator bool() {
@@ -22,7 +57,7 @@ namespace drawtypes {
       std::copy(m_tokens.begin(), m_tokens.end(), back_it);
     }
     return factory_util::shared<label>(m_text, m_foreground, m_background, m_underline, m_overline, m_font, m_padding,
-        m_margin, m_maxlen, m_ellipsis, move(tokens));
+        m_margin, m_minlen, m_maxlen, m_alignment, m_ellipsis, move(tokens));
   }
 
   void label::clear() {
@@ -216,15 +251,30 @@ namespace drawtypes {
         token.suffix = token_str.substr(pos + 1, token_str.size() - pos - 2);
       }
     }
+    size_t minlen = conf.get(section, name + "-minlen", 0_z);
+    string alignment_conf_value = conf.get(section, name + "-alignment", "left"s);
+    alignment label_alignment;
+    if (alignment_conf_value == "right") {
+      label_alignment = alignment::RIGHT;
+    } else if (alignment_conf_value == "left") {
+      label_alignment = alignment::LEFT;
+    } else if (alignment_conf_value == "center") {
+      label_alignment = alignment::CENTER;
+    } else {
+      throw application_error(sstream() << "Label " << section << "." << name << " has invalid alignment "
+                                        << alignment_conf_value << ", expecting one of: right, left, center.");
+    }
 
     size_t maxlen = conf.get(section, name + "-maxlen", 0_z);
+    if (maxlen > 0 && maxlen < minlen) {
+      throw application_error(sstream() << "Label " << section << "." << name << " has maxlen " << maxlen
+                                        << " which is smaller than minlen " << minlen);
+    }
     bool ellipsis = conf.get(section, name + "-ellipsis", true);
 
-    if(ellipsis && maxlen > 0 && maxlen < 3) {
-      throw application_error(sstream()
-          << "Label " << section << "." << name
-          << " has maxlen " << maxlen
-          << ", which is smaller than length of ellipsis (3)");
+    if (ellipsis && maxlen > 0 && maxlen < 3) {
+      throw application_error(sstream() << "Label " << section << "." << name << " has maxlen " << maxlen
+                                        << ", which is smaller than length of ellipsis (3)");
     }
 
     // clang-format off
@@ -236,7 +286,9 @@ namespace drawtypes {
         conf.get(section, name + "-font", 0),
         padding,
         margin,
+        minlen,
         maxlen,
+        label_alignment,
         ellipsis,
         move(tokens));
     // clang-format on
@@ -249,6 +301,6 @@ namespace drawtypes {
     return load_label(conf, move(section), move(name), false, move(def));
   }
 
-}
+}  // namespace drawtypes
 
 POLYBAR_NS_END
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index a09d2907..33c9c26b 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -55,9 +55,9 @@ add_unit_test(utils/string unit_tests)
 add_unit_test(utils/file)
 add_unit_test(components/command_line)
 add_unit_test(components/bar)
-add_unit_test(components/builder)
 add_unit_test(components/parser)
 add_unit_test(components/config_parser)
+add_unit_test(drawtypes/label)
 
 # Run make check to build and run all unit tests
 add_custom_target(check
diff --git a/tests/unit_tests/components/builder.cpp b/tests/unit_tests/components/builder.cpp
deleted file mode 100644
index f23bfa14..00000000
--- a/tests/unit_tests/components/builder.cpp
+++ /dev/null
@@ -1,68 +0,0 @@
-#include <tuple>
-
-#include "common/test.hpp"
-#include "components/builder.hpp"
-#include "components/types.hpp"
-#include "utils/factory.hpp"
-#include "drawtypes/label.hpp"
-
-using namespace polybar;
-using namespace std;
-
-/**
- * \brief Testing-only subclass of builder to change access level
- */
-class TestableBuilder : public builder {
-  using builder::builder;
-  public: using builder::get_label_text;
-};
-
-class Builder : public ::testing::Test {
-  protected:
-
-    /**
-     * Generic bar settings
-     *
-     * Builder only needs spacing and background
-     */
-    bar_settings m_bar{};
-    TestableBuilder m_builder{m_bar};
-};
-
-// GetLabelTextTest {{{
-
-/**
- * \brief Class for parameterized tests on get_label_text
- *
- * The first element of the pair is the expected returned text, the second
- * element is a triple containing the original label text, m_ellipsis and
- * m_maxlen, in that order
- */
-class GetLabelTextTest :
-  public Builder,
-  public ::testing::WithParamInterface<pair<string, tuple<string, bool, int>>> {};
-
-vector<pair<string, tuple<string, bool, int>>> get_label_text_list = {
-  {"...", make_tuple("abcd", true, 3)},
-  {"abc", make_tuple("abc", true, 3)},
-  {"abc", make_tuple("abcdefgh", false, 3)},
-  {"a...", make_tuple("abcdefgh", true, 4)},
-  {"abcd...", make_tuple("abcdefgh", true, 7)},
-  {"abcdefgh", make_tuple("abcdefgh", true, 8)},
-};
-
-INSTANTIATE_TEST_SUITE_P(Inst, GetLabelTextTest,
-    ::testing::ValuesIn(get_label_text_list));
-
-TEST_P(GetLabelTextTest, correctness) {
-  label_t m_label = factory_util::shared<label>(get<0>(GetParam().second));
-  m_label->m_ellipsis = get<1>(GetParam().second);
-  m_label->m_maxlen = get<2>(GetParam().second);
-
-  auto text = m_builder.get_label_text(m_label);
-  EXPECT_EQ(GetParam().first, text);
-
-  EXPECT_LE(text.length(), m_label->m_maxlen) << "Returned text is longer than maxlen";
-}
-
-// }}}
diff --git a/tests/unit_tests/drawtypes/label.cpp b/tests/unit_tests/drawtypes/label.cpp
new file mode 100644
index 00000000..edc053dc
--- /dev/null
+++ b/tests/unit_tests/drawtypes/label.cpp
@@ -0,0 +1,109 @@
+#include "drawtypes/label.hpp"
+
+#include <memory>
+#include <tuple>
+
+#include "common.hpp"
+#include "common/test.hpp"
+#include "components/types.hpp"
+
+using namespace polybar;
+using namespace std;
+using namespace polybar::drawtypes;
+
+using GetTestParamLabel = tuple<string, bool, int, int, alignment>;
+using GetTestParam = pair<string, GetTestParamLabel>;
+/**
+ * \brief Class for parameterized tests label::get
+ *
+ * The first element of the pair is the expected returned text, the second
+ * element is a 5-tuple (text, ellipsis, minlen, maxlen, alignment)
+ */
+class GetTest : public ::testing::Test, public ::testing::WithParamInterface<GetTestParam> {};
+
+vector<GetTestParam> get_list = {
+    {"...", make_tuple("abcd", true, 0, 3, alignment::RIGHT)},
+    {"abc", make_tuple("abc", true, 0, 3, alignment::RIGHT)},
+    {"abc", make_tuple("abcdefgh", false, 0, 3, alignment::RIGHT)},
+    {"a...", make_tuple("abcdefgh", true, 0, 4, alignment::RIGHT)},
+    {"abcd...", make_tuple("abcdefgh", true, 0, 7, alignment::RIGHT)},
+    {"abcdefgh", make_tuple("abcdefgh", true, 0, 8, alignment::RIGHT)},
+};
+INSTANTIATE_TEST_SUITE_P(Inst, GetTest, ::testing::ValuesIn(get_list));
+
+// No alignment needed
+vector<GetTestParam> get_no_align_list = {
+    {"abc", make_tuple("abc", true, 3, 0, alignment::LEFT)},
+    {"abc", make_tuple("abc", true, 2, 0, alignment::CENTER)},
+    {"abc", make_tuple("abc", true, 1, 0, alignment::RIGHT)},
+};
+
+INSTANTIATE_TEST_SUITE_P(NoAlignment, GetTest, ::testing::ValuesIn(get_no_align_list));
+
+// Left alignment
+vector<GetTestParam> get_left_align_list = {
+    {"a ", make_tuple("a", true, 2, 0, alignment::LEFT)},
+    {"a  ", make_tuple("a", true, 3, 0, alignment::LEFT)},
+    {"abcde     ", make_tuple("abcde", true, 10, 0, alignment::LEFT)},
+};
+
+INSTANTIATE_TEST_SUITE_P(LeftAlignment, GetTest, ::testing::ValuesIn(get_left_align_list));
+
+// Center alignment
+vector<GetTestParam> get_center_align_list = {
+    {" a ", make_tuple("a", true, 3, 0, alignment::CENTER)},
+    {"  a  ", make_tuple("a", true, 5, 0, alignment::CENTER)},
+    {"   abcd   ", make_tuple("abcd", true, 10, 0, alignment::CENTER)},
+};
+
+INSTANTIATE_TEST_SUITE_P(CenterAlignment, GetTest, ::testing::ValuesIn(get_center_align_list));
+
+// Right alignment
+vector<GetTestParam> get_right_align_list = {
+    {"  a", make_tuple("a", true, 3, 0, alignment::RIGHT)},
+    {" abc", make_tuple("abc", true, 4, 0, alignment::RIGHT)},
+    {"       abc", make_tuple("abc", true, 10, 0, alignment::RIGHT)},
+};
+
+INSTANTIATE_TEST_SUITE_P(RightAlignment, GetTest, ::testing::ValuesIn(get_right_align_list));
+
+vector<GetTestParam> get_min_max_list = {
+    {"a ", make_tuple("a", true, 2, 2, alignment::CENTER)},
+    {"abc ", make_tuple("abc", true, 4, 4, alignment::CENTER)},
+    {"abc", make_tuple("abcd", false, 3, 3, alignment::RIGHT)},
+    {"...", make_tuple("abcd", true, 1, 3, alignment::RIGHT)},
+    {" ", make_tuple("", true, 1, 3, alignment::RIGHT)},
+    {" a", make_tuple("a", true, 2, 3, alignment::RIGHT)},
+    {"...", make_tuple("....", true, 2, 3, alignment::RIGHT)},
+    {"...", make_tuple("....", false, 2, 3, alignment::RIGHT)},
+    {"abc...", make_tuple("abcdefg", true, 6, 6, alignment::RIGHT)},
+};
+
+INSTANTIATE_TEST_SUITE_P(MinMax, GetTest, ::testing::ValuesIn(get_min_max_list));
+
+unique_ptr<label> create_alignment_test_label(GetTestParamLabel params) {
+  /* It's simpler in this case to create a label with all the constructor defaults and then set the relevant
+   * fields individually.
+   */
+  auto test_label = make_unique<label>(get<0>(params));
+  test_label->m_ellipsis = get<1>(params);
+  test_label->m_minlen = get<2>(params);
+  test_label->m_maxlen = get<3>(params);
+  test_label->m_alignment = get<4>(params);
+  return test_label;
+}
+
+TEST_P(GetTest, correctness) {
+  auto m_label = create_alignment_test_label(GetParam().second);
+
+  auto expected = GetParam().first;
+  auto actual = m_label->get();
+  EXPECT_EQ(expected, actual);
+}
+
+TEST_P(GetTest, soundness) {
+  auto m_label = create_alignment_test_label(GetParam().second);
+  auto actual = m_label->get();
+  EXPECT_TRUE(m_label->m_maxlen == 0 || actual.length() <= m_label->m_maxlen) << "Returned text is longer than maxlen";
+  EXPECT_GE(actual.length(), m_label->m_minlen) << "Returned text is shorter than minlen";
+}