Tenor API integrated with GIF Picker by ThomazFB · Pull Request #1 · ThomazFB/WordPress-Android

ThomazFB · 2020-03-25T02:05:04Z

Summary

This Pull Request brings integration for the Tenor API within the pre-existent GIF selection code inside the app. As described at issue 11456.

The solution proposes the following implementation:

A GifProvider interface must be the only coupling between all code for the GIF picker and the Tenor implementation, avoiding any reference to the Tenor library and making it easy to replace the Provider as we're doing now
The TenorProvider implements the GifProvider interface and is injected within the DataSource via Dagger, this way only the AppModule knows about the TenorProvider. Also, the TenorProvider is covered by unit tests in the TenorProviderTest class.
The already implemented pagination feature and list actions are all kept and integrated to work smoothly with the GifProvider, this integration happens through the available PositionalDataSource, The Tenor implementation makes sure that positioning tracking is respected
The original integration with the MediaBrowserActivity was restored, making GIF selection available again

Known missing points for upcoming Pull Requests

Integration with EditPostActivity not restored yet
All Giphy packages and class naming is still missing, this will be the last modification to not storm the code review with name changes within the Pull Request
Change Giphy icons
Analytics support still to be added
Unit tests for PickerDataSource

Testing

Create a Tenor API key and paste it associated with the wp.tenor.api_key as the value inside your gradle.properties
Go to the Media Browser
Click on the Add button at the top right. Select the Choose from Tenor option
Type in the search bar to query GIFs
Once inside the Gif Picker, make sure that selection, preview and add are all working as expected

PR submission checklist

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…ed to the PickerDataSourceFactory

…ewModel and GifPickerDataSource

…t cases

…dded more documentation

…eature/insert-tenor-api

planarvoid

thanks for the changes! Overall it looks really good 👍 I have some comments, feel free to let me know what you think.

I've encountered a crash when searching for the string "isengard" (don't ask me why):

java.lang.IllegalArgumentException: PositionalDataSource requires initial load size to be a multiple of page size to support internal tiling. loadSize 3, position 0, totalCount 4, pageSize 21
        at androidx.paging.PositionalDataSource$LoadInitialCallbackImpl.onResult(PositionalDataSource.java:223)
        at org.wordpress.android.viewmodel.giphy.GiphyPickerDataSource$requestInitialSearch$1.invoke(GiphyPickerDataSource.kt:91)
        at org.wordpress.android.viewmodel.giphy.GiphyPickerDataSource$requestInitialSearch$1.invoke(GiphyPickerDataSource.kt:14)
        at org.wordpress.android.viewmodel.giphy.provider.TenorProvider$search$$inlined$simpleSearch$1.success(TenorProvider.kt:195)
        at org.wordpress.android.viewmodel.giphy.provider.TenorProvider$search$$inlined$simpleSearch$1.success(TenorProvider.kt:104)
        at com.tenor.android.core.response.WeakRefCallback.success(WeakRefCallback.java:184)
        at com.tenor.android.core.response.WeakRefCallback.access$100(WeakRefCallback.java:30)
        at com.tenor.android.core.response.WeakRefCallback$1.run(WeakRefCallback.java:88)
        at com.tenor.android.core.weakref.WeakRefRunnable.run(WeakRefRunnable.java:34)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

planarvoid · 2020-03-25T12:43:06Z

...Press/src/test/java/org/wordpress/android/viewmodel/giphy/provider/TenorProviderTestUtils.kt

+import com.tenor.android.core.model.impl.Result
+import org.wordpress.android.viewmodel.giphy.MutableGiphyMediaViewModel
+
+class TenorProviderTestUtils {


I think it would be cleaner to make this an object and remove the companion object.
And personally I'd call this class Fixtures even though I see there is no Fixture class yet in the WPAndroid project.

I liked a lot your suggestion for Fixture nomenclature, it makes a lot more sense. Maybe we could suggest starting inserting this kind of unit test code from now on? I've committed the change in 61e7bca. What do you think?

sounds good, thanks for the change

planarvoid · 2020-03-25T12:51:46Z

WordPress/src/test/java/org/wordpress/android/viewmodel/giphy/provider/TenorProviderTest.kt

+        verify(gifSearchCall, times(1)).enqueue(callbackCaptor.capture())
+        val capturedCallback = callbackCaptor.value
+        capturedCallback.success(ApplicationProvider.getApplicationContext(), gifResponse)
+        assertTrue("onSuccess should be called", onSuccessWasCalled)


I usually prefer usage of assertThat(onSuccessWasCalled).isTrue() from the org.assertj.core.api.Assertions.assertThat package. This is not very important but it tends to be more readable if you have more than one assertion.

Awesome suggestion, it really improves the assertion readability, it's done! Changed in 11fde9b.

planarvoid · 2020-03-25T12:54:16Z

WordPress/src/main/java/org/wordpress/android/viewmodel/giphy/provider/TenorProvider.kt

+     * the init will use the default implementation provided by the Tenor library.
+     */
+    init {
+        ApiService.Builder(context, IApiClient::class.java).apply {


I was wondering about initializing the api client in the init method. I was thinking that maybe it might be nicer to either do it in the ApplicationModule and directly inject the proper ApiClient or if this doesn't work, use the laze initialization on the apiClient by lazy {}.

Nice! Makes a lot of sense, I was intentionally trying to keep the API configuration inside the Provider, but your suggestion to initializing it inside the ApplicationModule created an opening for a more concise dependency construction inside the TenorProvider. Thanks! The change is committed at 0b7035d.

Thanks for the change! I think both options (application module and kotlin lazy load) would be fine, it just felt that the init is somehow between these two approaches 👍

planarvoid · 2020-03-25T12:54:34Z

WordPress/src/main/java/org/wordpress/android/viewmodel/giphy/provider/TenorProvider.kt

+
+internal class TenorProvider @JvmOverloads constructor(
+    val context: Context,
+    tenorClient: IApiClient? = null


Is this field planned to be injected? Or could it be removed?

This field was created to make an easy opening for unit tests but allowing it as an optional value without disregard the API initialization. I think the change at 0b7035d already solved this point since the initialization of the API is now being made outside the TenorProvider, at the ApplicationModule, and the injection of the IApiClient happens right there after the API key configuration.

What do you think?

good point about the unit tests, I like the way it's done now

planarvoid · 2020-03-25T12:55:05Z

WordPress/src/main/java/org/wordpress/android/viewmodel/giphy/provider/TenorProvider.kt

+    /**
+     * To better refers to the Tenor API maximum GIF limit per request
+     */
+    private val maximumAllowedLoadSize = 50


Just for consistency, I'd move this to the companion object and use snake case.

Done! Changed in 6ee5513

planarvoid · 2020-03-25T13:14:54Z

WordPress/src/main/java/org/wordpress/android/viewmodel/giphy/GiphyPickerViewModel.kt

+                .setEnablePlaceholders(false)
+                .setPrefetchDistance(15)
+                .setInitialLoadSizeHint(42)
+                .setPageSize(21)


Maybe pull out all of these constants to a companion object?

Done! Changed in e5eef80.

…actored the class to become a object

…dability

ThomazFB · 2020-03-25T22:17:50Z

thanks for the changes! Overall it looks really good 👍 I have some comments, feel free to let me know what you think.

I've encountered a crash when searching for the string "isengard" (don't ask me why):

java.lang.IllegalArgumentException: PositionalDataSource requires initial load size to be a multiple of page size to support internal tiling. loadSize 3, position 0, totalCount 4, pageSize 21
        at androidx.paging.PositionalDataSource$LoadInitialCallbackImpl.onResult(PositionalDataSource.java:223)
        at org.wordpress.android.viewmodel.giphy.GiphyPickerDataSource$requestInitialSearch$1.invoke(GiphyPickerDataSource.kt:91)
        at org.wordpress.android.viewmodel.giphy.GiphyPickerDataSource$requestInitialSearch$1.invoke(GiphyPickerDataSource.kt:14)
        at org.wordpress.android.viewmodel.giphy.provider.TenorProvider$search$$inlined$simpleSearch$1.success(TenorProvider.kt:195)
        at org.wordpress.android.viewmodel.giphy.provider.TenorProvider$search$$inlined$simpleSearch$1.success(TenorProvider.kt:104)
        at com.tenor.android.core.response.WeakRefCallback.success(WeakRefCallback.java:184)
        at com.tenor.android.core.response.WeakRefCallback.access$100(WeakRefCallback.java:30)
        at com.tenor.android.core.response.WeakRefCallback$1.run(WeakRefCallback.java:88)
        at com.tenor.android.core.weakref.WeakRefRunnable.run(WeakRefRunnable.java:34)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Hi @planarvoid! Thank you for your review! I replied to every conversation with the proposed fix!

About this crash, it was a desync between the position track returned by Tenor and our PositionalDataSource when a small list of GIFs is returned for a given query, so the reason why "isengard" was crashing the app is that this query only returns 3 GIFs, triggering this bug. With that said, I made the DataSource more resilient to this kind of scenario. The change is committed in f7ec187.

planarvoid

Thanks for the changes and for the fix! The code now looks great and the feature works well 👍. I cannot merge this branch but feel free to merge it into the feature branch.

ThomazFB added 21 commits March 20, 2020 21:33

Added Tenor library and API key reference

8f6d5cf

Changed giphy package name to gifs

4fe0f09

Created TenorProvider infrastructure and basic search

e36e349

Added GifProvider and TenorProvider documentation

7f42109

GifProvider is now available through injection with Dagger and provid…

1a4398a

…ed to the PickerDataSourceFactory

Replaced Giphy name to Gif from all packages and classes

40b4a4d

Connected the TenorProvider implementation to the expected GifMediaVi…

90b4f03

…ewModel and GifPickerDataSource

Improved test suite for TenorProvider with code refactor and more tes…

e45b108

…t cases

Improved code reuse, naming and deprecated assertion methods

10ef963

Gif data class removed, using the MutableGifMediaViewModel instead. A…

e6f2d5e

…dded more documentation

Gif data class removed, using the MutableGifMediaViewModel instead. A…

7566d20

…dded more documentation

Merge remote-tracking branch 'origin/feature/insert-tenor-api' into f…

2f02897

…eature/insert-tenor-api

Fixed GiphyPickerActivity reference on AndroidManifest

ca77671

Integrated pagination into the Tenor requests

40d485c

Reverted Giphy renaming

591d36e

Reverted Giphy renaming for packages

49f9edb

Additional code refactoring and new test for TenorProvider

b62904a

Reconnected GIF search with MediaBrowserActivity

c7ce801

Initial load changed to 42

c562361

Improved documentation for GifProvider

ce388ed

Added code refactoring for parameter organization

676c6e7

ThomazFB changed the title ~~Tenor API integrated with GIF Picker #1~~ Tenor API integrated with GIF Picker Mar 25, 2020

ThomazFB added Media [Type] Enhancement labels Mar 25, 2020

ThomazFB added 3 commits March 25, 2020 00:09

Removed unwanted Tenor text reference

bbf1d8f

Added code refactoring for TenorProvider

99f2fb1

Improved documentation over TenorProvider methods

d80f24c

planarvoid reviewed Mar 25, 2020

View reviewed changes

ThomazFB added 2 commits March 25, 2020 16:47

Added resilience to the initial load fetching

f7ec187

Changed constant to better fit code convention

6ee5513

ThomazFB added 4 commits March 25, 2020 17:13

Changed TenorProviderTestUtils to TenorProviderTestFixtures, also ref…

61e7bca

…actored the class to become a object

Changed test assertion to use assertThat pattern and improve test rea…

11fde9b

…dability

Moved Tenor API initialization to the outside of TenorProvider

0b7035d

Extracted magic numbers to constants

e5eef80

planarvoid approved these changes Mar 26, 2020

View reviewed changes

ThomazFB merged commit 78106a9 into issue/11456-add-tenor-support Mar 26, 2020

ThomazFB mentioned this pull request Apr 4, 2020

Add Tenor support wordpress-mobile/WordPress-Android#11587

Merged

3 tasks

Conversation

ThomazFB commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Known missing points for upcoming Pull Requests

Testing

PR submission checklist

Uh oh!

planarvoid left a comment

Choose a reason for hiding this comment

Uh oh!

planarvoid Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomazFB Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomazFB Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomazFB commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

planarvoid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ThomazFB commented Mar 25, 2020 •

edited

Loading

planarvoid Mar 25, 2020 •

edited

Loading

ThomazFB Mar 25, 2020 •

edited

Loading

ThomazFB Mar 25, 2020 •

edited

Loading

ThomazFB commented Mar 25, 2020 •

edited

Loading

planarvoid left a comment •

edited

Loading