Interceptors are not called for null data

[Tolriq]

Describe the bug

This is between bug and feature request, but in order to workaround current limitations I'm going the interceptor road to properly handle the XXXResId stuff.
Unfortunately the interceptors are not called for null data so we can't use that to intercept what will be done with the fallback resId and even if we wrapped in a type the interceptors can't put null as a data to have the default follow-up and need to build the response ourselves.

I guess that due to the fact that we can't put null to data in interceptor that it's all wanted and so not a bug, but wanted to be sure why it's not possible to intercept null data to eventually do things on the request.

Edit: Coil 2.0

[colinrtwhite]

This is intended behaviour. Setting data to null short circuits the request early before the interceptors to allow the request to fail fast and avoid having to wait for size resolution and loading a placeholder (since it will be replaced by the error drawable immediately). As you noted, I don't think it's possible to intercept the fallback drawable due to this, however changing the fail fast behaviour could break other user's use cases + have performance implications.

[Tolriq]

@colinrtwhite so can you confirm if your composable solution is targeted for 2.0 as asked in previous issue or not?

Wrapping 100% of the requests is possible to handle all my use cases but this have huge design impact so I'd like to know what road I take.

[colinrtwhite]

@Tolriq It's likely for 2.0, though there's no ETA and it could arrive later. It'll likely be built on top of the existing Coil APIs so you could build a custom composable that wraps rememberImagePainter for yourself. If you need specific custom APIs added to Coil, I'd recommend forking the library.

[Tolriq]

Forking is always last resort :) specially on core library.

I maintain an optimized moshi fork as the solution to the need was refused in the end but I always first discuss and propose PR when discussion is possible.

But here tinting drawables on compose without having to provide different vector xml files or loading drawables on main thread is not really a specific need. It's common need specially with material you and simple compose theming there's no more just 2 xml to provide.

Everyone will expect a way to tint those stuff easily and in a performant way. (So not loading XML on main thread for every requests even those who will be cancelled during scroll)

Edit: Just to be clear in case the issue with compose is not clear: Pre compose it's often all about theme attributes and with appcompat you can use theme colors as tint directly in the XMLs. In full compose world there's no theme, no attribute to use and the tint must be done by code at some point.

[colinrtwhite]

@Tolriq You can already tint placeholder drawables by loading them up front and then setting them using ImageRequest.placeholder(Drawable). Placeholders are always loaded on the main thread (because they need to be set synchronously) when you execute a request so this doesn't do any extra work that wouldn't have been done normally. For error/fallback drawables you can access the drawable in the onError callback of the ImageRequest.Listener, which allows you to tint it using Drawable.setColorFilter. Though, in the case the error/fallback needs to be specified up front using a resource ID.

As an aside, please keep in mind that I maintain Coil for free mostly in my free time and I'm not able (or obligated) to answer all questions or provide a solution to every problem. Using ImageRequest.error(Drawable) is a simple, valid solution that works for most users. If you have specific performance requirements, it's not necessarily true for all users.

[Tolriq]

@colinrtwhite Just to be clear I never said that you where obligated to anything, I said that I always discuss and do PR when discussion leads to acceptance of the need. I just don't do blind PR before discussion for time efficiency since like here it would have been rejected.

If everyone when they need something directly do fork and do not try to contribute by requesting / making PR it would be quite absurd.

If you have specific performance requirements, it's not necessarily true for all users.

This is mostly a POV situation, every bit of performance that can be gained simply do matter. If an app does not drop frame on 75% of the devices most devs will be ok with that (actually 90% of the devs don't check and certainly don't check on low end devices), if a simple optimization brings that number to 99% then was that performance increase useless and not true for all?

My main app on store is 750K MAU worldwide with lots of low end devices (and many last tier countries) dedicated to the app, displays tons of images in 95% of the screen and is at the top of all peer groups in all performance / crash metrics. This is the result of 10 years of study of data and improvements made to the app, not just some random asks.
This was the first things I looked into when I started asking questions about Coil 1.5 years ago. So it's still not enormous app and I do not pretend to be a good dev, but this is enough history and data to act on at least on my side. Most people do no check their metrics and real performance so don't report issues on that.

About performance I still think parallelism handling with a semaphore in the middle and no limits on the fetcher is a problem. Can be workarounded by a custom fetcher or different dispatchers (that requires context switches currently, luckily this should be solved with coroutines 1.6 and Kotlin/kotlinx.coroutines#2918) but still.

Caching resulting images to avoid always decoding 4k image for a 48dp image is still relatively hard due to some hidden internals to better calculate keys, making us invent the wheel again.

The drawable on main thread is the last one, a part can be workarounded with interceptors but require wrapping to handle fallback.

About placeholder, at least on compose it's displayed quite late when you fastscroll, since the placeholder does not require measuring it should maybe be set at Empty stage and not at when starting after measure. Since it's currently set at starting it's already late and don't see the need for loading in main thread.

Anyway TL;DR: I never forced you to anything sorry for my bad English, I just tried to insist on advanced performance topics that matters for app that may be different from the one you usually see or get reports from (US/Canada devices are very different from the whole world). (And by side effects benefits all)

As a final note, thanks for the work on Coil it's a nice library and specially 2.0 that brings most of the missing pieces to plug advanced needs, so I'll stop bothering you with perf and keep doing things that only benefits me.