flutter_svg icon indicating copy to clipboard operation
flutter_svg copied to clipboard

Defs and refs

Open ikbendewilliam opened this issue 1 year ago • 2 comments

Fixes #102 Defs and refs can now be used anywhere, the parsing is now partially async and will await for the def/ref to be defined or throw a warning (like previously) when not found and the full file is processed. This is a very different approach than #748 which only solves defs and has a lower impact This is also not (yet) compatible with #754 I haven't done a benchmark yet, so I don't know the full impact yet, but I've tried to minimise it. Let me know if this is something you expect 😄.

ikbendewilliam avatar Aug 27 '22 16:08 ikbendewilliam

I haven't read this whole PR, but a couple comments:

  • This is making a lot of unrelated changes. In particular, dropping "unnecessary" asserts should not be done yet - it's not really a good idea to do until sound null-safety is the only option in the Dart VM, which is on the roadmap but a little ways away. The problem is that programs running in mixed mode still may benefit from these asserts.
  • I'm concerned about making changes to this library that will make parsing more expensive. Parsing takes a very long time right now and it's a problem for performance sensitive customers as it is.
  • I have a plan to potentially get rid of all of this logic and just make this package use vector_graphics_compiler (but probably without all optimizations) under the hood.

dnfield avatar Aug 27 '22 22:08 dnfield

@dnfield Thank you for your feedback.

  1. Right, you are correct! That makes sense then why it was still there
  2. I knew this would be your response. Nothing we can do about it :-) I am planning to do a benchmark, but it's a long shot to get this merged seeing as there are quite a few drawbacks. (speed and complexity being the main ones)
  3. I agree that that would be optimal. People that need the optimizations should then use the compiler and the binary format.

ikbendewilliam avatar Aug 27 '22 22:08 ikbendewilliam

Obsolete - this feature now works after some major refactoring.

dnfield avatar Nov 18 '22 22:11 dnfield