From 176510745d850a42555d77f0f3a24b0b01b52bbe Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Fri, 30 May 2025 20:24:49 +0200 Subject: [PATCH 1/9] Add a method to check equivalence of SPObjects For the purposes of reusing SPObjects instead of creating new ones which would be identical for all practical purposes (for example to avoid duplicate swatches), it makes sense to introduce a concept of "equivalence" of SPObjects. We add a method isEquivalent() which can tell us whether this SPObject is effectively equivalent to another one. --- src/object/sp-object.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/object/sp-object.h b/src/object/sp-object.h index a6ec4a0e61..3c9ec611ff 100644 --- a/src/object/sp-object.h +++ b/src/object/sp-object.h @@ -569,6 +569,10 @@ public: _successor = successor; } + /// Tell whether the other object is, for all practical purposes, identical to this one. + bool isEquivalent(SPObject const &other) const { return _isEquivalent(other); } + + // FIXME: the three methods below are a hack; remove them. /** * Indicates that another object supercedes temporaty this one. */ @@ -852,9 +856,10 @@ private: */ Glib::ustring textualContent() const; + virtual bool _isEquivalent(SPObject const &) const { return false; } + /* Real handlers of repr signals */ -private: // XML::NodeObserver functions void notifyAttributeChanged(Inkscape::XML::Node &node, GQuark key, Inkscape::Util::ptr_shared oldval, Inkscape::Util::ptr_shared newval) final; -- GitLab From 50ee4faf34afc04e808ff7371c0a2524c5ede75d Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Fri, 30 May 2025 20:28:45 +0200 Subject: [PATCH 2/9] Implement overrides of SPObject::_isEquivalent() Provide overrides of the method _isEquivalent() for two derived classes of SPObject with "resource" semantics in order to prevent the creation of duplicates: SPGradient and LivePathEffectObject. --- src/live_effects/lpeobject.cpp | 9 +++++++++ src/live_effects/lpeobject.h | 3 ++- src/object/sp-gradient.cpp | 11 ++++++++++- src/object/sp-gradient.h | 2 ++ 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/live_effects/lpeobject.cpp b/src/live_effects/lpeobject.cpp index 2f520c8016..60262e1d65 100644 --- a/src/live_effects/lpeobject.cpp +++ b/src/live_effects/lpeobject.cpp @@ -127,6 +127,15 @@ bool LivePathEffectObject::is_similar(LivePathEffectObject *that) return true; } +bool LivePathEffectObject::_isEquivalent(SPObject const &other) const +{ + if (auto *other_lpe = cast(&other)) { + // TODO: fix const correctness of is_similar() + return const_cast(this)->is_similar(const_cast(other_lpe)); + } + return false; +} + /** * Set lpeobject is on clipboard */ diff --git a/src/live_effects/lpeobject.h b/src/live_effects/lpeobject.h index 663fa75767..24268f03ea 100644 --- a/src/live_effects/lpeobject.h +++ b/src/live_effects/lpeobject.h @@ -42,7 +42,7 @@ public: bool deleted{false}; bool isOnClipboard() const { return _isOnClipboard; }; // dont check values only structure and ID - bool is_similar(LivePathEffectObject *that); + bool is_similar(LivePathEffectObject *that); ///< @todo: Ensure const correctness, use a reference argument LivePathEffectObject *fork_private_if_necessary(unsigned int nr_of_allowed_users = 1); @@ -66,6 +66,7 @@ private: bool _isOnClipboard = false; friend LPENodeObserver; // for static_cast LPENodeObserver &nodeObserver() { return *this; } + bool _isEquivalent(SPObject const &other) const override; }; #endif // INKSCAPE_LIVEPATHEFFECT_OBJECT_H diff --git a/src/object/sp-gradient.cpp b/src/object/sp-gradient.cpp index 29aacd5d4b..11a097d95e 100644 --- a/src/object/sp-gradient.cpp +++ b/src/object/sp-gradient.cpp @@ -105,6 +105,14 @@ void SPGradient::setPinned(bool pinned) } } +bool SPGradient::_isEquivalent(SPObject const &other) const +{ + if (auto *other_gradient = cast(&other)) { + // TODO: fix const correctness of isEquivalent() + return const_cast(this)->isEquivalent(const_cast(other_gradient)); + } + return false; +} /** * return true if this gradient is "equivalent" to that gradient. @@ -116,7 +124,8 @@ bool SPGradient::isEquivalent(SPGradient *that) //TODO Make this work for mesh gradients bool status = false; - + + // TODO: Remove this useless "loop" (╯°□°)╯︵ ┻━┻ while(true){ // not really a loop, used to avoid deep nesting or multiple exit points from function if (this->getStopCount() != that->getStopCount()) { break; } if (this->hasStops() != that->hasStops()) { break; } diff --git a/src/object/sp-gradient.h b/src/object/sp-gradient.h index 9b18454e3a..0aa9c7173d 100644 --- a/src/object/sp-gradient.h +++ b/src/object/sp-gradient.h @@ -133,6 +133,7 @@ public: SPStop* getFirstStop(); int getStopCount() const; + // TODO: Improve const correctness in these two methods bool isEquivalent(SPGradient *b); bool isAligned(SPGradient *b); @@ -219,6 +220,7 @@ private: bool invalidateArray(); void rebuildVector() const; void rebuildArray(); + bool _isEquivalent(SPObject const &other) const override; protected: void build(SPDocument *document, Inkscape::XML::Node *repr) override; -- GitLab From f29e3d6d8e8acdab8b258d83973c2169b7c4aa6c Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 31 May 2025 16:39:01 +0200 Subject: [PATCH 3/9] Implement _isEquivalent() for SPSymbol Provide a mechanism for detecting duplicate symbols in clipboard which supersedes the pre-existing logic in the "defs import" routine of SPDocument. --- src/object/sp-symbol.cpp | 19 +++++++++++++++++++ src/object/sp-symbol.h | 3 +++ 2 files changed, 22 insertions(+) diff --git a/src/object/sp-symbol.cpp b/src/object/sp-symbol.cpp index 138d77a8aa..5c4a53bfee 100644 --- a/src/object/sp-symbol.cpp +++ b/src/object/sp-symbol.cpp @@ -302,6 +302,25 @@ void SPSymbol::print(SPPrintContext* ctx) { } } +bool SPSymbol::_isEquivalent(SPObject const &other) const +{ + auto const *other_symbol = cast(&other); + if (!other_symbol) { + return false; + } + if (other_symbol == this) { + return true; + } + + // Prevent duplication of symbols... could be more clever. + // The tag "_inkscape_duplicate" is added to "id" by ClipboardManagerImpl::copySymbol(). + Glib::ustring const other_id = other_symbol->getId(); + if (size_t const pos = other_id.find("_inkscape_duplicate"); pos != Glib::ustring::npos) { + return getId() == other_id.substr(0, pos); + } + return false; +} + /* Local Variables: mode:c++ diff --git a/src/object/sp-symbol.h b/src/object/sp-symbol.h index ab55995e80..a20e60efcf 100644 --- a/src/object/sp-symbol.h +++ b/src/object/sp-symbol.h @@ -45,6 +45,9 @@ public: Geom::OptRect bbox(Geom::Affine const &transform, SPItem::BBoxType type) const override; void hide (unsigned int key) override; +private: + bool _isEquivalent(SPObject const &other) const override; + public: // reference point SVGLength refX; -- GitLab From 614345a51ca5196494ce39f9ff382582443a578f Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 31 May 2025 17:07:15 +0200 Subject: [PATCH 4/9] Remove clipboard-specific logic from ID clash lib In the module responsible for ID renaming and removing ID clashes, there is no need for logic specific to the operation of the Clipboard system. Instead, the decision on whether two objects are equivalent should be delegated to the SPObjects themselves, via the polymorphic isEquivalent() method. --- src/file.cpp | 4 +-- src/id-clash.cpp | 60 ++++++++++++++------------------------------ src/id-clash.h | 2 +- src/ui/clipboard.cpp | 4 +-- 4 files changed, 24 insertions(+), 46 deletions(-) diff --git a/src/file.cpp b/src/file.cpp index b4a2bd125e..5736c1c0c8 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -585,7 +585,7 @@ sp_file_save_template(Gtk::Window &parentWindow, Glib::ustring name, Inkscape::Extension::FILE_SAVE_METHOD_INKSCAPE_SVG); } } - + // remove this node from current document after saving it as template root->removeChild(templateinfo_node); @@ -815,7 +815,7 @@ file_import(SPDocument *in_doc, const std::string &path, Inkscape::Extension::Ex Inkscape::XML::rebase_hrefs(doc.get(), in_doc->getDocumentBase(), false); Inkscape::XML::Document *xml_in_doc = in_doc->getReprDoc(); - prevent_id_clashes(doc.get(), in_doc, true); + prevent_id_clashes(doc.get(), in_doc); sp_file_fix_lpe(doc.get()); in_doc->importDefs(doc.get()); diff --git a/src/id-clash.cpp b/src/id-clash.cpp index 498f39629d..976d900ccc 100644 --- a/src/id-clash.cpp +++ b/src/id-clash.cpp @@ -177,7 +177,7 @@ fix_ref(IdReference const &idref, SPObject *to_obj, const char *old_id) { * FIXME: There are some types of references not yet dealt with here * (e.g., ID selectors in CSS stylesheets, and references in scripts). */ -static void find_references(SPObject *elem, refmap_type &refmap, bool from_clipboard) +static void find_references(SPObject *elem, refmap_type &refmap) { if (elem->cloned) return; Inkscape::XML::Node *repr_elem = elem->getRepr(); @@ -369,7 +369,7 @@ static void find_references(SPObject *elem, refmap_type &refmap, bool from_clipb // recurse for (auto& child: elem->children) { - find_references(&child, refmap, from_clipboard); + find_references(&child, refmap); } } @@ -378,47 +378,26 @@ static void find_references(SPObject *elem, refmap_type &refmap, bool from_clipb * a list of those changes that will require fixing up references. */ static void change_clashing_ids(SPDocument *imported_doc, SPDocument *current_doc, SPObject *elem, - refmap_type const &refmap, id_changelist_type *id_changes, bool from_clipboard) + refmap_type const &refmap, id_changelist_type *id_changes) { - const gchar *id = elem->getId(); - bool fix_clashing_ids = true; + gchar const *id = elem->getId(); + if (!id) { + return; + } - if (id && current_doc->getObjectById(id)) { + if (auto *current_doc_object = current_doc->getObjectById(id)) { // Choose a new ID. // To try to preserve any meaningfulness that the original ID // may have had, the new ID is the old ID followed by a hyphen // and one or more digits. - if (is(elem)) { - SPObject *cd_obj = current_doc->getObjectById(id); - - if (cd_obj && is(cd_obj)) { - auto cd_gr = cast(cd_obj); - if ( cd_gr->isEquivalent(cast(elem))) { - fix_clashing_ids = false; - } - } - } - - auto lpeobj = cast(elem); - if (lpeobj) { - SPObject *cd_obj = current_doc->getObjectById(id); - auto cd_lpeobj = cast(cd_obj); - if (cd_lpeobj && lpeobj->is_similar(cd_lpeobj)) { - fix_clashing_ids = from_clipboard; - } - } - + bool const fix_clashing_ids = current_doc_object->isEquivalent(*elem); if (fix_clashing_ids) { - std::string old_id(id); + std::string const old_id(id); std::string new_id(old_id + '-'); - for (;;) { - new_id += "0123456789"[std::rand() % 10]; - const char *str = new_id.c_str(); - if (current_doc->getObjectById(str) == nullptr && - imported_doc->getObjectById(str) == nullptr) break; - } - // Change to the new ID + do { + new_id += '0' + (std::rand() % 10); + } while (current_doc->getObjectById(new_id) || imported_doc->getObjectById(new_id)); elem->setAttribute("id", new_id); // Make a note of this change, if we need to fix up refs to it @@ -428,11 +407,10 @@ static void change_clashing_ids(SPDocument *imported_doc, SPDocument *current_do } } - // recurse for (auto& child: elem->children) { - change_clashing_ids(imported_doc, current_doc, &child, refmap, id_changes, from_clipboard); + change_clashing_ids(imported_doc, current_doc, &child, refmap, id_changes); } } @@ -461,14 +439,14 @@ fix_up_refs(refmap_type const &refmap, const id_changelist_type &id_changes) * clash with IDs in the existing document are changed, and references to * those IDs are updated accordingly. */ -void prevent_id_clashes(SPDocument *imported_doc, SPDocument *current_doc, bool from_clipboard) +void prevent_id_clashes(SPDocument *imported_doc, SPDocument *current_doc) { refmap_type refmap; id_changelist_type id_changes; SPObject *imported_root = imported_doc->getRoot(); - find_references(imported_root, refmap, from_clipboard); - change_clashing_ids(imported_doc, current_doc, imported_root, refmap, &id_changes, from_clipboard); + find_references(imported_root, refmap); + change_clashing_ids(imported_doc, current_doc, imported_root, refmap, &id_changes); fix_up_refs(refmap, id_changes); } @@ -482,7 +460,7 @@ change_def_references(SPObject *from_obj, SPObject *to_obj) SPDocument *current_doc = from_obj->document; std::string old_id(from_obj->getId()); - find_references(current_doc->getRoot(), refmap, false); + find_references(current_doc->getRoot(), refmap); refmap_type::const_iterator pos = refmap.find(old_id); if (pos != refmap.end()) { @@ -585,7 +563,7 @@ void rename_id(SPObject *elem, Glib::ustring const &new_name) SPDocument *current_doc = elem->document; refmap_type refmap; - find_references(current_doc->getRoot(), refmap, false); + find_references(current_doc->getRoot(), refmap); std::string old_id(elem->getId()); if (current_doc->getObjectById(id)) { diff --git a/src/id-clash.h b/src/id-clash.h index 8668203c90..1d2bcab4be 100644 --- a/src/id-clash.h +++ b/src/id-clash.h @@ -15,7 +15,7 @@ class SPDocument; class SPObject; -void prevent_id_clashes(SPDocument *imported_doc, SPDocument *current_doc, bool from_clipboard = false); +void prevent_id_clashes(SPDocument *imported_doc, SPDocument *current_doc); void rename_id(SPObject *elem, Glib::ustring const &newname); void change_def_references(SPObject *replace_obj, SPObject *with_obj); Glib::ustring generate_similar_unique_id(SPDocument *document, Glib::ustring const &base_name); diff --git a/src/ui/clipboard.cpp b/src/ui/clipboard.cpp index 79196e35ac..a65d501ade 100644 --- a/src/ui/clipboard.cpp +++ b/src/ui/clipboard.cpp @@ -497,7 +497,7 @@ void ClipboardManagerImpl::insertSymbol(SPDesktop *desktop, Geom::Point const &s return; } - prevent_id_clashes(symbol.get(), desktop->getDocument(), true); + prevent_id_clashes(symbol.get(), desktop->getDocument()); auto *root = symbol->getRoot(); // Synthesize a clipboard position in order to paste the symbol where it got dropped. @@ -562,7 +562,7 @@ bool ClipboardManagerImpl::paste(SPDesktop *desktop, bool in_place, bool on_page } // copy definitions - prevent_id_clashes(tempdoc.get(), desktop->getDocument(), true); + prevent_id_clashes(tempdoc.get(), desktop->getDocument()); sp_import_document(desktop, tempdoc.get(), in_place, on_page); // _copySelection() has put all items in groups, now ungroup them (preserves transform -- GitLab From 132ebf2dcaf0d1ebbfb3b2a19f2d364c396d7ffd Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 31 May 2025 17:20:26 +0200 Subject: [PATCH 5/9] Use pointers to const in repr map --- src/document.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/document.h b/src/document.h index 06566aad0b..72bec1c779 100644 --- a/src/document.h +++ b/src/document.h @@ -414,7 +414,7 @@ private: // Find items ---------------------------- std::map iddef; - std::map reprdef; + std::map reprdef; // Find items by geometry -------------------- mutable std::map> _node_cache; // Used to speed up search. -- GitLab From 92f979d43871dda2057697f8b9fd30b6b9560b0c Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 31 May 2025 17:22:36 +0200 Subject: [PATCH 6/9] Rewrite SPDocument::importDefs() The function for importing the from an external document into the current one is rewritten. The rewrite takes advantage of the functions defined in id-clash.cpp, some of which were not available at the time when SPDocument code was written. In addition to eliminating a separate, duplicate ID clash resolution mechanism (which is useless when we already use id-clash.cpp), the logic for special handling of certain SPObject types is eliminated. This logic didn't really belong in the SPDocument; in fact, a document is a "container" of sorts for many different objects and should not know or care about the specific needs of these objects. One way in which the logic of this function has been radically simplified is that the resolution of ID clashes is no longer performed. Instead, the function expects as a precondition that there are no shared IDs between the current document and the source document from which to import . --- src/document.cpp | 240 ++++++++++--------------------------------- src/document.h | 22 +++- src/file.cpp | 4 +- src/ui/clipboard.cpp | 4 +- 4 files changed, 75 insertions(+), 195 deletions(-) diff --git a/src/document.cpp b/src/document.cpp index 7345f55b32..12c0f74286 100644 --- a/src/document.cpp +++ b/src/document.cpp @@ -79,7 +79,6 @@ #include "object/sp-namedview.h" #include "object/sp-page.h" #include "object/sp-root.h" -#include "object/sp-symbol.h" #include "ui/widget/canvas.h" #include "ui/widget/desktop-widget.h" #include "util/units.h" @@ -2004,204 +2003,69 @@ void SPDocument::setModifiedSinceSave(bool modified) _saved_or_modified_signal.emit(); } -/** - * Paste SVG defs from the document retrieved from the clipboard or imported document into the active document. - * @param clipdoc The document to paste. - * @pre @c clipdoc != NULL and pasting into the active document is possible. - */ -void SPDocument::importDefs(SPDocument *source) -{ - Inkscape::XML::Node *root = source->getReprRoot(); - Inkscape::XML::Node *target_defs = this->getDefs()->getRepr(); - std::vector defsNodes = sp_repr_lookup_name_many(root, "svg:defs"); - - prevent_id_clashes(source, this); - - for (auto & defsNode : defsNodes) { - _importDefsNode(source, const_cast(defsNode), target_defs); - } -} - -void SPDocument::_importDefsNode(SPDocument *source, Inkscape::XML::Node *defs, Inkscape::XML::Node *target_defs) -{ - int stagger=0; - - /* Note, "clipboard" throughout the comments means "the document that is either the clipboard - or an imported document", as importDefs is called in both contexts. - - The order of the records in the clipboard is unpredictable and there may be both - forward and backwards references to other records within it. There may be definitions in - the clipboard that duplicate definitions in the present document OR that duplicate other - definitions in the clipboard. (Inkscape will not have created these, but they may be read - in from other SVG sources.) - - There are 3 passes to clean this up: - - In the first find and mark definitions in the clipboard that are duplicates of those in the - present document. Change the ID to "RESERVED_FOR_INKSCAPE_DUPLICATE_DEF_XXXXXXXXX". - (Inkscape will not reuse an ID, and the XXXXXXXXX keeps it from automatically creating new ones.) - References in the clipboard to the old clipboard name are converted to the name used - in the current document. - - In the second find and mark definitions in the clipboard that are duplicates of earlier - definitions in the clipbard. Unfortunately this is O(n^2) and could be very slow for a large - SVG with thousands of definitions. As before, references are adjusted to reflect the name - going forward. - - In the final cycle copy over those records not marked with that ID. - - If an SVG file uses the special ID it will cause problems! - - If this function is called because of the paste of a true clipboard the caller will have passed in a - COPY of the clipboard items. That is good, because this routine modifies that document. If the calling - behavior ever changes, so that the same document is passed in on multiple pastes, this routine will break - as in the following example: - 1. Paste clipboard containing B same as A into document containing A. Result, B is dropped - and all references to it will point to A. - 2. Paste same clipboard into a new document. It will not contain A, so there will be unsatisfied - references in that window. - */ - - std::string DuplicateDefString = "RESERVED_FOR_INKSCAPE_DUPLICATE_DEF"; - - /* First pass: remove duplicates in clipboard of definitions in document */ - for (Inkscape::XML::Node *def = defs->firstChild() ; def ; def = def->next()) { - if(def->type() != Inkscape::XML::NodeType::ELEMENT_NODE)continue; - /* If this clipboard has been pasted into one document, and is now being pasted into another, - or pasted again into the same, it will already have been processed. If we detect that then - skip the rest of this pass. */ - Glib::ustring defid = def->attribute("id"); - if( defid.find( DuplicateDefString ) != Glib::ustring::npos )break; - - SPObject *src = source->getObjectByRepr(def); - - // Prevent duplicates of solid swatches by checking if equivalent swatch already exists - auto s_gr = cast(src); - auto s_lpeobj = cast(src); - if (src && (s_gr || s_lpeobj)) { - for (auto& trg: getDefs()->children) { - auto t_gr = cast(&trg); - if (src != &trg && s_gr && t_gr) { - if (s_gr->isEquivalent(t_gr)) { - // Change object references to the existing equivalent gradient - Glib::ustring newid = trg.getId(); - if (newid != defid) { // id could be the same if it is a second paste into the same document - change_def_references(src, &trg); - } - gchar *longid = g_strdup_printf("%s_%9.9d", DuplicateDefString.c_str(), stagger++); - def->setAttribute("id", longid); - g_free(longid); - // do NOT break here, there could be more than 1 duplicate! - } - } - auto t_lpeobj = cast(&trg); - if (src != &trg && s_lpeobj && t_lpeobj) { - if (t_lpeobj->is_similar(s_lpeobj)) { - // Change object references to the existing equivalent gradient - Glib::ustring newid = trg.getId(); - if (newid != defid) { // id could be the same if it is a second paste into the same document - change_def_references(src, &trg); - } - gchar *longid = g_strdup_printf("%s_%9.9d", DuplicateDefString.c_str(), stagger++); - def->setAttribute("id", longid); - g_free(longid); - // do NOT break here, there could be more than 1 duplicate! - } - } - } - } - } +void SPDocument::importDefs(SPDocument &source) +{ + prevent_id_clashes(&source, this); - /* Second pass: remove duplicates in clipboard of earlier definitions in clipboard */ - for (Inkscape::XML::Node *def = defs->firstChild() ; def ; def = def->next()) { - if(def->type() != Inkscape::XML::NodeType::ELEMENT_NODE)continue; - Glib::ustring defid = def->attribute("id"); - if( defid.find( DuplicateDefString ) != Glib::ustring::npos )continue; // this one already handled - SPObject *src = source->getObjectByRepr(def); - auto s_lpeobj = cast(src); - auto s_gr = cast(src); - if (src && (s_gr || s_lpeobj)) { - for (Inkscape::XML::Node *laterDef = def->next() ; laterDef ; laterDef = laterDef->next()) { - SPObject *trg = source->getObjectByRepr(laterDef); - auto t_gr = cast(trg); - if (trg && (src != trg) && s_gr && t_gr) { - Glib::ustring newid = trg->getId(); - if (newid.find(DuplicateDefString) != Glib::ustring::npos) - continue; // this one already handled - if (t_gr && s_gr->isEquivalent(t_gr)) { - // Change object references to the existing equivalent gradient - // two id's in the clipboard should never be the same, so always change references - change_def_references(trg, src); - gchar *longid = g_strdup_printf("%s_%9.9d", DuplicateDefString.c_str(), stagger++); - laterDef->setAttribute("id", longid); - g_free(longid); - // do NOT break here, there could be more than 1 duplicate! - } - } - auto t_lpeobj = cast(trg); - if (trg && (src != trg) && s_lpeobj && t_lpeobj) { - Glib::ustring newid = trg->getId(); - if (newid.find(DuplicateDefString) != Glib::ustring::npos) - continue; // this one already handled - if (t_lpeobj->is_similar(s_lpeobj)) { - // Change object references to the existing equivalent gradient - // two id's in the clipboard should never be the same, so always change references - change_def_references(trg, src); - gchar *longid = g_strdup_printf("%s_%9.9d", DuplicateDefString.c_str(), stagger++); - laterDef->setAttribute("id", longid); - g_free(longid); - // do NOT break here, there could be more than 1 duplicate! - } - } - } - } + for (auto defsNode : sp_repr_lookup_name_many(source.rroot, "svg:defs")) { + _importDefsNode(source, const_cast(defsNode)); } +} - /* Final pass: copy over those parts which are not duplicates */ - for (Inkscape::XML::Node *def = defs->firstChild() ; def ; def = def->next()) { - if(def->type() != Inkscape::XML::NodeType::ELEMENT_NODE)continue; - - /* Ignore duplicate defs marked in the first pass */ - Glib::ustring defid = def->attribute("id"); - if( defid.find( DuplicateDefString ) != Glib::ustring::npos )continue; - - bool duplicate = false; - SPObject *src = source->getObjectByRepr(def); +/** Import all children of the defs_to_import into the defs of this document. + * + * @pre There are no ID clashes between this document and source_document. + * + * @param[in,out] source_document The document from which to import defs. + * @param[in,out] defs_to_import A specific node whose children to import. + * + * If a resource in the source_document's defs is identical to a resource we already have, we simply change the ID of + * this resource in the source_document to match the ID of the equivalent resource in this document. After this + * operation, the IDs will no longer be unique between our document and the source document. However, objects that were + * referencing the resource within the source_document will be correctly repointed to the equivalent resource in our + * , so these references will still work after these objects are imported into our document. + */ +void SPDocument::_importDefsNode(SPDocument &source_document, Inkscape::XML::Node *defs_to_import) +{ + for (auto *input_defs_child_node = defs_to_import->firstChild(); input_defs_child_node; + input_defs_child_node = input_defs_child_node->next()) { + if (input_defs_child_node->type() != Inkscape::XML::NodeType::ELEMENT_NODE) { + continue; + } - // Prevent duplication of symbols... could be more clever. - // The tag "_inkscape_duplicate" is added to "id" by ClipboardManagerImpl::copySymbol(). - // We assume that symbols are in defs section (not required by SVG spec). - if (src && is(src)) { + auto &our_defs = getDefs()->children; + auto *foreign_resource = source_document.getObjectByRepr(input_defs_child_node); - Glib::ustring id = src->getRepr()->attribute("id"); - size_t pos = id.find( "_inkscape_duplicate" ); - if( pos != Glib::ustring::npos ) { + auto const existing_equivalent_resource = + std::find_if(our_defs.begin(), our_defs.end(), [foreign_resource](auto const &our_object) { + return foreign_resource->isEquivalent(our_object); + }); - // This is our symbol, now get rid of tag - id.erase( pos ); + if (existing_equivalent_resource != our_defs.end()) { + // We have an identical object, so we just need to give the two objects + // identical IDs. However, if that were to cause an ID clash within the source_document, + // we must still make a copy. + auto const id_in_our_document = existing_equivalent_resource->getId(); + auto const id_in_source_document = foreign_resource->getId(); - // Check that it really is a duplicate - for (auto& trg: getDefs()->children) { - if (is(&trg) && src != &trg) { - Glib::ustring id2 = trg.getRepr()->attribute("id"); + if (id_in_our_document == id_in_source_document) { + continue; // IDs are already identical, nothing to do. + } - if( !id.compare( id2 ) ) { - duplicate = true; - break; - } - } - } - if ( !duplicate ) { - src->setAttribute("id", id); - } + // Try to change the ID in source_document, but only if that ID is not taken yet. + // Although we started without ID clashes, this could happen if we rewrote the ID + // of another foreign resource identical to our existing equivalent resource. + if (!source_document.getObjectById(id_in_our_document)) { + change_def_references(source_document.getObjectByRepr(input_defs_child_node), + &*existing_equivalent_resource); + continue; } } - if (!duplicate) { - Inkscape::XML::Node * dup = def->duplicate(this->getReprDoc()); - target_defs->appendChild(dup); - Inkscape::GC::release(dup); - } + // We do not have this resource yet, so we must import it + auto *copy = input_defs_child_node->duplicate(rdoc); + getDefs()->appendChild(copy); + Inkscape::GC::release(copy); } } diff --git a/src/document.h b/src/document.h index 72bec1c779..8bbb12b46d 100644 --- a/src/document.h +++ b/src/document.h @@ -166,7 +166,7 @@ public: const Inkscape::Colors::DocumentCMS &getDocumentCMS() const { return *_cms_manager; } private: - void _importDefsNode(SPDocument *source, Inkscape::XML::Node *defs, Inkscape::XML::Node *target_defs); + void _importDefsNode(SPDocument &source_document, Inkscape::XML::Node *defs_to_import); SPObject *_activexmltree; std::unique_ptr _page_manager; @@ -187,7 +187,23 @@ private: public: void clearNodeCache() { _node_cache.clear(); } - void importDefs(SPDocument *source); + + /** + * Import the elements from all nodes of a source document into our document. + * + * If the ID of an element of the source document clashes with an ID already used in this document, the function + * will change the clashing ID in the source document to something else, relinking all references to maintain the + * consistency of the source document. + * + * It may happen that an object in the source_document's returns true from isEquivalent() when passed an + * object from this document's . In this case, the ID in the source object will be simply changed to match + * ours, provided that it is not taken yet, but such element will not be imported into our in order to + * prevent unnecessary duplication. + * + * @param[in,out] source The document from which to import all element nodes contained in . + * NOTE: The source document may be modified if IDs are rewritten. Please pass a copy if this is an issue for you. + */ + void importDefs(SPDocument &source); unsigned int vacuumDocument(); @@ -334,7 +350,7 @@ public: */ static SPItem *getItemFromListAtPointBottom(unsigned int dkey, SPGroup *group, const std::vector &list, Geom::Point const &p, bool take_insensitive = false); - + // TODO: remove this entire section: a document should not know about 3D Boxes or LPEs! // Box tool ------------------------------- void setCurrentPersp3D(Persp3D * const persp); /* diff --git a/src/file.cpp b/src/file.cpp index 5736c1c0c8..5895b14562 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -635,7 +635,7 @@ void sp_import_document(SPDesktop *desktop, SPDocument *clipdoc, bool in_place, } // copy definitions - desktop->doc()->importDefs(clipdoc); + desktop->doc()->importDefs(*clipdoc); Inkscape::XML::Node* clipboard = nullptr; // copy objects @@ -818,7 +818,7 @@ file_import(SPDocument *in_doc, const std::string &path, Inkscape::Extension::Ex prevent_id_clashes(doc.get(), in_doc); sp_file_fix_lpe(doc.get()); - in_doc->importDefs(doc.get()); + in_doc->importDefs(*doc); // The extension should set it's pages enabled or disabled when opening // in order to indicate if pages are being imported or if objects are. diff --git a/src/ui/clipboard.cpp b/src/ui/clipboard.cpp index a65d501ade..abd263d2fc 100644 --- a/src/ui/clipboard.cpp +++ b/src/ui/clipboard.cpp @@ -844,7 +844,7 @@ bool ClipboardManagerImpl::pasteStyle(ObjectSet *set) if (pasted) { // pasted style might depend on defs from the source - set->document()->importDefs(tempdoc.get()); + set->document()->importDefs(*tempdoc); } return pasted; @@ -939,7 +939,7 @@ bool ClipboardManagerImpl::pastePathEffect(ObjectSet *set) if ( clipnode ) { char const *effectstack = clipnode->attribute("inkscape:path-effect"); if ( effectstack ) { - set->document()->importDefs(tempdoc.get()); + set->document()->importDefs(*tempdoc); // make sure all selected items are converted to paths first (i.e. rectangles) set->toLPEItems(); auto itemlist= set->items(); -- GitLab From b8ccb38326e3a9079f0abaff87fdd79cf813b19a Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 31 May 2025 19:22:12 +0200 Subject: [PATCH 7/9] Fix Paste Path Effect This fixes the operation of "pasting a path effect" which is supposed to apply the LPE of a previously copied object onto other objects, onto which we paste. Fixes https://gitlab.com/inkscape/inkscape/-/issues/5694 --- src/ui/clipboard.cpp | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/ui/clipboard.cpp b/src/ui/clipboard.cpp index abd263d2fc..faa6db405a 100644 --- a/src/ui/clipboard.cpp +++ b/src/ui/clipboard.cpp @@ -919,6 +919,7 @@ bool ClipboardManagerImpl::pasteSize(ObjectSet *set, bool separately, bool apply */ bool ClipboardManagerImpl::pastePathEffect(ObjectSet *set) { + static constexpr auto PATH_EFFECT_ATTRIBUTE = "inkscape:path-effect"; /** @todo FIXME: pastePathEffect crashes when moving the path with the applied effect, segfaulting in fork_private_if_necessary(). */ @@ -934,22 +935,29 @@ bool ClipboardManagerImpl::pastePathEffect(ObjectSet *set) _retrieveClipboard("image/x-inkscape-svg"); auto &tempdoc = _clipboardSPDoc; if (tempdoc) { - Inkscape::XML::Node *root = tempdoc->getReprRoot(); - Inkscape::XML::Node *clipnode = sp_repr_lookup_name(root, "inkscape:clipboard", 1); - if ( clipnode ) { - char const *effectstack = clipnode->attribute("inkscape:path-effect"); - if ( effectstack ) { - set->document()->importDefs(*tempdoc); - // make sure all selected items are converted to paths first (i.e. rectangles) - set->toLPEItems(); - auto itemlist= set->items(); - for(auto i=itemlist.begin();i!=itemlist.end();++i){ - SPItem *item = *i; - _applyPathEffect(item, effectstack); - item->doWriteTransform(item->transform); - } + if (auto *clipnode = sp_repr_lookup_name(tempdoc->getReprRoot(), "inkscape:clipboard", 1)) { + if (char const *effectstack = clipnode->attribute(PATH_EFFECT_ATTRIBUTE)) { + Glib::ustring clipboard_lpe_id = effectstack; + clipboard_lpe_id = clipboard_lpe_id.substr(clipboard_lpe_id.find_last_of('#') + 1); + + if (auto const *lpe_object = tempdoc->getObjectById(clipboard_lpe_id)) { + set->document()->importDefs(*tempdoc); + + // Issue https://gitlab.com/inkscape/inkscape/-/issues/5694: The ID can change + // during the import of as a part of ID clash prevention between documents. + clipboard_lpe_id = lpe_object->getId(); + auto const lpe_href = '#' + clipboard_lpe_id; + clipnode->setAttribute(PATH_EFFECT_ATTRIBUTE, lpe_href); + + // make sure all selected items are converted to paths first (i.e. rectangles) + set->toLPEItems(); + for (auto item : set->items()) { + _applyPathEffect(item, lpe_href.c_str()); + item->doWriteTransform(item->transform); + } - return true; + return true; + } } } } -- GitLab From c632e8c2f946077514e6dc53a4be3855ff306837 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Mon, 9 Jun 2025 18:33:52 +0200 Subject: [PATCH 8/9] Testing system improvements Enable linking an explicit library to a unit test. Fix the detection of main source file. Eliminate ambiguous base class method calls by using virtual inheritance. --- CMakeScripts/UnitTest.cmake | 24 ++++++++++++++++++------ testfiles/src/mocks/xml-document-mock.h | 4 ++-- testfiles/src/mocks/xml-node-mock.h | 2 +- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/CMakeScripts/UnitTest.cmake b/CMakeScripts/UnitTest.cmake index 85ac12dee1..e896e5115f 100644 --- a/CMakeScripts/UnitTest.cmake +++ b/CMakeScripts/UnitTest.cmake @@ -8,15 +8,17 @@ add_custom_target(unit_tests) function(add_unit_test test_name) set(MULTI_VALUE_ARGS "SOURCES" "EXTRA_LIBS") cmake_parse_arguments(ARG "UNUSED_OPTIONS" "TEST_SOURCE" "${MULTI_VALUE_ARGS}" ${ARGN}) - foreach(source_file ${ARG_SOURCES}) - list(APPEND test_sources "${CMAKE_SOURCE_DIR}/src/${source_file}") - endforeach() + if(EXISTS "${CMAKE_SOURCE_DIR}/testfiles/src/${test_name}.cpp") list(APPEND test_sources "${CMAKE_SOURCE_DIR}/testfiles/src/${test_name}.cpp") endif() - if(EXISTS "${CMAKE_SOURCE_DIR}/testfiles/src/${TEST_SOURCE}") - list(APPEND test_sources "${CMAKE_SOURCE_DIR}/testfiles/src/${TEST_SOURCE}") + if(NOT ("${ARG_TEST_SOURCE}" STREQUAL "") AND EXISTS "${CMAKE_SOURCE_DIR}/testfiles/src/${ARG_TEST_SOURCE}") + list(APPEND test_sources "${CMAKE_SOURCE_DIR}/testfiles/src/${ARG_TEST_SOURCE}") endif() + foreach(source_file ${ARG_SOURCES}) + list(APPEND test_sources "${CMAKE_SOURCE_DIR}/src/${source_file}") + endforeach() + list(REMOVE_DUPLICATES test_sources) add_executable(${test_name} ${test_sources}) target_include_directories(${test_name} SYSTEM PRIVATE ${GTEST_INCLUDE_DIRS}) @@ -27,7 +29,17 @@ function(add_unit_test test_name) target_link_options(${test_name} PRIVATE "-fsanitize=address") target_link_libraries(${test_name} GTest::gtest GTest::gmock GTest::gmock_main ${ARG_EXTRA_LIBS}) + # Some EXTRA_LIBS may be CMake targets, while others may be directly provided linker flags + # such as -lmylib. Only add the targets as dependencies, skipping the direct linker flags. + foreach(extra_lib ${ARG_EXTRA_LIBS}) + string(FIND "${extra_lib}" "-l" direct_link_prefix) + if (NOT direct_link_prefix EQUAL 0) + list(APPEND dependency_libs "${extra_lib}") + endif() + endforeach() + + add_test(NAME ${test_name} COMMAND ${test_name}) - add_dependencies(unit_tests ${test_name} ${ARG_EXTRA_LIBS}) + add_dependencies(unit_tests ${test_name} ${dependency_libs}) endfunction(add_unit_test) diff --git a/testfiles/src/mocks/xml-document-mock.h b/testfiles/src/mocks/xml-document-mock.h index b3997cdf33..621295aa69 100644 --- a/testfiles/src/mocks/xml-document-mock.h +++ b/testfiles/src/mocks/xml-document-mock.h @@ -23,8 +23,8 @@ namespace Inkscape::XML::Mock { struct Document - : Mock::Node - , XML::Document + : virtual Mock::Node + , virtual XML::Document { Mock::Node &asNode() { return *this; } diff --git a/testfiles/src/mocks/xml-node-mock.h b/testfiles/src/mocks/xml-node-mock.h index 4ccddd9c58..f1c76b5737 100644 --- a/testfiles/src/mocks/xml-node-mock.h +++ b/testfiles/src/mocks/xml-node-mock.h @@ -22,7 +22,7 @@ namespace Inkscape::XML::Mock { -struct Node : public XML::Node +struct Node : virtual XML::Node { MOCK_CONST_METHOD0(type, NodeType()); MOCK_CONST_METHOD0(name, char const *()); -- GitLab From 70231b4dcd90717c1b75e52cf09ab52b67df563d Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Mon, 9 Jun 2025 18:35:17 +0200 Subject: [PATCH 9/9] Add an integration test for importDefs() This is an integration test which checks that the ID clash resolution works during the importDefs() operation and that moreover any elements referencing objects whose IDs are changed are correctly relinked. --- testfiles/CMakeLists.txt | 1 + testfiles/src/document-import-defs-test.cpp | 269 ++++++++++++++++++++ 2 files changed, 270 insertions(+) create mode 100644 testfiles/src/document-import-defs-test.cpp diff --git a/testfiles/CMakeLists.txt b/testfiles/CMakeLists.txt index 9764424c70..598f63beca 100644 --- a/testfiles/CMakeLists.txt +++ b/testfiles/CMakeLists.txt @@ -89,6 +89,7 @@ set(TEST_SOURCES colors/spaces-xyz-test colors/utils-test colors/xml-color-test + document-import-defs-test uri-test util-test util-units-test diff --git a/testfiles/src/document-import-defs-test.cpp b/testfiles/src/document-import-defs-test.cpp new file mode 100644 index 0000000000..ffb397679c --- /dev/null +++ b/testfiles/src/document-import-defs-test.cpp @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/** + * Authors: + * Rafał Siejakowski + * + * @copyright + * Copyright (C) 2025 Authors + * + * Released under GNU GPL v2+, read the file 'COPYING' for more information. + * + * @file @brief Unit tests SPDocument::importDefs() + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "document.h" +#include "inkscape.h" +#include "object/sp-defs.h" +#include "object/sp-object.h" +#include "object/sp-path.h" +#include "object/sp-use.h" +#include "style.h" + +using namespace ::testing; + +namespace { + +/// Create a span not including the null-terminator. +constexpr std::span static_string_to_span(char const *compile_time_string) +{ + return {compile_time_string, compile_time_string + std::char_traits::length(compile_time_string)}; +} +} // namespace + +struct ImportDefsTest : public Test +{ + ImportDefsTest() + { + if (!Inkscape::Application::exists()) { + Inkscape::Application::create(false); + } + } + + static constexpr auto OUR_DOC = static_string_to_span( + R"EOD( + + + + + + )EOD"); +}; + +/// Check that external doc's defs are simply appended to ours when there are no ID clashes +TEST_F(ImportDefsTest, NoClashCase) +{ + auto our_doc = SPDocument::createNewDocFromMem(OUR_DOC, true); + + static constexpr auto EXTERNAL_DOC_NO_CLASH = static_string_to_span( + R"EOD( + + + + + + )EOD"); + auto external_doc = SPDocument::createNewDocFromMem(EXTERNAL_DOC_NO_CLASH, true); + + our_doc->importDefs(*external_doc); + + auto const *defs_after_import = our_doc->getDefs(); + ASSERT_TRUE(defs_after_import); + + auto const &defs_children = defs_after_import->children; + EXPECT_EQ(defs_children.size(), 4U); + + std::set def_ids; + std::transform(defs_children.begin(), defs_children.end(), std::inserter(def_ids, def_ids.begin()), + std::mem_fn(&SPObject::getId)); + std::set const expected_ids{"our-circle", "our-rect", "their-circle", "their-rect"}; + EXPECT_EQ(def_ids, expected_ids); +} + +/// Check that ID clashes are successfully resolved +TEST_F(ImportDefsTest, ClashResolution) +{ + auto our_doc = SPDocument::createNewDocFromMem(OUR_DOC, true); + + static constexpr auto EXTERNAL_DOC_CLASH = static_string_to_span( + R"EOD( + + + + + + )EOD"); + auto external_doc = SPDocument::createNewDocFromMem(EXTERNAL_DOC_CLASH, true); + + our_doc->importDefs(*external_doc); + + auto const *defs_after_import = our_doc->getDefs(); + ASSERT_TRUE(defs_after_import); + + auto const &defs_children = defs_after_import->children; + EXPECT_EQ(defs_children.size(), 4U); + + std::set def_ids; + std::transform(defs_children.begin(), defs_children.end(), std::inserter(def_ids, def_ids.begin()), + std::mem_fn(&SPObject::getId)); + + // We expect that the IDs haven't changed except to resolve the conflicts + std::set const expected_ids{"our-circle", "our-rect", "some-rect"}; + + // Check that the with a clashing ID has a new ID + std::set new_ids; + std::set_difference(def_ids.cbegin(), def_ids.cend(), expected_ids.begin(), expected_ids.end(), + std::inserter(new_ids, new_ids.begin())); + ASSERT_EQ(new_ids.size(), 1U); + + auto const *path = our_doc->getObjectById(new_ids.begin()->c_str()); + ASSERT_TRUE(path); + EXPECT_TRUE(is(path)); +} + +/// Check that ID clash resolution triggers an update of hrefs in source document. +TEST_F(ImportDefsTest, ClashResolutionRelinks) +{ + auto our_doc = SPDocument::createNewDocFromMem(OUR_DOC, true); + + static constexpr auto EXTERNAL_DOC_REFERENCE_TO_CLASH = static_string_to_span( + R"EOD( + + + + + + + )EOD"); + auto external_doc = SPDocument::createNewDocFromMem(EXTERNAL_DOC_REFERENCE_TO_CLASH, true); + + our_doc->importDefs(*external_doc); + + auto const *defs_after_import = our_doc->getDefs(); + ASSERT_TRUE(defs_after_import); + + EXPECT_EQ(defs_after_import->children.size(), 4U); + + // Check that the use element is still correctly linking to something + auto const *referencing_element = external_doc->getObjectById("use-element"); + ASSERT_TRUE(referencing_element); + auto const *use_element = cast(referencing_element); + ASSERT_TRUE(use_element); + + std::string const new_href = use_element->href; + auto const *referenced_element = use_element->trueOriginal(); + ASSERT_TRUE(referenced_element); + + // Check that we have the new element with the same ID + auto const expected_href = std::string("#") + referenced_element->getId(); + EXPECT_EQ(expected_href, new_href); + EXPECT_TRUE(our_doc->getObjectById(referenced_element->getId())); + EXPECT_TRUE(our_doc->getObjectByHref(expected_href)); +} + +/// Check that an identical swatch in an external document is reused +TEST_F(ImportDefsTest, ReuseSwatch) +{ + static constexpr auto DOC_WITH_SWATCH = static_string_to_span( + R"EOD( + + + + + + + + )EOD"); + auto our_doc = SPDocument::createNewDocFromMem(DOC_WITH_SWATCH, true); + + static constexpr auto DOC_WITH_SAME_SWATCH = static_string_to_span( + R"EOD( + + + + + + + + )EOD"); + auto external_doc = SPDocument::createNewDocFromMem(DOC_WITH_SAME_SWATCH, true); + + auto const *defs_before_import = our_doc->getDefs(); + ASSERT_TRUE(defs_before_import); + EXPECT_EQ(defs_before_import->children.size(), 1U); + + our_doc->importDefs(*external_doc); + + auto const *defs_after_import = our_doc->getDefs(); + ASSERT_TRUE(defs_after_import); + + // Expect that the swatch is not duplicated + EXPECT_EQ(defs_after_import->children.size(), 1U); + EXPECT_STREQ(defs_after_import->firstChild()->getId(), "swatch"); + + // Check that other document's rect is relinked to refer to "#swatch" as fill + auto const *external_rect = external_doc->getObjectById("their-rect"); + ASSERT_TRUE(external_rect); + ASSERT_TRUE(external_rect->style); + ASSERT_TRUE(external_rect->style->fill.href); + + auto const *uri = external_rect->style->fill.href->getURI(); + ASSERT_TRUE(uri); + + EXPECT_STREQ(uri->getFragment(), "swatch"); // Was "same-swatch" +} + +/// Check that a swatch is still imported if it is different +TEST_F(ImportDefsTest, CopySwatchIfDifferent) +{ + static constexpr auto DOC_WITH_SWATCH = static_string_to_span( + R"EOD( + + + + + + + + )EOD"); + auto our_doc = SPDocument::createNewDocFromMem(DOC_WITH_SWATCH, true); + + static constexpr auto DOC_WITH_DIFFERENT_SWATCH = static_string_to_span( + R"EOD( + + + + + + + + )EOD"); + auto external_doc = SPDocument::createNewDocFromMem(DOC_WITH_DIFFERENT_SWATCH, true); + + auto const *defs_before_import = our_doc->getDefs(); + ASSERT_TRUE(defs_before_import); + EXPECT_EQ(defs_before_import->children.size(), 1U); + + our_doc->importDefs(*external_doc); + + auto const *defs_after_import = our_doc->getDefs(); + ASSERT_TRUE(defs_after_import); + + // Expect that we have two swatches now + auto const &defs_children = defs_after_import->children; + EXPECT_EQ(defs_children.size(), 2U); + + std::set const expected_swatch_ids{"swatch", "different-color-swatch"}; + std::set actual_swatch_ids; + std::transform(defs_children.begin(), defs_children.end(), + std::inserter(actual_swatch_ids, actual_swatch_ids.begin()), std::mem_fn(&SPObject::getId)); + + EXPECT_EQ(actual_swatch_ids, expected_swatch_ids); +} -- GitLab