Do not require main asset files if variants are provided #10901
Conversation
|
You might also need to update dartdocs in various places, comments in the pubspec.yaml.tmpl file, and various things on our Web site. |
|
I have updated the pubspec.yaml file and will have to update the web site (https://flutter.io/assets-and-images/.) separately. |
There was a problem hiding this comment.
printError('$e'); is probably more idiomatic. Hopefully it isn't slower. :-)
There was a problem hiding this comment.
Is this an invariant at this point, or is this the place where you're checking that the input is valid?
Asserts won't always run in flutter_tools, since we sometimes run it in release mode.
There was a problem hiding this comment.
If this is just an invariant that you're checking, leave a comment pointing to where we do the input validation.
There was a problem hiding this comment.
The original version of this check had been an invariant but it probably should throw an error. With a mighty helpful error message :-).
There was a problem hiding this comment.
variantsFor takes a single string, but you're passing two here, did you mean 'assets/foo'?
There was a problem hiding this comment.
Oops, yes. I'd forgotten to update the doc.
There was a problem hiding this comment.
This is O(N*M), could _excluded be a Set somehow? I guess the right data structure here is a prefix tree, but that may be going off into the weeds a bit prematurely.
There was a problem hiding this comment.
I can just substitute an ordinary loop over the file system entities that builds the list of paths.
There was a problem hiding this comment.
Writing a loop here saves the cost of creating a list of paths and then a copy of that after filtering out the excluded paths. The excluded list is length 2, I think that's probably enough tweaking.
|
LGTM (mostly... a bit RSLGTM, @tvolkert may be a better reviewer). The only suggestion I would have is to add an end-to-end test (in the devicelab) that verifies that this works (i.e. that if you only have 2x and 3x variants, on a 1x device, you get the 2x variant). |
This assumes that flutter/flutter#10901 will land.
There was a problem hiding this comment.
assetVariants can't be empty, since we're iterating through it.
There was a problem hiding this comment.
Good catch, that was supposed to be assetVariants[asset].isEmpty
There was a problem hiding this comment.
... which makes this if check redundant
HansMuller
force-pushed
the
asset_variants
branch
from
766cf9e to
5733386
Compare
This assumes that flutter/flutter#10901 will land.
4 participants
If variants are found for an asset then we don't require that the "main" asset's file exists.
Replaced the N-squared algorithm that matches assets and variants; now it's O(N).
Fixes #8231