Tighten asset variant detection criteria to only include device-pixel-ratio variants #110721

andrewkolos · 2022-08-31T22:15:10Z

Background

If unfamiliar with the concept of asset variants, see Asset variants. Also see #69388.

Currently, when Flutter finds an asset file to use, it will scan adjacent directories for files that have the same filename and consider these files to be variants of the first file (also, the first file is considered to be a variant of itself).

For example, take this pubspec.yaml

...
flutter:
  assets:
    - assets/

and this application directory

.../pubspec.yaml
.../assets/my_icon.jpg
.../assets/2x/my_icon.jpg
.../assets/dark/my_icon.jpg
...etc.

In this scenario, assets/my_icon.jpg, assets/2x/my_icon.png, and assets/dark/my_icon.jpg are considered to be variants of assets/my_icon.jpg. Currently, this behavior is only utilized when looking for higher-resolution versions of images for devices with high pixel ratios (see Declaring resolution-aware image assets).

However, there are scenarios where an image file could be detected as a variant of another when the user meant for them to be two distinct assets. Using the same pubspec from before, suppose we have this application directory:

...
.../assets/icon.jpg
.../assets/foo_screen/icon.jpg
.../assets/bar_screen/icon.jpg

In this scenario, attempts to load icon.jpg will result in one of the other two variants being loaded instead.
This is because all three of these files are considered to be variants of icon.jpg, and the implementation will just happen to pick the last one it encountered when going through the list of candidates. Specifically, all three files are considered to be 1x device-pixel-ratio (i.e. the default) variants of icon.jpg (code).

Change

Flutter will now consider an image asset to be a variant of one in its grandparent directory if and only if the parent directory matches the pattern for a device-pixel-ratio (e.g. 2x, 3.0x, etc.). Example:

.../assets/my_icon.jpg
.../assets/2x/my_icon.jpg // 2x device-pixel-ratio variant of /assets/my_icon.jpg
.../assets/dark/my_icon.jpg // No longer a variant of /assets/my_icon.jpg
...etc.

This change would call for a documentation change/overhaul of the Asset variants documentation. In particular, the dark variant example should be removed/replaced.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

christopherfujino · 2022-09-08T18:16:38Z

I suspect you'll need to rebase upstream to fix ci.yaml validation:

FormatException: ERROR: Linux_android flutter_gallery__transition_perf is a new builder added. it needs to be marked bringup: true
If ci.yaml wasn't changed, try git fetch upstream && git merge upstream/master

christopherfujino · 2022-09-19T18:49:10Z

I'm unsubscribing so I stop seeing notifications every time you push, please ping me when this is ready for review :)

packages/flutter_tools/test/general.shard/asset_bundle_variant_test.dart

christopherfujino · 2022-09-27T16:24:41Z

dev/integration_tests/flutter_gallery/pubspec.yaml

    - packages/flutter_gallery_assets/products/table.png
    - packages/flutter_gallery_assets/products/teaset.png
    - packages/flutter_gallery_assets/products/top.png
-    - packages/flutter_gallery_assets/people/ali.png


was this asset never used?

It doesn't exist! 😄

mind blown. that's not a build error?

It became one with this fix!

@christopherfujino see #110721 (comment) for an explanation

packages/flutter_tools/test/general.shard/asset_bundle_variant_test.dart

packages/flutter_tools/lib/src/asset.dart

jonahwilliams · 2022-09-27T16:41:11Z

packages/flutter_tools/lib/src/asset.dart

-      .listSync()
-      .whereType<File>();
+    final Iterable<FileSystemEntity> entities = _fileSystem.directory(directoryPath).listSync();
+


Suggested change

I like the blank like between entities and files because it puts some distance between the for loop that iterates over the files. The reason I like this separation is that entities is not only used for the file for loop--it's also used for the for loop.

jonahwilliams · 2022-09-27T17:43:13Z

packages/flutter_tools/lib/src/asset.dart

    final List<String> excludeDirs = <String>[
      assetDirPath,
-      getBuildDirectory(),
      if (flutterProject.ios.existsSync())


Double checking here: The problem these solved is that we'd sometimes pickup variants from build output directories

Something like:

assets/foo.txt build/bar/baz/intermediates/foo.txt

Would get picked up as a variant. But with this change, the bar/baz/intermediates/ should no longer match the variant code right?

If so, I believe you could remove all excludeDirs except for assetDirPath

Yeah. We should only pick up variants that 1) are exactly one directly down from the main asset and 2) have a parent folder matching the pixel ratio pattern (2x, 3.0x, etc.). Before, we would look at any file under the directory (regardless of depth) of any asset.

I also completely removed all references to assetDirPath from the file, including AssetBundle, as it is unused. This should require a g3fix, right?

is assetDirPath used by the google3 version of the manifest? If not, you can g3fix. If it is used I would just leave it for now

I was slightly inaccurate in my previous comment, but the idea is the same. It was used here exclusively as an exclude directory, and I removed excludeDirs altogether. With that gone, I removed it altogether.

To answer your question, yes, g3 doesn't have any special use for it.

packages/flutter_tools/lib/src/asset.dart

jonahwilliams

LGTM (with nit)

…ce-pixel-ratio variants (flutter/flutter#110721)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 31, 2022

andrewkolos added a: assets Packaging, accessing, or using assets a: images Loading, displaying, rendering images labels Aug 31, 2022

andrewkolos changed the title ~~[WIP] Tighten asset variant detection criteria~~ [WIP] Tighten asset variant detection criteria to only include device-pixel-ratio variants Aug 31, 2022

andrewkolos changed the title ~~[WIP] Tighten asset variant detection criteria to only include device-pixel-ratio variants~~ Tighten asset variant detection criteria to only include device-pixel-ratio variants Sep 6, 2022

andrewkolos force-pushed the tighten-asset-variant-detection-criteria branch 2 times, most recently from 0a92e44 to bea7e2b Compare September 13, 2022 18:11

jonahwilliams self-requested a review September 13, 2022 21:02

andrewkolos force-pushed the tighten-asset-variant-detection-criteria branch 2 times, most recently from 041cc8b to 6ddf3ac Compare September 19, 2022 08:20

andrewkolos requested review from christopherfujino and keyonghan as code owners September 19, 2022 16:27

andrewkolos force-pushed the tighten-asset-variant-detection-criteria branch 4 times, most recently from 527705c to ddb1aab Compare September 19, 2022 18:48

andrewkolos removed request for christopherfujino, jonahwilliams and keyonghan September 19, 2022 21:20

flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 19, 2022

andrewkolos requested review from christopherfujino and jonahwilliams September 19, 2022 23:02

christopherfujino reviewed Sep 19, 2022

View reviewed changes