971939 - File picker should use a normal intent chooser

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Wesley Johnston (:wesj)]

Right now our File Pickers roll their own custom intent chooser dialog. Since everything in them is a real intent we can use the native system for them.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This flips us to the native system. It turned out to be more complex than I thought because we actually show apps for multiple different intents in our chooser. The Android chooser expects to have a non-implicit intent for its main intent though. i.e. this has to

1.) Find a "base" intent for the chooser
2.) Find all the apps that that "base" intent will show in the chooser
3.) Find all the apps that "other" intents will show in the chooser
4.) Filter the "other" list so that we just have apps that 
the "other" intents would show, and none of the intents that the "base" intent would show.

Then Android fills in the "base" intents and we supply the extra ones.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 8375096 [details] [diff] [review]
Patch

Argh. I didn't realize I'm not getting back a result here. I don't think this is going to work well even if we did though. We use a different result handler based on the type of intent that's sent, and Android doesn't tell us what intent was actually selected. i.e. we won't know which handler to use...

There must be a good reason we're not getting a result, and maybe we can reduce things down to a single handler that works for Video/Audio/Camera/etc. Not impossible, just more work than I've done here....

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Reduce handlers

This grew a bit, but I like it. This removes the three different handlers we had and instead falls back to one that falls back through several different fallbacks. Normally I'd like keeping this separate, but these had diverged a lot over time. This way we can start with the simplest returns and only progressively move to the complex ones.

For instance, every file picker I've been able to test, the Video cursor always works and doesn't require us to copy the data. i.e. no more stray files unless we really really need 'em.

Here I try one and if it fails try the other. There aren't clear docs on how to tell if a Loader fails. The best way I could find was to try this manual query.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Rename Handler

We have two handlers now FilePickerResultHandler and FileResultHandler. I'm renaming one to just ResultHandler to simplify a little.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 3 - Native Intent choosers

Use native intent choosers. See the comments above about this code.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8375345 [details] [diff] [review]
Patch 1/3 - Reduce handlers

Empty patch? :-)

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Reduce handlers

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8375565 [details] [diff] [review]
Patch 1/3 - Reduce handlers

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

Less code, win :-)

::: mobile/android/base/FilePickerResultHandler.java
@@ +52,5 @@
>      }
>  
> +    private void sendResult(String res) {
> +        if (mFilePickerResult != null)
> +            mFilePickerResult.offer(res);

nit: use {}'s

@@ +54,5 @@
> +    private void sendResult(String res) {
> +        if (mFilePickerResult != null)
> +            mFilePickerResult.offer(res);
> +
> +        if (mHandler != null)

Ditto.

@@ +110,5 @@
> +
> +    public String generateImageName() {
> +        Time now = new Time();
> +        now.setToNow();
> +        mImageName = now.format("%Y-%m-%d %H.%M.%S") + ".jpg";

I find it slightly surprising that a method called generateSomething actually alters the object's state. Not a big deal I guess.

@@ +166,5 @@
> +        }
> +
> +        @Override
> +        public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
> +            if (cursor.moveToFirst()) {

nit: early return?

if (!cursor.moveToFirst()) {
    sendResult("");
    return;
}

...

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8375346 [details] [diff] [review]
Patch 2 - Rename Handler

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

Cool.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8375347 [details] [diff] [review]
Patch 3 - Native Intent choosers

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

Giving r- go get some answers.

::: mobile/android/base/ActivityHandlerHelper.java
@@ +104,3 @@
>      }
>  
> +    private Collection<Intent> getIntentsForFilePicker(final Context context,

Why Collection? Isn't the order of elements relevant?

@@ +111,5 @@
> +        Intent baseIntent;
> +        // A HashMap of Activities the base intent will show in the chooser. This is used
> +        // to filter activities from other intents so that we don't show duplicates.
> +        HashMap<String, Intent> baseIntents = new HashMap<String, Intent>();
> +        // A list of other activities to shwo in the picker (and the intents to launch them).

typo: show

@@ +199,5 @@
>          }
>  
> +        Iterator<Intent> it = intents.iterator();
> +        Intent base = it.next();
> +        it.remove();

Why do you need to remove the first item? If you need to access specific indexes in the data structured returned by getIntentsForFilePicker(), just use a List instead.

Comment 11

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Why Collection? Isn't the order of elements relevant?

Historical :) I originally used a HashMap to build a list of unique intents, and just returned HashMap.values(). That didn't work out, but I left the Collection bits in. Replaced it with a list here.

> Why do you need to remove the first item? If you need to access specific
> indexes in the data structured returned by getIntentsForFilePicker(), just
> use a List instead.

Can do. We need to pull the top item off this list.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 3 - Native Intent choosers

The Collection is just a historical artifact from an earlier version of the patch tried to just return HashMap.values(). Switched it to a List here.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8375735 [details] [diff] [review]
Patch 3 - Native Intent choosers

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

Nice.

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/b8d0cbe94d15
https://hg.mozilla.org/integration/fx-team/rev/6ac71c1149a5
https://hg.mozilla.org/integration/fx-team/rev/d3855e27cb32

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android bustage.
https://hg.mozilla.org/integration/fx-team/rev/844c2ceff9fc

https://tbpl.mozilla.org/php/getParsedLog.php?id=34690031&tree=Fx-Team

Comment 16

[Wesley Johnston (:wesj)]

Dumb mistake. Pushed an older version. This builds fine locally:

https://hg.mozilla.org/integration/fx-team/rev/94c8a67eee87
https://hg.mozilla.org/integration/fx-team/rev/cb83f363f469
https://hg.mozilla.org/integration/fx-team/rev/7b8b62250d46

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/94c8a67eee87
https://hg.mozilla.org/mozilla-central/rev/cb83f363f469
https://hg.mozilla.org/mozilla-central/rev/7b8b62250d46