meerk40t / svgelements

SVG Parsing for Elements, Paths, and other SVG Objects.
MIT License
127 stars 28 forks source link

original id of <use> elements is lost #196

Open xmarduel opened 1 year ago

xmarduel commented 1 year ago

it would be nice if the shapes defined through <use ...> could keep their original id - or at least have an attribute like "original_id" in the values dictionary - as well as for other properties like "style". (or an"original_values" attribute to held all original data)

Indeed for such 2 elements:

<defs><circle id="circle" cx="0" cy="0" r="5" style="opacity:1" /></defs>
<g>
  <use href="#circle" id="circle1" transform="translate(10,10)" style="opacity:0.5">
  <use href="#circle" id="circle2" transform="translate(20,20)" style="opacity:0.2">
</g>

the resulting 2 circles position are Ok, but both shapes have the same id "circle" instead of "circle1" and "circle2" which seems to be completely lost. The style as well is the one defined in the <defs> paragraph and not the one defined in the <use>.

tatarize commented 1 year ago
    def test_issue_196(self):
        q1 = io.StringIO(u'''<?xml version='1.0' encoding='UTF-8'?>
<svg version='1.1' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'>
<defs><circle id="circle" cx="0" cy="0" r="5" style="opacity:1" /></defs>
<g>
  <use href="#circle" id="circle1" transform="translate(10,10)" style="opacity:0.5"/>
  <use href="#circle" id="circle2" transform="translate(20,20)" style="opacity:0.2"/>
</g>
</svg>''')
        layout = SVG.parse(
            source=q1,
            reify=False,
            ppi=DEFAULT_PPI,
            color="black",
            transform=None,
            context=None
        )
        elements = list(layout.elements())
        for e in elements:
            print(f"ID: {e.id}: {e}")

Gives:

C:\Users\Tat\AppData\Local\Programs\Python\Python38-32\python.exe "C:/Program Files/JetBrains/PyCharm Community Edition 2022.1.3/plugins/python-ce/helpers/pycharm/_jb_unittest_runner.py" --target test_use.TestElementUse.test_issue_196 
Testing started at 7:37 PM ...
Launching unittests with arguments python -m unittest test_use.TestElementUse.test_issue_196 in C:\Users\Tat\PycharmProjects\svgelements\test

ID: None: [[[Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 10, 10), id='circle')], [Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 20, 20), id='circle')]]]
ID: None: [[Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 10, 10), id='circle')], [Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 20, 20), id='circle')]]
ID: circle1: [Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 10, 10), id='circle')]
ID: circle: Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 10, 10), id='circle')
ID: circle2: [Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 20, 20), id='circle')]
ID: circle: Circle(cx=0, cy=0, r=5, fill='#000000', transform=Matrix(1, 0, 0, 1, 20, 20), id='circle')

Ran 1 test in 0.006s

OK

Process finished with exit code 0

You'll notice that circle1 and circle2 are the id on the <use> and the circle inside the use has the id=circle.

print(layout.get_element_by_id("circle1").id) Gives circle1. It's a Use class object containing a circle with the id=circle.

There were a couple bugs that kicked off without doing things like this. Which was the last breaking change to make them work more correctly.

xmarduel commented 1 year ago

Dear tatarize, thanks for your quick answer (and for the beautiful module).

The whole point of the issue is "how to setup a mapping between one original use object (whose id is circle1 in this example) and its (from svgelements calculated) Shape counterpart (whose id is circle)?

OK, I see from the output that from the list of layout.elements() , for one item of type SVGElement the following item is its related Shape object (is it always so ?), so following this ordering a mapping can be setup. Good, but somehow not optimal, therefore the naive idea for a Shape object to store the id of the<use> object it derives from in whatever new attribute it may be - the Shape object id can freely remain circle. By the way, the problem of merging the styles also remain.

Best regards, XM

tatarize commented 1 year ago

If you can show that this is what Chrome or Inkscape uses for those ids. I would absolutely change it. The spec says Use is the object there and is a structural element. And that id belongs to the Use.

chrome

The issue here is in part that svgelements gives you the render tree and not the dom tree. In the dom tree the circles don't actually exist at all and as such they don't have ids. I invented them since I'm doing the rendering but they don't actually have any ids, so I read them out of the parsed tree structure. I didn't add them to get_element_by_id since that caused some serious issues with different bugs, and they strictly speaking there's not supposed to be a 1:1 set of changes between DOM tree and render tree. But, svgelements sorts of blends that.

Now granted these objects aren't really in the dom tree and the providing of the id is just useful (really rendered objects have no requirements to provide ids) you may be right. But, I could certainly see a lot of use cases where people might prefer the id given actually be the original id since you can actually reference a large branch of the dom tree in a use object.

So while this isn't covered by spec, it's generally okay, to spitball a little. I can't use the id of the use as every subobject's id, since that could basically wipe out the entire tree's ids. But, you are correct that this isn't really what I would expect to have intuitively.

What about setting the id equal to circle1 circle and circle2 circle respectively for those elements. Namely since use is going to create those elements out of basically rehashed versions of the dom tree we can use a somewhat standardish looking CSS Selector meaning Select all <element> inside <element> which is just a space there. We could also leave this in the proper lookup to be accessed with get_element_by_id() since they would be unique we know this because real ids can't have spaces in them (since allowing that would mess up the aforementioned CSS Selectors).

We wouldn't have multiple circles with the id circle and we would know which circle came from where. Though in the case of no id use objects we'd potentially duplicate the ids again since this would be <sub-id> where we just add in a space. There's still a few edge cases but this would be fairly straight-forward. And it would probably get classified as a breaking change so 1.9.x (since ids of objects under a use were previously the original duplicated ids).

xmarduel commented 1 year ago

Dear tatarize, thank you for your comments. Ok, Ok, you are in all cases right. Best regards, XM

tatarize commented 1 year ago

I'm open to some changes. It wouldn't be ideal but we could certainly make the id of objects within the use reference to be <use_id> <object_id> since those objects don't exist in the original DOM tree it makes some sense to add them.

tatarize commented 1 year ago

The writing of use objects that are created in actual tree might be a problem for writing out the data since by default it would reuse the id. I think this should actually be shifted so the use items are actually <use_id>.<object_id> since that would at least be a valid id for a save file.

snoyer commented 1 year ago

How about a version of select that would keep track of and expose the parent <use> elements?

from io import StringIO
from typing import Callable, Iterable, Optional

from svgelements import SVG, Group, Shape, SVGElement, Use

def select_with_parent_uses(group: Group, predicate: Optional[Callable[[SVGElement], bool]]=None) -> Iterable[tuple[SVGElement, tuple[Use,...]]]:
    def select(e:SVGElement, parents: tuple[Use,...]):
        if predicate is None or predicate(e):
            yield e, parents

        if isinstance(e, Use):
            new_parents = *parents, e
            for e2 in e:
                yield from select(e2, new_parents)
        elif isinstance(e, Group):
            for e2 in e:
                yield from select(e2, parents)

    yield from select(group, tuple())

if __name__ == '__main__':
    src = StringIO(u'''<?xml version='1.0' encoding='UTF-8'?>
<svg width="500" viewBox="-10 -10 35 30" version='1.1' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'>
<defs>
  <circle id="circle" cx="0" cy="0" r="5" style="opacity:1" />
  <g id="two-circles">
    <use href="#circle" id="blue_fill" transform="translate(-6 0)" fill="blue"/>
    <use href="#circle" id="red_fill" transform="translate(+6 0)" fill="red"/>
  </g>
</defs>
<g>
  <use href="#two-circles" id="white_stroke" transform="translate(5,0)" stroke="white"/>
  <use href="#two-circles" id="black_stroke" transform="translate(11,10)" stroke="black"/>
</g>
</svg>''')
    parsed = SVG.parse(source=src, reify=False)

    for e,uses in select_with_parent_uses(parsed, lambda e: isinstance(e, Shape)):
        print('/'.join(u.id for u in [*uses,e]), f'-> fill: {e.fill}, stroke: {e.stroke}')

        use_ids = set(u.id for u in uses)
        assert ('blue_fill' in use_ids) == (str(e.fill) == '#0000ff')
        assert ('red_fill' in use_ids) == (str(e.fill) == '#ff0000')
        assert ('white_stroke' in use_ids) == (str(e.stroke) == '#ffffff')
        assert ('black_stroke' in use_ids) == (str(e.stroke) == '#000000')

prints:

white_stroke/blue_fill/circle -> fill: #0000ff, stroke: #ffffff
white_stroke/red_fill/circle -> fill: #ff0000, stroke: #ffffff
black_stroke/blue_fill/circle -> fill: #0000ff, stroke: #000000
black_stroke/red_fill/circle -> fill: #ff0000, stroke: #000000