Closed Bug 880230 Opened 11 years ago Closed 11 years ago

Quick share item swaps between top item despite never selecting it


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox22 unaffected, firefox23 unaffected, firefox24 affected, firefox25 affected, firefox26 affected, firefox27 affected, firefox28 verified, fennec28+)

Firefox 28
Tracking Status
firefox22 --- unaffected
firefox23 --- unaffected
firefox24 --- affected
firefox25 --- affected
firefox26 --- affected
firefox27 --- affected
firefox28 --- verified
fennec 28+ ---


(Reporter: aaronmt, Assigned: wesj)




(2 files, 1 obsolete file)

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


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)
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'.
Here's a video because you're probably confused:
I'm playing with this ... as usual, if someone needs to jump in faster, please let me know.
Assignee: nobody → markcapella
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'.


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*
Releasing since I haven't looked at this in a while
Assignee: markcapella → nobody
This is still a complete nuisance.
tracking-fennec: --- → ?
Blocks: 937828
Assignee: nobody → sriram
tracking-fennec: ? → 28+
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.
Assignee: sriram → wjohnston
Attached patch Patch 1/2 (obsolete) — Splinter Review
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.
Attachment #8337118 - Flags: review?(
Attached patch Patch 2/2Splinter Review
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.
Attachment #8337119 - Flags: review?(
Comment on attachment 8337118 [details] [diff] [review]
Patch 1/2

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

::: mobile/android/base/widget/
@@ +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.
Attachment #8337118 - Flags: review?( → review-
Comment on attachment 8337119 [details] [diff] [review]
Patch 2/2

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

::: mobile/android/base/widget/
@@ +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 ;-)
Attachment #8337119 - Flags: review?( → review+
Attached patch Patch 1/2Splinter Review
Good idea.
Attachment #8337118 - Attachment is obsolete: true
Attachment #8337849 - Flags: review?(
Comment on attachment 8337849 [details] [diff] [review]
Patch 1/2

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


::: mobile/android/base/widget/
@@ +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.
Attachment #8337849 - Flags: review?( → review+
(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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Keywords: verifyme
Keywords: verifyme
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.