Skip to content

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Feb 23, 2023

Relands #118782 with some changes.

The original PR added a behavior change--when trying to load an asset and we don't find its key in the asset manifest file, we would throw an error (code). The idea was that this would provide a more useful error message than the one that would get shown when the image inevitably fails to load (code). However, this caused the errorBuilder functionality of Image to not work in the case of a missing asset (this was revealed by an internal test breakage). Fixing this appears non-trivial, so I think it's better to scope this out of this PR and perhaps look at it again later.

This also makes a small update to the recently-added AssetManifest.getAssetVariants API (still in beta). When given a key that is not found within the asset manifest file, the method will return null instead of throwing an error. Throwing an error is a bit too opinionated for an API that's just meant to provide read-access to the asset manifest file. The tradeoff is that callers will have to do null checking/assertions.
Let me know if you feel strongly that this should be its own PR.

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.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 23, 2023
@andrewkolos andrewkolos force-pushed the reland-use-binary-asset-manifest-for-asset-resolution branch from 5244790 to e00f17a Compare February 24, 2023 19:11
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Mar 5, 2023
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the asset loading API sound reasonable.

LGTM!

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 6, 2023
@auto-submit auto-submit bot merged commit 57c7aa5 into flutter:master Mar 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023
jonahwilliams pushed a commit that referenced this pull request Mar 11, 2023
…ed asset manifest for image resolution (#121322)"

This reverts commit 57c7aa5.
auto-submit bot pushed a commit that referenced this pull request Mar 11, 2023
…ed asset manifest for image resolution (#121322)" (#122449)

Revert "Reland "Speed up first asset load by using the binary-formatted asset manifest for image resolution"
@andrewkolos andrewkolos deleted the reland-use-binary-asset-manifest-for-asset-resolution branch April 21, 2023 19:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants