-
Notifications
You must be signed in to change notification settings - Fork 646
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
Proposal: Switch to just speak Call.Factory #8
Conversation
Quick note: for those who don't know - |
I really like the idea of using
^ This doesn't cover the case where a user lazy inits the |
Agreed, confirmed this is why Retrofit does it as well 👍
Yep I'd agree with that. That was the idea behind extracting it to the function. I do think it should be one or the other though. Trying to balance all of them together would create confusing API imo, as it'd have |
I think you're right that I'm likely going to ship |
Sounds good, just let me know 👍 |
private var okHttpClient: OkHttpClient? = null | ||
private var okHttpClientBuilder: (OkHttpClient.Builder.() -> Unit)? = null | ||
private var callFactory: Call.Factory? = null | ||
private var canRebuildWithOptimizations: Boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove canRebuildWithOptimizations
since we should only apply these optimizations if we need to build the default call factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By happy coincidence, I forgot to wire this up anywhere. Just had to delete the field. 34b0090
*/ | ||
fun okHttpClient(client: OkHttpClient) = apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this (but remove okHttpClientBuilder
) for discoverability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | ||
* Applies the preferred optimizations for Coil (image loading library) to this given Builder instance. | ||
*/ | ||
fun OkHttpClient.Builder.applyCoilOptimizations(context: Context): OkHttpClient.Builder = apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into Utils
, set Utils
to public, and set every other method to internal
? I'd also change the name to applyOkHttpClientOptimizations
. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZacSweers Left a few comments. Still not 100% sure on having a Also can you rebase/merge in master to have tests run? Thanks! |
Done and rebased |
/** | ||
* [OkHttpClient] utilities for Coil. | ||
*/ | ||
object OkHttpClients { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this + make applyCoilOptimizations
a top level function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9384a04
@@ -0,0 +1,28 @@ | |||
package coil.util |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@file:JvmName("OkHttpClients")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9384a04
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
* Switch internals to use Call.Factory interface * Update ImageLoaderBuilder to just accept a Call.Factory * Update sample demonstrating deferred init * Simplify sample * Move applyOkHttpClientOptimizations to Utils with new name * Add back okHttpClient() * Remove unused canRebuildWithOptimizations property * Extract OkHttpClients util and revert changes in Utils.kt * Make applyCoilOptimizations top level w/ file name
This is a proposal PR that switches the
ImageLoaderBuilder
+ internals to just speakCall.Factory
. This is similar to how Retrofit handles it, and allows for consumers to provide their own implementations ofCall.Factory
.Two primary examples:
Call.Factory
, using the interface to allow them to interop with popular libraries like Retrofit still. This would enable those as well.In order to sanely support this, I think the builder helper method didn't have a place, and instead made the logic for applying those configurations available via public helper extension function. This way consumers can use it in their own construction, and also better supports sharing connection pools between instances. If clients want to use the optimized configuration, they can just call
newBuilder()
on their existing instance to share the connection pool while gettin the optimized configuration for a new instance ultimately provided intocallFactory
.