Adds support for generic Uri strings.

[rharter]

Fixes #24

Changes

I added a generic UriStringMapper to handle generic strings. It is intentionally added at the end of the mappers in the RealImageLoader so that HttpStringMapper is preferred. I also updated the StringMapper to only handle strings supported by HttpUrl instead of throwing an exception.

Alternative

In the process of implementing this I realized that there could be an alternative approach that might be more flexible, but could come with a performance/complexity overhead. Since there is already a Mapper<Uri, HttpUrl>, the data mapping could be updated to continue mapping data until there are no more mappers that can handle the data.

var mapped = data
var mapper = registry.getMapper(mapped)
while (mapper != null) {
    mapped = mapper.map(mapped)
    mapper = registry.getMapper(mapped)
}

This would result in a url string going String > Uri > HttpUrl and a file string going String > Uri, but would also probably require more code to handle cases like map cycles and could result in a very long conversion route in extreme cases.

Because that's a larger change, I decided to simply implement the simple approach for this PR.

[rharter]

FYI the test script failed, but it failed on Master in the same place (TimeoutException after socket closed in integration tests), so I've submitted anyway (as there is no difference between master and my branch in the test report).

[colinrtwhite]

Thanks! Will take a look at this after pushing 0.6.1. The socket timeout issue in the instrumentation tests is a known issue I'm working on.

[colinrtwhite]

I don't have benchmarks to verify this, but I believe this adds a non-zero amount of overhead to every String URL request since we end up calling HttpUrl.get twice. Mappers are also called on the main thread so they need to be fast + non-blocking.

[colinrtwhite]

Instead of having two separate mappers, I think my preferred way to implement this would be something like this:

class StringMapper : Mapper<String, Any> {

    override fun map(data: String): Any {
        val uri = Uri.parse(data)
        val scheme = checkNotNull(data.scheme) { "Invalid Uri/URL. Missing scheme." }
        return if (scheme == "http" || scheme == "https") {
            HttpUrl.get(data)
        } else {
            uri
        }
    }
}

Still not sure if it's best to treat null schemes as http, https, or throw an error. Also, ideally I'd like to measure the extra overhead of Uri.parse since this will be called on the main thread for String-based requests.

[colinrtwhite]

Re the alternative approach: I originally had Mappers set up to map in a loop, but ultimately decided it's simpler + more performant to only map data once.

Also if you rebase/merge in master tests should pass.

[rharter]

I'd like to push back on the Mapper<String, Any> approach, since that loses the distinction of the source/destination pair.

Does a Mapper<String, Any> really handle source strings representing any destination? What if I want to map resource names to a resource id via getResourceId? Are we going to have to add that here? What if a user wants to add a mapper to standard images, like static images for different weather strings?

While you're correct that the overhead to call HttpUrl.get(data) is non-zero, it amounts to parsing the input string to validate that it is a valid Http url, and it fails fast as soon as it realizes it isn't a valid url. This type of validation is pretty standard and I'm not sure it's avoidable in an API that does proper validation of inputs. (The current implementation seems to simply assume all matching types are valid and throw exceptions if they don't match, so the StringMapper will claim it handles the string something_awesome, but throw a runtime exception when it can't actually parse a URL from that).

So without implementing the handles method the only option is to make the Mapper convert the input type, String in this case, to any possible output type, which is not really feasible.

An alternative approach that would allow for improved performance, while keeping mappers focused on single source -> destination processes could be to update the Mapper interface to remove the handles method and simply return a mapped value or null, similarly to how Retrofit's Converter.Factory works. (This was some feedback that was discussed in an old Picasso PR that I submitted to help support stuff like this.)

interface Mapper<T : Any, V : Any> {
    /**
     * Convert [data] into [V], or return null.
     */
    fun map(data: T): V?
}

This provides the same information as the previous API, but means that each mapper that matches the type only has to attempt to decode the source a single time, instead of the current architecture which requires two decodes.

The updated StringHttpUrlMapper would look like this:

class StringHttpUrlMapper : Mapper<String, HttpUrl> {
  override fun map(data: String): HttpUrl? = HttpUrl.get(data)
}

The call-site to get the mapped data also becomes simplified, and quite performant when using Kotlin Sequences.

mappers.asSequence()
    .filter { it.type.isAssignableFrom(data::class.java) }
    .mapNotNull { (it.converter as Mapper<Any, *>).map(data) }
    .firstOrNull()

I realize this would be an API change, but it could help simplify and improve performance, while reducing the need to have single God Mappers. Thoughts?

[colinrtwhite]

@rharter Hey sorry for the late response on this - wanted to think through your suggestion for a couple days.

I don't think I want to remove handles for 0.7.0, but I might be up for it after that release. Currently, Mappers follow the same pattern as the Fetcher and Decoder APIs, which also have handles methods. There's possibly an argument for having those also return null if they can't handle the input, but that would be a big change.

With Retrofit, Converter.Factory instances either return null or a Converter. This behaviour is slightly different from Coil where we would either return null or fetch/decode. I'd be worried about bad Fetchers/Decoders that partially fetch/decode, but then return null. The separation between "can I handle this" and the fetch/decode operations isn't as clear. Based off the Picasso PR you linked, Jake suggested Decoders should become Decoder.Factorys as well, which I'm not sure about.

[colinrtwhite]

For HttpUrls specifically I've decided I want to defer that initialization until a HttpUriFetcher on a background thread so I think we'll only need a String -> Uri Mapper.

[colinrtwhite]

@rharter I reworked some of the behaviour of Mappers based on your feedback here: #75

[rharter]

Sorry for the silence on this, I've been travelling for conferences and family. I'll circle back here this coming week.

[colinrtwhite]

@rharter No worries. I ended up performing a refactor here based on this PR + some of your feedback. I think it covers the fixes proposed by this PR, but let me know if I missed anything.