Closed Bug 645485 Opened 9 years ago Closed 9 years ago

about:support's extensions are not sorted, unlike in the Add-Ons "page"


(Toolkit :: General, defect, trivial)

Not set





(Reporter: miguel.ojeda.sandonis, Assigned: miguel.ojeda.sandonis)





(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

The about:support's extensions are not sorted (or it is not obvious how they are sorted).

The attached patch sorts them by "Enabled", then "Name" and finally "Version". This also resembles the sorting in the Add-Ons "page" in FF4.

Reproducible: Always
Version: unspecified → 4.0 Branch
Attachment #522193 - Flags: review?(dao)
Product: Firefox → Toolkit
QA Contact: general → general
Version: 4.0 Branch → Trunk
Ever confirmed: true
Comment on attachment 522193 [details] [diff] [review]
Trivial patch that sorts them by Enabled, Name and Version.

I'm not sure that sorting by Enabled first is really useful, or that sorting by Version after Name is necessary...
Attachment #522193 - Flags: review?(dao) → review+
Dao, thanks for the review! I should have explained the logic:

Sorting Version after Name: It is useful for cases like the "Java Console" module where you may have several versions of it.

Sorting by Enabled first: In the Support page it is not that useful probably, but it is nice to sort them the same way as in the "Add-Ons Manager". Also, it allows to see which add-ons are installed and enabled when a user pastes the about:support's information somewhere else.
Attached patch Case insensitive sort for Name. (obsolete) — Splinter Review
Dao, please consider this one as well, which makes the Name's sort case insensitive.
Attachment #523279 - Flags: review?(dao)
Comment on attachment 523279 [details] [diff] [review]
Case insensitive sort for Name.

You should use toLocaleLowerCase instead of toLowerCase.

Probably need to use localeCompare as well instead of >, now that I think of it. And the sort function should return -1, 0 or 1.
Attachment #523279 - Flags: review?(dao) → review-
Attached patch Patch v3 - Using localeCompare() (obsolete) — Splinter Review
Thanks Dao. Fixed patch, case insensitive.
Attachment #522193 - Attachment is obsolete: true
Attachment #523279 - Attachment is obsolete: true
Attachment #523294 - Flags: review?(dao)
Comment on attachment 523294 [details] [diff] [review]
Patch v3 - Using localeCompare()

>+        return b.isActive;

>+        return a.version > b.version;

These should be -1 / 1 too.

This patch doesn't use toLocaleLowerCase. Is this intentional?
Attachment #523294 - Flags: review?(dao) → review-
You're right, I was relying on Fx's handling of true/false in sort() instead of the standard, sorry.

About localeCompare(), yeah. The function knows whether to perform a case insensitive sort or not in the current locale (in my case, it sorts in a case insensitive way).
Attachment #523294 - Attachment is obsolete: true
Attachment #523304 - Flags: review?(dao)
Attachment #523304 - Flags: review?(dao) → review+
Assignee: nobody → miguel.ojeda.sandonis
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.