diff --git a/CMakeScripts/UnitTest.cmake b/CMakeScripts/UnitTest.cmake index 85ac12dee15e4aed6a433d922eab1a7462db5ffb..e896e5115f7faf2f6e82d1ae24f84d14bc6e0040 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/src/document.cpp b/src/document.cpp index 7345f55b32bd27a05f02ba59603ddd144f2806b7..12c0f742864d2e20ef5fdf2c533f0907dfd0a6a3 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 06566aad0b17f5d8ce73f0a04e6b83f4a9a36aa4..8bbb12b46dbfbb9f8460f5eac7678d0cc0657540 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); /* @@ -414,7 +430,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. diff --git a/src/file.cpp b/src/file.cpp index b4a2bd125eecc8eb075e48546c458a9ff01c9830..5895b1456253c4123e23fc0f9ca7cee487e32578 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); @@ -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 @@ -815,10 +815,10 @@ 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()); + 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/id-clash.cpp b/src/id-clash.cpp index 498f39629da186221307f8be57adced9c813504a..976d900ccc8a79bd24970065e1b99ea09d58775d 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 8668203c901690a68e76f649a0bfee0dac3dce47..1d2bcab4be7282df1370b4def3debce451cb6671 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/live_effects/lpeobject.cpp b/src/live_effects/lpeobject.cpp index 2f520c8016973b8b25b179a3cbb502f321dd4084..60262e1d65818bcb01d5aeb08b24139337aee461 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 663fa7576779746b27cbaeefbc81a28ef942b59d..24268f03ea0dfe9e4013413872e608b88c9cbe3c 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 29aacd5d4b99f50d984848f11771339e974825c9..11a097d95e2a21deeafff804cee12e944bdd7712 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 9b18454e3a714dca06d6c6292e01baa73a128f44..0aa9c7173dbeab6bdd1cc42de02aca5082261942 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; diff --git a/src/object/sp-object.h b/src/object/sp-object.h index a6ec4a0e6101b9729530b2eb0d79ce2fdb9ea0e7..3c9ec611ff97ccc1c192656d7759ad1d8bbe2038 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; diff --git a/src/object/sp-symbol.cpp b/src/object/sp-symbol.cpp index 138d77a8aa8677bcacce0159c214a4cad0e99279..5c4a53bfee0927268a8e45f9d6888c2890d82742 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 ab55995e809abd9a7d0109bc5b5e51b6b45d2bb3..a20e60efcfd4432325d2215d55b40886afbcb838 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; diff --git a/src/ui/clipboard.cpp b/src/ui/clipboard.cpp index 79196e35ac02588a52c091d7286fbbbed3ca9faa..faa6db405a8b47df2d121aff85c66bdfc1bd5774 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 @@ -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; @@ -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.get()); - // 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; + } } } } diff --git a/testfiles/CMakeLists.txt b/testfiles/CMakeLists.txt index 9764424c702d35ebac6fab6db92c4cd78841bdda..598f63beca39c8a488457d281bb1da8680f867a9 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 0000000000000000000000000000000000000000..ffb397679c93abac7ba6b2afa170de01b80ebc46 --- /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); +} diff --git a/testfiles/src/mocks/xml-document-mock.h b/testfiles/src/mocks/xml-document-mock.h index b3997cdf33754e9e9a39705702e639270b0c0633..621295aa69b23962c505c71579bcfd39863c3a1d 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 4ccddd9c582a05a95b5a3dd4312a97e11e5ba0b9..f1c76b573711a148f0228b56585aac749130dc22 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 *());