[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13612

jd-alexander · 2020-12-16T07:10:54Z

gutenberg: WordPress/gutenberg#27401
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2854
WPiOS PR: wordpress-mobile/WordPress-iOS#15420
FluxCAndroid wordpress-mobile/WordPress-FluxC-Android#1823

Solution

This solution seeks to address the ability to add an audio file to the Audio Block from the device or WordPress media library utilizing the media picker components that are present within the codebase. A similar implementation was created for the File Block. This PR integrates the menu options from the Audio block with the consolidated media picker.
The primary changes are:

introducing a SINGLE_AUDIO_FILE_PICKER type in the MediaBrowserType so the behavior that's specific to the Audio Block can be constrained by this picker type.
The GutenbergEditor's WP Media Library AudioButtonClicked behavior was wired to the WP Media Library and local file source behavior that opens the pickers.
showFilePicker was duplicated and a showAudioFilePicker variant was created to manage the device file picker for audio.

Prerequisite For All Testing

Ensure you are utilizing a site that is on a premium plan as free plans aren't able to upload audio files.

To test:

Consolidated Media Picker

  Picking local files.

Create a new post.
Add an Audio Block.
Press Choose Audio.
Select “Choose from device”.
The File Picker should be visible.
You should be able to pick an audio file. ( No other file type should be present.)
You should see the "Audio Player goes here" comment with upload progress.
You can preview the post and download the file for further validation.

Select file	See Picker

Normal Media Picker

  Picking local files.

Create a new post.
Add an Audio Block.
Press Choose Audio.
Select “Choose from device”.
The local file picker should be visible.
You should be able to pick an audio file. ( No other file type should be present.)
You should see the "Audio Player goes here" comment with upload progress.
You can preview the post and download the file for further validation.

Select file	See Picker

Consolidated Media Picker

Picking WP media picker files.

Create a new post.
Add an Audio Block.
Press Choose Audio.
Select “WordPress Media Library”
Select an audio file. ( No other file type should be present.)
You should see the "Audio Player goes here" comment.
You can preview the post and download the file for further validation.

Select Option	See Picker

Normal Media Picker

Picking WP media picker files.

Create a new post.
Add an Audio Block.
Press Choose Audio.
Select “WordPress Media Library”
Select an audio file. ( No other file type should be present.)
You should see the "Audio Player goes here" comment.
You can preview the post and download the file for further validation.

Select Option	See Picker

File Block - Consolidated and Normal Media Picker

Verify that the File Block's behavior of being able to select any type of file either locally or from the WordPress Media Library is intact and no regressions have occurred since most of its behavior is shared with the Audio block.

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.

peril-wordpress-mobile · 2020-12-16T07:11:01Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2020-12-16T07:30:21Z

You can test the changes on this Pull Request by downloading the APK here.

jd-alexander · 2020-12-17T00:34:39Z

WordPress/src/main/java/org/wordpress/android/ui/photopicker/MediaPickerLauncher.kt

@@ -212,6 +212,32 @@ class MediaPickerLauncher @Inject constructor(
        }
    }

+    fun showAudioFilePicker(activity: Activity, canMultiselect: Boolean = false) {
+        if (consolidatedMediaPickerFeatureConfig.isEnabled()) {


I am not sure if this approach is fine. What I did here was I duplicated the showFilePicker method because I need the allowTypes to be different for the file and audio pickers. At first, I thought of making this property a default one in the method params but this doesn't work with Java. Is this behavior okay? Or should I do some form of method override? Let me know.

Also, I didn't bother to bring the Normal Picker behavior up to speed here because it seems like the consolidated media picker will be the only one in use for the near future so I thought I would waste time building it out and there's a high possibility we won't do a rollback. Was that the right judgment call? ? I did make modifications in the past to support the File Block's file selection behavior on the normal and consolidated picker hence my question.

What I would consider doing here is creating a private fun that incorporates the common code and that accepts (activity, canMultiselect, allowedTypes) then the publicly exposed methods (showFilePicker, showAudioFilePicker) can call this private fun passing the desired allowedTypes (I would not expose the allowedTypes as public fun params though so the calling function should not worry about it). This in a way also ease the second item:

Also, I didn't bother to bring the Normal Picker behavior up to speed here because it seems like the consolidated media picker will be the only one in use for the near future so I thought I would waste time building it out and there's a high possibility we won't do a rollback.

I think the rationale here was as you mention to allow an easy rollback (doing it by remote) in case of any issues with the consolidated media picker. To allow this we need to have the standard media picker fallback in place. I agree it's pretty unlikely we should rollback since the media picker has been released in 16.2 already, but until we do not remove this duplicated approach (probably in other couple of release or so) I would keep this consistent. wdyt?

This is great insight @develric I am going to do this implementation and then share my findings with you.

Done ff71990 3ca76e1

develric

Hey @jd-alexander 👋 ! I checked the code and it looks good overall. I left a couple of comments for you to review. Let me know wdyt 🙇 .

This said, if I'm not wrong I see the Audio block is not in the APK from this PR; I tried to build the GB from sources adding wp.BUILD_GUTENBERG_FROM_SOURCE=true to my gradle.properties but I think I have some missing piece in my local setup to build GB locally (getting every kind of errors 😄 ). I think I need to spend some time to get my setup good for local GB building or maybe there is another way we can have the CI built APK have audio block enabled? Let's sync when you come online on this in case I'm missing anything, thanks 🙇‍♂️..

develric · 2020-12-21T10:56:29Z

WordPress/src/main/java/org/wordpress/android/ui/photopicker/MediaPickerLauncher.kt

@@ -212,6 +212,32 @@ class MediaPickerLauncher @Inject constructor(
        }
    }

+    fun showAudioFilePicker(activity: Activity, canMultiselect: Boolean = false) {
+        if (consolidatedMediaPickerFeatureConfig.isEnabled()) {


What I would consider doing here is creating a private fun that incorporates the common code and that accepts (activity, canMultiselect, allowedTypes) then the publicly exposed methods (showFilePicker, showAudioFilePicker) can call this private fun passing the desired allowedTypes (I would not expose the allowedTypes as public fun params though so the calling function should not worry about it). This in a way also ease the second item:

Also, I didn't bother to bring the Normal Picker behavior up to speed here because it seems like the consolidated media picker will be the only one in use for the near future so I thought I would waste time building it out and there's a high possibility we won't do a rollback.

I think the rationale here was as you mention to allow an easy rollback (doing it by remote) in case of any issues with the consolidated media picker. To allow this we need to have the standard media picker fallback in place. I agree it's pretty unlikely we should rollback since the media picker has been released in 16.2 already, but until we do not remove this duplicated approach (probably in other couple of release or so) I would keep this consistent. wdyt?

develric · 2020-12-21T11:24:35Z

...ressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

+        int chooseAudioFileResourceId =
+                getResources().getIdentifier("photo_picker_choose_file", "string", packageName);
+
+        otherMediaOptions.add(new MediaOption(MEDIA_SOURCE_AUDIO_FILE, getString(chooseAudioFileResourceId)));


When it comes to GB, GlueCode etc I'm still on the learning path 😄 , so not sure the following is worth or can have any constraint, but was wondering given the only difference seems to be the MEDIA_SOURCE_FILE vs MEDIA_SOURCE_AUDIO_FILE parameter, should we try to extract here also the common code with initOtherMediaFileOptions? wdyt?

That sounds like a good idea @develric Thanks! I am going to look into doing this.

Done e4039c0 c724657

guarani · 2021-01-04T19:37:24Z

Bumping the milestone in light of wordpress-mobile/gutenberg-mobile#2854 (comment)

Refactored the prepareIntent methods since behavior was shared between them all.

WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java

develric

Thanks for providing an APK since the ci built APK seems to not contain the audio block for me (maybe some flag we should take care of, ci built APK are really handy when reviewing 😄 ).

Code looks good overall, good job 👍 . I left a couple of comment inline for your review 🙇 . I also tested using your APK. Choosing from WP Media lib works as expected both with new and old media picker, but I found the following. Let me know 😊

unless the apk did not rebuild correctly or I'm missing something, I still cannot pick mp3 and wav audio files from the device internal storage (like mp3 and wav are grayed out); ogg seems to work. This is true in GB and media picker also. It can be a different task from this PR, but actually for the Media Lib I don't remember which was the expected behaviour (cc @planarvoid ), should we have a dedicated Choose audio from device menu or leverage Choose file from device? @jd-alexander let me know if you can reproduce or you need me to make some additional check for that 🙇‍♂️. (I was using a Business plan test site if this matters)
don't think it's related to this PR but I'm mentioning to cross check with you. I think currently I cannot play the uploaded audio file from inside the editor right? So I tried to open a preview of the post I was editing and I get the audio player with the file I uploaded. If I then try to play that file and exit the preview, the audio is still playing and I need to kill the app to stop it. If this happens to you also maybe we should open a GH issue for it.

PS: approved the fluxc companion PR if you want to tag and relink it here 👍 .

develric · 2021-01-13T21:21:49Z

WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java

        } else if ((mBrowserType.isSingleImagePicker() || mBrowserType.isSingleMediaPicker() || mBrowserType
-                .isSingleFilePicker()) && !isLongClick) {
+                .isSingleFilePicker()) || mBrowserType
+                .isSingleAudioFilePicker() && !isLongClick) {


I suspect the parenthesis here are not what we want? I would put them to group all the || before the && !isLongClick . wdyt?

Sounds about right. Thanks 3309187

develric · 2021-01-13T21:46:54Z

WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java

        if (multiSelect) {
-            intent.putExtra(Intent.EXTRA_ALLOW_MULTIPLE, true);
+            intent.putExtra(Intent.EXTRA_ALLOW_MULTIPLE, multiSelect);


tbh I would rather keep the true, it's like more obvious we actually want the only possible value to be true (in fact we check for if (multiselect) and we don't want to have a false value there (but rather no value at all) when we don't want to multiselect. wdyt?

Oh yes, good spotting that!

Good catch. Resolved here 1e5aa6d

jd-alexander · 2021-01-14T02:19:06Z

Thanks for providing an APK since the ci built APK seems to not contain the audio block for me (maybe some flag we should take care of, ci built APK are really handy when reviewing 😄 ).

No problem 🙇

Code looks good overall, good job 👍 . I left a couple of comment inline for your review 🙇 . I also tested using your APK. Choosing from WP Media lib works as expected both with new and old media picker, but I found the following. Let me know 😊

I appreciate it :)

unless the apk did not rebuild correctly or I'm missing something, I still cannot pick mp3 and wav audio files from the device internal storage (like mp3 and wav are grayed out); ogg seems to work. This is true in GB and media picker also. It can be a different task from this PR,
This is interesting because I am not experiencing this issue on Google Pixel 3 XL. Below is a gif utilizing all these file types.

Consolidated Media Picker	Normal Picker

Which file app are you utilizing? Could you create a video recording of your screen so I can see exactly what is happening?
The File Manager apps out there have their own means of interpreting the intent and the MIME types supported so there's a possibility that some may not do this as well as we would like. This hypothesis would have to be tested. I am working on a way to test a popular file app to see what the results are.

but actually for the Media Lib I don't remember which was the expected behaviour (cc @planarvoid ), should we have a dedicated Choose audio from device menu or leverage Choose file from device? @jd-alexander let me know if you can reproduce or you need me to make some additional check for that 🙇‍♂️. (I was using a Business plan test site if this matters)

Yes, if you could reproduce this it would be great as I am learning about the consolidated media picker project as I go through the code.

don't think it's related to this PR but I'm mentioning to cross check with you. I think currently I cannot play the uploaded audio file from inside the editor right?

Good question! This behavior will probably be in an updated version of the Audio block.

So I tried to open a preview of the post I was editing and I get the audio player with the file I uploaded. If I then try to play that file and exit the preview, the audio is still playing and I need to kill the app to stop it. If this happens to you also maybe we should open a GH issue for it.

I am going to check this out. Thanks for reporting!

PS: approved the fluxc companion PR if you want to tag and relink it here 👍 .

Thanks much!

…picker_integration

develric · 2021-01-14T23:58:14Z

Hey @jd-alexander 👋 . I tried to dig a bit forcing some behaviour with the debugger, what I can say is:

Using the Pixel 3 Emu API 30 the system picker seems to work as expected for me
Using the default system picker in my OnePlus 7T I'm getting the issues I reported above. So, as you were suggesting, it seems something related to specific sistem picker (but I'm using the OnePlus stock 🤷 ).

Maybe we should further dig the ways we are opening the system picker (at least for audio) also on other devices. I see we are setting the following:

type: */*
EXTRA_MIME_TYPES: our audio supported mime types (like audio/mpeg, audio/ogg etc)

so basically relaying on the EXTRA_MIME_TYPES to filter out the unwanted mime types.

In my case with the above I get some audio files grayed out but cannot understand with which logic (maybe something in the files but they are basically the test files we are using in the Pixel Emu also)

I get the same results if I keep the EXTRA_MIME_TYPES but set type as audio/*. If I set type as audio/* and do not set the EXTRA_MIME_TYPES I actually get something that seems more correct

but in this case I guess we are potentially not filtering out not supported audio mime types that's why we set the EXTRA_MIME_TYPES.

Googling quickly I see that there can be also the possibility to put multiple sets in the type like audio/mpeg|audio/ogg but running out of time and couldn't try it.

Unfortunately it seems I still cannot build GB with my setup, so not easy to check things, but since we have still some time before the end of the sprint maybe we should spend a bit more time to check how we can broaden device support when coming to picking from system picker (at least for audio). For example in this PR we are using type */* but thinking to it maybe an audio/* would be more correct (at least in the editor)? Are we releasing the audio block with this PR or this is an interstitial PR? We can probably address this in a separate PR but let's keep in touch on slack to decide how to proceed 🙇

…picker_integration

jd-alexander · 2021-01-15T02:47:13Z

Thanks for digging into this @develric and I agree that we will have to play with the params below that are being passed to the intent to achieve behavior that's compatible with all file pickers.

intent.setType(intentType);
intent.putExtra(Intent.EXTRA_MIME_TYPES, mimeTypes);

Are we releasing the audio block with this PR or this is an interstitial PR? We can probably address this in a separate PR but let's keep in touch on slack to decide how to proceed 🙇

Yes, this is an intermediary PR, so there are several other PRs depending on it that need to be merged in so it would be good to get these PRs merged in, and then we can create an issue and address the improvements we need separately. Thanks much for the thorough review and actually spotting this on a different device configuration.

…picker_integration

develric

Thanks for all the work and improvements here @jd-alexander 🙇 . Let's make sure to open a ticket to further dig those intent parameters for system pickers support before final release of audio block (let me know if you want me to open it). This said LGTM and good job 👍 .

jd-alexander · 2021-01-16T00:15:20Z

Thanks for all the work and improvements here @jd-alexander 🙇 . Let's make sure to open a ticket to further dig those intent parameters for system pickers support before final release of audio block (let me know if you want me to open it). This said LGTM and good job 👍 .

Thanks again @develric I created an issue to track the intent params issue here so we can continue looking into it.

jd-alexander · 2021-01-19T21:12:50Z

This PR was merged before the gutenberg and gutenberg mobile PRs were approved, so a revert PR was created to remove the changes from develop and a new PR was reopened with the changes pending another review.

jd-alexander added 4 commits December 16, 2020 00:52

Added GUTENBERG_SINGLE_AUDIO_FILE_PICKER to the MediaBrowserType

1e75b68

Added support for selecting Audio from Media Library or Local Device.

08bd4aa

wired media pickers to the click actions of the Audio block.

9c89fa3

Updated gutenberg ref.

6b389c4

jd-alexander added [Type] Task gutenberg-mobile labels Dec 16, 2020

jd-alexander added this to the 16.5 milestone Dec 16, 2020

jd-alexander force-pushed the gutenberg/audio_block_consolidated_media_picker_integration branch from be8e680 to 6b389c4 Compare December 17, 2020 00:31

Trigger Build

10992e1

jd-alexander commented Dec 17, 2020

View reviewed changes

jd-alexander requested review from planarvoid and develric and removed request for planarvoid December 17, 2020 00:38

jd-alexander marked this pull request as ready for review December 17, 2020 00:38

jd-alexander mentioned this pull request Dec 17, 2020

Gutenberg - Audio Block Processor #13575

Merged

3 tasks

develric self-assigned this Dec 21, 2020

develric reviewed Dec 21, 2020

View reviewed changes

guarani modified the milestones: 16.5, 16.6 Jan 4, 2021

jd-alexander added 8 commits January 6, 2021 19:29

Changed location of prepare picture intent and fixed multiselect params

4163371

Added prepareAudioLibraryIntent method.

b286de9

Added prepareAudioLibraryIntent and did refactoring of duplicate logic.

b73c139

Refactored the prepareIntent methods since behavior was shared between them all.

added showMediaPicker for file and audio.

ff71990

created a function that allows the audio file picker to be launched.

203de09

added the editor_audio_file_added event to track audio files.

a8a1e82

Integrated the Audio Library Request Code in the necessary activities.

408cf6d

Refactored the initOtherMediaFileOptions to support both audio and file.

e4039c0

jd-alexander added 3 commits January 7, 2021 20:50

Added a string value for choosing audio files and refactored launchFile

fbc51cf

Added a method for GUTENBERG_SINGLE_AUDIO_FILE_PICKER to filter the grid

b87c11e

refactored the launchFileLibrary to work for audio and file intents.

97abd5a

jd-alexander commented Jan 8, 2021

View reviewed changes

WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java Show resolved Hide resolved

jd-alexander added 4 commits January 12, 2021 20:44

Added the audio picker to the single selection.

8262169

Added AUDIO to the ChooserContext

aedea20

Added AUDIO to the populateIconClickEvent

beb48b7

Added test for Audio System Picker

7815032

jd-alexander force-pushed the gutenberg/audio_block_consolidated_media_picker_integration branch from 45a3ade to 7815032 Compare January 13, 2021 17:44

develric reviewed Jan 13, 2021

View reviewed changes

jd-alexander added 5 commits January 13, 2021 21:59

removed multiSelect intent param and utilized true.

1e5aa6d

Put the .SingleAudioFilePicker call in the appropriate group.

3309187

Merge branch 'develop' into gutenberg/audio_block_consolidated_media_…

adbb939

…picker_integration

Updated fluxC hash.

9f745ae

Updated gb ref.

a75a1b3

Merge branch 'develop' into gutenberg/audio_block_consolidated_media_…

21e7726

…picker_integration

jd-alexander requested a review from develric January 15, 2021 03:20

Merge branch 'develop' into gutenberg/audio_block_consolidated_media_…

8025aaf

…picker_integration

develric approved these changes Jan 15, 2021

View reviewed changes

jd-alexander mentioned this pull request Jan 16, 2021

Audio MIME types not supportive by some File Pickers #13792

Open

develric merged commit 9285a4c into develop Jan 18, 2021

develric deleted the gutenberg/audio_block_consolidated_media_picker_integration branch January 18, 2021 16:10

develric restored the gutenberg/audio_block_consolidated_media_picker_integration branch January 18, 2021 17:29

develric mentioned this pull request Jan 18, 2021

Revert "[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration" #13805

Merged

jd-alexander mentioned this pull request Jan 18, 2021

[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13808

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13612

[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13612

jd-alexander commented Dec 16, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 16, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 16, 2020 •

edited

Loading

jd-alexander Dec 17, 2020

jd-alexander Dec 17, 2020

develric Dec 21, 2020

jd-alexander Jan 6, 2021

jd-alexander Jan 8, 2021

develric left a comment

develric Dec 21, 2020

develric Dec 21, 2020

jd-alexander Jan 5, 2021

jd-alexander Jan 8, 2021

guarani commented Jan 4, 2021

develric left a comment •

edited

Loading

develric Jan 13, 2021

jd-alexander Jan 14, 2021

develric Jan 13, 2021

jd-alexander Jan 14, 2021

jd-alexander Jan 14, 2021

jd-alexander commented Jan 14, 2021 •

edited

Loading

develric commented Jan 14, 2021

jd-alexander commented Jan 15, 2021

develric left a comment

jd-alexander commented Jan 16, 2021

jd-alexander commented Jan 19, 2021

[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13612

[Gutenberg] Audio block - Consolidated & Normal Media Picker Integration #13612

Conversation

jd-alexander commented Dec 16, 2020 • edited Loading

Solution

Prerequisite For All Testing

Consolidated Media Picker

Picking local files.

Normal Media Picker

Picking local files.

Consolidated Media Picker

Picking WP media picker files.

Normal Media Picker

Picking WP media picker files.

File Block - Consolidated and Normal Media Picker

peril-wordpress-mobile bot commented Dec 16, 2020 • edited Loading

peril-wordpress-mobile bot commented Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

develric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guarani commented Jan 4, 2021

develric left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jd-alexander commented Jan 14, 2021 • edited Loading

develric commented Jan 14, 2021

jd-alexander commented Jan 15, 2021

develric left a comment

Choose a reason for hiding this comment

jd-alexander commented Jan 16, 2021

jd-alexander commented Jan 19, 2021

jd-alexander commented Dec 16, 2020 •

edited

Loading

  Picking local files.

  Picking local files.

peril-wordpress-mobile bot commented Dec 16, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 16, 2020 •

edited

Loading

develric left a comment •

edited

Loading

jd-alexander commented Jan 14, 2021 •

edited

Loading