Pass Extras to NetworkClient via NetworkRequest

[HeroBrine1st]

With KTor client it is possible to use onDownload method to listen on download progress without interceptors. However, this approach requires using interceptors due to impossibility to pass lambda to the NetworkClient.

This PR adds extras property to NetworkRequest class. Due to manual cache serialisation (i.e. no kotlinx.serialisation), it should not affect caching, as this property is not read or written by CacheStrategy. This also has a side effect: it encourages passing various request parameters via Extras while not being serialised into cache, which is a documentation issue but this PR does not alter any existing NetworkClients, so I think it has low impact.

With extras passed, it is possible to write such NetworkClient that uses lambda passed via extras in onDownload, enabling easy progress loading listening in UI while also being generic as possible.

I should also note that writing such client is possible without this PR, but it requires copying NetworkFetcher for the same customisation. I haven't done that, but I think it should work, and so this PR.

Also this PR breaks binary compatibility with something that creates and copies NetworkRequest objects.

Various info on why I can't test this using ./test.sh

Basic verification is performed by inspect code in Intellij IDEA and also by these gradle tasks: :buildSrc:assemble :coil-bom:assemble :coil:assemble :coil-compose:assemble :coil-compose-core:assemble :coil-core:assemble :coil-gif:assemble :coil-network-cache-control:assemble :coil-network-core:assemble :coil-network-ktor2:assemble :coil-network-ktor3:assemble :coil-network-okhttp:assemble :coil-svg:assemble :coil-test:assemble :coil-video:assemble. I can't assemble everything in this project as :internal:assemble sets up an emulator and my system is not configured to do that, so it fails with code 127.

The test that I touched passes on jvm, android debug and android release (not instrumented).

Then, ./test.sh. gradlew apiCheck spotlessCheck works and actually found that I forgot to update API. Next line fails on :coil:allTasks due to karma not finding chromeHeadless (I have ungoogled-chromium and firefox-bin in my NixOS system). I also can't find any mention of karma in build configuration, so that I can't change it.

So, I rely on your github actions checks. If they fail, I'll need to use act (or other solution to run github actions locally). I think there needs a Dockerfile for running ./test.sh or something like that.

[colinrtwhite]

@HeroBrine1st Can you explain more about your use case? How does adding extras support using HttpRequestBuilder.onDownload?

[HeroBrine1st]

I'm not at my PC right now, so no code examples, but I can add them tomorrow

As onDownoad can be specific to a request, it is possible to get download progress on one concrete image, but NetworkClient has no way to pass that data to the UI. On the other hand, there is that extras that you can put arbitrary data into. That arbitrary data includes lamdas and their closure, so lambda can modify e.g. Compose state percentage. That lambda can be passed to Extras, from where NetworkClient gets it and passes to onDownload, and then loading progress goes back to Compose state.

But.. I just realized I have not actually checked that you can modify Extras from client code. A quick glance gives me that it is impossible so current approach won't work. I can add method to ImageRequest.Builder to support providing modified Extras but I think this is invasive enough to introduce accidental bugs due to abuse (though we can isolate it in nested Extras). Are you interested in that?

I have also found that there is fetcherFactory method and it can be used to pass not lambda but whole NetworkFetcher.Factory with its customised NetworkClient, which goes to the same solution except slightly bigger memory footprint (as you create factories for every request). This may be a good solution for initial problem, as it requires no modifications to Coil and NetworkFetcher. However, I personally see it as a workaround because UI (or where ImageRequest is created) will depend on concrete implementation of Fetcher.Factory interface.

UPD: new ImageRequest parameter will probably be used only by NetworkClient. So.. bad idea? Or mark as "use only if you know what you're doing", as Fetchers already receive Extras instance (so they'll be able to use that new parameter without other changes in Coil)?

[HeroBrine1st]

No, there is a way to edit extras already, so this PR is good as-is.

So, in my head it looks like that (mostly pseudocode):

val progressCallbackLambdaKey = Key<(progress: Float) -> Unit>(default = {})

...

var painterState by remember { mutableStateOf<AsyncImagePainter.State>(Empty) }
var progress by remember { mutableStateOf(-1f) } // -1f means request is still in fly
val imageRequest = remember { ImageRequest.Builder(context)
    .data(...)
    .apply {
        extras[progressCallbackLambdaKey] = { progress = it }
    }
    .build() }
Box(contentAlignment = Alignment.Center) {
    AsyncImage(model = imageRequest, onState = { painterState = it })
    if(painterState is Loading) {
        if(progress == -1f) CircularProgressIndicator()
        else CircularProgressIndicator(progress = { progress })
    }
}

Meanwhile, NetworkClient looks like that (it is supposed that clients copy code and customise it):

value class KtorNetworkClient(
    private val httpClient: HttpClient,
) : NetworkClient {
    override suspend fun <T> executeRequest(
        request: NetworkRequest,
        block: suspend (response: NetworkResponse) -> T,
    ) = httpClient.prepareRequest(request.toHttpRequestBuilder().apply { 
        request.extras[progressCallbackLambdaKey]?.let { callback ->
            onDownload { received, length ->
                callback(received.toFloat() / length)
            }
        }
    } ).execute { response ->
        block(response.toNetworkResponse())
    }
}

And the neat part is that it allows for arbitrary ad-hoc request modifications not limited by Ktor. The only downside is that it avoids cache.

I have also noticed that I forgot to add commas in some places, I'll do it in a minute