Feature/photo picker for all editors #5644

nbradbury · 2017-04-13T19:06:43Z

Enables the new photo picker for all editors (prior to this PR, it was only enabled for the Aztec editor), which finally does away with the seven-item menu monstrosity 🎉

A simple way to test this is to manually change these two variables.

cc: @daniloercoli

daniloercoli · 2017-04-13T20:32:33Z

@mzorz or @aforcier - Would you mind to review this PR since I'm AFK for few days?

…ss-Android into feature/photo-picker-for-all-editors

aforcier · 2017-04-14T12:03:28Z

Haven't dived into the code yet, but I have a few broad concerns about this removing features that already exist in the legacy/hybrid editor (though not, yet, in Aztec):

This seems to remove gallery creation from the editor entirely - shouldn't we wait to have that implemented in the new picker before dropping the existing monstrosity?
While the overlay part of the photo picker seems to show both photos and videos, there doesn't seem to be a way to launch the native picker (using the middle bottom bar icon) to include video (as the third option in the monstrosity did)
Similar to above (but far less important imo), we're unable to launch the camera for video capture (fourth option in monstrosity)

nbradbury · 2017-04-14T12:35:03Z

there doesn't seem to be a way to launch the native picker (using the middle bottom bar icon) to include video

Oof, I hadn't noticed that. I guess we have two options here:

Add a separate icon for picking a video
When the middle icon is tapped, show a popup with "Choose photo" and "Choose video."

I'm not wild about either of those, but I think I'd go with the second option. What do you think?

aforcier · 2017-04-14T12:43:57Z

I guess we have two options here:

Add a separate icon for picking a video

When the middle icon is tapped, show a popup with "Choose photo" and "Choose video."

I'm not wild about either of those, but I think I'd go with the second option. What do you think?

I'd say the second option sounds better too, but I'm not wild about either one either. What do you think we should do here @bysl?

Whatever we decide, should we do the same for the camera (still photo vs video)?

aforcier · 2017-04-14T14:37:03Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

@@ -855,62 +840,6 @@ public void onClick(DialogInterface dialog, int id) {
        return false;
    }

-    @Override
-    public void openContextMenu(View view) {


With the context menu removed, we should also drop now-dead calls to registerForContextMenu() and getActivity().openContextMenu() in all three editor fragments.

aforcier · 2017-04-14T14:55:44Z

Should we disable the rest of the format bar while in media mode for the visual and legacy editors (as we do for Aztec)?

aforcier · 2017-04-14T14:56:32Z

This is very minor and should be a separate (low priority) issue:

With the media picker visible the editor screen is grayed, which suggests that I can't input any text, but with a hardware keyboard I can keep typing. If possible we should probably prevent that.

nbradbury · 2017-04-15T00:57:17Z

there doesn't seem to be a way to launch the native picker (using the middle bottom bar icon) to include video

I corrected this in 8f2633e. Now tapping the camera or picker icons shows a popup which enables choosing between photos and videos.

Should we disable the rest of the format bar while in media mode for the visual and legacy editors (as we do for Aztec)?

I think we're okay not doing this since those editors won't be with us much longer. Note that this PR hides the picker when the link or html buttons are pressed, and pressing any of the other buttons doesn't really impact the picker.

With the media picker visible the editor screen is grayed, which suggests that I can't input any text, but with a hardware keyboard I can keep typing.

Blah, hadn't thought about that. If it's okay with you, I'd like to open a separate issue for that.

…ss-Android into feature/photo-picker-for-all-editors

aforcier · 2017-04-17T15:19:09Z

Now tapping the camera or picker icons shows a popup which enables choosing between photos and videos.

Looks great @nbradbury!

Should we disable the rest of the format bar while in media mode for the visual and legacy editors (as we do for Aztec)?

I think we're okay not doing this since those editors won't be with us much longer. Note that this PR hides the picker when the link or html buttons are pressed, and pressing any of the other buttons doesn't really impact the picker.

I can live with that 👍

With the media picker visible the editor screen is grayed, which suggests that I can't input any text, but with a hardware keyboard I can keep typing.

Blah, hadn't thought about that. If it's okay with you, I'd like to open a separate issue for that.

Absolutely 👍

aforcier · 2017-04-17T15:24:00Z

To summarize, I think there are just three things left for this PR:

In the WP media library screen, enable multi-select and use that to create a gallery, so we're not dropping support for it (as we discussed)
This small cleanup nit
Some design input on the photo/video popup behavior you added (and the coming gallery implementation) (cc @bysl)

aforcier · 2017-04-18T16:41:39Z

I found a crash when launching the camera, taking a photo, and attempting to use it:

java.lang.SecurityException: Permission Denial: opening provider android.support.v4.content.FileProvider from ProcessRecord{43a76ae8 13380:com.sec.android.app.camera/u0a10083} (pid=13380, uid=10083) that is not exported from uid 10159
at android.os.Parcel.readException(Parcel.java:1431)
at android.os.Parcel.readException(Parcel.java:1385)
at android.app.ActivityManagerProxy.getContentProvider(ActivityManagerNative.java:2906)
at android.app.ActivityThread.acquireProvider(ActivityThread.java:4755)
at android.app.ContextImpl$ApplicationContentResolver.acquireUnstableProvider(ContextImpl.java:2495)
at android.content.ContentResolver.acquireUnstableProvider(ContentResolver.java:1152)
at android.content.ContentResolver.openAssetFileDescriptor(ContentResolver.java:667)
at android.content.ContentResolver.openOutputStream(ContentResolver.java:540)
at android.content.ContentResolver.openOutputStream(ContentResolver.java:516)
at com.sec.android.app.camera.Camera$LastContentUriCallback.onCompleted(Camera.java:5908)
at com.sec.android.app.camera.Camera.onLaunchGallery(Camera.java:5799)
at com.sec.android.app.camera.Camera.onImageStoringCompleted(Camera.java:5167)
at com.sec.android.app.camera.CameraEngine.imageStoringCompleted(CameraEngine.java:2326)
at com.sec.android.app.camera.CeStateInitialized.handleMessage(CeStateInitialized.java:47)
at com.sec.android.app.camera.CameraEngine$StateMessageHandler.handleMessage(CameraEngine.java:253)

This was on a Samsung S3 API18, and also on an API16 emulator.

aforcier · 2017-04-18T16:46:44Z

Related to permissions, I tried disabling permissions on my Pixel and then trying to use the picker. I get two permissions pop-ups, one for storage and one for camera.

If I grant the storage permission but deny the camera permission, I get a Permissions required in order to access media toast and can't use the picker.

Shouldn't we instead launch the picker but re-prompt for permission if and when the user taps the camera button? Or even only ask for storage permission the first time and hold off on camera until it's needed (I'm not sure if that might get complicated with the middle, native picker button though).

sonderingheights · 2017-04-21T02:43:38Z

Some design input on the photo/video popup behavior you added (and the coming gallery implementation) (cc @bysl)

This looks okay to me. Prefer it over another icon where it would be less clear than the text.

nbradbury · 2017-04-22T19:22:18Z

I found a crash when launching the camera, taking a photo, and attempting to use it:

@aforcier I'm not able to reproduce this, but I don't think it's an issue specific to this PR because I didn't change anything with the camera (it uses pre-existing code). Does the same problem occur in develop?

Related to permissions, I tried disabling permissions on my Pixel and then trying to use the picker. I get two permissions pop-ups, one for storage and one for camera.

This is also pre-existing code. We should open an issue for this, but we want to ping @kwonye since he's working on soft asks and this is related.

In the WP media library screen, enable multi-select and use that to create a gallery, so we're not dropping support for it (as we discussed)

Addressed separately in #5721.

With the context menu removed, we should also drop now-dead calls to registerForContextMenu() and getActivity().openContextMenu() in all three editor fragments.

Addressed in 762ef92

…ss-Android into feature/photo-picker-for-all-editors

aforcier · 2017-04-24T14:53:01Z

I found a crash when launching the camera, taking a photo, and attempting to use it:

@aforcier I'm not able to reproduce this, but I don't think it's an issue specific to this PR because I didn't change anything with the camera (it uses pre-existing code). Does the same problem occur in develop?

Hmm you're right, I can reproduce in develop - could've sworn this flow has been tested before. I'll look into it a bit more and open an issue for that - nothing to do in this PR. (For the record, this was using the Genymotion API16 emulator, which might make a difference.)

Related to permissions, I tried disabling permissions on my Pixel and then trying to use the picker. I get two permissions pop-ups, one for storage and one for camera.

This is also pre-existing code. We should open an issue for this, but we want to ping @kwonye since he's working on soft asks and this is related.

👍

aforcier · 2017-04-24T14:54:03Z

Looks good! Once #5721 is merged (and the rotation issue mentioned there also fixed) and this branch is updated I'll give it a final pass.

…ss-Android into feature/photo-picker-for-all-editors

aforcier · 2017-04-25T12:49:15Z

I think we're good to go @nbradbury - if you could merge develop in one more time I'll give it a final pass.

…ss-Android into feature/photo-picker-for-all-editors

nbradbury · 2017-04-25T13:52:01Z

If you could merge develop in one more time I'll give it a final pass.

Done!

aforcier · 2017-04-25T16:48:16Z

🎖 🎈 🍺

nbradbury added 7 commits April 13, 2017 14:15

Always enable new photo picker

65b4a82

Removed the seven item menu monster

cab1795

Uncomment enableMediaMode now that it's available

e3ff9d8

Removed unused strings

62e3798

Hide photo picker when either the HTML or Link icons are tapped

3cc67ad

Hide photo picker when option item is selected

8d89f25

Undo prefs changes

8c8e9fb

nbradbury added the Media label Apr 13, 2017

aforcier self-assigned this Apr 13, 2017

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

a0f7ac3

…ss-Android into feature/photo-picker-for-all-editors

aforcier reviewed Apr 14, 2017

View reviewed changes

nbradbury added 2 commits April 14, 2017 20:38

Added popup menus to choose between video/photo

8f2633e

Don't show menus when option to only show photos is enabled

3ee7c53

nbradbury added 2 commits April 14, 2017 21:13

Removed unused video options from PhotoPickerActivity

a4b283f

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

9fd2180

…ss-Android into feature/photo-picker-for-all-editors

nbradbury mentioned this pull request Apr 22, 2017

Feature/wpmedia multiselect #5721

Merged

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

cbf4f23

…ss-Android into feature/photo-picker-for-all-editors

Removed registerForContextMenu and openContextMenu from editor fragments

762ef92

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

e5d7563

…ss-Android into feature/photo-picker-for-all-editors

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

7d68b93

…ss-Android into feature/photo-picker-for-all-editors

aforcier merged commit 1f00ad1 into develop Apr 25, 2017

aforcier deleted the feature/photo-picker-for-all-editors branch April 25, 2017 16:48

hypest mentioned this pull request Feb 5, 2019

When user clicks on the Edit button he should see action sheet with media options wordpress-mobile/gutenberg-mobile#532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/photo picker for all editors #5644

Feature/photo picker for all editors #5644

nbradbury commented Apr 13, 2017

daniloercoli commented Apr 13, 2017

aforcier commented Apr 14, 2017

nbradbury commented Apr 14, 2017

aforcier commented Apr 14, 2017

aforcier Apr 14, 2017

aforcier commented Apr 14, 2017

aforcier commented Apr 14, 2017

nbradbury commented Apr 15, 2017

aforcier commented Apr 17, 2017 •

edited

Loading

aforcier commented Apr 17, 2017

aforcier commented Apr 18, 2017

aforcier commented Apr 18, 2017

sonderingheights commented Apr 21, 2017

nbradbury commented Apr 22, 2017 •

edited

Loading

aforcier commented Apr 24, 2017

aforcier commented Apr 24, 2017

aforcier commented Apr 25, 2017

nbradbury commented Apr 25, 2017

aforcier commented Apr 25, 2017

Feature/photo picker for all editors #5644

Feature/photo picker for all editors #5644

Conversation

nbradbury commented Apr 13, 2017

daniloercoli commented Apr 13, 2017

aforcier commented Apr 14, 2017

nbradbury commented Apr 14, 2017

aforcier commented Apr 14, 2017

aforcier Apr 14, 2017

Choose a reason for hiding this comment

aforcier commented Apr 14, 2017

aforcier commented Apr 14, 2017

nbradbury commented Apr 15, 2017

aforcier commented Apr 17, 2017 • edited Loading

aforcier commented Apr 17, 2017

aforcier commented Apr 18, 2017

aforcier commented Apr 18, 2017

sonderingheights commented Apr 21, 2017

nbradbury commented Apr 22, 2017 • edited Loading

aforcier commented Apr 24, 2017

aforcier commented Apr 24, 2017

aforcier commented Apr 25, 2017

nbradbury commented Apr 25, 2017

aforcier commented Apr 25, 2017

aforcier commented Apr 17, 2017 •

edited

Loading

nbradbury commented Apr 22, 2017 •

edited

Loading