Proposal: Switch to just speak Call.Factory #8

ZacSweers · 2019-08-13T05:02:14Z

This is a proposal PR that switches the ImageLoaderBuilder + internals to just speak Call.Factory. This is similar to how Retrofit handles it, and allows for consumers to provide their own implementations of Call.Factory.

Two primary examples:

A clever technique to improve startup perf is to lazily instantiate the OkHttpClient stack (instance + cache) by deferring its construction until the first network call. This way, construction is done off the main thread. I demoed this in c03e7c2, and can revert 69ca796 if you want to keep that in the sample.
Some apps provide entirely different network implementations behind 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 into callFactory.

ZacSweers · 2019-08-13T05:18:28Z

Quick note: for those who don't know - OkHttpClient is an implementation of Call.Factory, so clients can be passed directly as well via callFactory(client). Retrofit exposes both callFactory(OkHttpClient) and callFactory(Call.Factory), where the former is just a shorthand for the latter. I've pinged them to ask why they offer both and will update if there's an added advantage to it 👍

colinrtwhite · 2019-08-14T00:36:44Z

I really like the idea of using Call.Factory. A few thoughts:

We should keep the okHttpClient(OkHttpClient) method, but also provide a callFactory(CallFactory) method similar to how Retrofit handles it. This helps with discovery.
I'm not a huge fan of applyCoilOptimizations. I think I would prefer to keep okHttpClient(builder: OkHttpClient.Builder.() -> Unit) and only apply the optimizations in the case where a user doesn't set a client or call factory. If the user sets an OkHttpClient builder, I'd apply the optimizations there as well.

^ This doesn't cover the case where a user lazy inits the OkHttpClient, but still wants Coil's default OkHttpClient configuration. Maybe it would make sense to make a public Utils method for that? What do you think?

ZacSweers · 2019-08-14T02:32:17Z

We should keep the okHttpClient(OkHttpClient) method, but also provide a callFactory(CallFactory) method similar to how Retrofit handles it. This helps with discovery.

Agreed, confirmed this is why Retrofit does it as well 👍

Maybe it would make sense to make a public Utils method for that? What do you think?

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 okHttpClient(), callFactory(), and okHttpClient(Builder.()) where they all have different implications/behavior. Applying the builder onto the directly-handed OkHttpClient also seems potentially problematic if the user is trying to limit the behavior of the client they pass in. What do you think?

colinrtwhite · 2019-08-15T07:00:59Z

I think you're right that okHttpClient(Builder.()) should be removed as it has behaviour that the other two methods don't, which can be confusing. Going to think about this a little more since ideally I'd like to avoid a Utils method since it's not super discoverable.

I'm likely going to ship 0.6.1 tomorrow, but the conversion to Call.Factory would be a good candidate for 0.7.0.

ZacSweers · 2019-08-16T07:31:48Z

Sounds good, just let me know 👍

colinrtwhite · 2019-08-19T21:19:27Z

coil-base/src/main/java/coil/ImageLoaderBuilder.kt

- private var okHttpClient: OkHttpClient? = null
- private var okHttpClientBuilder: (OkHttpClient.Builder.() -> Unit)? = null
+ private var callFactory: Call.Factory? = null
+ private var canRebuildWithOptimizations: Boolean = false


Let's remove canRebuildWithOptimizations since we should only apply these optimizations if we need to build the default call factory.

By happy coincidence, I forgot to wire this up anywhere. Just had to delete the field. 34b0090

colinrtwhite · 2019-08-19T21:20:00Z

coil-base/src/main/java/coil/ImageLoaderBuilder.kt

 */
- fun okHttpClient(client: OkHttpClient) = apply {


Let's keep this (but remove okHttpClientBuilder) for discoverability.

colinrtwhite · 2019-08-19T21:27:43Z

coil-base/src/main/java/coil/ImageLoaderBuilder.kt

+ /**
+ * Applies the preferred optimizations for Coil (image loading library) to this given Builder instance.
+ */
+ fun OkHttpClient.Builder.applyCoilOptimizations(context: Context): OkHttpClient.Builder = apply {


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!

colinrtwhite · 2019-08-19T21:29:51Z

@ZacSweers Left a few comments. Still not 100% sure on having a Utils method to apply the optimizations, but I wanted to avoid delaying this PR more.

Also can you rebase/merge in master to have tests run? Thanks!

ZacSweers · 2019-08-19T21:56:02Z

Done and rebased

colinrtwhite · 2019-08-19T22:15:13Z

coil-base/src/main/java/coil/util/OkHttpClients.kt

+/**
+ * [OkHttpClient] utilities for Coil.
+ */
+object OkHttpClients {


Can you remove this + make applyCoilOptimizations a top level function?

Done in 9384a04

colinrtwhite · 2019-08-19T22:15:33Z

coil-base/src/main/java/coil/util/OkHttpClients.kt

@@ -0,0 +1,28 @@
+package coil.util


@file:JvmName("OkHttpClients")

Done in 9384a04

colinrtwhite

💯

* 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

colinrtwhite mentioned this pull request Aug 19, 2019

Creating default OkHttp Cache can trigger strict mode violation. #48

Closed

colinrtwhite reviewed Aug 19, 2019

View reviewed changes

ZacSweers added 7 commits August 19, 2019 17:55

Switch internals to use Call.Factory interface

bbc06fa

Update ImageLoaderBuilder to just accept a Call.Factory

861f4ee

Update sample demonstrating deferred init

4bdecf8

Simplify sample

32d56dd

Move applyOkHttpClientOptimizations to Utils with new name

ad0d666

Add back okHttpClient()

fbdd36a

Remove unused canRebuildWithOptimizations property

34b0090

Extract OkHttpClients util and revert changes in Utils.kt

5ed13b9

colinrtwhite reviewed Aug 19, 2019

View reviewed changes

Make applyCoilOptimizations top level w/ file name

9384a04

colinrtwhite approved these changes Aug 19, 2019

View reviewed changes

colinrtwhite merged commit 9c3810b into coil-kt:master Aug 19, 2019

ZacSweers deleted the z/callFactory branch August 19, 2019 23:08

iNoles mentioned this pull request Jul 5, 2020

OkHttp Wishlist square/okhttp#2903

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Switch to just speak Call.Factory #8

Proposal: Switch to just speak Call.Factory #8

ZacSweers commented Aug 13, 2019 •

edited

Loading

ZacSweers commented Aug 13, 2019 •

edited

Loading

colinrtwhite commented Aug 14, 2019

ZacSweers commented Aug 14, 2019 •

edited

Loading

colinrtwhite commented Aug 15, 2019

ZacSweers commented Aug 16, 2019

colinrtwhite Aug 19, 2019

ZacSweers Aug 19, 2019

colinrtwhite Aug 19, 2019

ZacSweers Aug 19, 2019

colinrtwhite Aug 19, 2019 •

edited

Loading

ZacSweers Aug 19, 2019

colinrtwhite commented Aug 19, 2019 •

edited

Loading

ZacSweers commented Aug 19, 2019

colinrtwhite Aug 19, 2019

ZacSweers Aug 19, 2019

colinrtwhite Aug 19, 2019 •

edited

Loading

ZacSweers Aug 19, 2019

colinrtwhite left a comment

Proposal: Switch to just speak Call.Factory #8

Proposal: Switch to just speak Call.Factory #8

Conversation

ZacSweers commented Aug 13, 2019 • edited Loading

ZacSweers commented Aug 13, 2019 • edited Loading

colinrtwhite commented Aug 14, 2019

ZacSweers commented Aug 14, 2019 • edited Loading

colinrtwhite commented Aug 15, 2019

ZacSweers commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite commented Aug 19, 2019 • edited Loading

ZacSweers commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite left a comment

Choose a reason for hiding this comment

ZacSweers commented Aug 13, 2019 •

edited

Loading

ZacSweers commented Aug 13, 2019 •

edited

Loading

ZacSweers commented Aug 14, 2019 •

edited

Loading

colinrtwhite Aug 19, 2019 •

edited

Loading

colinrtwhite commented Aug 19, 2019 •

edited

Loading

colinrtwhite Aug 19, 2019 •

edited

Loading