Skip to content
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

refactor: Using precacheImage in initState #9

Open
cekrozl1 opened this issue Feb 3, 2022 · 4 comments
Open

refactor: Using precacheImage in initState #9

cekrozl1 opened this issue Feb 3, 2022 · 4 comments

Comments

@cekrozl1
Copy link

cekrozl1 commented Feb 3, 2022

Using precacheImage in initState

Hi!

I read a couple of posts such as https://stackoverflow.com/questions/51343735/flutter-image-preload which say that initState might not be the right place to call precacheImage from, since precacheImage requires the context which is not (or might not be) available from initState.

I see that the puzzle code is using it here : https://github.com/VGVentures/slide_puzzle/blob/release/lib/app/view/app.dart.
I also see that there is no issues running this code.

Is precacheImage in initState appropriate here?

@orestesgaolin
Copy link
Contributor

Using context in the initState is not wrong on its own.

If we take a look at the implementation of the precacheImage, you'll notice it uses createLocalImageConfiguration and it states that if this is called outside build method, then you'll not be notified about the environment changes (e.g. pixel ratio or resolution).

So in our case we lose the ability to respond to resolution changes. However, as we usually have just a one size of the image, then it's not super problematic.

We don't want to invoke precaching in build method as it could be called multiple times, e.g. when resizing the window. I'm not sure if this method is smart enough to not cache the same image multiple times.

One potential optimization would be to not precache some variants of images that we have several sizes of (e.g. simple_dash_x) instead of all of them. However, the increased complexity of this in my opinion is not worth it. The images have relatively small memory footprint.

Wdyt @bselwe?

@bselwe
Copy link
Contributor

bselwe commented Feb 4, 2022

Thanks for this issue @cekrozl1!

@orestesgaolin It seems that createLocalImageConfiguration may be called outside the build method, but internally it uses BuildContext.dependOnInheritedWidgetOfExactType which is inappropriate to use in initState. Normally, depending on an inherited widget in initState should throw an exception. It might be that there are no issues in the sample as we're delaying precacheImage by 20 milliseconds.

I can see that the recommended approach to use precacheImage is in didChangeDependencies, which, according to documentation, is called immediately after initState and it is safe to depend on inherited widgets from this method.

I think it would be okay to move this logic to didChangeDependencies. Also, I believe that calling precacheImage multiple times should be safe as it uses ImageCache.putIfAbsent which should probably only cache once for the same image and configuration.

Let me know what you think.

@orestesgaolin
Copy link
Contributor

Yeah, I think that'd be good solution

@cekrozl1
Copy link
Author

cekrozl1 commented Feb 5, 2022

Hi @orestesgaolin, hi @bselwe !

I was only using your project as a template to enhance my own projects structures, and I didn't really expect an enhancement here. But I'm glad to see an improvement.

@bselwe, I was the one asking if this specific project was "production-ready" 2 weeks ago here
As for this one, the VGV linter found 2K+ enhancements that the flutter one didn't. So, there's my answer =)

Shout-out to the entire VGV team. I love what you're doing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants