562795 - Sorting by name in the digest view is case-sensitive for all types of add-ons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a5pre) Gecko/20100429 Firefox/3.7a5pre

Tools > Add-ons > Plugins

iPhotoCast appears at the bottom when they're sorted by name (which is the default).  It should appear between "Default plugin" and "Lala download".

Comment 1

[Alice0775 White]

Tools > Add-ons > Extensions
Also case sensitive.
Should be ignore case to sort by name.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100429 Minefield/3.7a5pre ID:20100429143047

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Adds sort hints for the numeric fields and makes sorting default to ascending for name sorts. Tests the sorting.

The change to automation.py is because I neglected to change its value when we renumbered the scopes in bug 555486.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 451720 [details] [diff] [review]
patch rev 1

Note: This also has the fix for bug 566760 (unregistering providers), which is needed for the initial work from bug 561687 (the mocking provider), which is needed for testing this. Dependency chains are fun!


>+user_pref("extensions.enabledScopes", 5);

Can you add a comment here - 5 is not descriptive.


>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>-    var size = Math.floor(Math.random() * 1024 * 1024 * 2);
>-    size = ("00000000000" + size).slice(-10); // HACK: nsIXULSortService doesn't do numerical sorting (bug 379745)
>-    item.setAttribute("size", size); // XXXapi - bug 561261
>+    if (aObj.size)
>+      item.setAttribute("size", aObj.size);

I think you'll also need to update _updateSize() in extensions.xml.

>+    if (["dateUpdated", "size", "relevancescore"].indexOf(aSortBy) >= 0)

This array used a couple of times (and might be used again in the future, eg bug 562622) - so should be a global constant.

>+    if (["dateUpdated", "size", "relevancescore"].indexOf(aSortBy) >= 0)

Ditto.

>+          // Name sorting defaults to ascending, others to descending
>+          this.ascending = aSort == "name";

Are we sure the name will be the only thing that will ever be sorted ascending by default? I'd rather have a global constant array, like above.

r=me with those changes.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated from comments, waiting on some other bugs to land first though.

Comment 8

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/14383ec3611b

Comment 9

[Dave Townsend [:mossop]]

Had to revert the changes to automation.py.in due to update test failures. Also had to disable the testcase for the following common-random problem:

TEST-INFO | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_sorting.js | Running test 4
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_sorting.js | Should have seen the right order - Got ["[e-mail]", "[e-mail]", "[e-mail]", "[e-mail]", "[e-mail]"], expected ["[e-mail]", "[e-mail]", "[e-mail]", "[e-mail]", "[e-mail]"]

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to comment #9)
> Had to revert the changes to automation.py.in due to update test failures.
Were the failures for extension or app update?

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> (In reply to comment #9)
> > Had to revert the changes to automation.py.in due to update test failures.
> Were the failures for extension or app update?

App update, it's a little concerning perhaps since the change would have basically made the default theme visible during the test run. Mean to look at it tomorrow but if you have time then you're likely to spot the problem sooner than me

Comment 12

[Robert Strong (they/them - no direct email)]

That is a tad disconcerting... it might be due to my not excluding SCOPE_APPLICATION in the test since I assumed that mochitest wouldn't return SCOPE_APPLICATION add-ons. I'll take a look.

Comment 13

[Dave Townsend [:mossop]]

Ok, the test failure was actually a real problem, no idea why it doesn't show up on OSX but the sort service only supports sorting 32-bit integers. The date numbers we use are larger than that so the sort service drops back to plain string sorting in that case. Fix coming up.

Comment 14

[Robert Strong (they/them - no direct email)]

Turns out it is what I thought...  it is failing since it tries to disable the active theme. Easy enough to fix

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: app update test fix

Dave, I'm taking PTO tomorrow so please feel free to check this in if you r+ this patch... it works for me with extensions.enabledScopes = 5 and makes the test consistent with the app update code by excluding SCOPE_APPLICATION add-ons.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 452169 [details] [diff] [review]
app update test fix

Yeah I'll land that tomorrow once I've done a tryserver run with the automation change too

Comment 17

[Dave Townsend [:mossop]]

Attachment: switch dates to fit in 32-bit integers

This will tide us over until 2038

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 452177 [details] [diff] [review]
switch dates to fit in 32-bit integers

> This will tide us over until 2038

Famous last words ;)

>+      item.setAttribute("dateUpdated", aObj.updateDate.getTime() / 1000);

I'm a bit worried about the reason for this being lost in the mists of time (read: I'll see this, wonder why its there, and change it back). I can haz comment?

Comment 19

[Dave Townsend [:mossop]]

Landed the new patches:

http://hg.mozilla.org/mozilla-central/rev/892d1e84a0ef
http://hg.mozilla.org/mozilla-central/rev/fcf45fdb4747

Comment 20

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

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre