Skip to content

FadeInImage cacheWidth and cacheHeight support #43286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 26, 2019

Conversation

GaryQian
Copy link
Contributor

Description

This adds support for cacheWidth and cacheHeight in FadeInImage. This is a continuation of #41415 and #26194

Related Issues

#26194

This change was requested in #42785.

@GaryQian GaryQian added c: new feature Nothing broken; request for a new capability a: images Loading, displaying, rendering images labels Oct 22, 2019
@GaryQian GaryQian self-assigned this Oct 22, 2019
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 22, 2019
@fluttergithubbot
Copy link
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@GaryQian
Copy link
Contributor Author

Tests in progress.

@@ -16,6 +16,14 @@ import 'transitions.dart';
// Examples can assume:
// Uint8List bytes;

// Same as in painting/image.dart.
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in a common place and reuse it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it could be a static method on ImageProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a static on ResizeImage

Copy link

@nightfallsad nightfallsad Oct 31, 2019

Choose a reason for hiding this comment

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

Maybe it could be a static method on ImageProvider?

Will this PR merge to the current last stable version v1.9.1-hotfixes?

@@ -137,6 +158,10 @@ class FadeInImage extends StatelessWidget {
this.alignment = Alignment.center,
this.repeat = ImageRepeat.noRepeat,
this.matchTextDirection = false,
int placeholderCacheWidth,
Copy link
Member

Choose a reason for hiding this comment

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

Why not use two Size arguments instead of four ints?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. The other Images also expose it as separate ints. I guess we should be consistent.

@@ -66,6 +74,12 @@ class FadeInImage extends StatelessWidget {
/// Creates a widget that displays a [placeholder] while an [image] is loading,
/// then fades-out the placeholder and fades-in the image.
///
/// To include a custom decode/cache size such as those provided by
Copy link
Member

Choose a reason for hiding this comment

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

nit: This sentence is very complicated and hard to parse. What are you trying to say?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this basically just saying that placeholder and image may also be ResizeImages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a simplified explanation

};
// image.placeholder should be an instance of MemoryImage instead of ResizeImage
final MemoryImage memoryImage = image.placeholder;
expect(image.placeholder is MemoryImage, true);
Copy link
Member

Choose a reason for hiding this comment

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

expect(image.placeholder, isA<MemoryImage>)

Also, the type in the line above should probably be ImageProvider instead of MemoryImage. Otherwise you may never get to this expect.

expect(cacheHeight, 30);
return PaintingBinding.instance.instantiateImageCodec(bytes, cacheWidth: cacheWidth, cacheHeight: cacheHeight);
};
final ResizeImage resizeImage = image.placeholder;
Copy link
Member

Choose a reason for hiding this comment

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

Should we have an expect about the type here as well like in the other test?

return PaintingBinding.instance.instantiateImageCodec(bytes, cacheWidth: cacheWidth, cacheHeight: cacheHeight);
};
final ResizeImage resizeImage = image.placeholder;
resizeImage.load(await resizeImage.obtainKey(ImageConfiguration.empty), decode);
Copy link
Member

Choose a reason for hiding this comment

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

It makes me a bit uncomfortable that the test doesn't verify that the decode callback was actually invoked. If for some reason it doesn't run, the expects in there also wouldn't execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verifies it is called now!

@@ -84,6 +84,10 @@ FadeInImageParts findFadeInImage(WidgetTester tester) {
}
}

class ClosureTracker {
Copy link
Member

Choose a reason for hiding this comment

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

You don't really need an extra class for this. Just define a bool variable outside the closure, initialize it with false and set it to true within the closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I tried this initially and it didnt seem to work, I probably did it wrong. I'll try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok it works now, idk what I did earlier for this to not work..

final ResizeImage resizeImage = image.placeholder;
resizeImage.load(await resizeImage.obtainKey(ImageConfiguration.empty), decode);
final ImageProvider resizeImage = image.placeholder;
expect(image.placeholder is ResizeImage, true);
Copy link
Member

@goderbauer goderbauer Oct 25, 2019

Choose a reason for hiding this comment

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

expect(image.placeholder, isA<ResizeImage>());

return PaintingBinding.instance.instantiateImageCodec(bytes, cacheWidth: cacheWidth, cacheHeight: cacheHeight);
};
// image.placeholder should be an instance of MemoryImage instead of ResizeImage
final MemoryImage memoryImage = image.placeholder;
final ImageProvider memoryImage = image.placeholder;
expect(image.placeholder is MemoryImage, true);
Copy link
Member

@goderbauer goderbauer Oct 25, 2019

Choose a reason for hiding this comment

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

expect(image.placeholder, isA<MemoryImage>());

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@GaryQian GaryQian merged commit 652fb97 into flutter:master Oct 26, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: images Loading, displaying, rendering images c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants