Fixed: circles and ellipses had wrong element names in XML editor (Issue #116)

15 files changed, 141 insertions, 9 deletions

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..b0c163784 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)
+    {
+        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..700eba9e8 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 = 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 {