Add stock media source to media picker #13009
Conversation
…ture/12641-add-pexel-source-to-media-picker
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Generated by 🚫 dangerJS |
|
You can test the changes on this Pull Request by downloading the APK here. |
planarvoid
changed the title
…ture/12641-add-pexel-source-to-media-picker
|
Hi @planarvoid , I'm still reviewing the code of this PR but wanted to anticipate couple of issues I had while quick testing (using Pixel 3 Emu API 29); case 1
case 2Both in the Media Library and GB editor scenarios, at least for me the upload does not complete and the upload dialog stay opened; in logcat get the following error If I do the same from the same emulator and same user logged in but with vanilla, I do not get the issue. |
|
thanks for the review @develric ! I've fixed case 1. The issue was that we were setting recycler view visibility to GONE. I've changed that to INVISIBLE and I wasn't able to reproduce the crash ever since. I've also noticed another issue that the progress dialog was leaking the activity context. That's fixed as well. I'm not able to reproduce the second case. Could you check whether it's happening on all of your pages? |
There was a problem hiding this comment.
Hi @planarvoid , thanks for the follow up. I have few comments below and in line. Let me know wdyt, thanks 🙇 .
- The case 1 issue with
java.lang.NullPointerException: Attempt to invoke virtual methodseems fixed for me. I was not able to replicate it after your modifications 👍 - For case 2 issue, I think it could be related to the leak you fixed maybe (the error I was getting is I think because I was using a test site I have where the storage is full). But please see comment here for something I was thinking while looking into it, let me know 😄
- While checking for Adding from device scenario I get an issue reported in Permission issue while reading images from device #13055 . I don't think it's related to this PR so probably we can manage it with a dedicated PR. wdyt?
- Also in Adding from device scenario, I got the following error while adding an image to the GB block from device:
2020-10-05 16:40:17.821 28814-28814/org.wordpress.android.beta E/WindowManager: android.view.WindowLeaked: Activity org.wordpress.android.ui.mediapicker.MediaPickerActivity has leaked window DecorView@1a6a515[MediaPickerActivity] that was originally added here
at android.view.ViewRootImpl.<init>(ViewRootImpl.java:511)
at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:346)
at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:93)
at android.app.Dialog.show(Dialog.java:329)
at androidx.appcompat.app.AlertDialog$Builder.show(AlertDialog.java:1009)
at org.wordpress.android.ui.mediapicker.MediaPickerFragment$setupProgressDialog$1.onChanged(MediaPickerFragment.kt:427)
at org.wordpress.android.ui.mediapicker.MediaPickerFragment$setupProgressDialog$1.onChanged(MediaPickerFragment.kt:63)
at androidx.lifecycle.LiveData.considerNotify(LiveData.java:131)
at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:149)
at androidx.lifecycle.LiveData.setValue(LiveData.java:307)
at androidx.lifecycle.MutableLiveData.setValue(MutableLiveData.java:50)
at org.wordpress.android.util.LiveDataUtilsKt$map$1.onChanged(LiveDataUtils.kt:309)
at androidx.lifecycle.MediatorLiveData$Source.onChanged(MediatorLiveData.java:152)
at androidx.lifecycle.LiveData.considerNotify(LiveData.java:131)
at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:149)
at androidx.lifecycle.LiveData.setValue(LiveData.java:307)
at androidx.lifecycle.MutableLiveData.setValue(MutableLiveData.java:50)
at org.wordpress.android.util.LiveDataUtilsKt$merge$19.onChanged(LiveDataUtils.kt:274)
I think it could be related to the fact that the upload progress dialog is opened for a second and maybe it should not (could not take a picture for that since is pretty fast but I also think it is mentioning the stock in the dialog text so probably we should remove or customize it in this case? wdyt)
| } | ||
| is InsertModel.Error -> { | ||
| job = null | ||
| _showProgressDialog.postValue(Hidden) |
There was a problem hiding this comment.
Not sure if I'm missing something, but from what I got in case of error we end up closing the progress dialog but not reporting anything to user (is this correct? 🤔 ). If it's the case maybe we should use a toast or something, wdyt?
There was a problem hiding this comment.
yes, definitely. This is something I wanted to implement but forgot about. Good catch!
| val recyclerViewState = recycler.layoutManager?.onSaveInstanceState() | ||
| adapter.loadData(uiModel.items) | ||
| recycler.layoutManager?.onRestoreInstanceState(recyclerViewState) |
There was a problem hiding this comment.
We are removing from the new setupAdapter function the save/restore instance call on the recycler. I'm not sure why we were doing that tbh (maybe to save scroll position or similar? but we are loading new data so 🤷♂️). Just mentioning to be sure it was done on purpose, let me know wdyt 🙇
There was a problem hiding this comment.
I did it on purpose because I don't think it's necessary (it was one of the attempts to fix the crash). I don't think the scroll position is updated when we use the diff utils adapter. What do you think?
|
hey @develric, thanks for the review 👍 . I've implemented an upgraded solution for the progress dialog leak. Now we only show the progress dialog when the operation is taking longer than 100ms (and we don't show it at all in the super fast cases). Now it's shown only in the Stock media upload and in the featured image upload. I've also added a snackbar solution and now we show a snackbar when the insert fails. I think this is ready for another check |
There was a problem hiding this comment.
Hi @planarvoid 😄 . Thanks for the changes. I tested them in a Pixel 3 Emu API 28 and all worked as expected. Also the snackbar worked well in reporting an upload issue in the Stock picker 👍 . I'm approving this PR with the notes below (not merging to let you update the FluxC tag 🙇 ).
Related to the #13063 with API 29 since we have an open PR that should possibly fix it (#13063) and also the issue in my understanding should be bound to the new picker only that's behind feature flag. I think it's same case that the 13063, but mentioning here in case I'm missing anything. With Pixel 3 Emu API 29, in Media Library when adding media from device getting the following error (with API 28 it's working fine). Seems same case but let me know if you prefer me to open a dedicated issue 🙇
2020-10-06 23:05:38.807 26385-26582/org.wordpress.android.beta E/AndroidRuntime: FATAL EXCEPTION: DefaultDispatcher-worker-4
Process: org.wordpress.android.beta, PID: 26385
java.lang.IllegalStateException: storagePublicDirectory.listFiles() must not be null
at org.wordpress.android.ui.mediapicker.loader.DeviceListBuilder$addDownloads$2.invokeSuspend(DeviceListBuilder.kt:116)
at org.wordpress.android.ui.mediapicker.loader.DeviceListBuilder$addDownloads$2.invoke(Unknown Source:10)
at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:91)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:154)
at kotlinx.coroutines.BuildersKt.withContext(Unknown Source:1)
at org.wordpress.android.ui.mediapicker.loader.DeviceListBuilder.addDownloads(DeviceListBuilder.kt:114)
at org.wordpress.android.ui.mediapicker.loader.DeviceListBuilder$load$2$invokeSuspend$$inlined$map$lambda$4.invokeSuspend(DeviceListBuilder.kt:55)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:727)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:667)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:655)
2020-10-06 23:05:44.408 26827-26827/org.wordpress.android.beta E/BitmapFactory: Unable to decode stream: java.io.FileNotFoundException: /storage/emulated/0/Download/pexels-photo-531767.jpeg: open failed: EACCES (Permission denied)
2020-10-06 23:05:44.409 26827-26827/org.wordpress.android.beta E/BitmapFactory: Unable to decode stream: java.io.FileNotFoundException: /storage/emulated/0/Download/pexels-photo-531767.jpeg: open failed: EACCES (Permission denied)
Fixes #12641
This PR introduces the Stock media data source. Since the behaviour of the data source is different than the device list and the WP media library, it also introduces some changes to the MediaSource interface and the MediaLoader. The interface they use now is much simpler (with one simple
loadmethod).The behaviour of the stock media picker is that it starts empty (the empty screen is not yet implemented as it should be) with an always enabled
searchbar. Once you search a term longer than 2 letters, it searches for the items and populates the list.Since the stock media need to perform an upload operation once they are selected, I've introduced
MediaInsertUseCaseinterface for a use cases that handles the upload. Theinsertmethod is called when the user clicks oninsertand it shows a progress dialog. This could be used by any of the media sources. The default implementation doesn't do anything.I'll add more unit tests in the following PR
To test
+To test 2
To test 3
PR submission checklist:
RELEASE-NOTES.txtif necessary.