Account for transforms in layer metadata

[jni]

Closes #26

I haven't accounted for point/line extent in this PR. This was broken before
this and will stay broken after this, but is a somewhat orthogonal problem so
I'd rather leave it to a subsequent PR.

Similarly, we could optimise by pro-actively removing the no-op transforms from
the SVG before writing it out. I can do that here but I would rather focus on
getting it working first and then optimising later.

[jni]

@brisvag @andy-sweet I don't have a test but this is working! As you can see in the commit list it's been a journey. 😅

Re testing: indeed @andy-sweet I immediately ran into annoying differences in the test svg, like width and height being 128.0 instead of 128 and translate being -0 instead of 0. Maybe as a test dependency we could add https://pypi.org/project/svgelements/, which would help us test actual semantic info about the produced svg vs the reference svg, rather than straight-up text?

Maybe the same can be done with the plain xml tree but I have little experience working with it...

[andy-sweet]

e testing: indeed @andy-sweet I immediately ran into annoying differences in the test svg, like width and height being 128.0 instead of 128 and translate being -0 instead of 0. Maybe as a test dependency we could add https://pypi.org/project/svgelements/, which would help us test actual semantic info about the produced svg vs the reference svg, rather than straight-up text?

And all it took was one PR!

The diff itself (as a reviewer of the PR) is very easy to read, especially with the rendered visual diff provided by GitHub.

svgelements seems like the kind of thing that I'd want for more extensive testing that also doesn't rely on comparisons with blessed/golden-file checks. Probably best to add an issue where the main work is to define tests that actually check the content of the written SVGs and then mention svgelements as a useful option for parsing.

I.e. I don't think we should block this PR.

[andy-sweet]

I did some very brief manual testing with a points layer, and behavior was as expected after reading this PR. I made some optional suggestions and there are lots of future improvements, but nothing that should block this being merged.

[andy-sweet]

Given there is a top-level group element (in xml_to_svg) and the transform for all the circles is the same, I would expect the transform to be defined once there and for each circle to have no transform.

Does that make sense? Or am I misunderstanding something about the SVG spec?

Doesn't necessarily block this PR, but might be worth doing if it's simple and makes the PR smaller.

[andy-sweet]

I just realized we are using that transform to shift everything so the view-box can start at 0, so maybe this isn't as easy as I'd hoped.

[jni]

yeah so actually you're both right and wrong... There should be one transform per layer. When I looked at the svg changes I thought "huh, I guess he put each point in a different layer. No idea why you would do that, maybe to test multiple layers, but ok." But now I look at the code and I see that there's one transform per shape!

The top level transform is indeed used to make a tight boundary around the data. But looking at the svg spec again it does look like I can nest things with g elements: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/g, so a layer should actually go in a g element and the transforms should go in there. I'll have a look shortly.

[jni]

After looking at the code a bit I'm not super confident about the updates, and they will definitely require updating the reference svg, so I'd like to experiment after this PR. I'm going to merge and create issues for the remaining work. Thanks for the review @andy-sweet!

[andy-sweet]

I love the detailed comments here.