search

BigBadaboom / androidsvg

SVG rendering library for Android
http://bigbadaboom.github.io/androidsvg/
Apache License 2.0
1.21k stars 231 forks source link

Restructure parser options handling #203

Closed maniac103 closed 4 years ago

maniac103 commented 4 years ago

Instead of having global singletons for parser options (external file resolver, entity expansion), additionally make them settable on a per-parser basis and inherit those properties to the parsed SVG in that case. This allows e.g. having one file resolver per server in case multiple SVGs are fetched and parsed from different servers simultaneously.

Closes #199

BigBadaboom commented 4 years ago

Thank you! This looks very good. I hope to have some time next week to catch up on my backlog of AndroidSVG stuff. At which time I can do the merge, and write some tests.

BigBadaboom commented 4 years ago

@maniac103 Are you using the RenderOptions.setExternalFileResolver() method yourself? I feel that it is more of a parsing funtion than a render related thing. That's the only thing I would consider removing from this patch when I merge.

I'm open to arguments for it to stay though.

maniac103 commented 4 years ago

@BigBadaboom No, we aren't using this, we're only interested in per-SVG-parser setting of the external file resolver. For the RenderOptions case, I wanted to still allow specifying a resolver for the css() method, since that used the static resolver instance before. One could argue that that method indeed does parsing, so needs that resolver. I don't have any strong feeling towards whether this should be implemented via setter (as is currently) vs. adding an optional parameter for the resolver, though, I'm totally fine with either way.

BigBadaboom commented 4 years ago

@maniac103 Thanks

On Sun, 16 Aug 2020 at 05:57, maniac103 notifications@github.com wrote:

@BigBadaboom https://github.com/BigBadaboom No, we aren't using this, we're only interested in per-SVG-parser setting of the external file resolver. For the RenderOptions case, I wanted to still allow specifying a resolver for the css() method, since that used the static resolver instance before. One could argue that that method indeed does parsing, so needs that resolver. I don't have any strong feeling towards whether this should be implemented via setter (as is currently) vs. adding an optional parameter for the resolver, though, I'm totally fine with either way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BigBadaboom/androidsvg/pull/203#issuecomment-674429708, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGNEYBNJNDF5FZXOYDKIBTSA3EARANCNFSM4NSQVB3A .