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

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Miguel Ojeda]

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

Comment 1

[Miguel Ojeda]

Attachment: Trivial patch that sorts them by Enabled, Name and Version.

Comment 2

[Dão Gottwald [:dao]]

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

Comment 3

[Miguel Ojeda]

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.

Comment 4

[Miguel Ojeda]

Attachment: Case insensitive sort for Name.

Dao, please consider this one as well, which makes the Name's sort case insensitive.

Comment 5

[Dão Gottwald [: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.

Comment 6

[Miguel Ojeda]

Attachment: Patch v3 - Using localeCompare()

Thanks Dao. Fixed patch, case insensitive.

Comment 7

[Dão Gottwald [: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?

Comment 8

[Miguel Ojeda]

Attachment: Patch v4 - Using localeCompare(), 1/-1 instead of true/false.

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

Comment 9

[Dão Gottwald [:dao]]

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