920170 - Provide an intent chooser dialog type

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We have several bugs that need some sort of intent chooser (bug 780379, bug 851693, and an unfiled spin off from bug 899376 that will hide unrelevent apps from helper apps). We need some "simple" way of using them.

My initial instinct here is that we just create this as a new type of PromptInput, a gridview with a list of items/icons in it. Since we'd need to show app icons from outside FF, we'd need something like bug 914740, and the JS caller would then be responsible for taking action based on what was tapped.

Alternatively, we could write a separate JS-Java bridge that was more specific for this particular use case. That could even do something like take a url, and automatically find relevent items to show for you. But overall, I don't think it saves us much.

Relevent Android classes:
Basically sub classes AlertDialog
http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/com/android/internal/app/ResolverActivity.java

Inflates a gridview from
http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/res/res/layout/resolver_grid.xml and inserts items into it from:

And grid items from:
androidxref.com/4.2.2_r1/xref/frameworks/base/core/res/res/layout/resolve_list_item.xml

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Move prompt files to their own namespace

This moves the current prompt code into its own namespace. I want to add a new PromptInput type here, and it felt like maybe it would be better to isolate them.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Refactor Prompt.java

The Prompt.show() method in Prompt.java is way to big. This just pulls two chunks of it out into subfunctions so its a bit more readable.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Update addInputs to handle lists

And this refactors one of those methods a bit more. The idea here, is that the GridView should dismiss the dialog when its tapped. The GridView doesn't know anything about the dialog its in though. Instead, I want the dialog to listen to the GridView, and (when we're not in multiselect mode) to automatically close itself.

As I upload I remembered, this breaks our normal pattern with PromptInput's where they define their return value when the dialog closes. i.e. with this you'd get a return value (in js) of:

{ button: 3, icongrid: "" }

when you should get:

{ icongrid: "3" }

I'm going to modify this so that that works in a more expected way, but still going to upload things to save my place :)

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Add the gridview

And this is the basic IconGrid layout. Pretty straightforward. Adds a GridView with an ArrayAdapter. Most of the size/layout values are taken from Android.

Comment 5

[:Margaret Leibovic]

Comment on attachment 810764 [details] [diff] [review]
Move prompt files to their own namespace

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

r+ if it builds :)

Comment 6

[:Margaret Leibovic]

Comment on attachment 810768 [details] [diff] [review]
Refactor Prompt.java

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

This is looking good, but I have some questions/suggestions. Also, I want to come back to look at this again, because it still confuses me a bit :)

::: mobile/android/base/prompts/Prompt.java
@@ -129,5 @@
>          if (!TextUtils.isEmpty(aText)) {
>              builder.setMessage(aText);
>          }
>  
> -        int length = mInputs == null ? 0 : mInputs.length;

Oh man, this method definitely is confusing right now.

@@ +134,5 @@
>          if (aMenuList != null && aMenuList.length > 0) {
> +            addLists(builder, aMenuList, aMultipleSelection);
> +        } else {
> +            // If we failed to add any input elements, don't show the dialog
> +            if (!addInputs(builder)) {

So you can have either a list of items *or* inputs, but not both? I think we should document any assumptions/invariants we're baking in here.

@@ +205,5 @@
>  
>          finishDialog(ret);
>      }
>  
> +    private void addLists(AlertDialog.Builder builder, PromptListItem[] aMenuList, boolean aMultipleSelection) {

If there's just one list to add, I think this would be better named addList. Or addListItems? Also, now's a nice opportunity to add some documentation :)

@@ +208,5 @@
>  
> +    private void addLists(AlertDialog.Builder builder, PromptListItem[] aMenuList, boolean aMultipleSelection) {
> +        int resourceId = android.R.layout.simple_list_item_1;
> +
> +        // If we have an mSelected property, we're showing a list with checked rows of some sort

Thanks, this comment helps :)

@@ +220,5 @@
> +
> +        PromptListAdapter adapter = new PromptListAdapter(mContext, resourceId, aMenuList);
> +
> +        if (mSelected != null && mSelected.length > 0) {
> +            if (aMultipleSelection) {

These are the same checks you're doing up above. Why not set the resourceId in here instead? I guess it's probably because you need the resourceId to get the adapter... I feel like there must be something we can do here to make all this conditional logic cleaner.

@@ +281,5 @@
> +                try {
> +                    finishDialog(new JSONObject("{\"button\": -1}"));
> +                } catch(JSONException e) { }
> +                return false;
> +            }

Since these catch statements are the same, can we put the length checks and their corresponding logic inside the try statement, then just have one catch statement?

@@ +291,5 @@
>      public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
>          ThreadUtils.assertOnUiThread();
> +
> +        // If this is a multi select list, don't close the dialog
> +        if (parent instanceof AbsListView && ((AbsListView)parent).getChoiceMode() == ListView.CHOICE_MODE_MULTIPLE) {

Can we use an interface instead of this instanceof check?

@@ +304,5 @@
> +        JSONObject ret = new JSONObject();
> +        try {
> +            ret.put("button", position);
> +        } catch(Exception ex) { }
> +        finishDialog(ret);

Can you explain why these extra changes are necessary? It doesn't look like you changed when/where we add this item click listener, so I'm confused by why we need these changes.

Comment 7

[:Margaret Leibovic]

Comment on attachment 810772 [details] [diff] [review]
Update addInputs to handle lists

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

I still need to think about this more... it's hard to figure out how all this prompt code works, even though I've looked at it in the past. But here are some comments so far.

::: mobile/android/base/prompts/Prompt.java
@@ +133,5 @@
>  
>          if (aMenuList != null && aMenuList.length > 0) {
>              addLists(builder, aMenuList, aMultipleSelection);
>          } else {
> +            View input = getInputs();

How about calling this inputView? Or naming the method getInputView? It's not immediately obvious what that returns.

@@ +248,5 @@
>              mSelected = null;
>          }
>      }
>  
> +    private View getInputs() {

You should add some documentation here about what this method does.

@@ +254,5 @@
> +        if (length == 0) {
> +            return null;
> +        }
> +
> +        boolean hasGroup = false;

Add a comment about what this is.

@@ +262,5 @@
> +
> +        try {
> +            for (int i = 0; i < length; i++) {
> +                View input = mInputs[i].getView(mContext);
> +                if (input instanceof ViewGroup) {

Similarly to what I said in my last comment, should we consider using an interface here instead of this instanceof check?

It's not obvious what different kinds of PromptInputs exist, and which ones would be ViewGroups or AdapterViews, so I think interfaces could make this code easier to understand.

@@ +289,5 @@
> +            // If its the only view, we might as well just return it alone
> +            if (length == 1) {
> +                View v = linearLayout.getChildAt(0);
> +                linearLayout.removeView(v);
> +                return v;

I think it might be clearer to do this check up above, rather than going through the trouble of creating the LinearLayout, right? Although I guess you'd also need to check if that one input is a ViewGroup as well.

Comment 8

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/9b9b07d16466

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 2/4 - Refactor Prompt.java

This breaks things up a bit more to hopefully make the logic a little clearer. Comments too. I have a set of tests running locally (pushing to try before I upload them) that pass fine with this.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Refactor Prompt.java

Grrr. Sublime decided to paste a comment everywhere apparently. Fixed.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 2/4 - Refactor Prompt.java

Comment 12

[Wesley Johnston (:wesj)]

Attachment: IconGrid input type

This is the new input type, now with a selection state. The grid isn't auto-dismissed when you tap on something. Also wrote some pretty basic roboextender tests for this.

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9b9b07d16466

Comment 14

[:Margaret Leibovic]

Comment on attachment 813309 [details] [diff] [review]
Patch 2/4 - Refactor Prompt.java

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

I like this a lot, it just looks like the patch got generated in some in-between state while you were changing things, so I'd like to see a new version before giving an r+.

::: mobile/android/base/prompts/Prompt.java
@@ +116,5 @@
>      public void show(JSONObject message) {
>          processMessage(message);
>      }
>  
> +    public void show(String aTitle, String aText, PromptListItem[] listitems, boolean aMultipleSelection) {

Nit: aListItems, just to be consistent with the other parameters here.

@@ +117,5 @@
>          processMessage(message);
>      }
>  
> +    public void show(String aTitle, String aText, PromptListItem[] listitems, boolean aMultipleSelection) {
> +        Log.i(LOGTAG, "Show " + aTitle);

Remove logging when landing (or use Log.d)

@@ +137,5 @@
> +        if (listitems != null && listitems.length > 0) {
> +            addListItems(builder, listitems, aMultipleSelection);
> +        } else {
> +            // If we failed to add any requested input elements, don't show the dialog
> +            if (!addInputs(builder)) {

You could just combine this if statement with the else up above.

@@ +138,5 @@
> +            addListItems(builder, listitems, aMultipleSelection);
> +        } else {
> +            // If we failed to add any requested input elements, don't show the dialog
> +            if (!addInputs(builder)) {
> +                Log.i(LOGTAG, "No inputs?");

You log the exception down below in addInputs, so we don't need to log anything here.

@@ +209,5 @@
>  
>          finishDialog(ret);
>      }
>  
> +    /* Adds a set of list items to the prompt. This can be used for either context menu type dialogs, checked list's,

s/list's/lists

@@ +211,5 @@
>      }
>  
> +    /* Adds a set of list items to the prompt. This can be used for either context menu type dialogs, checked list's,
> +     * or multiple selection lists. If mSelected is set in the prompt before addListItems is called, the items will be
> +     * shown with "checkmarks" on their left side.

s/items/items in mSelected

(I was a bit confused and had to look to see that mSelected is actually an array)

@@ +214,5 @@
> +     * or multiple selection lists. If mSelected is set in the prompt before addListItems is called, the items will be
> +     * shown with "checkmarks" on their left side.
> +     *
> +     * @param builder
> +     *        the alert builder currently building this dialog.

Nit: Capitalize the first word here.

@@ +215,5 @@
> +     * shown with "checkmarks" on their left side.
> +     *
> +     * @param builder
> +     *        the alert builder currently building this dialog.
> +     * @param listitems

Nit: camelCase this parameter name?

@@ +238,5 @@
> +     * multi-choice lists by default so instead we insert our own custom list so that we can do fancy things
> +     * to the rows like disabling/indenting them.
> +     *
> +     * @param builder
> +     *        the alert builder currently building this dialog.

Same nit about consistent capitalization.

@@ +261,5 @@
> +     *        The items to add.
> +     */
> +    private boolean addInputs(AlertDialog.Builder builder) {
> +        int length = mInputs == null ? 0 : mInputs.length;
> +        try {

Huh? Did something go wrong generating this patch? It doesn't look like this would build.

@@ +293,5 @@
> +     *        the alert builder currently building this dialog.
> +     * @param listitems
> +     *        The items to add.
> +     */
> +    private void setupMenuList(AlertDialog.Builder builder, PromptListItem[] listitems) {

How about instead of using setupFooList for these method names, we use addFooList, to be consistent with addInputs?

@@ +308,5 @@
> +     *        returns true if the inputs were added successfully. They may fail
> +     *        if the requested input is incompatible with this Android version
> +     */
> +    private boolean addInputs(AlertDialog.Builder builder) {
> +        int length = mInputs == null ? 0 : mInputs.length;

I think it would be clearer to just return early here if mInputs is null or mInputs.length is 0, since we don't actually do anything below if length is 0.

@@ +326,5 @@
> +                view.addView(linearLayout);
> +                builder.setView(applyInputStyle(view));
> +            }
> +        } catch(Exception ex) {
> +            Log.i(LOGTAG, "Error showing prompt inputs", ex);

Let's use Log.e instead.

Comment 15

[:Margaret Leibovic]

Comment on attachment 813458 [details] [diff] [review]
IconGrid input type

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

I didn't look through the test files yet, but this is looking good so far. Just setting f+ for now, since you'll need to make a new version of the patch anyway.

::: mobile/android/base/prompts/IconGridInput.java
@@ +57,5 @@
> +        }
> +
> +        if (mPadding < 0) {
> +            mPadding = context.getResources().getDimensionPixelSize(R.dimen.icongrid_padding);
> +        }

Instead of setting the columnWidth and padding programatically, would it be more efficient to just define a GridView in an XML file and inflate that?

@@ +99,5 @@
> +    public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +        mSelected = position;
> +    }
> +
> +    public String getValue() {

Add the @Override tag for methods you're overriding.

@@ +105,5 @@
> +        // int pos = v.getSelectedItemPosition();
> +        return Integer.toString(mSelected);
> +    }
> +
> +    public boolean getScrollable() {

Add an @Override here.

@@ +114,5 @@
> +        public IconGridAdapter(Context context, int resource, List<IconGridItem> items) {
> +            super(context, resource, items);
> +        }
> +
> +        public View getView(int position, View convert, ViewGroup parent) {

Add an @Override here.

@@ +123,5 @@
> +            bindView(convert, context, position);
> +            return convert;
> +        }
> +
> +        private View newView(Context context, int position, ViewGroup parent) {

I'm tempted to say these private methods are unnecessary, but it does make the logic more self-documenting.

@@ +146,5 @@
> +            ViewGroup.LayoutParams lp = icon.getLayoutParams();
> +            lp.width = lp.height = mIconSize;
> +        }
> +    }
> +    

Whitespace.

::: mobile/android/base/prompts/Prompt.java
@@ -262,5 @@
> -     */
> -    private boolean addInputs(AlertDialog.Builder builder) {
> -        int length = mInputs == null ? 0 : mInputs.length;
> -        try {
> -

Heh, here's where you fixed that problem from the last patch!

@@ +292,5 @@
>       *
>       * @param builder
>       *        the alert builder currently building this dialog.
> +     * @param listitems
> +     *        The items to add.

I don't see this parameter in the method below...

@@ +298,5 @@
>      private boolean addInputs(AlertDialog.Builder builder) {
>          int length = mInputs == null ? 0 : mInputs.length;
> +        if (length == 0) {
> +            return true;
> +        }

Haha, you addressed one of my review comments from the last patch here!

@@ +307,3 @@
>              if (length == 1) {
> +                root = mInputs[0].getView(mContext);
> +                scrollable |= mInputs[0].getScrollable();

Can you add some comments about what's going on with this scrollable logic?

::: mobile/android/base/prompts/PromptInput.java
@@ +376,4 @@
>          return "";
>      }
> +
> +    public boolean getScrollable() {

Can this just be package access? Maybe it would be a good follow-up to tighten the access levels on the methods in this file.

::: mobile/android/base/resources/layout/icon_grid_item.xml
@@ +14,5 @@
> +** distributed under the License is distributed on an "AS IS" BASIS,
> +** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +** See the License for the specific language governing permissions and
> +** limitations under the License.
> +*/

Can we get rid of these asterisks (and the Java-style comment syntax) in here?

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Sorry about that. My hatred of Sublime is growing.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This is the input part (and the test, although I'd happily put this test in its own patch in order to land this).

Comment 18

[:Margaret Leibovic]

Comment on attachment 816087 [details] [diff] [review]
Patch 1/2

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

Very nice. Two small comments to address before landing (both due to my review comment requests).

::: mobile/android/base/prompts/Prompt.java
@@ +294,5 @@
> +     */
> +    private boolean addInputs(AlertDialog.Builder builder) {
> +        int length = mInputs == null ? 0 : mInputs.length;
> +        if (length == 0) {
> +            return;

I believe you mean to return true here.

@@ +380,5 @@
>                  mInputs[i] = PromptInput.getInput(inputs.getJSONObject(i));
>              } catch(Exception ex) { }
>          }
>  
> +        PromptListItem[] menuitems = getListItemArray(geckoObject, "listItems");

Shouldn't this still be "listitems"?

Comment 19

[:Margaret Leibovic]

Comment on attachment 816089 [details] [diff] [review]
Patch 2/2

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

Looking good, I just have some comments about making the test logic easier to follow. Adding some comments in there might also help.

This test is pretty neat. Are there docs somewhere on writing roboextender tests like this? If not, maybe you can add a blurb to our robocop wiki page.

::: mobile/android/base/prompts/IconGridInput.java
@@ +49,5 @@
> +    private int mSelected = -1;           // Currenct selection
> +
> +    public IconGridInput(JSONObject obj) {
> +        super(obj);
> +    }

Do you need to declare a constructor here if you're just calling super?

@@ +115,5 @@
> +        public IconGridAdapter(Context context, int resource, List<IconGridItem> items) {
> +            super(context, resource, items);
> +        }
> +
> +        public View getView(int position, View convert, ViewGroup parent) {

You didn't address my comment about an @Override tag here.

::: mobile/android/base/prompts/Prompt.java
@@ +288,5 @@
>       *
>       * @param builder
>       *        the alert builder currently building this dialog.
> +     * @param listitems
> +     *        The items to add.

Looks like this @param comment doesn't belong here.

::: mobile/android/base/tests/roboextender/robocop_prompt_gridinput.html
@@ +4,5 @@
> +    <meta name="viewport" content="initial-scale=1.0"/>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> +    <script type="application/javascript">
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Doesn't look like you're using these imports.

@@ +9,5 @@
> +Components.utils.import("resource://gre/modules/Prompt.jsm");
> +
> +function start() {
> +  var test = location.hash.substring(1);
> +  window[test]();

This is clever, but not very intuitive... do you have plans for extending this test in the future and that's why you did this?

::: mobile/android/base/tests/testPromptGridInput.java.in
@@ +23,5 @@
> +    protected int index = 1;
> +    public void testPromptGridInput() {
> +        blockForGeckoReady();
> +
> +        test(1);

Similarly to my comment above, passing an integer like this isn't very intuitive. I would prefer to see more descriptive names used for different test cases, rather than integers.

@@ +40,5 @@
> +
> +        mSolo.clickOnText("Icon 11");
> +        mSolo.clickOnText("OK");
> +
> +        mAsserter.ok(waitForText("PASS"), "test passed", "PASS");

Clever.

Comment 20

[:Margaret Leibovic]

Also I just tried building with these two patches and I got a build error:
24:48.63 make[5]: *** No rule to make target `/Users/leibovic/code/mozilla-central/mobile/android/base/resources/layout/icon_grid.xml', needed by `res/values/.mkdir.done'.  Stop.

Am I missing something?

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Yes. Yes, I did leave that file out.

Comment 22

[:Margaret Leibovic]

Comment on attachment 816143 [details] [diff] [review]
Patch 2/2

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

r+ with comments addressed.

::: mobile/android/base/prompts/IconGridInput.java
@@ +42,5 @@
> +    public static final String LOGTAG = "GeckoIconGridInput";
> +
> +    private ArrayAdapter<IconGridItem> mAdapter; // An adapter holding a list of items to show in the grid
> +
> +    private static int mColumnWidth = -1;  // The maximum number of columns to show

This looks like an incorrect comment.

@@ +45,5 @@
> +
> +    private static int mColumnWidth = -1;  // The maximum number of columns to show
> +    private static int mMaxColumns = -1;  // The maximum number of columns to show
> +    private static int mIconSize = -1;    // Size of icons in the grid
> +    private int mSelected = -1;           // Currenct selection

Typo: Current selection

@@ +49,5 @@
> +    private int mSelected = -1;           // Currenct selection
> +
> +    public IconGridInput(JSONObject obj) {
> +        super(obj);
> +    }

I think you can get rid of this constructor.

@@ +144,5 @@
> +            ViewGroup.LayoutParams lp = icon.getLayoutParams();
> +            lp.width = lp.height = mIconSize;
> +        }
> +    }
> +    

Whitespace.

@@ +154,5 @@
> +        Drawable icon;
> +
> +        public IconGridItem(final Context context, final JSONObject obj) {
> +            label = obj.optString("name");
> +            iconUrl = obj.optString("iconUri");

It annoys me that we're so inconsistent with the way we use url/uri in variable names. Also, ou don't actually use iconUrl for anything outside of this constructor, so it could just be a local variable.

::: mobile/android/base/prompts/Prompt.java
@@ -289,5 @@
>       * @param builder
>       *        the alert builder currently building this dialog.
> -     * @return
> -     *        returns true if the inputs were added successfully. They may fail
> -     *        if the requested input is incompatible with this Android version

You shouldn't get rid of this documentation, right?

::: mobile/android/base/prompts/PromptInput.java
@@ +41,5 @@
>  import java.util.concurrent.ConcurrentLinkedQueue;
>  import java.util.concurrent.TimeUnit;
>  
>  public class PromptInput {
> +    protected final JSONObject mJSONInput;

It doesn't look like mJSONInput is actually used for anything right now, and in this patch you're just using it to get the items, so can we just store mItems similarly to how we store label/type/etc, and not store mJSONInput at all?

Comment 23

[Wesley Johnston (:wesj)]

Landing this patch series in chunks to avoid mass backouts. Refactor piece:

https://hg.mozilla.org/integration/fx-team/rev/c5cc4233220f

Comment 24

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/1960e499d49f

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c5cc4233220f
https://hg.mozilla.org/mozilla-central/rev/1960e499d49f

Comment 26

[Aaron Train [:aaronmt]]

Examples to test with here?

Comment 27

[Wesley Johnston (:wesj)]

No visible uses yet. Need one more patch in bug 780379 before it lands, then clicking http://qthttp.apple.com.edgesuite.net/1010qwoeiuryfg/0150_vod.m3u8 will show this (assuming you have apps that are registered for m3u8 files).