Make parameter string generation a job of Inkscape::Extension

For this take a first step towards properly tracking children
of each parameter and provide functions to look up all widgets.

7 files changed, 77 insertions, 94 deletions

diff --git a/src/extension/extension.cpp b/src/extension/extension.cpp
index c713a83ee..1da909594 100644
--- a/src/extension/extension.cpp
+++ b/src/extension/extension.cpp
@@ -766,10 +766,27 @@ Extension::autogui (SPDocument *doc, Inkscape::XML::Node *node, sigc::signal<voi
 void
 Extension::paramListString (std::list <std::string> &retlist)
 {
+    // first collect all widgets in the current extension
+    std::vector<const InxWidget *> widget_list;
     for (auto widget : _widgets) {
-        InxParameter *parameter = dynamic_cast<InxParameter *>(widget); // filter InxParameters from InxWidgets
+        widget->get_widgets(widget_list);
+    }
+
+    // then build a list of parameter strings from parameter names and values, as '--name=value'
+    for (auto widget : widget_list) {
+        const InxParameter *parameter = dynamic_cast<const InxParameter *>(widget); // filter InxParameters from InxWidgets
         if (parameter) {
-            parameter->build_param_string_list(retlist);
+            const char *name = parameter->name();
+            std::string value = parameter->value_to_string();
+
+            if (name && !value.empty()) { // TODO: Shouldn't empty string values be allowed?
+                std::string parameter_string;
+                parameter_string += "--";
+                parameter_string += name;
+                parameter_string += "=";
+                parameter_string += value;
+                retlist.push_back(parameter_string);
+            }
         }
     }
 
diff --git a/src/extension/prefdialog/parameter-notebook.cpp b/src/extension/prefdialog/parameter-notebook.cpp
index 50c21b3eb..8566a6dfd 100644
--- a/src/extension/prefdialog/parameter-notebook.cpp
+++ b/src/extension/prefdialog/parameter-notebook.cpp
@@ -46,7 +46,7 @@ ParamNotebook::ParamNotebookPage::ParamNotebookPage(Inkscape::XML::Node *xml, In
             if (!strcmp(chname, "param") || !strcmp(chname, "_param")) {
                 InxParameter *param = InxParameter::make(child_repr, ext);
                 if (param) {
-                    parameters.push_back(param);
+                    _children.push_back(param);
                 }
             }
 
@@ -58,15 +58,8 @@ ParamNotebook::ParamNotebookPage::ParamNotebookPage(Inkscape::XML::Node *xml, In
 ParamNotebook::ParamNotebookPage::~ParamNotebookPage ()
 {
     // destroy parameters
-    for (auto parameter : parameters) {
-        delete parameter;
-    }
-}
-
-void ParamNotebook::ParamNotebookPage::build_param_string_list(std::list <std::string> &list) const
-{
-    for (auto parameter : parameters) {
-        parameter->build_param_string_list(list);
+    for (auto child : _children) {
+        delete child;
     }
 }
 
@@ -86,16 +79,16 @@ Gtk::Widget *ParamNotebook::ParamNotebookPage::get_widget(SPDocument *doc, Inksc
     vbox->set_spacing(GUI_BOX_SPACING);
 
     // add parameters onto page (if any)
-    for (auto parameter : parameters) {
-        Gtk::Widget *parameter_widget = parameter->get_widget(doc, node, changeSignal);
-        if (parameter_widget) {
-            int indent = parameter->get_indent();
-            parameter_widget->set_margin_start(indent *GUI_INDENTATION);
-            vbox->pack_start(*parameter_widget, false, false, 0);
-
-            const char *tooltip = parameter->get_tooltip();
+    for (auto child : _children) {
+        Gtk::Widget *child_widget = child->get_widget(doc, node, changeSignal);
+        if (child_widget) {
+            int indent = child->get_indent();
+            child_widget->set_margin_start(indent *GUI_INDENTATION);
+            vbox->pack_start(*child_widget, false, false, 0);
+
+            const char *tooltip = child->get_tooltip();
             if (tooltip) {
-                parameter_widget->set_tooltip_text(tooltip);
+                child_widget->set_tooltip_text(tooltip);
             }
         }
     }
@@ -111,14 +104,14 @@ InxParameter *ParamNotebook::ParamNotebookPage::get_param(const char *name)
     if (name == nullptr) {
         throw Extension::param_not_exist();
     }
-    if (this->parameters.empty()) {
-        // the list of parameters is empty
+    if (_children.empty()) {
         throw Extension::param_not_exist();
     }
 
-    for (auto param : parameters) {
-        if (!strcmp(param->name(), name)) {
-            return param;
+    for (auto child : _children) {
+        InxParameter *parameter = dynamic_cast<InxParameter *>(child);
+        if (parameter && !strcmp(parameter->name(), name)) {
+            return parameter;
         }
     }
 
@@ -142,14 +135,17 @@ ParamNotebook::ParamNotebook(Inkscape::XML::Node *xml, Inkscape::Extension::Exte
                 page = new ParamNotebookPage(child_repr, ext);
 
                 if (page) {
-                    pages.push_back(page);
+                    _pages.push_back(page);
                 }
             }
             child_repr = child_repr->next();
         }
     }
-    if (pages.empty()) {
+    if (_pages.empty()) {
         g_warning("No (valid) pages for parameter '%s' in extension '%s'", _name, _extension->get_id());
+    } else {
+        // TODO: We should fully replace _pages with _children eventually; only difference is pointer type.
+        _children.insert(_children.end(), _pages.begin(), _pages.end());
     }
 
     // get value (initialize with value of first page if pref is empty)
@@ -159,8 +155,8 @@ ParamNotebook::ParamNotebook(Inkscape::XML::Node *xml, Inkscape::Extension::Exte
     g_free(pref_name);
 
     if (_value.empty()) {
-        if (!pages.empty()) {
-            _value = pages[0]->_name;
+        if (!_pages.empty()) {
+            _value = _pages[0]->_name;
         }
     }
 }
@@ -168,7 +164,7 @@ ParamNotebook::ParamNotebook(Inkscape::XML::Node *xml, Inkscape::Extension::Exte
 ParamNotebook::~ParamNotebook ()
 {
     //destroy pages
-    for (auto page : pages) {
+    for (auto page : _pages) {
         delete page;
     }
 }
@@ -187,8 +183,8 @@ ParamNotebook::~ParamNotebook ()
  */
 const Glib::ustring& ParamNotebook::set(const int in, SPDocument * /*doc*/, Inkscape::XML::Node * /*node*/)
 {
-    int i = in < pages.size() ? in : pages.size()-1;
-    ParamNotebookPage *page = pages[i];
+    int i = in < _pages.size() ? in : _pages.size()-1;
+    ParamNotebookPage *page = _pages[i];
 
     if (page) {
         _value = page->_name;
@@ -202,17 +198,6 @@ const Glib::ustring& ParamNotebook::set(const int in, SPDocument * /*doc*/, Inks
     return _value;
 }
 
-void ParamNotebook::build_param_string_list(std::list <std::string> &list) const
-{
-    // call base-class method to add the parameter string for the notebook itself
-    InxParameter::build_param_string_list(list);
-
-    // iterate over notebook pages
-    for (auto page : pages) {
-        page->build_param_string_list(list);
-    }
-}
-
 std::string ParamNotebook::value_to_string() const
 {
     return _value;
@@ -266,7 +251,7 @@ InxParameter *ParamNotebook::get_param(const char *name)
     if (name == nullptr) {
         throw Extension::param_not_exist();
     }
-    for (auto page : pages) {
+    for (auto page : _pages) {
         InxParameter *subparam = page->get_param(name);
         if (subparam) {
             return subparam;
@@ -293,7 +278,7 @@ Gtk::Widget *ParamNotebook::get_widget(SPDocument *doc, Inkscape::XML::Node *nod
     // add pages (if any) and switch to previously selected page
     int current_page = -1;
     int selected_page = -1;
-    for (auto page : pages) {
+    for (auto page : _pages) {
         current_page++;
 
         Gtk::Widget *page_widget = page->get_widget(doc, node, changeSignal);
diff --git a/src/extension/prefdialog/parameter-notebook.h b/src/extension/prefdialog/parameter-notebook.h
index 90d032af3..4f23940e5 100644
--- a/src/extension/prefdialog/parameter-notebook.h
+++ b/src/extension/prefdialog/parameter-notebook.h
@@ -45,23 +45,18 @@ private:
      */
     class ParamNotebookPage : public InxParameter {
         friend class ParamNotebook;
-    private:
-        /** A table to store the parameters for this page.
-          * This only gets created if there are parameters on this page */
-        std::vector<InxParameter *> parameters;
     public:
         ParamNotebookPage(Inkscape::XML::Node *xml, Inkscape::Extension::Extension *ext);
         ~ParamNotebookPage() override;
 
         Gtk::Widget *get_widget(SPDocument *doc, Inkscape::XML::Node *node, sigc::signal<void> *changeSignal) override;
-        void build_param_string_list(std::list <std::string> &list) const override;
         char *get_text() { return _text; };
         InxParameter *get_param(const char *name) override;
     }; /* class ParamNotebookPage */
 
     /** A table to store the pages with parameters for this notebook.
       * This only gets created if there are pages in this notebook */
-    std::vector<ParamNotebookPage*> pages;
+    std::vector<ParamNotebookPage*> _pages;
 
 public:
     ParamNotebook(Inkscape::XML::Node *xml, Inkscape::Extension::Extension *ext);
@@ -69,12 +64,6 @@ public:
 
     Gtk::Widget *get_widget(SPDocument *doc, Inkscape::XML::Node *node, sigc::signal<void> *changeSignal) override;
 
-    /**
-     * A function to get the currentpage and the parameters in a string form.
-     * @return A string with the 'value' and all the parameters on all pages as command line arguments.
-     */
-    void build_param_string_list(std::list <std::string> &list) const override;
-
     std::string value_to_string() const override;
 
     InxParameter *get_param (const char *name) override;
diff --git a/src/extension/prefdialog/parameter.cpp b/src/extension/prefdialog/parameter.cpp
index 8618f434b..7d8751353 100644
--- a/src/extension/prefdialog/parameter.cpp
+++ b/src/extension/prefdialog/parameter.cpp
@@ -272,21 +272,6 @@ std::string InxParameter::value_to_string() const
     return "";
 }
 
-void InxParameter::build_param_string_list(std::list <std::string> &list) const
-{
-    std::string value_string = value_to_string();
-    if (!value_string.empty()) {
-        std::string parameter_string; // --param=value
-        parameter_string += "--";
-        parameter_string += name();
-        parameter_string += "=";
-        parameter_string += value_string;
-
-        list.insert(list.end(), parameter_string);
-    }
-}
-
-
 InxParameter *InxParameter::get_param(const char */*name*/)
 {
     return nullptr;
diff --git a/src/extension/prefdialog/parameter.h b/src/extension/prefdialog/parameter.h
index a5e8c0f71..493824cbf 100644
--- a/src/extension/prefdialog/parameter.h
+++ b/src/extension/prefdialog/parameter.h
@@ -38,7 +38,7 @@ public:
     InxParameter(Inkscape::XML::Node *in_repr,
                  Inkscape::Extension::Extension *ext);
 
-    ~InxParameter() override;
+    virtual ~InxParameter() override;
 
     /** Wrapper to cast to the object and use its function. */
     bool get_bool(SPDocument const *doc, Inkscape::XML::Node const *node) const;
@@ -108,16 +108,6 @@ public:
     const char *get_tooltip() const override { return _description; }
 
     /**
-     * Build a list of parameter strings for the current parameter and it's children (if it has any)
-     *
-     * The individual parameter strings have the form "--param=value", so they can be passed to
-     * script extensions as-is
-     *
-     * @param list Reference to a list of strings that will be appended with the new parameter strings
-     */
-    virtual void build_param_string_list(std::list <std::string> &list) const;
-
-    /**
      * Gets the current value of the parameter in a string form.
      *
      * @return String representation of the parameter's value.
diff --git a/src/extension/prefdialog/widget.cpp b/src/extension/prefdialog/widget.cpp
index ae85b9fbd..05711b58a 100644
--- a/src/extension/prefdialog/widget.cpp
+++ b/src/extension/prefdialog/widget.cpp
@@ -132,6 +132,14 @@ const char *InxWidget::get_translation(const char* msgid) {
     return _extension->get_translation(msgid, _context);
 }
 
+void InxWidget::get_widgets(std::vector<const InxWidget *> &list) const
+{
+    list.push_back(this);
+    for (auto child : _children) {
+        child->get_widgets(list);
+    }
+}
+
 }  // namespace Extension
 }  // namespace Inkscape
 
diff --git a/src/extension/prefdialog/widget.h b/src/extension/prefdialog/widget.h
index ba688a70b..b31fc1d53 100644
--- a/src/extension/prefdialog/widget.h
+++ b/src/extension/prefdialog/widget.h
@@ -60,7 +60,6 @@ public:
     /** Checks if name is a valid widget name, i.e. a widget can be constructed from it using make() */
     static bool is_valid_widget_name(const char *name);
 
-
     virtual Gtk::Widget *get_widget(SPDocument *doc, Inkscape::XML::Node *node, sigc::signal<void> *changeSignal);
 
     virtual const char *get_tooltip() const { return nullptr; } // tool-tips are exclusive to InxParameters for now
@@ -72,14 +71,21 @@ public:
     int get_indent() const { return _indent; }
 
 
+    /**
+     * Recursively construct a list containing the current widget and all of it's child widgets (if it has any)
+     *
+     * @param list Reference to a vector of pointers to \a InxWidget that will be appended with the new \a InxWidgets
+     */
+    virtual void get_widgets(std::vector<const InxWidget *> &list) const;
+
 
-    /** Recommended margin of boxes containing multiple Widgets (in px) */
+    /** Recommended margin of boxes containing multiple widgets (in px) */
     const static int GUI_BOX_MARGIN = 10;
-    /** Recommended spacing between multiple Widgets packed into a box (in px) */
+    /** Recommended spacing between multiple widgets packed into a box (in px) */
     const static int GUI_BOX_SPACING = 4;
-    /** Recommended indentation width of Widgets(in px) */
+    /** Recommended indentation width of widgets(in px) */
     const static int GUI_INDENTATION = 12;
-    /** Recommended maximum line length for wrapping textual Widgets (in chars) */
+    /** Recommended maximum line length for wrapping textual wdgets (in chars) */
     const static int GUI_MAX_LINE_LENGTH = 60;
 
 protected:
@@ -90,16 +96,19 @@ protected:
     /** Which extension is this Widget attached to. */
     Inkscape::Extension::Extension *_extension = nullptr;
 
-    /** Whether the Widget is visible. */
+    /** Child widgets of this widget (might be empty if there are none) */
+    std::vector<InxWidget *> _children;
+
+    /** Whether the widget is visible. */
     bool _hidden = false;
 
-    /** Indentation level of the Widget. */
+    /** Indentation level of the widget. */
     int _indent = 0;
 
-    /** Appearance of the Widget (not used by all Widgets). */
+    /** Appearance of the widget (not used by all widgets). */
     char *_appearance = nullptr;
 
-    /** Is Widget translatable? */
+    /** Is widget translatable? */
     Translatable _translatable = UNSET;
 
     /** context for translation of translatable strings. */
@@ -110,7 +119,7 @@ protected:
 
     /** gets the gettext translation for msgid
       *
-      * Handles translation domain of the extension and message context of the Widget internally
+      * Handles translation domain of the extension and message context of the widget internally
       *
       * @param msgid String to translate
       * @return      Translated string