Unit test for model/StrokeStyle and refactor (#4702)

* Put StrokeStyle under unit test
* Use 'const std::string&' instread of 'const char *' for LineStyle::parseStyle
* Extend ObjectStream with read/write vector<T>
* Refactor LineStyle - use of vector for dashes
* Add helper Util::cairo_set_dash_from_vector
* Convert StrokeStyle to a namespace

src/core/control/Control.cpp

@@ -3181,7 +3181,7 @@ void Control::setFill(bool fill) {
 }
 
 void Control::setLineStyle(const string& style) {
-    LineStyle stl = StrokeStyle::parseStyle(style.c_str());
+    LineStyle stl = StrokeStyle::parseStyle(style);
 
     EditSelection* sel = nullptr;
     if (this->win) {

src/core/control/ToolHandler.cpp

@@ -476,7 +476,7 @@ void ToolHandler::loadSettings() {
 
             std::string style;
             if (st.getString("style", style)) {
-                tool->setLineStyle(StrokeStyle::parseStyle(style.c_str()));
+                tool->setLineStyle(StrokeStyle::parseStyle(style));
             }
 
             std::string value;

src/core/control/tools/EditSelection.cpp

@@ -29,6 +29,7 @@
 #include "undo/InsertUndoAction.h"                 // for InsertsUndoAction
 #include "undo/UndoRedoHandler.h"                  // for UndoRedoHandler
 #include "util/Range.h"                            // for Range
+#include "util/Util.h"                             // for cairo_set_dash_from_vector
 #include "util/i18n.h"                             // for _
 #include "util/serializing/ObjectInputStream.h"    // for ObjectInputStream
 #include "util/serializing/ObjectOutputStream.h"   // for ObjectOutputStream
@@ -994,8 +995,8 @@ void EditSelection::paint(cairo_t* cr, double zoom) {
     // set the line always the same size on display
     cairo_set_line_width(cr, 1);
 
-    const double dashes[] = {10.0, 10.0};
-    cairo_set_dash(cr, dashes, sizeof(dashes) / sizeof(dashes[0]), 0);
+    const std::vector<double> dashes = {10.0, 10.0};
+    Util::cairo_set_dash_from_vector(cr, dashes, 0);
     gdk_cairo_set_source_rgba(cr, &selectionColor);
 
     cairo_rectangle(cr, std::min(x, x + width) * zoom, std::min(y, y + height) * zoom, std::abs(width) * zoom,

src/core/model/LineStyle.cpp

@@ -1,6 +1,7 @@
 #include "LineStyle.h"
 
 #include <cstring>  // for memcpy
+#include <vector>   // for vector
 
 #include <glib.h>  // for g_free, g_malloc
 
@@ -10,88 +11,28 @@
 
 LineStyle::LineStyle() = default;
 
-LineStyle::LineStyle(const LineStyle& other) { *this = other; }
+LineStyle::~LineStyle() = default;
 
-LineStyle::~LineStyle() {
-    g_free(this->dashes);
-    this->dashes = nullptr;
-    this->dashCount = 0;
-}
-
-void LineStyle::operator=(const LineStyle& other) {
-    if (this == &other) {
-        return;
-    }
-    const double* dashes = nullptr;
-    int dashCount = 0;
-
-    other.getDashes(dashes, dashCount);
-    setDashes(dashes, dashCount);
-}
-
-#ifndef NDEBUG
-bool LineStyle::operator==(const LineStyle& other) const {
-    return (this->dashes == nullptr && other.dashes == nullptr) ||
-           (this->dashes != nullptr && other.dashes != nullptr &&
-            std::equal(this->dashes, this->dashes + this->dashCount, other.dashes, other.dashes + other.dashCount));
-}
-#endif
+auto LineStyle::operator==(const LineStyle& rhs) const -> bool { return dashes == rhs.dashes; }
 
 void LineStyle::serialize(ObjectOutputStream& out) const {
     out.writeObject("LineStyle");
 
-    out.writeData(this->dashes, this->dashCount, sizeof(double));
+    out.writeData(this->dashes);
 
     out.endObject();
 }
 
 void LineStyle::readSerialized(ObjectInputStream& in) {
     in.readObject("LineStyle");
 
-    g_free(this->dashes);
-    this->dashes = nullptr;
-    this->dashCount = 0;
-    in.readData(reinterpret_cast<void**>(&this->dashes), &this->dashCount);
+    in.readData(dashes);
 
     in.endObject();
 }
 
-/**
- * Get dash array and count
- *
- * @return true if dashed
- */
-auto LineStyle::getDashes(const double*& dashes, int& dashCount) const -> bool {
-    dashes = this->dashes;
-    dashCount = this->dashCount;
-
-    return this->dashCount > 0;
-}
+auto LineStyle::getDashes() const -> const std::vector<double>& { return dashes; }
 
-/**
- * Set the dash array and count
- *
- * @param dashes Dash data, will be copied
- * @param dashCount Count of entries
- */
-// Todo(fabian): memmory use after free
-void LineStyle::setDashes(const double* dashes, int dashCount) {
-    g_free(this->dashes);
-    if (dashCount == 0 || dashes == nullptr) {
-        this->dashCount = 0;
-        this->dashes = nullptr;
-        return;
-    }
-
-    this->dashes = static_cast<double*>(g_malloc(dashCount * sizeof(double)));
-    this->dashCount = dashCount;
-
-    memcpy(this->dashes, dashes, this->dashCount * sizeof(double));
-}
+void LineStyle::setDashes(std::vector<double>&& dashes) { this->dashes = std::move(dashes); }
 
-/**
- * Get dash array and count
- *
- * @return true if dashed
- */
-auto LineStyle::hasDashes() const -> bool { return this->dashCount > 0; }
+auto LineStyle::hasDashes() const -> bool { return !dashes.empty(); }

src/core/model/LineStyle.h

@@ -11,6 +11,8 @@
 
 #pragma once
 
+#include <vector>
+
 #include "util/serializing/Serializable.h"  // for Serializable
 
 class ObjectInputStream;
@@ -20,49 +22,34 @@ class ObjectOutputStream;
 class LineStyle: public Serializable {
 public:
     LineStyle();
-    LineStyle(const LineStyle& other);
     ~LineStyle() override;
 
-    void operator=(const LineStyle& other);
-#ifndef NDEBUG
-    // Only use in assert() for now
     bool operator==(const LineStyle& other) const;
-#endif
 
 public:
     // Serialize interface
     void serialize(ObjectOutputStream& out) const override;
     void readSerialized(ObjectInputStream& in) override;
 
-public:
     /**
-     * Get dash array and count
+     * Get dash vector
      *
-     * @return true if dashed
+     * @return dashes
      */
-    bool getDashes(const double*& dashes, int& dashCount) const;
+    const std::vector<double>& getDashes() const;
 
     /**
      * @return true if dashed
      */
     bool hasDashes() const;
 
     /**
-     * Set the dash array and count
+     * Set the dash vector and count
      *
-     * @param dashes Dash data, will be copied
-     * @param dashCount Count of entries
+     * @param dashes Dash data, will be moved, and continuous use from caller invalid
      */
-    void setDashes(const double* dashes, int dashCount);
+    void setDashes(std::vector<double>&& dashes);
 
 private:
-    /**
-     * Dash definition (nullptr for no Dash)
-     */
-    double* dashes = nullptr;
-
-    /**
-     * Dash count (0 for no dash)
-     */
-    int dashCount = 0;
+    std::vector<double> dashes;
 };

src/core/model/StrokeStyle.cpp

@@ -1,91 +1,76 @@
 #include "StrokeStyle.h"
 
-#include <cstring>  // for memcmp, strcmp, strncmp
-#include <vector>   // for vector
-
-#include <glib.h>  // for g_ascii_strtod, g_free, g_strdup_printf
+#include <cstring>   // for memcmp, strcmp, strncmp
+#include <iomanip>   // for setprecision
+#include <iterator>  // for ostream_iterator
+#include <map>       // for map
+#include <sstream>   // for istringstream
+#include <vector>    // for vector
 
 #include "model/LineStyle.h"  // for LineStyle
 
-StrokeStyle::StrokeStyle() = default;
+namespace {
 
-StrokeStyle::~StrokeStyle() = default;
+constexpr auto CUSTOM_KEY = "cust: ";
 
-const double dashLinePattern[] = {6, 3};
-const double dashDotLinePattern[] = {6, 3, 0.5, 3};
-const double dotLinePattern[] = {0.5, 3};
+const std::map<std::string, std::vector<double>> predefinedPatterns = {
+        {"dash", {6, 3}}, {"dashdot", {6, 3, 0.5, 3}}, {"dot", {0.5, 3}}};
 
-#define PARSE_STYLE(name, def)                                \
-    if (strcmp(style, name) == 0) {                           \
-        LineStyle style;                                      \
-        style.setDashes(def, sizeof(def) / sizeof((def)[0])); \
-        return style;                                         \
-    }
+auto formatStyle(const std::vector<double>& dashes) -> std::string {
 
-auto StrokeStyle::parseStyle(const char* style) -> LineStyle {
-    PARSE_STYLE("dash", dashLinePattern);
-    PARSE_STYLE("dashdot", dashDotLinePattern);
-    PARSE_STYLE("dot", dotLinePattern);
+    // Check if dashes match named predefined dashes.
+    for (auto& pair: predefinedPatterns) {
+        if (pair.second == dashes) {
+            return pair.first;
+        }
+    }
 
+    // Else generate custom dashes string
+    std::ostringstream custom;
+    custom << std::setprecision(2) << std::fixed;
+    custom << CUSTOM_KEY;
+    std::copy(dashes.begin(), dashes.end(), std::ostream_iterator<double>(custom, " "));
 
-    if (strncmp("cust: ", style, 6) != 0) {
-        return LineStyle();
-    }
+    // Return dashes string with traling space removed.
+    return custom.str().substr(0, custom.str().length() - 1);
+}
 
-    std::vector<double> dash;
+}  // namespace
 
-    const char* widths = style + 6;
-    while (*widths != 0) {
-        char* tmpptr = nullptr;
-        double val = g_ascii_strtod(widths, &tmpptr);
-        if (tmpptr == widths) {
-            break;
-        }
-        widths = tmpptr;
-        dash.push_back(val);
+auto StrokeStyle::parseStyle(const std::string& style) -> LineStyle {
+    auto it = predefinedPatterns.find(style);
+    if (it != predefinedPatterns.end()) {
+        LineStyle ls;
+        std::vector<double> dashes = it->second;
+        ls.setDashes(std::move(dashes));
+        return ls;
     }
 
-    if (dash.empty()) {
+    if (style.substr(0, strlen(CUSTOM_KEY)) != CUSTOM_KEY) {
         return LineStyle();
     }
 
-    auto* dashesArr = new double[dash.size()];
-    for (int i = 0; i < static_cast<int>(dash.size()); i++) { dashesArr[i] = dash[i]; }
-
-    LineStyle ls;
-    ls.setDashes(dashesArr, static_cast<int>(dash.size()));
-    delete[] dashesArr;
+    std::stringstream dashStream(style);
+    std::vector<double> dashes;
 
-    return ls;
-}
-
-#define FORMAT_STYLE(name, def)                                                                            \
-    if (count == (sizeof(def) / sizeof((def)[0])) && memcmp(dashes, def, count * sizeof((def)[0])) == 0) { \
-        return name;                                                                                       \
+    dashStream.seekg(strlen(CUSTOM_KEY));
+    for (double value; dashStream >> value;) {
+        dashes.push_back(value);
     }
 
-auto StrokeStyle::formatStyle(const double* dashes, int count) -> std::string {
-    FORMAT_STYLE("dash", dashLinePattern);
-    FORMAT_STYLE("dashdot", dashDotLinePattern);
-    FORMAT_STYLE("dot", dotLinePattern);
-
-    std::string custom = "cust:";
-
-    for (int i = 0; i < count; i++) {
-        custom += " ";
-        char* str = g_strdup_printf("%0.2lf", dashes[i]);
-        custom += str;
-        g_free(str);
+    if (dashes.empty()) {
+        return LineStyle();
     }
 
-    return custom;
+    LineStyle ls;
+    ls.setDashes(std::move(dashes));
+    return ls;
 }
 
 auto StrokeStyle::formatStyle(const LineStyle& style) -> std::string {
-    const double* dashes = nullptr;
-    int dashCount = 0;
-    if (style.getDashes(dashes, dashCount)) {
-        return StrokeStyle::formatStyle(dashes, dashCount);
+    const auto& dashes = style.getDashes();
+    if (!dashes.empty()) {
+        return ::formatStyle(dashes);
     }
 
     // Should not be returned, in this case the attribute is not written

src/core/model/StrokeStyle.h

@@ -15,15 +15,16 @@
 
 #include "LineStyle.h"  // for LineStyle
 
-class StrokeStyle {
-private:
-    StrokeStyle();
-    virtual ~StrokeStyle();
-
-public:
-    static LineStyle parseStyle(const char* style);
-    static std::string formatStyle(const double* dashes, int count);
-    static std::string formatStyle(const LineStyle& style);
-
-public:
-};
+namespace StrokeStyle {
+/** Parse LineStyle from string.
+ *
+ * @return LineStyle deserialized from string
+ */
+LineStyle parseStyle(const std::string& style);
+/** Convert a LineStyle to a string.
+ *
+ * @param style to be serialized
+ * @return string containing serialized version of LineStyle
+ */
+std::string formatStyle(const LineStyle& style);
+}  // namespace StrokeStyle