Last Comment Bug 730289 - Filepicker should allow picking or capturing media instead of having a specific button for capture
: Filepicker should allow picking or capturing media instead of having a specif...
Status: RESOLVED FIXED
[strings]
: late-l10n
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla14
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 733749 738288 739824 734382 737394
Blocks: 748349 708175 727866 853475
  Show dependency treegraph
 
Reported: 2012-02-24 06:55 PST by Mounir Lamouri (:mounir)
Modified: 2013-03-21 08:49 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Patch v1 (38.95 KB, patch)
2012-03-02 09:13 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (38.95 KB, patch)
2012-03-02 09:15 PST, Mounir Lamouri (:mounir)
wjohnston2000: review-
Details | Diff | Splinter Review
Patch v2 (46.10 KB, patch)
2012-03-07 07:10 PST, Mounir Lamouri (:mounir)
doug.turner: review-
Details | Diff | Splinter Review
Image picker (59.48 KB, image/png)
2012-03-08 06:42 PST, Mounir Lamouri (:mounir)
no flags Details
Audio picker (41.97 KB, image/png)
2012-03-08 06:43 PST, Mounir Lamouri (:mounir)
no flags Details
Video picker (60.94 KB, image/png)
2012-03-08 06:43 PST, Mounir Lamouri (:mounir)
no flags Details
Regular file picker (68.59 KB, image/png)
2012-03-08 06:43 PST, Mounir Lamouri (:mounir)
no flags Details
Native context menu for intent (the one we are trying to immitate) (58.24 KB, image/png)
2012-03-08 06:48 PST, Mounir Lamouri (:mounir)
no flags Details
Patch v3 (46.26 KB, patch)
2012-03-14 10:09 PDT, Mounir Lamouri (:mounir)
doug.turner: review+
wjohnston2000: review+
Details | Diff | Splinter Review
Patch v4 (46.26 KB, patch)
2012-03-14 11:30 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-02-24 06:55:23 PST
The current UI we have to take a picture is terrible. Instead of having one button to select a file and one to capture one, we should put capturing inside the same menu. That bug would also allow accept="video/*" and accept="audio/*" to work the same way.

I have a WIP patch that does that. I'm now trying to improve the UI (with icons!) and be more integrated with the current Native Fennec code base. Should be done soon.
Comment 1 Mounir Lamouri (:mounir) 2012-03-02 09:13:36 PST
Created attachment 602387 [details] [diff] [review]
Patch v1
Comment 2 Mounir Lamouri (:mounir) 2012-03-02 09:15:31 PST
Created attachment 602388 [details] [diff] [review]
Patch v1.1

I forgot a TODO in that previous patch.

@wesj, could you review the changes in PromptService.java ?
Comment 3 Mounir Lamouri (:mounir) 2012-03-02 09:16:09 PST
Comment on attachment 602388 [details] [diff] [review]
Patch v1.1

Doug, could you review the patch in general without focusing on the changes in PromptService.java?
Comment 4 Wesley Johnston (:wesj) 2012-03-05 10:21:07 PST
Comment on attachment 602388 [details] [diff] [review]
Patch v1.1

Review of attachment 602388 [details] [diff] [review]:
-----------------------------------------------------------------

I like this patch (and have never liked the current UI to get the camera picker up)! Just had some questions mostly about the way things are being done and a few nits. I mostly looked over the promptservice bits (although I glanced around a bit).

::: mobile/android/base/GeckoApp.java
@@ +2440,5 @@
> +    }
> +
> +    private String getFilePickerTitle(String aMimeType) {
> +        if (aMimeType.equals("audio/*")) {
> +            return "Pick or record a sound";

These strings need to be localizable.

@@ +2463,2 @@
>          try {
> +            while (null == (promptServiceResult = GeckoAppShell.sPromptQueue.poll(1, TimeUnit.MILLISECONDS))) {

I'd be happy moving this little bit to a static helper method somewhere. I think we call it here and in GeckoAppShell now? String PromptService.waitForReturn(); or something like that?

@@ +2465,4 @@
>                  GeckoAppShell.processNextNativeEvent();
>              }
>          } catch (InterruptedException e) {
> +            Log.i(LOGTAG, "showing prompt failed: ",  e);

I think maybe we should return early here...

::: mobile/android/base/PromptService.java
@@ +406,5 @@
>          public boolean inGroup = false;
>          public boolean disabled = false;
>          public int id = 0;
> +        // TODO: how to get that from JS?
> +        public Drawable icon = null;

From JS we will likely have to pass icons as data strings. See what we do for shortcuts:

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoAppShell.java#662

(although we should move that code to BitmapUtils if we're going to share it). That will give us a bitmap, not a drawable, so there's a couple options for what we store here, but whoever fixes it can pick the one they like best I think.

I don't think you should worry about that for this patch though. Can you file a follow up for it?

@@ +407,5 @@
>          public boolean disabled = false;
>          public int id = 0;
> +        // TODO: how to get that from JS?
> +        public Drawable icon = null;
> +        public Intent intent = null;

I don't really like having something this specific to your use case in here. I wish we returned something better than a String to you (we could do that, and serialize it in GeckoAppShell I think...), but even with the string you should be able to pull out the selected item and build the Intent when the dialog returns?

@@ +481,5 @@
> +                    Resources res = GeckoApp.mAppContext.getResources();
> +
> +                    // Set padding inside the item.
> +                    int paddingLR = (int)TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP,
> +                                                                   10, res.getDisplayMetrics());

Move these paddings to constants. We also apply a padding to this same textview above if the element is in an opt group (bug 715925), which led to some still not fixed problems (bug 724570). I think we can be tricky here and make sure these icons are indented in groups as well.

If you're feeling ambitious, you can check and see if there's a fix for that other problem at the same time. Its probably due to me setting the right side padding to zero. I wonder if we can read out the default paddings before we set our own.
Comment 5 Mounir Lamouri (:mounir) 2012-03-07 06:18:23 PST
(In reply to Wesley Johnston (:wesj) from comment #4)
> ::: mobile/android/base/GeckoApp.java
> @@ +2440,5 @@
> > +    }
> > +
> > +    private String getFilePickerTitle(String aMimeType) {
> > +        if (aMimeType.equals("audio/*")) {
> > +            return "Pick or record a sound";
> 
> These strings need to be localizable.

Can I do that from Java code?

> I don't think you should worry about that for this patch though. Can you
> file a follow up for it?

I will.

> @@ +407,5 @@
> >          public boolean disabled = false;
> >          public int id = 0;
> > +        // TODO: how to get that from JS?
> > +        public Drawable icon = null;
> > +        public Intent intent = null;
> 
> I don't really like having something this specific to your use case in here.
> I wish we returned something better than a String to you (we could do that,
> and serialize it in GeckoAppShell I think...), but even with the string you
> should be able to pull out the selected item and build the Intent when the
> dialog returns?

So, it's not really that much related to my use case. MenuListItem have an intent member too.
I could remove that if you think it's needed though.

> I wonder if we can read out the default paddings
> before we set our own.

I guess you can always call getPadding{Left,Right,Top,Bottom}.
Comment 6 Mounir Lamouri (:mounir) 2012-03-07 07:10:54 PST
Created attachment 603712 [details] [diff] [review]
Patch v2

New patch with all the comments fixed (including l10n and .intent in PromptListItem).
Comment 7 Mounir Lamouri (:mounir) 2012-03-07 07:13:11 PST
Comment on attachment 603712 [details] [diff] [review]
Patch v2

By the way, I'm adding Doug to review the stuff other than PromptService but if Wesley can/wants to review the entire patch, I will be more than happy to have only one reviewer for this patch.
Comment 8 Mounir Lamouri (:mounir) 2012-03-08 02:43:18 PST
I think I should ask for a ui-review for that patch (for the general new UI and the texts). Who should I ask this review to?
Comment 9 Doug Turner (:dougt) 2012-03-08 03:15:08 PST
mounir, post a screen shot and ask madhava to ui-review.
Comment 10 Mounir Lamouri (:mounir) 2012-03-08 06:42:27 PST
Created attachment 604039 [details]
Image picker
Comment 11 Mounir Lamouri (:mounir) 2012-03-08 06:43:00 PST
Created attachment 604041 [details]
Audio picker
Comment 12 Mounir Lamouri (:mounir) 2012-03-08 06:43:29 PST
Created attachment 604042 [details]
Video picker
Comment 13 Mounir Lamouri (:mounir) 2012-03-08 06:43:55 PST
Created attachment 604043 [details]
Regular file picker
Comment 14 Mounir Lamouri (:mounir) 2012-03-08 06:46:55 PST
Comment on attachment 603712 [details] [diff] [review]
Patch v2

Madhava, could you review the ui of the new menus. The appearance is similar to the native context menus but I did some choices like:
- showing the camera options always on top;
- the titles (I'm especially worried for the regular file picker title that is a bit too long, we could use the same one Android is using "Choose File").

In addition of that, we are loosing the "Capture" button but I guess no one is going to complain for that...
Comment 15 Mounir Lamouri (:mounir) 2012-03-08 06:48:52 PST
Created attachment 604044 [details]
Native context menu for intent (the one we are trying to immitate)
Comment 16 Brad Lassey [:blassey] (use needinfo?) 2012-03-08 07:00:12 PST
two questions. First, why is the activity chooser showing when there is only one choice? Second, can we change the button label to something other than browse when we're going to capture audio or video?
Comment 17 Mounir Lamouri (:mounir) 2012-03-08 07:06:42 PST
(In reply to Brad Lassey [:blassey] from comment #16)
> two questions. First, why is the activity chooser showing when there is only
> one choice?

We could by-pass the UI showing in that case, indeed. I would prefer to handle that in a follow-up because it's going to adds some special casing code that might  increase the review complexity. If you want, I could do that in that bug and just adds another patch.

> Second, can we change the button label to something other than
> browse when we're going to capture audio or video?

Jonas wants to re-do the entire file picker in-page UI on mobile. That should be part of this project I believe. We need UI/UX thoughts on that though.
Comment 18 Brad Lassey [:blassey] (use needinfo?) 2012-03-08 07:15:34 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #17)
> (In reply to Brad Lassey [:blassey] from comment #16)
> > two questions. First, why is the activity chooser showing when there is only
> > one choice?
> 
> We could by-pass the UI showing in that case, indeed. I would prefer to
> handle that in a follow-up because it's going to adds some special casing
> code that might  increase the review complexity. If you want, I could do
> that in that bug and just adds another patch.
doing that in a follow up is probably best

 
> > Second, can we change the button label to something other than
> > browse when we're going to capture audio or video?
> 
> Jonas wants to re-do the entire file picker in-page UI on mobile. That
> should be part of this project I believe. We need UI/UX thoughts on that
> though.
sounds good
Comment 19 Madhava Enros [:madhava] 2012-03-14 09:15:51 PDT
(In reply to Brad Lassey [:blassey] from comment #18)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #17)
> > (In reply to Brad Lassey [:blassey] from comment #16)
> > > two questions. First, why is the activity chooser showing when there is only
> > > one choice?
> > 
> > We could by-pass the UI showing in that case, indeed. I would prefer to
> > handle that in a follow-up because it's going to adds some special casing
> > code that might  increase the review complexity. If you want, I could do
> > that in that bug and just adds another patch.
> doing that in a follow up is probably best

In practice, there should almost never be just one choice, though, shouldn't there? There's always a gallery and the camera...

 
> > > Second, can we change the button label to something other than
> > > browse when we're going to capture audio or video?
> > 
> > Jonas wants to re-do the entire file picker in-page UI on mobile. That
> > should be part of this project I believe. We need UI/UX thoughts on that
> > though.
> sounds good

Yes - we can do this in a follow-up. Please add the "ui-wanted" keyword to that bug.
Comment 20 Madhava Enros [:madhava] 2012-03-14 09:20:02 PDT
Also - let's use "Choose" rather than "Pick" to be more Android-like and a little less English colloquial.

In the general file case, how about just "Choose File" rather than "Choose and action to select a file." I think it will still be clear, and it's more consistent (and shorter).
Comment 21 Madhava Enros [:madhava] 2012-03-14 09:21:13 PDT
With those changes, I think I'd UI-R+
Comment 22 Doug Turner (:dougt) 2012-03-14 09:47:19 PDT
Comment on attachment 603712 [details] [diff] [review]
Patch v2

Review of attachment 603712 [details] [diff] [review]:
-----------------------------------------------------------------

looks pretty good.  minusing for now.  Most things are nits.  Fix those up, and get madhava to ux+.

::: mobile/android/base/GeckoApp.java
@@ +2348,5 @@
> +    private class FilePickerPromptRunnable implements Runnable {
> +        public FilePickerPromptRunnable(String aTitle, PromptService.PromptListItem[] aItems) {
> +            super();
> +
> +            mTitle = aTitle;

No space needed above mTitle.

@@ +2364,5 @@
> +    private int addIntentActivitiesToList(Intent intent, ArrayList<PromptService.PromptListItem> items, ArrayList<Intent> aIntents) {
> +        PackageManager pm = mAppContext.getPackageManager();
> +        final List<ResolveInfo> lri =
> +            pm.queryIntentActivityOptions(GeckoApp.mAppContext.getComponentName(), null, intent, 0);
> +        final int N = lri != null ? lri.size() : 0;

return early if lri is null?

@@ +2366,5 @@
> +        final List<ResolveInfo> lri =
> +            pm.queryIntentActivityOptions(GeckoApp.mAppContext.getComponentName(), null, intent, 0);
> +        final int N = lri != null ? lri.size() : 0;
> +
> +        for (int i=0; i<N; i++) {

you are free to use longer variable names. ;)

@@ +2373,5 @@
> +            rintent.setComponent(new ComponentName(
> +                    ri.activityInfo.applicationInfo.packageName,
> +                    ri.activityInfo.name));
> +
> +            PromptService.PromptListItem item = new PromptService.PromptListItem(ri.loadLabel(pm) + "");

ri.loadLabel(pm).toString()

::: mobile/android/base/GeckoAppShell.java
@@ +1688,5 @@
>                  });
>  
>                  String promptServiceResult = "";
>                  try {
> +                    promptServiceResult = PromptService.waitForReturn();

okay... hiding this code in PromptService.  Can you make waitForReturn() not throw so you can remove this try block?

::: mobile/android/components/CapturePicker.js
@@ -1,3 @@
> -/* -*- Mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil; -*- */
> -/* ***** BEGIN LICENSE BLOCK *****
> - * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Go here - http://www.mozilla.org/MPL/headers/

Much better license block headers.

@@ -45,5 @@
> -Cu.import("resource://gre/modules/Services.jsm");
> -
> -function CapturePicker() {
> -
> -}

no need for ws between the {}

::: widget/android/AndroidBridge.cpp
@@ +719,5 @@
>          return;
>  
>      AutoLocalJNIFrame jniFrame(env); 
> +    jstring jstrFilers = env->NewString(nsPromiseFlatString(aExtensions).get(),
> +                                            aExtensions.Length());

align aExtensions with nsPromiseFlatString

@@ +740,5 @@
> +    jstring jstrFilers = env->NewString(nsPromiseFlatString(aMimeType).get(),
> +                                            aMimeType.Length());
> +    jstring jstr =  static_cast<jstring>(env->CallStaticObjectMethod(
> +                                             mGeckoAppShellClass,
> +                                             jShowFilePickerForMimeType, jstrFilers));

Same thing.  the ws looks off.
Comment 23 Mounir Lamouri (:mounir) 2012-03-14 10:09:01 PDT
Created attachment 605821 [details] [diff] [review]
Patch v3

Based on comment 21, I guess this patch has ui-r=madhava.

This patch should fix Doug's comments.
Comment 24 Wesley Johnston (:wesj) 2012-03-14 10:33:38 PDT
Comment on attachment 605821 [details] [diff] [review]
Patch v3

Review of attachment 605821 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +2407,5 @@
> +        ArrayList<PromptService.PromptListItem> items = new ArrayList<PromptService.PromptListItem>();
> +
> +        if (aMimeType.equals("audio/*")) {
> +            if (AddFilePickingActivities(items, "audio/*", aIntents) <= 0) {
> +              AddFilePickingActivities(items, "*/*", aIntents);

4 space indents (I think?)

@@ +2469,5 @@
> +        String promptServiceResult = "";
> +        try {
> +            promptServiceResult = PromptService.waitForReturn();
> +        } catch (InterruptedException e) {
> +            Log.i(LOGTAG, "showing prompt failed: ",  e);

Log.e

@@ +2506,4 @@
>                  GeckoAppShell.processNextNativeEvent();
>              }
>          } catch (InterruptedException e) {
> +            Log.i(LOGTAG, "showing file picker failed: ",  e);

Log.e

::: mobile/android/base/PromptService.java
@@ +82,5 @@
> +
> +    private final static int GROUP_PADDING_SIZE = 32; // in dip units
> +    private static int mGroupPaddingSize = 0; // calculated from GROUP_PADDING_SIZE. In pixel units
> +
> +    private final static int LEFT_RIGHT_TEXT_WITH_ICON_PADDING = 10;

// in dip units

@@ +520,5 @@
> +
> +                    // Set padding inside the item.
> +                    t1.setPadding(item. inGroup ? mLeftRightTextWithIconPadding + mGroupPaddingSize : mLeftRightTextWithIconPadding,
> +                                  mTopBottomTextWithIconPadding,
> +                                  mLeftRightTextWithIconPadding, mTopBottomTextWithIconPadding);

I think I would rather this section looked more like:

int left = TypedValue.applyDimension(DIPS, t1.getPaddingLeft(), resources);
...
if (inGroup)
  left += mGroupPaddingSize;
...
if (item.icon)
  left += mLeftRightTextWithIconPading // do we need this or is the default padding enough?
...
t1.setPadding(left, top, right, bottom);

I'm being lazy, because doing something like that should fix bug 724570 for me as well. If that proves to be more difficult than that, feel free to ping back here though, and I'll tackle it in the other bug.
Comment 25 Wesley Johnston (:wesj) 2012-03-14 11:29:35 PDT
Comment on attachment 605821 [details] [diff] [review]
Patch v3

Review of attachment 605821 [details] [diff] [review]:
-----------------------------------------------------------------

From IRC sounds like this is going to be a bit more work (and testing). We'll work out the padding in the other bug.
Comment 26 Mounir Lamouri (:mounir) 2012-03-14 11:30:38 PDT
Created attachment 605861 [details] [diff] [review]
Patch v4

All comments being applied except the last one that should happen in a follow-up.

Got wesj r+ over IRC.
Comment 27 Mounir Lamouri (:mounir) 2012-03-14 11:53:17 PDT
(In reply to Madhava Enros [:madhava] from comment #19)
> In practice, there should almost never be just one choice, though, shouldn't
> there? There's always a gallery and the camera...

For accept="image/*" yes, but for accept="audio/*" some phones will have only one choice out of the box. And, if the phone doesn't have a camera, chances are there will be one choice most of the time.
Comment 28 Phil Ringnalda (:philor) 2012-03-17 17:12:51 PDT
https://hg.mozilla.org/mozilla-central/rev/b94319584a7a
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-17 19:00:40 PDT
I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame:

https://hg.mozilla.org/mozilla-central/rev/e94edfdb1f5b

Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound
--------------------------------------------------------------
    Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6
    New     : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4
    Change  : +547.522 (20.6% / z=6.232)
    Graph   : http://mzl.la/zD3EWy
Comment 30 Marco Bonardo [::mak] 2012-03-20 03:54:58 PDT
https://hg.mozilla.org/mozilla-central/rev/66cf09c44270
Comment 31 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-24 09:00:09 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b966ab71091

Note You need to log in before you can comment on or make changes to this bug.