Closed
Bug 624808
Opened 14 years ago
Closed 14 years ago
Add-ons should be grouped in list view according to their status (enabled, disabled, etc)
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b11
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: jboriss, Assigned: mconley)
References
Details
(Keywords: polish, Whiteboard: [softblocker])
Attachments
(3 files, 2 obsolete files)
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•14 years ago
|
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Whiteboard: [softblocker]
Updated•14 years ago
|
blocking2.0: ? → final+
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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 7•14 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•14 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.
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
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-
Comment 11•14 years ago
|
||
After thought I've decided that this needs beta exposure or we shouldn't take it for Firefox 4
blocking2.0: final+ → betaN+
Comment 12•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
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)
Assignee | ||
Comment 15•14 years ago
|
||
Reporter | ||
Comment 16•14 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!
Comment 17•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
Absolutely. Let me comment over there to have it tracked on the right bug.
Comment 20•14 years ago
|
||
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-
Assignee | ||
Comment 21•14 years ago
|
||
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)
Comment 22•14 years ago
|
||
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+
Assignee | ||
Comment 23•14 years ago
|
||
Not sure if this needs approval, since its a soft-blocker. But I'm just gonna go for it.
Keywords: checkin-needed
Comment 24•14 years ago
|
||
(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]
Comment 25•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/547b0ea9f1c9
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [softblocker][has patch][needs landing] → [softblocker]
Target Milestone: --- → mozilla2.0b11
Updated•14 years ago
|
Flags: in-testsuite+
Flags: in-litmus-
Comment 26•14 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•14 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.
Comment 28•14 years ago
|
||
(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•14 years ago
|
||
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
Comment 31•14 years ago
|
||
(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•14 years ago
|
||
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.
Description
•