Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

original id of <use> elements is lost #196

Open
xmarduel opened this issue Sep 25, 2022 · 7 comments
Open

original id of <use> elements is lost #196

xmarduel opened this issue Sep 25, 2022 · 7 comments

Comments

@xmarduel
Copy link

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
Copy link
Member

tatarize commented Sep 26, 2022

    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
Copy link
Author

xmarduel commented Sep 29, 2022

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
Copy link
Member

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
Copy link
Author

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

@tatarize
Copy link
Member

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 tatarize reopened this Sep 30, 2022
@tatarize
Copy link
Member

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
Copy link

snoyer commented Jan 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants