Closed Bug 971939 Opened 6 years ago Closed 6 years ago

File picker should use a normal intent chooser

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: wesj, Unassigned)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8375096 - Flags: review?(bnicholson)
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....
Attachment #8375096 - Flags: review?(bnicholson)
Attached patch Patch 1/3 - Reduce handlers (obsolete) — Splinter Review
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.
Attachment #8375096 - Attachment is obsolete: true
Attachment #8375345 - Flags: review?(lucasr.at.mozilla)
We have two handlers now FilePickerResultHandler and FileResultHandler. I'm renaming one to just ResultHandler to simplify a little.
Attachment #8375346 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 3 - Native Intent choosers (obsolete) — Splinter Review
Use native intent choosers. See the comments above about this code.
Attachment #8375347 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8375345 [details] [diff] [review]
Patch 1/3 - Reduce handlers

Empty patch? :-)
Attachment #8375345 - Flags: review?(lucasr.at.mozilla)
Attachment #8375345 - Attachment is obsolete: true
Attachment #8375565 - Flags: review?(lucasr.at.mozilla)
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;
}

...
Attachment #8375565 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8375346 [details] [diff] [review]
Patch 2 - Rename Handler

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

Cool.
Attachment #8375346 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8375347 - Flags: review?(lucasr.at.mozilla) → review-
(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.
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.
Attachment #8375347 - Attachment is obsolete: true
Attachment #8375735 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8375735 [details] [diff] [review]
Patch 3 - Native Intent choosers

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

Nice.
Attachment #8375735 - Flags: review?(lucasr.at.mozilla) → review+
Depends on: 1001851
You need to log in before you can comment on or make changes to this bug.