slide_puzzle icon indicating copy to clipboard operation
slide_puzzle copied to clipboard

refactor: Using precacheImage in initState

Open cekrozl1 opened this issue 3 years ago • 4 comments

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?

cekrozl1 avatar Feb 03 '22 09:02 cekrozl1

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?

orestesgaolin avatar Feb 04 '22 09:02 orestesgaolin

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.

bselwe avatar Feb 04 '22 10:02 bselwe

Yeah, I think that'd be good solution

orestesgaolin avatar Feb 04 '22 10:02 orestesgaolin

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!

cekrozl1 avatar Feb 05 '22 16:02 cekrozl1