880230 - Quick share item swaps between top item despite never selecting it

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently when one always selects the same item in the share menu, the wrong item will be displayed in quick-share.

STR:

i) Open the share menu and select 'copy to clipboard'
ii) Repeat the first step a few times

Expected: 'Copy to clipboard is quick-shared
Actual: Different item is quick-shared, in my case, Google Drive

--
HTC One (Android 4.1)
Nightly (06/06)

Comment 1

[Aaron Train [:aaronmt]]

Each time afterwards that I select 'Copy to clipboard' as an item, it swaps back between the top-most item, in my case, Drive, and 'Copy to clipboard'.

Comment 2

[Aaron Train [:aaronmt]]

Here's a video because you're probably confused: http://www.youtube.com/watch?v=jnrrIAJDyQc

Comment 3

[Mark Capella [:capella]]

I'm playing with this ... as usual, if someone needs to jump in faster, please let me know.

Comment 4

[Aaron Train [:aaronmt]]

Confirmed that this happens with Google Drive installed (or maybe any package that adds additional activities). Installing Google Drive [1], adds a 'send text to clipboard activity'.

[1] com.google.android.apps.docs.shareitem.UploadSharedItemActivity
[2] com.google.android.apps.docs.SendTextToClipboardActivity

Uninstalling Google Drive removes both activities.

I think the problem here is logic getting confused on which activity to pick when multiple are available under a common package like com.google.android.apps.docs.*

Comment 5

[Mark Capella [:capella]]

Releasing since I haven't looked at this in a while

Comment 6

[Aaron Train [:aaronmt]]

This is still a complete nuisance.

Comment 7

[Wesley Johnston (:wesj)]

Taking this. Aaron was right. We're using the packageName to compare these. For packages that provide multiple intents (with different classes handling them) that leads to confusion.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Two fixes. This fixes the count returned from the Model. This is used to determine if we show one or two items in the menu. Since the db holds a record for each time share was used, using it twice with the same service was causing this to return 2 before. This fixes it to only count services with unique names.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This fixes the real bug here. Uses the un-ambiguous packageName/class in this map of activities so that Drive and Copy to clipboard don't get thrown together into the same key.

Comment 10

[Lucas Rocha (:lucasr)]

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

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

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +663,5 @@
>       */
>      public int getHistorySize() {
>          synchronized (mInstanceLock) {
>              ensureConsistentState();
> +            List<String> packages = new ArrayList<String>();

nit: final

@@ +669,5 @@
> +              String activity = record.activity.flattenToString();
> +              if (!packages.contains(activity)) {
> +                packages.add(activity);
> +              }
> +            }

This will change the semantics of the existing API which might lead to confusion later. The notion of 'history' in the current API is about tracking every single time the user has chosen an activity to share something. Which in turn is different than things like getActivityCount().

It seems our UI needs something like the number of unique activities in history. So I'd probably add a new method like getActivityCountInHistory() or something.

Comment 11

[Lucas Rocha (:lucasr)]

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

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

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +980,5 @@
>              final int lastShareIndex = historicalRecords.size() - 1;
>              float nextRecordWeight = 1;
>              for (int i = lastShareIndex; i >= 0; i--) {
>                  HistoricalRecord historicalRecord = historicalRecords.get(i);
> +                String packageName = historicalRecord.activity.flattenToString();

Maybe file a bug in upstream Android as well? They'll probably ignore it but, hey, we're doing our part ;-)

Comment 12

[Wesley Johnston (:wesj)]

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

Good idea filing: https://code.google.com/p/android/issues/detail?id=62810

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Good idea.

Comment 14

[Lucas Rocha (:lucasr)]

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

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

Nice.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +667,5 @@
>              return mHistoricalRecords.size();
>          }
>      }
>  
> +    public int getNumUniqueActivitiesInHistory() {

The reason why I suggested getActivityCountInHistory() is because is that it would match well with the existing getActivityCount() method. I don't feel strongly about it. Up to you.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #14)

> > +    public int getNumUniqueActivitiesInHistory() {
> 
> The reason why I suggested getActivityCountInHistory() is because is that it
> would match well with the existing getActivityCount() method. I don't feel
> strongly about it. Up to you.

My OCD likes getActivityCountInHistory or getDistinctActivityCountInHistory

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b738cfb4e9d4

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/075a42c831f6