Fix #116: Wrong element names shown in XML editor for circles and ellipses

17 files changed, 186 insertions, 34 deletions

diff --git a/src/object/sp-ellipse.cpp b/src/object/sp-ellipse.cpp
index 361c90428..09be158eb 100644
--- a/src/object/sp-ellipse.cpp
+++ b/src/object/sp-ellipse.cpp
@@ -250,8 +250,8 @@ Inkscape::XML::Node *SPGenericEllipse::write(Inkscape::XML::Document *xml_doc, I
         }
     }
 
-    if( type != new_type ) {
-        switch( new_type ) {
+    if (type != new_type) {
+        switch (new_type) {
             case SP_GENERIC_ELLIPSE_ARC:
                 repr->setCodeUnsafe(g_quark_from_string("svg:path"));
                 break;
@@ -265,10 +265,6 @@ Inkscape::XML::Node *SPGenericEllipse::write(Inkscape::XML::Document *xml_doc, I
                 std::cerr << "SPGenericEllipse::write(): unknown type." << std::endl;
         }
         type = new_type;
-
-        // FIXME: The XML dialog won't update the element name. We need
-        // a notifyElementNameChanged callback added to the XML observers
-        // to trigger a refresh.
     }
 
     // std::cout << "  type: " << g_quark_to_string( repr->code() ) << std::endl;
diff --git a/src/widgets/sp-xmlview-tree.cpp b/src/widgets/sp-xmlview-tree.cpp
index 75e375ae5..4557bb84a 100644
--- a/src/widgets/sp-xmlview-tree.cpp
+++ b/src/widgets/sp-xmlview-tree.cpp
@@ -33,6 +33,8 @@ static void element_child_added (Inkscape::XML::Node * repr, Inkscape::XML::Node
 static void element_attr_changed (Inkscape::XML::Node * repr, const gchar * key, const gchar * old_value, const gchar * new_value, bool is_interactive, gpointer data);
 static void element_child_removed (Inkscape::XML::Node * repr, Inkscape::XML::Node * child, Inkscape::XML::Node * ref, gpointer data);
 static void element_order_changed (Inkscape::XML::Node * repr, Inkscape::XML::Node * child, Inkscape::XML::Node * oldref, Inkscape::XML::Node * newref, gpointer data);
+static void element_name_changed (Inkscape::XML::Node* repr, gchar const* oldname, gchar const* newname, gpointer data);
+static void element_attr_or_name_change_update(Inkscape::XML::Node* repr, NodeData* data);
 
 static void text_content_changed (Inkscape::XML::Node * repr, const gchar * old_content, const gchar * new_content, gpointer data);
 static void comment_content_changed (Inkscape::XML::Node * repr, const gchar * old_content, const gchar * new_content, gpointer data);
@@ -56,7 +58,8 @@ static const Inkscape::XML::NodeEventVector element_repr_events = {
         element_child_removed,
         element_attr_changed,
         nullptr, /* content_changed */
-        element_order_changed
+        element_order_changed,
+        element_name_changed
 };
 
 static const Inkscape::XML::NodeEventVector text_repr_events = {
@@ -64,7 +67,8 @@ static const Inkscape::XML::NodeEventVector text_repr_events = {
         nullptr, /* child_removed */
         nullptr, /* attr_changed */
         text_content_changed,
-        nullptr  /* order_changed */
+        nullptr  /* order_changed */,
+        nullptr  /* element_name_changed */
 };
 
 static const Inkscape::XML::NodeEventVector comment_repr_events = {
@@ -72,7 +76,8 @@ static const Inkscape::XML::NodeEventVector comment_repr_events = {
         nullptr, /* child_removed */
         nullptr, /* attr_changed */
         comment_content_changed,
-        nullptr  /* order_changed */
+        nullptr  /* order_changed */,
+        nullptr  /* element_name_changed */
 };
 
 static const Inkscape::XML::NodeEventVector pi_repr_events = {
@@ -80,7 +85,8 @@ static const Inkscape::XML::NodeEventVector pi_repr_events = {
         nullptr, /* child_removed */
         nullptr, /* attr_changed */
         pi_content_changed,
-        nullptr  /* order_changed */
+        nullptr  /* order_changed */,
+        nullptr  /* element_name_changed */
 };
 
 GtkWidget *sp_xmlview_tree_new(Inkscape::XML::Node * repr, void * /*factory*/, void * /*data*/)
@@ -228,32 +234,50 @@ void element_child_added (Inkscape::XML::Node * /*repr*/, Inkscape::XML::Node *
     add_node (data->tree, &data_iter, &before, child);
 }
 
-void element_attr_changed(Inkscape::XML::Node * repr, const gchar * key, const gchar * /*old_value*/, const gchar * new_value, bool /*is_interactive*/, gpointer ptr)
+void element_attr_changed(
+        Inkscape::XML::Node* repr, gchar const* key,
+        gchar const* /*old_value*/, gchar const* /*new_value*/, bool /*is_interactive*/,
+        gpointer ptr)
 {
-    NodeData *data = static_cast<NodeData *>(ptr);
-    gchar *label;
-
-    if (data->tree->blocked) return;
-
     if (0 != strcmp (key, "id") && 0 != strcmp (key, "inkscape:label"))
         return;
+    element_attr_or_name_change_update(repr, static_cast<NodeData*>(ptr));
+}
+
+void element_name_changed(
+        Inkscape::XML::Node* repr,
+        gchar const* /*oldname*/, gchar const* /*newname*/, gpointer ptr)
+{
+    element_attr_or_name_change_update(repr, static_cast<NodeData*>(ptr));
+}
 
-    new_value = repr->attribute("id");
-    const gchar *layer = repr->attribute("inkscape:label");
+void element_attr_or_name_change_update(Inkscape::XML::Node* repr, NodeData* data)
+{
+    if (data->tree->blocked) {
+        return;
+    }
 
-    if (new_value && layer) {
-        label = g_strdup_printf ("<%s id=\"%s\" inkscape:label=\"%s\">", repr->name(), new_value, layer);
-    } else if (new_value) {
-        label = g_strdup_printf ("<%s id=\"%s\">", repr->name(), new_value);
+    gchar const* node_name = repr->name();
+    gchar const* id_value = repr->attribute("id");
+    gchar const* label_value = repr->attribute("inkscape:label");
+    gchar* display_text;
+
+    if (id_value && label_value) {
+        display_text = g_strdup_printf ("<%s id=\"%s\" inkscape:label=\"%s\">", node_name, id_value, label_value);
+    } else if (id_value) {
+        display_text = g_strdup_printf ("<%s id=\"%s\">", node_name, id_value);
+    } else if (label_value) {
+        display_text = g_strdup_printf ("<%s inkscape:label=\"%s\">", node_name, label_value);
     } else {
-        label = g_strdup_printf ("<%s>", repr->name());
+        display_text = g_strdup_printf ("<%s>", node_name);
     }
 
     GtkTreeIter iter;
     if (tree_ref_to_iter(data->tree, &iter,  data->rowref)) {
-        gtk_tree_store_set (GTK_TREE_STORE(data->tree->store), &iter, STORE_TEXT_COL, label, -1);
+        gtk_tree_store_set (GTK_TREE_STORE(data->tree->store), &iter, STORE_TEXT_COL, display_text, -1);
     }
-    g_free (label);
+
+    g_free(display_text);
 }
 
 void element_child_removed(Inkscape::XML::Node * /*repr*/, Inkscape::XML::Node * child, Inkscape::XML::Node * /*ref*/, gpointer ptr)
diff --git a/src/xml/composite-node-observer.cpp b/src/xml/composite-node-observer.cpp
index 1f9c08681..77a1bd2fd 100644
--- a/src/xml/composite-node-observer.cpp
+++ b/src/xml/composite-node-observer.cpp
@@ -101,6 +101,17 @@ void CompositeNodeObserver::notifyAttributeChanged(
     _finishIteration();
 }
 
+void CompositeNodeObserver::notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name)
+{
+    _startIteration();
+    for (auto& iter : _active) {
+        if (!iter.marked) {
+            iter.observer.notifyElementNameChanged(node, old_name, new_name);
+        }
+    }
+    _finishIteration();
+}
+
 void CompositeNodeObserver::add(NodeObserver &observer) {
     if (_iterating) {
         _pending.push_back(ObserverRecord(observer));
@@ -148,6 +159,12 @@ public:
             vector.attr_changed(&node, g_quark_to_string(name), old_value, new_value, false, data);
         }
     }
+
+    void notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name) override {
+        if (vector.element_name_changed) {
+            vector.element_name_changed(&node, g_quark_to_string(old_name), g_quark_to_string(new_name), data);
+        }
+    }
 };
 
 }
diff --git a/src/xml/composite-node-observer.h b/src/xml/composite-node-observer.h
index 73d0b1442..96f1c9144 100644
--- a/src/xml/composite-node-observer.h
+++ b/src/xml/composite-node-observer.h
@@ -80,6 +80,8 @@ public:
                                 Util::ptr_shared old_value,
                                 Util::ptr_shared new_value) override;
 
+    void notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name) override;
+
 private:
     unsigned _iterating;
     ObserverRecordList _active;
diff --git a/src/xml/event.cpp b/src/xml/event.cpp
index 481c12bb6..86e042a2b 100644
--- a/src/xml/event.cpp
+++ b/src/xml/event.cpp
@@ -120,6 +120,11 @@ public:
     {
         node.setContent(new_value);
     }
+
+    void notifyElementNameChanged(Node& node, GQuark /*old_value*/, GQuark new_value) override
+    {
+        node.setCodeUnsafe(new_value);
+    }
 };
 
 }
@@ -180,6 +185,12 @@ void Inkscape::XML::EventChgOrder::_undoOne(
     observer.notifyChildOrderChanged(*this->repr, *this->child, this->newref, this->oldref);
 }
 
+void Inkscape::XML::EventChgElementName::_undoOne(
+    Inkscape::XML::NodeObserver& observer
+) const {
+    observer.notifyElementNameChanged(*this->repr, this->new_name, this->old_name);
+}
+
 void Inkscape::XML::replay_log_to_observer(
     Inkscape::XML::Event const *log,
     Inkscape::XML::NodeObserver &observer
@@ -239,6 +250,12 @@ void Inkscape::XML::EventChgOrder::_replayOne(
     observer.notifyChildOrderChanged(*this->repr, *this->child, this->oldref, this->newref);
 }
 
+void Inkscape::XML::EventChgElementName::_replayOne(
+    Inkscape::XML::NodeObserver &observer
+) const {
+    observer.notifyElementNameChanged(*this->repr, this->new_name, this->old_name);
+}
+
 Inkscape::XML::Event *
 sp_repr_coalesce_log (Inkscape::XML::Event *a, Inkscape::XML::Event *b)
 {
@@ -398,6 +415,17 @@ Inkscape::XML::Event *Inkscape::XML::EventChgOrder::_optimizeOne() {
     }
 }
 
+Inkscape::XML::Event* Inkscape::XML::EventChgElementName::_optimizeOne() {
+    auto next_chg_element_name = dynamic_cast<Inkscape::XML::EventChgElementName*>(this->next);
+    if (next_chg_element_name && next_chg_element_name->repr == this->repr) {
+        // Combine name changes to the same element.
+        this->old_name = next_chg_element_name->old_name;
+        this->next = next_chg_element_name->next;
+        delete next_chg_element_name;
+    }
+    return this;
+}
+
 namespace {
 
 class LogPrinter : public Inkscape::XML::NodeObserver {
@@ -482,6 +510,12 @@ public:
             g_warning("Event: Unset content of %s", node_to_string(node).c_str());
         }
     }
+
+    void notifyElementNameChanged(Node& node, GQuark old_value, GQuark new_value) override
+    {
+        g_warning("Event: Changed name of %s from %s to %s\n",
+                node_to_string(node).c_str(), g_quark_to_string(old_value), g_quark_to_string(new_value));
+    }
 };
 
 }
diff --git a/src/xml/event.h b/src/xml/event.h
index f8e832621..4256eea0d 100644
--- a/src/xml/event.h
+++ b/src/xml/event.h
@@ -225,6 +225,25 @@ private:
     void _replayOne(NodeObserver &observer) const override;
 };
 
+/**
+ * @brief Object representing element name change.
+ */
+class EventChgElementName : public Event {
+public:
+    EventChgElementName(Node* repr, GQuark old_name, GQuark new_name, Event* next)
+        : Event(repr, next), old_name(old_name), new_name(new_name) {}
+
+    /// GQuark corresponding to the old element name.
+    GQuark old_name;
+    /// GQuark corresponding to the new element name.
+    GQuark new_name;
+
+private:
+    Event* _optimizeOne() override;
+    void _undoOne(NodeObserver& observer) const override;
+    void _replayOne(NodeObserver& observer) const override;
+};
+
 }
 }
 
diff --git a/src/xml/helper-observer.cpp b/src/xml/helper-observer.cpp
index 340e7627a..05161e9ca 100644
--- a/src/xml/helper-observer.cpp
+++ b/src/xml/helper-observer.cpp
@@ -62,6 +62,11 @@ void SignalObserver::notifyContentChanged(XML::Node&, Util::ptr_shared, Util::pt
 void SignalObserver::notifyAttributeChanged(XML::Node&, GQuark, Util::ptr_shared, Util::ptr_shared)
 { signal_changed()(); }
 
+void SignalObserver::notifyElementNameChanged(Node&, GQuark, GQuark)
+{
+    signal_changed()();
+}
+
 sigc::signal<void>& SignalObserver::signal_changed()
 {
     return _signal_changed;
diff --git a/src/xml/helper-observer.h b/src/xml/helper-observer.h
index fc8a3f75c..4bf172bd2 100644
--- a/src/xml/helper-observer.h
+++ b/src/xml/helper-observer.h
@@ -36,6 +36,7 @@ public:
     void notifyChildOrderChanged(Node&, Node&, Node*, Node*) override;
     void notifyContentChanged(Node&, Util::ptr_shared, Util::ptr_shared) override;
     void notifyAttributeChanged(Node&, GQuark, Util::ptr_shared, Util::ptr_shared) override;
+    void notifyElementNameChanged(Node&, GQuark, GQuark) override;
     sigc::signal<void>& signal_changed();
 private:
     sigc::signal<void> _signal_changed;
diff --git a/src/xml/log-builder.cpp b/src/xml/log-builder.cpp
index e89be2ef3..3fd831a9c 100644
--- a/src/xml/log-builder.cpp
+++ b/src/xml/log-builder.cpp
@@ -61,6 +61,12 @@ void LogBuilder::setAttribute(Node &node, GQuark name,
     _log = _log->optimizeOne();
 }
 
+void LogBuilder::setElementName(Node& node, GQuark old_name, GQuark new_name)
+{
+    _log = new Inkscape::XML::EventChgElementName(&node, old_name, new_name, _log);
+    _log = _log->optimizeOne();
+}
+
 }
 }
 
diff --git a/src/xml/log-builder.h b/src/xml/log-builder.h
index 9407a88fa..bd7628bb2 100644
--- a/src/xml/log-builder.h
+++ b/src/xml/log-builder.h
@@ -63,6 +63,8 @@ public:
     void setAttribute(Node &node, GQuark name,
                       Util::ptr_shared old_value,
                       Util::ptr_shared new_value);
+
+    void setElementName(Node& node, GQuark old_name, GQuark new_name);
     /*@}*/
 
 private:
diff --git a/src/xml/node-event-vector.h b/src/xml/node-event-vector.h
index 518bc625a..263751e59 100644
--- a/src/xml/node-event-vector.h
+++ b/src/xml/node-event-vector.h
@@ -59,6 +59,7 @@ struct NodeEventVector {
 	void (* attr_changed) (Node *repr, char const *key, char const *oldval, char const *newval, bool is_interactive, void* data);
 	void (* content_changed) (Node *repr, char const *oldcontent, char const *newcontent, void * data);
 	void (* order_changed) (Node *repr, Node *child, Node *oldref, Node *newref, void* data);
+	void (* element_name_changed) (Node* repr, char const* oldname, char const* newname, void* data);
 };
 
 }
diff --git a/src/xml/node-observer.h b/src/xml/node-observer.h
index 043ea2b32..e3173a846 100644
--- a/src/xml/node-observer.h
+++ b/src/xml/node-observer.h
@@ -145,6 +145,22 @@ public:
         INK_UNUSED(old_value);
         INK_UNUSED(new_value);
     }
+
+    /**
+     * @brief Element name change callback.
+     *
+     * This method is called whenever an element node's name is changed.
+     *
+     * @param node The changed XML node.
+     * @param old_name GQuark corresponding to the old element name.
+     * @param new_name GQuark corresponding to the new element name.
+     */
+    virtual void notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name) {
+        INK_UNUSED(node);
+        INK_UNUSED(old_name);
+        INK_UNUSED(new_name);
+    }
+
 };
 
 } // namespace XML
diff --git a/src/xml/node.h b/src/xml/node.h
index 11a23c6ac..b6a0df353 100644
--- a/src/xml/node.h
+++ b/src/xml/node.h
@@ -212,16 +212,15 @@ public:
                       value.empty() ? nullptr : value.c_str(), is_interactive);
     }
     //@}
-
     /**
-     * @brief Directly set the integer GQuark code for the name of the node
+     * @brief Set the integer GQuark code for the name of the node.
      *
-     * This function is a hack to easily move elements with no namespace to the SVG namespace.
      * Do not use this function unless you really have a good reason.
      *
-     * @param code The integer value corresponding to the string to be set as the name of this node
+     * @param code The integer value corresponding to the string to be set as
+     *        the name of this node
      */
-    virtual void setCodeUnsafe(int code)=0;
+    virtual void setCodeUnsafe(int code) = 0;
     
     /*@}*/
 
diff --git a/src/xml/simple-document.cpp b/src/xml/simple-document.cpp
index d7a3d84db..c1227ac08 100644
--- a/src/xml/simple-document.cpp
+++ b/src/xml/simple-document.cpp
@@ -115,9 +115,15 @@ void SimpleDocument::notifyAttributeChanged(Node &node,
     }
 }
 
+void SimpleDocument::notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name)
+{
+    if (_in_transaction) {
+        _log_builder.setElementName(node, old_name, new_name);
+    }
 }
 
-}
+} // end namespace XML
+} // end namespace Inkscape
 
 /*
   Local Variables:
diff --git a/src/xml/simple-document.h b/src/xml/simple-document.h
index 2b55a86bc..5dc175093 100644
--- a/src/xml/simple-document.h
+++ b/src/xml/simple-document.h
@@ -61,6 +61,8 @@ public:
                                 Util::ptr_shared old_value,
                                 Util::ptr_shared new_value) override;
 
+    void notifyElementNameChanged(Node& node, GQuark old_name, GQuark new_name) override;
+
 protected:
     SimpleDocument(SimpleDocument const &doc)
     : Node(), SimpleNode(doc), Document(), NodeObserver(),
diff --git a/src/xml/simple-node.cpp b/src/xml/simple-node.cpp
index 8996bd990..a1f0c7024 100644
--- a/src/xml/simple-node.cpp
+++ b/src/xml/simple-node.cpp
@@ -149,6 +149,15 @@ public:
     }
 };
 
+class DebugSetElementName : public DebugXMLNode {
+public:
+    DebugSetElementName(Node const& node, GQuark name)
+    : DebugXMLNode(node, "set-name")
+    {
+        _addProperty("name", g_quark_to_string(name));
+    }
+};
+
 }
 
 using Util::ptr_shared;
@@ -399,6 +408,21 @@ SimpleNode::setAttribute(gchar const *name, gchar const *value, bool const /*is_
     g_free( cleaned_value );
 }
 
+void SimpleNode::setCodeUnsafe(int code) {
+    GQuark old_code = static_cast<GQuark>(_name);
+    GQuark new_code = static_cast<GQuark>(code);
+
+    Debug::EventTracker<> tracker;
+    tracker.set<DebugSetElementName>(*this, new_code);
+
+    _name = static_cast<int>(new_code);
+
+    if (new_code != old_code) {
+        _document->logger()->notifyElementNameChanged(*this, old_code, new_code);
+        _observers.notifyElementNameChanged(*this, old_code, new_code);
+    }
+}
+
 void SimpleNode::addChild(Node *generic_child, Node *generic_ref) {
     g_assert(generic_child);
     g_assert(generic_child->document() == _document);
diff --git a/src/xml/simple-node.h b/src/xml/simple-node.h
index 8d0b25bf5..55cf3fd9c 100644
--- a/src/xml/simple-node.h
+++ b/src/xml/simple-node.h
@@ -38,9 +38,7 @@ class SimpleNode
 public:
     char const *name() const override;
     int code() const override { return _name; }
-    void setCodeUnsafe(int code) override {
-        _name = code;
-    }
+    void setCodeUnsafe(int code) override;
 
     Document *document() override { return _document; }
     Document const *document() const override {