Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add-ons should be grouped in list view according to their status (enabled, disabled, etc)

VERIFIED FIXED in mozilla2.0b11

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Boriss, Assigned: mconley)

Tracking

({polish})

Trunk
mozilla2.0b11
polish
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 502882 [details]
Mockup: Add-ons grouped in list view according to status

Currently in list view, add-ons are displayed alphabetically no matter what their current status.  This presents a visually busy "striping" effect with enabled and disabled add-ons are interspersed.

Instead, add-ons should be grouped with add-ons of similar status, as follows:

1. Top of list view: enabled add-ons
2. Second in list view: incompatible add-ons
3. Third in list view: disabled add-ons
4. Bottom of list view: blocked add-ons

This sorting prevents the striping effect and also sorts add-ons according to how likely they are to be relevant.  Listing enabled add-ons at the top means that with minimal scrolling, users see the add-ons that are currently interacting with their browser.  Add-ons not currently interacting with the browser, such as disabled and blocked add-ons, are listed last.

Within each category, add-ons should be alphabetically sorted.

If a user makes a change to an add-on which changes its state right away (eg disabling a restartless add-on), it should not move to the disabled category until the user leaves the add-ons manager.  This is to prevent add-ons from jumping around while the user is interacting with it.
(Reporter)

Updated

7 years ago
Blocks: 623250
Duplicate of this bug: 562232
(Reporter)

Updated

7 years ago
Keywords: polish, uiwanted
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
Whiteboard: [softblocker]
blocking2.0: ? → final+
Assignee: nobody → mconley
Created attachment 504776 [details] [diff] [review]
Splits addons into alphabetized groups (active, incompatible, etc)
Attachment #504776 - Flags: review?(dtownsend)
So I just posted a patch.  I was going to write tests for this, but I couldn't seem to find any tests for the Addons Manager...

I did find this:  http://mxr.mozilla.org/mozilla-central/source/testing/mozmill/tests/shared-modules/testAddonsAPI.js

Though after an MXR search, it looks like nothing uses it.

Let me know if you'd like tests for this, and if so, please advise.  Thanks,

-Mike
(In reply to comment #3)
> I did find this: 
> http://mxr.mozilla.org/mozilla-central/source/testing/mozmill/tests/shared-modules/testAddonsAPI.js

Those are Mozmill tests and we will start soon to implement the first tests which cannot be automated with Mochitest. 

What you want is:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/
Depends on: 623207
(In reply to comment #0)
> Created attachment 502882 [details]
> Mockup: Add-ons grouped in list view according to status
> 
> Currently in list view, add-ons are displayed alphabetically no matter what
> their current status.  This presents a visually busy "striping" effect with
> enabled and disabled add-ons are interspersed.
> 
> Instead, add-ons should be grouped with add-ons of similar status, as follows:
> 
> 1. Top of list view: enabled add-ons
> 2. Second in list view: incompatible add-ons
> 3. Third in list view: disabled add-ons
> 4. Bottom of list view: blocked add-ons

Where do pending uninstalls go?
And pending installs...

Comment 7

7 years ago
I'd suggest sticking any pending actions in the group they'll go into once the pending operation is complete. Pending installs would be highlighted in the enabled group and pending uninstalls would be highlighted in the disabled group (which for the moment they are).

Comment 8

7 years ago
Clarification: Restartless pending uninstalls would go into the disabled group. Restart requiring pending uninstalls would have to stay in the enabled group until uninstalled.

The other obvious solution to this is to just stick all pending operations up top in one highlighted group.
(In reply to comment #8)
> Clarification: Restartless pending uninstalls would go into the disabled group.
> Restart requiring pending uninstalls would have to stay in the enabled group
> until uninstalled.

Restartless pending uninstalls have to stay wherever they were when the user clicks uninstall per comment 0. After that they disappear from view anyway.
Comment on attachment 504776 [details] [diff] [review]
Splits addons into alphabetized groups (active, incompatible, etc)

I don't think that this is the right approach to take here. Instead the more straightforward way is to just make sortElements accept an array for aSortBy (and update all callers to pass one) of fields to sort by in order of preference. If the elements match by the first field then move onto the second and so on. For the non-search list views use a specially named field "uiState" and then use your function as the comparison function for that field.

There are indeed a number of tests that these changes will probably break and will need fixing as well as new tests writing in toolkit/mozapps/extensions/test/browser
Attachment #504776 - Flags: review?(dtownsend) → review-
After thought I've decided that this needs beta exposure or we shouldn't take it for Firefox 4
blocking2.0: final+ → betaN+
Note that I also suspect it'll likely be a problem if this lands in the beta and bug 623207 does not which probably means that bug 623207 has to make the last beta as well if this is to be able to land.
Created attachment 505194 [details] [diff] [review]
Re-implementation following review.  This patch requires the patch from Bug 623207.

This patch requires patch v2 (https://bugzilla.mozilla.org/attachment.cgi?id=505006) from Bug 523207 (https://bugzilla.mozilla.org/show_bug.cgi?id=623207).

I've changed sortElements so that it expects aSort to be an Array of attributes to sort on, with decreasing priority. I've also added a special "uiState" attribute to do the stratifying into the Enabled, Incompatible, Disabled and Blocked groups.

Add-ons that are to be installed are currently put into the group that they'll exist in once the installation is complete.  Likewise, add-ons that are to be uninstalled remain in the group that they're in.

I had to update a few tests, and I expanded browser_sorting.js to account for the stratification.
Attachment #504776 - Attachment is obsolete: true
Attachment #505194 - Flags: review?(dtownsend)
Whoops - I meant Bug 623207.  Sorry for the spam.
Created attachment 505196 [details]
Screenshot: the new ordering of add-ons in the manager
(Reporter)

Comment 16

7 years ago
(In reply to comment #15)
> Created attachment 505196 [details]
> Screenshot: the new ordering of add-ons in the manager

That looks fantastic, Mike!
As more as I think about we should really have kept the sort selector for the "by state" sorting order. :/ With a dozen of add-ons installed it's hard to find the one you are looking for.
(In reply to comment #17)
> As more as I think about we should really have kept the sort selector for the
> "by state" sorting order. :/ With a dozen of add-ons installed it's hard to
> find the one you are looking for.

Isn't that an argument against bug 623207, rather than this bug? (I agree, FWIW)
Absolutely. Let me comment over there to have it tracked on the right bug.
Comment on attachment 505194 [details] [diff] [review]
Re-implementation following review.  This patch requires the patch from Bug 623207.

This is good work, just a couple of small issues I can see and then I think this will be ready

>diff -r 28b1ecf51f07 -r ddbb0c53bcef toolkit/mozapps/extensions/content/extensions.js

>+    for (let i = 0; i < aSortBy.length; i++) {
>+
>+      var sortBy = aSortBy[i];
>+      var sortFunc = stringCompare;
>+
>+      if (sortBy == "uiState")
>+        sortFunc = uiStateCompare;
>+      else if (DATE_FIELDS.indexOf(sortBy) != -1)
>+        sortFunc = dateCompare;
>+      else if (NUMERIC_FIELDS.indexOf(sortBy) != -1)
>+        sortFunc = numberCompare;

You can save doing this every time by just building an array of the sorting functions to use before you start sorting.

>+
>+      if (sortBy != "uiState" && !aAscending) {
>+        [a, b] = [b, a];
>+      }

If in the future we added some way to sort with 3 keys then this would behave strangely, inverting the order of each key. Instead you could do the inversion outside the for loop and just have uiStateCompare invert its result if !aAscending.

>+      var aValue = getValue(a, sortBy);
>+      var bValue = getValue(b, sortBy);
>+
>+      if (!aValue && !bValue)
>+        return 0;
>+      if (!aValue)
>+        return -1;
>+      if (!bValue)
>+        return 1;
>+      if (aValue != bValue)
>+        return sortFunc(aValue, bValue);

I think it'd be better to get the result from the sort function and then return it if it isn't 0.
Attachment #505194 - Flags: review?(dtownsend) → review-
Created attachment 505898 [details] [diff] [review]
Fix-ups following dtownsend review.  Re-implementation following review. This patch requires the patch from Bug 623207.

Follow-up after Dave Townsend's review (thanks!)

All suggestions implemented.  All feedback welcome.
Attachment #505194 - Attachment is obsolete: true
Attachment #505898 - Flags: review?(dtownsend)
(Reporter)

Updated

7 years ago
Keywords: uiwanted
Comment on attachment 505898 [details] [diff] [review]
Fix-ups following dtownsend review.  Re-implementation following review. This patch requires the patch from Bug 623207.

Looks good, thanks
Attachment #505898 - Flags: review?(dtownsend) → review+
Not sure if this needs approval, since its a soft-blocker.  But I'm just gonna go for it.
Keywords: checkin-needed
(In reply to comment #23)
> Not sure if this needs approval

Nope, its good to land, since its marked blocking2.0+ (betaN).
Whiteboard: [softblocker] → [softblocker][has patch][needs landing]
http://hg.mozilla.org/mozilla-central/rev/547b0ea9f1c9
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [softblocker][has patch][needs landing] → [softblocker]
Target Milestone: --- → mozilla2.0b11
Flags: in-testsuite+
Flags: in-litmus-

Comment 26

7 years ago
Some people actually prefer disabled extensions to stay in the same listed view place along with the rest of extensions in alphabetical order, not stacking right at the bottom of the panel in a seperate group. Is this just another cosmetic fix that gets implemented, anyone who dares goes against boriss consensus is ignored.

I'll be the first to join everyone else! calling this stupidity out, these are bad changes that are just unnecessary and causing more issues to crop up.

https://bugzilla.mozilla.org/show_bug.cgi?id=623207

Comment 27

7 years ago
Not even providing any compromise, like having it your way in default state, and allowing others to sort (yeah that's gone, bring it back) by 'Name' where all the extensions [enabled/disabled/blocked] are shown alphabetically.
(In reply to comment #26)
> I'll be the first to join everyone else! calling this stupidity out, these are
> bad changes that are just unnecessary and causing more issues to crop up.

You've already been told once in the other bug to take these comments to the newsgroups. If you're not interested in starting a discussion then that's fine but posting here is only serving to irritate people making it far more likely we'll ignore you in the future.
Depends on: 629521
Wasn't that easy to find an add-on which is block-listed but now I was able to verify that it works great on all platforms like Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

I have filed bug 629521 for the add-ons which are marked to get removed.
Status: RESOLVED → VERIFIED

Updated

7 years ago
Duplicate of this bug: 614798
Depends on: 629873
(In reply to comment #29)
> Wasn't that easy to find an add-on which is block-listed

When I need a blocklisted addon to test, I just manually edit blocklist.xml in the profile directory to blocklist an addon I already have installed - it'll show up as blocklisted once you restart Firefox. Saves hunting around for something (and/or messing with the blocklist timer).
Yes, figured that out after my last comment here while working on the verification for some blocklist bugs.
You need to log in before you can comment on or make changes to this bug.