629521 - Extensions and themes which are marked to get removed are not grouped but remain at the original location

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Attachment: screenshot

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

When you remove extensions or themes and switch the panes, the to be removed extensions are not grouped together. Because of the smaller and not bold add-on name it's kinda hard to spot. We should also add a group for those add-ons.

Steps:
1. Install multiple add-ons
2. Disable at least one add-on and switch panes
3. Remove at least one add-on and switch panes

After step 3 the to disable add-ons are grouped but not the add-ons which have to be removed. See the screenshot.

Comment 1

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

Where do we want to show this group? As the first group? The height is small enough and showing them first, will remind that those are getting removed. There is no undo uninstall possible yet.

Comment 2

[Dave Townsend [:mossop]]

Not going to block on this, would probably approve a safe tested patch before the last beta though.

Comment 3

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

Mike, would you have interest to fix the remaining issue for Firefox 4?

Comment 4

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

I would indeed.  I'll take a peek.

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

I recommend grouping these soon-to-be-deleted items at the very bottom of the add-ons manager.

By the time this grouping happens, the user has first marked an item to be removed and then left the current open pane.  It's safe to say the user's finished with the add-on at this point.  Bringing these items back to prominence by adding them to the top of the list would both remind the user of the add-on they don't want and, because of the vertical height of the item, possibly confuse users by looking like top level notifications.

Instead, if these notifications are at the bottom of the list, the progression is natural:

1. active (top)
2. incompatible (active but proceed with caution)
3. diasbled (not active)
4. nearly removed (on the way out)

Comment 6

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

Attachment: Uninstalled addons get put at end of browser after switching panes

This patch groups addons to be uninstalled to the very end of the addon browser.

Also, we weren't grouping disabled addons after switching panes.  This has been fixed.

I've also put in a "catch all" grouping in case we get some freaky addon that doesn't meet any of the group requirements.  I've put that group at the end.  Maybe need some UI review there.

Comment 7

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

Attachment: Screenshot showing the results of the patch in the addons manager UI.

You can see that the addons to be uninstalled have been grouped at the end of the browser.

Comment 8

[Jennifer Morrow [:Boriss] (UX)]

Dave will speak to this effect in the review, but we're making a slight change to the ordering of items based on status based on some helpful thoughts from Dave:

Bucket 1

* Enabled
* Incompatible but enabled (compatibility checking is off)
* Softblocked
* Waiting to be installed
* Waiting to be enabled

Bucket 2

* Waiting to be disabled

Bucket 3

* Waiting to be removed

Bucket 4

* Disabled
* Incompatible and disabled (normal case)
* Blocklisted

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 508597 [details] [diff] [review]
Uninstalled addons get put at end of browser after switching panes

I've been looking at some of the feedback and in particular concerns like bug 629873 make me think that we're splitting the list up a little too aggressively making users confused over why things are sorted they are. After talking with Boriss on IRC we think that the most sensible splitting is this:

Bucket 1

* Enabled
* Incompatible but enabled because compatibility checking is off
* Waiting to be installed
* Waiting to be enabled

Bucket 2

* Waiting to be disabled

Bucket 3

* Waiting to be removed

Bucket 4

* Disabled
* Incompatible
* Blocklisted

I think that rather than using an additional place for unknown stuff, instead just stick them with either the enabled/disabled items as appropriate.

Comment 10

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

Attachment: Groups add-ons into the 4 buckets described in Comment 9

Alright, I've updated the patch to split the add-ons into the four buckets.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 509510 [details] [diff] [review]
Groups add-ons into the 4 buckets described in Comment 9

>     addon = aObj.mAddon || aObj.mInstall;
>     if (!addon)
>       return null;
>+
>     if (aKey == "uiState") {
>-      if (addon.isActive)
>-        return "enabled";
>-      else if (!addon.isCompatible)
>-        return "incompatible";
>-      else if (addon.blocklistState == Ci.nsIBlocklistService.STATE_NOT_BLOCKED)
>-        return "disabled";
>-      else if (addon.isCompatible &&
>+      if ((addon.isActive &&
>+           (addon.pendingOperations != AddonManager.PENDING_DISABLE &&
>+            addon.pendingOperations != AddonManager.PENDING_UNINSTALL)
>+          ) ||
>+          addon.pendingOperations == AddonManager.PENDING_INSTALL ||
>+          addon.pendingOperations == AddonManager.PENDING_ENABLE)
>+        return "enabledInstalled";
>+      else if (addon.pendingOperations == AddonManager.PENDING_DISABLE)
>+        return "pendingDisable";
>+      else if (addon.pendingOperations == AddonManager.PENDING_UNINSTALL)
>+        return "pendingUninstall";
>+      else if (!addon.isActive || addon.appDisabled || addon.userDisabled ||
>                addon.blocklistState != Ci.nsIBlocklistService.STATE_NOT_BLOCKED)
>-        return "blocked";
>+        return "disabledIncompatibleBlocked";
>     }

With a bit of re-ordering I think this can become a lot cleaner.

The middle two buckets are the most straightforward cases so test for those first. The last bucket is really just anything !isActive except for things that are PENDING_INSTALL or PENDING_ENABLE. The first bucket is just everything else.

The bucket names probably don't need to be quite so descriptive either, just "enabled", "pendingDisable", "pendingUninstall" and "disabled" should do.

Comment 12

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

Attachment: Groups add-ons into the 4 buckets described in Comment 9 - take 2

Good call - thanks for the review.  Cleaned and simplified.  All add-ons browser tests still pass.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 509524 [details] [diff] [review]
Groups add-ons into the 4 buckets described in Comment 9 - take 2

Yeah this looks much better, thanks for the changes

Comment 14

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

Glad to help.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cff79fb2f133

Comment 16

[(no longer active)]

I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Relanded all but one of the patches in that push.  For this bug, that's:
http://hg.mozilla.org/mozilla-central/rev/a80bddafafb8

Comment 18

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre

Dave, are there any needs for a manual test? Looks like the Mochitest covers it pretty well.

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre)
> Gecko/20110209 Firefox/4.0b12pre
> 
> Dave, are there any needs for a manual test? Looks like the Mochitest covers it
> pretty well.

I think we're covered here