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

Use ImageLoaderFactory to create the singleton ImageLoader. #609

Merged
merged 3 commits into from Jul 22, 2021

Conversation

colinrtwhite
Copy link
Contributor

@colinrtwhite colinrtwhite commented Jul 21, 2021

Removes ProvideImageLoader and implements ImageLoaderFactory in the Crane and Owl apps. This has a couple benefits:

  • We only create one ImageLoader which is shared for every rememberImagePainter call. Each ImageLoader has its own memory cache, OkHttpClient, network observer, so it's cheaper to only create one.
  • ImageLoaderFactory creates the ImageLoader lazily the first time rememberImagePainter is called.
  • LocalImageLoader provides ImageLoader(context) sets LocalImageLoader.current for any child composables, however it doesn't set the singleton ImageLoader in the Coil object. If you call LocalContext.current.imageLoader elsewhere in the app, it'll return the singleton ImageLoader and not the value set in LocalImageLoader. Maybe there's a better way for Coil to handle this (open to suggestions!)...

@google-cla google-cla bot added the cla: yes label Jul 21, 2021
Copy link
Contributor

@nickbutcher nickbutcher left a comment

Choose a reason for hiding this comment

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

Thanks for the improvment! Please can we revert the AGP change, other than that LGTM.

@colinrtwhite
Copy link
Contributor Author

@nickbutcher Thanks for the quick review! Updated based on the feedback.

Copy link
Contributor

@nickbutcher nickbutcher left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@nickbutcher
Copy link
Contributor

Ahh, looks like this needs a gradlew spotlessApply please.

@colinrtwhite
Copy link
Contributor Author

@nickbutcher Updated

@nickbutcher nickbutcher merged commit e1d757b into android:main Jul 22, 2021
@nickbutcher
Copy link
Contributor

Thanks for the contribution!

@ColtonIdle
Copy link

ColtonIdle commented Jul 23, 2021

@colinrtwhite I have a question about the implementation here. I currently have coil working as it was in this repo (before you made this PR. I basically shamelessly copy pasted what was here 😄 ) but I was using it in a specific compose-only module.

The goal of this module is that everything is self contained, and so now with your changes I don't actually know where I would put

override fun newImageLoader(): ImageLoader {
        return ImageLoader.Builder(this).componentRegistry { add(MyInterceptor) }.build()
    }

as I don't have an Application class. Any tips?

@colinrtwhite
Copy link
Contributor Author

@ColtonIdle You can use Coil.setImageLoader; check out the docs here.

@ColtonIdle
Copy link

@colinrtwhite gotta be honest. Not sure where I would end up calling that setImageLoader method since my module is just filled with composables.

I have a ImageWithUrl that just wraps a Coil image. i.e.

@Composable
fun ImageWithUrl(
    imageUrl: String,
    modifier: Modifier = Modifier,
) {

I will try to play around with this a bit more as I'm probably going a bit off topic at this point.

@ColtonIdle
Copy link

For anyone following along... I ended up going this route with the guidance of @colinrtwhite

@Composable
fun ImageWithUrl(
    imageUrl: String,
) {
    Box(modifier) {
        Image(
            painter =
                rememberImagePainter(
                    data = imageUrl,
                    builder = { crossfade(true) },
                    imageLoader = getImageLoader(LocalContext.current),
                ),
            contentDescription = null)
    }
}
private var imageLoader: ImageLoader? = null
@Synchronized
private fun getImageLoader(context: Context): ImageLoader {
    imageLoader?.let {
        return it
    }
    return ImageLoader.Builder(context)
        .componentRegistry { add(MyInterceptor) }
        .build()
        .also { imageLoader = it }
}

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

Successfully merging this pull request may close these issues.

None yet

3 participants