Filepicker should allow picking or capturing media instead of having a specific button for capture

RESOLVED FIXED in Firefox 13

Status

()

Core
Widget: Android
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug, {late-l10n})

Trunk
mozilla14
All
Android
late-l10n
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

(Whiteboard: [strings])

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 727866
(Assignee)

Updated

6 years ago
Blocks: 708175
Blocks: 692955
(Assignee)

Updated

6 years ago
OS: All → Android
(Assignee)

Updated

6 years ago
No longer blocks: 692955
(Assignee)

Comment 1

6 years ago
Created attachment 602387 [details] [diff] [review]
Patch v1
Attachment #602387 - Flags: review?(wjohnston)
(Assignee)

Updated

6 years ago
Attachment #602387 - Flags: review?(doug.turner)
(Assignee)

Comment 2

6 years ago
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 ?
Attachment #602387 - Attachment is obsolete: true
Attachment #602388 - Flags: review?(wjohnston)
Attachment #602387 - Flags: review?(wjohnston)
Attachment #602387 - Flags: review?(doug.turner)
(Assignee)

Comment 3

6 years ago
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?
Attachment #602388 - Flags: review?(doug.turner)
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.
Attachment #602388 - Flags: review?(wjohnston) → review-
(Assignee)

Comment 5

6 years ago
(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}.
(Assignee)

Updated

6 years ago
Depends on: 733749
(Assignee)

Comment 6

6 years ago
Created attachment 603712 [details] [diff] [review]
Patch v2

New patch with all the comments fixed (including l10n and .intent in PromptListItem).
Attachment #602388 - Attachment is obsolete: true
Attachment #603712 - Flags: review?(wjohnston)
Attachment #602388 - Flags: review?(doug.turner)
(Assignee)

Comment 7

6 years ago
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.
Attachment #603712 - Flags: review?(doug.turner)
(Assignee)

Comment 8

6 years ago
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

6 years ago
mounir, post a screen shot and ask madhava to ui-review.
(Assignee)

Comment 10

6 years ago
Created attachment 604039 [details]
Image picker
(Assignee)

Comment 11

6 years ago
Created attachment 604041 [details]
Audio picker
(Assignee)

Comment 12

6 years ago
Created attachment 604042 [details]
Video picker
(Assignee)

Comment 13

6 years ago
Created attachment 604043 [details]
Regular file picker
(Assignee)

Comment 14

6 years ago
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...
Attachment #603712 - Flags: ui-review?(madhava)
(Assignee)

Comment 15

6 years ago
Created attachment 604044 [details]
Native context menu for intent (the one we are trying to immitate)
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?
(Assignee)

Comment 17

6 years ago
(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.
(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
(Assignee)

Updated

5 years ago
Depends on: 734382
(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.
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).
With those changes, I think I'd UI-R+
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.
Attachment #603712 - Flags: review?(doug.turner) → review-
(Assignee)

Comment 23

5 years ago
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.
Attachment #603712 - Attachment is obsolete: true
Attachment #605821 - Flags: review?(doug.turner)
Attachment #603712 - Flags: ui-review?(madhava)
Attachment #603712 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #605821 - Flags: review?(wjohnston)

Updated

5 years ago
Attachment #605821 - Flags: review?(doug.turner) → review+
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.
Attachment #605821 - Flags: review?(wjohnston) → review-
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.
Attachment #605821 - Flags: review- → review+
(Assignee)

Comment 26

5 years ago
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.
Attachment #605821 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/b94319584a7a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/66cf09c44270
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 737394
(Assignee)

Updated

5 years ago
Depends on: 738288
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b966ab71091
status-firefox13: --- → fixed
status-firefox14: --- → fixed

Updated

5 years ago
Keywords: late-l10n
Whiteboard: [strings]
(Assignee)

Updated

5 years ago
Depends on: 739824
(Assignee)

Updated

5 years ago
Blocks: 748349
Blocks: 853475
You need to log in before you can comment on or make changes to this bug.