VGVentures / slide_puzzle

MIT License
530 stars 362 forks source link

refactor: Using precacheImage in initState #9

Open cekrozl1 opened 2 years ago

cekrozl1 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Yeah, I think that'd be good solution

cekrozl1 commented 2 years ago

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!