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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

Attachment: 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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Splits addons into alphabetized groups (active, incompatible, etc)

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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/

Comment 5

[Dave Townsend [:mossop]]

(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?

Comment 6

[Dave Townsend [:mossop]]

And pending installs...

Comment 7

[Dave Garrett]

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

[Dave Garrett]

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.

Comment 9

[Dave Townsend [:mossop]]

(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 10

[Dave Townsend [:mossop]]

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

Comment 11

[Dave Townsend [:mossop]]

After thought I've decided that this needs beta exposure or we shouldn't take it for Firefox 4

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Whoops - I meant Bug 623207.  Sorry for the spam.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Screenshot: the new ordering of add-ons in the manager

Comment 16

[Jennifer Morrow [:Boriss] (UX)]

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

That looks fantastic, Mike!

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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)

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Absolutely. Let me comment over there to have it tracked on the right bug.

Comment 20

[Dave Townsend [:mossop]]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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.

Comment 22

[Dave Townsend [:mossop]]

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

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Not sure if this needs approval, since its a soft-blocker.  But I'm just gonna go for it.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #23)
> Not sure if this needs approval

Nope, its good to land, since its marked blocking2.0+ (betaN).

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/547b0ea9f1c9

Comment 26

[Xleon]

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

[Xleon]

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.

Comment 28

[Dave Townsend [:mossop]]

(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.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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).

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, figured that out after my last comment here while working on the verification for some blocklist bugs.