👍

I don't think the provider should be a place that decides on which thread the callback is called. This should be responsibility of the View that calls the method. Generally I'd expect the whole search call to happen on the main thread (and the tenor library to handle the background work itself since it implements an enqueue/callback mechanism).

I agree. We should take advantage of the retrofit callback thread dispatching. I've fixed this point in baade56. I've also added at the bottom a final comment given further context about my solution and how I decided to proceed.

I think passing these callback methods as params makes the code too complicated. Can you just have a simple buildResponse and buildError methods that return the objects you want to create and call the onSuccess/onFailure directly in the enqueueSearchRequest?

Yeah, I got way too exaggerated with this method extraction, and after the coroutine removal, this got even more unnecessary. I've inlined the operation directly into the onSuccess and onFailure declaration since they're simple enough to do so, it got way easier to understand now. I've fixed this point in baade56.

What do you think?

why is this necessary? isn't the call automatically cancelled when it returns success/failure?

For my own surprise, I've detected that the WeakRefCallback wasn't receiving a canceled Call, it was reaching the success and failure with the isCanceled returning false. But after the manual timeout implementation removal, this kind of handling lost its purpose since we now rely on the default retrofit Callback implementation. I've fixed this point in f6123f6. I've also added at the bottom a final comment given further context about my solution and how I decided to proceed.

This is also way too complicated tbh. If we want to implement a timeout, we can do something like this:

        val timeoutJob = scope.launch {
            delay(DEFAULT_SECONDS_TO_TIMEOUT * ONE_SECOND_IN_MILLIS)
             buildSearchCall.cancel()
        }

and do something like this on the onSuccess/onFailure callbacks:

                if (timeoutJob.isActive) {
                    timeoutJob.cancel()
                }

I've also looked into the ApiService builder and there is actually timeout built in:
com.tenor.android.core.network.ApiService.IBuilder#timeout. I think we should use that instead.

I decided to investigate further the implementation options and how the Tenor SDK was making use of the retrofit calls, and the solution I've found made the dispatchTimeoutClock unnecessary, so I completely removed it. The fix can be found at f6123f6. I've also added at the bottom a final comment given further context about my solution and how I decided to proceed.

Also, I liked a lot your suggestion for a timeout, it really simplifies the cancel handling, I’ll keep this solution in mind, thank you!

Do you think it makes sense to make an actual use of the scope and make the whole provider aware of the lifecycle? Maybe with a cancel method? This is not necessary, I'm just wondering about what you think about it 👍

After your review, I've decided to completely remove the coroutine usage since the manual timeout handling wasn't necessary anymore and given the fact we should let the thread handling be done internally. So right now the TenorProvider no longer receives a CoroutineScope. I've committed this in 9bc5cbb.

What do you think? Should we go for the lifecycle awareness even so?

I think you can remove this now 👍

Thank you for point this out, it's fixed in 3d8826a

I was looking into the test and I think it could be doable without Roboelectric. I'm generally against using Roboelectric because it usually means you're not testing something right (you're testing the Roboelectric framework, not your code). Sometimes you cannot avoid it but since all you're using it for here is building the Api client (which is an interface), I think you can replace it with MockitoJUnitRunner and pass in mocked ApiClient (and verify you call its methods). What do you think about this approach?

I've tried some further approaches trying to remove the Robolectric from the TenorProviderTestclass. The Context mocking was easy going, just needed to adjust the ApiClient.getServices(context) inside the TenorProvider and mock the getString method. But as we talked before, I didn't find a good solution for the Uri.parse, my implementation options were all some kind of workaround to avoid direct Uri usage or way too overkill for a proper solution.

I think we'll have to choose the tradeoffs between keep or remove the Robolectric. What do you think?

As discussed on Slack, let's keep Roboelectric in place. Thanks for the investigation!