Closed
Bug 971939
Opened 9 years ago
Closed 9 years ago
File picker should use a normal intent chooser
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: wesj, Unassigned)
References
Details
Attachments
(3 files, 3 obsolete files)
4.60 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
27.41 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
12.27 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
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)
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
Use native intent choosers. See the comments above about this code.
Attachment #8375347 -
Flags: review?(lucasr.at.mozilla)
Comment 6•9 years ago
|
||
Comment on attachment 8375345 [details] [diff] [review] Patch 1/3 - Reduce handlers Empty patch? :-)
Attachment #8375345 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 7•9 years ago
|
||
Attachment #8375345 -
Attachment is obsolete: true
Attachment #8375565 -
Flags: review?(lucasr.at.mozilla)
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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 10•9 years ago
|
||
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-
Reporter | ||
Comment 11•9 years ago
|
||
(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.
Reporter | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Reporter | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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
Reporter | ||
Comment 16•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/94c8a67eee87 https://hg.mozilla.org/mozilla-central/rev/cb83f363f469 https://hg.mozilla.org/mozilla-central/rev/7b8b62250d46
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee | ||
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•