zathras / jovial_svg

Flutter library for robust, efficient rendering of SVG static images
BSD 3-Clause "New" or "Revised" License
115 stars 21 forks source link

Make svg nodes cloning method public, please #107

Closed CyrusKrasnikov closed 4 months ago

CyrusKrasnikov commented 4 months ago

This cool library already implements cloning (i.e. SvgGroup._clone();), but it`s private. It would be nice to make it public, so the node could be copied and added to the dom easily.

I would like to clone and place a copy of SvgGroup (with changed transform attribute) to modify existing image on the fly.

I wonder how to update DOM lookup with ID of added Node to hit test node later. For example of cloning and adding;

final group = svg.dom.idLookup[exportedId] as SvgGroup;
final clonedNode = group.clone(); // clone method is private
clonedNode.id = "newId";
svg.dom.root.children.add(clonedNode);
//svg.dom.resetIDLookup(); // reset just nullifies lookup dictionary. I cannot hit test added node
zathras commented 4 months ago

That's not what the (internal) clone method is for. It does a deep copy, which is not what you want for the stated use case. Just do a shallow copy yourself - that's like two lines of code using the exposed API.

CyrusKrasnikov commented 4 months ago

Indeed I want a deep copy with children nodes, which can also have children and attributes. Thats why I was happy to find this method "_clone" to not do the same thing. Making it public would be useful in that scenario. Thank you anyway!

zathras commented 4 months ago

Well, that of course begs the question "how deep?" For example, would you expect a copy of the Uint8List in an SvgImage? This is a trick question. No matter which option you choose, someone would expect the other one.

Exposing a generic clone operation is fraught with peril, for this and other reasons. Check out https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#clone-- to see some of the things one needs to consider when going down the clone rabbit hole in a public API.

CyrusKrasnikov commented 4 months ago

Yeap it`s a question to think about! The current implementation (imageData = other.imageData) is sufficient enough since it shows the original SvgImage and cloned SvgImage as expected (underlying Uint8List is the same - even if one changes it then sees two identical images (clones) in different locations). If one needs to change cloned SvgImage.imageData then no problem - just copy Uint8List or replaces it in SvgImage.imageData with other Uint8List.

Currently I use XmlElement.copy to clone SVG group with attributes and all its descendants, then construct another svg string and use SvgDOMManager.fromString. Eventually I extract SvgGroup from a new SvgDOMManager and add it to the original image. Well it is a very long way for such simple thing as DOM cloning, which is public and available in browser DOM and in XmlElement.

By the way it would be nice to store non-svg attributes in SvgNode too. But it`s ok - I just use Map to find XmlElement that corresponds to SvgGroup and change my "data-' attributes :)

zathras commented 4 months ago

It seems like it would be easier, and a lot faster to just make your own copy method, using the public API and a typecase for the (presumably small number of) node types whose instances you actually want to modify.