Update FadeInImage to use new Image APIs #33370

tvolkert · 2019-05-26T17:00:08Z

Description

This updates FadeInImage to use the new Image.frameBuilder
API (added in #33369), to greatly
simplify the implementation of FadeInImage.

This also removes the FadeInImage.placeholderSemanticLabel property.
This property was added in #28799 for the sake of completeness (the bug
it fixed was the lack of any semantic label support in FadeInImage), but a
placeholder is a transient visual artifact, not something that affects the
underlying semantic meaning of the image.

Related Issues

Tests

I added full test coverage of the two new properties (verified in merged lcov.info). Due to the fact that the implementation of FadeInImage was completely rewritten, I replaced the existing tests with new tests that verify that FadeInImage:

animates an uncached image
handles updating the placeholder image
re-fades in the image when the target image is updated
doesn't interrupt in-progress animation when animation values are updated
Semantics:
- only one Semantics node appears within FadeInImage
- is excluded if excludeFromSemantics is true
- label
  - defaults to image label
  - is empty without any specified semantics labels

Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change -- https://groups.google.com/forum/#!topic/flutter-announce/inURQEHtKWM
No, this is not a breaking change.

packages/flutter/lib/src/widgets/fade_in_image.dart

tvolkert · 2019-05-28T07:39:24Z

@Hixie I significantly re-worked this class. In the process, I got rid of two instance variables:

skipFadeOnSynchronousLoad (was proposed new instance variable). I decided not to increase the API surface of the class until a real use case warrants it.
placeholderSemanticLabel. This property makes no sense - a placeholder is a transient visual artifact, not something that affects the underlying semantic meaning of the image. A quick code search turned up no usages of this property.

PTAL.

packages/flutter/lib/src/widgets/fade_in_image.dart

This updates FadeInImage to use the new Image.frameBuilder API (added in #33369), to greatly simplify the implementation of FadeInImage. This also removes the FadeInImage.placeholderSemanticLabel property. This property was added in #28799 for the sake of completeness (the bug it fixed was the lack of any semantic label support in FadeInImage), but a placeholder is a transient visual artifact, not something that affects the underlying semantic meaning of the image.

tvolkert · 2019-05-29T19:58:24Z

Ok, this has been rebased off of 6884146. I've updated the PR description to account for the breaking change.

PTAL.

packages/flutter/lib/src/widgets/fade_in_image.dart

HansMuller

I only reviewed _AnimatedFadeOutFadeIn; looks pretty good to me.

packages/flutter/lib/src/widgets/fade_in_image.dart

tvolkert · 2019-05-31T20:26:54Z

FYI, waiting for LGTM on this.

I also submitted #33674 for review to address the documentation in ImplicitlyAnimatedWidgetState

goderbauer

LGTM

This is a follow-on to #33370 based on review comments therein.

googlebot added the cla: yes label May 26, 2019

tvolkert requested review from Hixie and goderbauer May 26, 2019 17:00