SVG widget

[futurepaul]

I was correct, this was a lot of fun. The downside: it's a bloat monster because I'm relying on usvg to parse and simplify the incoming SVG for me. So perhaps it should be behind a feature flag?

One other caveat: clippy doesn't like the explicit return keyword in get_size(), but if I don't do that then the borrow checker gets mad about how long the *root.borrow() might live for.

[raphlinus]

Very cool! Initial thought is that for stuff that increases bloat but which not all clients will use, a feature flag is appropriate, though I'm sure we'll use this in Runebender.

I would file an issue against clippy. It feels like making a recommendation which breaks the code should be considered a bug. In the meantime, of course suppress the warning.

[Ralith]

Could this perhaps be a standalone crate that implements SVG drawing for piet, with an optional feature for a druid widget? I'd love to be able to easily pull in SVGs as part of more advanced custom rendering, which would be difficult to do when the logic's hardwired into a widget.

[futurepaul]

Could this perhaps be a standalone crate that implements SVG drawing for piet

Happy to make a standalone crate if that's what people prefer. In the meantime I could refactor this so the hardworking svg-to-piet functions are available without using the widget?

[Ralith]

Sounds like a good idea to me!

[futurepaul]

Turns out this specific borrow situation is a known problem with clippy: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

[cmyr]

Okay a few little observations but this is very cool!

[futurepaul]

I think everything is addressed. Only issue is that CI doesn't know about this feature flag. If it did it would notice those two (unrelated) clippy lints that show up when running with this flag.

[cmyr]

hmm, I guess we can't do cargo test --all-features?

[futurepaul]

I think it tries to run all the platform-specific stuff, so it has a ton of errors on my system.

[cmyr]

This looks reasonable to me. We'll want to revisit at some point when we support other image types, but I think this is a great addition.

[cmyr]

I think this is reasonable until we figure out a better error handling story. 🤷‍♀

[Ralith]

Is there a good reason not to return Result?

[cmyr]

not a great reason, but my thinking right now is that no other widget constructors are failable, and it feels weird to start here.

My long-term vision for this is that you shouldn't be loading data directly from the file system; there should be a 'resource bundle' that druid exposes that contains various types of resources (image files, font files, localization files) and that is validated at compile time, so that at runtime getting a resource looks something like,

let my_svg: SvgData = env.get_bundle_resource(MY_SVG_IDENTIFIER);

or maybe even in a very fancy world there's a build phase that generates per-app marker 'keys' (like env::Key) for each of the resources in this app's bundle?

[futurepaul]

I can open an issue for this if you'd like?

[cmyr]

sure!

[cmyr]

this looks good to me. @Ralith this is blocked on your requested changes, so if you want to take another look then we can merge on your ✅

[Ralith]

Couple style nits, looks good otherwise!

[Ralith]

Why not use conventional style here?

[futurepaul]

The borrow checker disagrees with clippy on the "needless" aspect: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

[RazrFalcon]

The downside: it's a bloat monster because I'm relying on usvg

Sad, but reasonable. I just wanted to note that usvg is absolutely stripped down. There are nothing to remove. The bloat comes mainly from harfbuzz bindings and there is nothing I can do about it. SVG is hard, text is hard.

PS: Fun fuct: I wrote cargo-bloat for resvg/usvg.

[futurepaul]

I didn't mean this as a criticism of usvg! Just in comparison to the average PR. Thank you for making this widget so straightforward to implement!