Reimplement the import of defs from an external document
Overview
Importing the contents of another document's <defs> into our document is an important operation used during:
- document import
- paste operations
- specialized paste-like operations (Paste LPE)
- drag & drop
The main difficulty here is the need to handle clashing IDs between the external document and our document, which could otherwise lead to invalid SVGs and bugs.
The status quo
The problem of ID clash resolution and ID change with relinking of references has been comprehensively solved in a dedicated library (id-clash.cpp). However, SPDocument::importDefs() did not use that system to its full potential, relying instead in its own, independent clash resolution mechanism.
These two mechanisms failed to work together correctly, leading to severe bugs such as #5694.
As a band-aid solution, a special "clipboard" logic was introduced at some point to switch between one or the other, but that too failed to work correctly.
In some cases, such as with swatches, we want to skip the import of elements which are identical to those already contained in the <defs> of our document, in order to prevent duplication. Unfortunately, this mechanism was implemented in a way which created unnecessary coupling between the SPDocument and those specific reuse concerns.
Reimplementation
The member function SPDocument::importDefs() has been radically simplified. The question of whether a resource already present in our <defs> is "equivalent" to an external one, and therefore could be reused, is delegated to the SPObjects themselves. Overrides are provided for object types that used this mechanism before: SPSymbol, SPGradient and LivePathEffectObject. The "clipboard-specific" logic is removed as it cannot be viewed as a part of the basic task of removing ID clashes.
Future design improvements
Managing <defs> is a sufficiently complex task that it should perhaps be split to its own class and perhaps merged with the "document resources" system. In this way, SPDocument would own a ResourceManager or DefsManager (name to be decided) in analogy to the PageManager or LayerManager today.
Moreover, the inclusion of the isEquivalent() method in SPObject is also not ideal, as it grows an already large interface. Better interface segregation could be obtained through the introduction of external resource reuse predicates. For now, an extension of the current object-oriented approach seems appropriate though.
Testing
An integration test has been added specifically for importDefs() on a few simple documents. The logic of reusing swatches is also tested.
This MR includes a few improvements to the unit testing setup. Unfortunately, it is not practical to test importDefs() with unit tests due to excessive coupling of SPDocument with other classes derived from SPObject. For example, SPDocument must know about SPRoot and SPDefs, and those in turn must know about SPGroup, SPItem and even SPLPEItem, which is very difficult to mock.
Further design improvements aimed at improving testability should include:
- Decoupling
SPDocumentfrom anything related to rendering such asDrawingItem; - Decoupling from libavoid (currently
SPDocumentowns anAvoid::Router); - Decoupling from libcroco (currently
SPDocumentconstructor calls libcroco functions); - Splitting off the resource management as mentioned above.
Manual testing
Manual testing should include all operations listed in the bulleted list above, both within the same document and between documents.
Fixes #5694