981405 - "Recent Folders" sorting has switched from case insensitive to case sensitive

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Jonathan Kamens]

In thunderbird 24.3, the "Recent Folders" view is sorted case-insensitive, which I think is what people expect in general nowadays and presumably also in particular from this view since it has been that way forever.

In 3.0.a1 (trunk) as of 2014-03-02, the sorting of that view is case-sensitive.

Comment 1

[:aceman]

The sorting algorithm got a change from me in bug 845992, but we still intend to have it case insensitive (see the discussion there). I think the new algorithm is more robust in case of accented characters.
Maybe there is something wrong in your locale setting. Can you find out what is your locale (language) of Thunderbird and your Operating system? Try Tools->Error console, type "navigator.language" into the "Code" textbox and Evaluate.

Comment 2

[Jonathan Kamens]

navigator.language evaluates to en-US.

Comment 3

[Jonathan Kamens]

[ec2-user@monitor ~]$ js
js> 'Inbox'.localeCompare('archive')
-24
js>

Comment 4

[Jonathan Kamens]

I have LC_COLLATE set to C. I thought that might be the problem, but even when I run "env -i js" to run the JavaScript interpreter with an empty environment, it still sorts Inbox before archive as shown above.

Similarly, if I run "env -i LANG=en_US DISPLAY=:0 thunderbird" to launch Daily with a nearly empty environment, the recent folders pane still sorts case-sensitive.  So I am having a hard time convincing myself that my locale settings are the issue.

Comment 5

[Jonathan Kamens]

Also: "env -i sort" sorts case-sensitive. "env -i LANG=en_US sort" sorts case-insensitive. So by that, "env -i LANG=en_US js" and "env -i LANG=en_US thunderbird" should both sort case-insensitive, but they don't.

Comment 6

[Jonathan Kamens]

Basically, localeCompare appears to be broken.

As per http://stackoverflow.com/questions/8996963/how-to-perform-case-insensitive-sorting-in-javascript , I think you need to do this when trying to do a case insensitive comparison of two strings:

a.toLowerCase().localeCompare(b.toLowerCase())

Comment 7

[:aceman]

For me 'Inbox'.localeCompare('archive') returns 1 (so does 'inbox'.localeCompare('Archive')).

So according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare there are some possible options for case sensitivity (e.g. "caseFirst"). But none look like forcing case-insensivity. Maybe we really need to force it via .toLowerCase(). This would need to be changed at more places where we use localeCompare().
Neil, what do you think?

Comment 8

[[e-mail]]

(In reply to aceman from comment #7)
> Maybe we really need to force it via .toLowerCase().

It looks as if localeCompare seems to be case insensitive on all of my builds too, so I don't know what's going on here either. I wouldn't be against adding .toLocaleLowerCase() though it's a shame because I just discovered stringArray.sort(String.localeCompare). (Alternatively for sorting folders .compareSortKeys might work, although it would sort special folders first.)

Comment 9

[Jonathan Kamens]

Is anybody here besides me using Linux? Is this a Linux vs. Windows vs. Mac OS thing?

I just tested on my Linux laptop and got the same results, so that's two different (Fedora 20) Linux machines with this behavior.

As for "(Alternatively for sorting folders .compareSortKeys might work, although it would sort special folders first.)", I personally thin kthat the special folders _should_ sort first, but maybe that's just me.

Comment 10

[:aceman]

No, I was developing it under Linux and it was case insensitive too. So it may depend just on the locale.

Is TB also running with the strange setting of LC_COLLATE=C ?

Comment 11

[:aceman]

(In reply to Jonathan Kamens from comment #4)
> I have LC_COLLATE set to C. I thought that might be the problem, but even
> when I run "env -i js" to run the JavaScript interpreter with an empty
> environment, it still sorts Inbox before archive as shown above.
> 
> Similarly, if I run "env -i LANG=en_US DISPLAY=:0 thunderbird" to launch
> Daily with a nearly empty environment, the recent folders pane still sorts
> case-sensitive.  So I am having a hard time convincing myself that my locale
> settings are the issue.

Does LANG override LC_COLLATE? I don't think so. So try LC_COLLATE=en_US .

Comment 12

[Jonathan Kamens]

As I illustrated in my previous examples, this is not an issue of my environment variable settings:

jik2:~!635$ env -i bash --norc
bash-4.2$ set | grep '^L'
LINES=24
bash-4.2$ js
js> "Inbox".localeCompare("archive")
"Inbox".localeCompare("archive")
-24
"inbox".localeCompare("archive")
8
js> 

You might think that the command-line JavaScript interpreter might differ from the one in Thunderbird, but I get the same results when I evaluate the same expressions in the error console after invoking Thunderbird from that shell with no locale environment variable settings.

Just for the heck of it, I created a completely new, vanilla user account, with no dot files or other configuration, logged in as that user, and ran the same test as above with the js interpreter, and got the same result.

Finally, I ran "LC_ALL=en_US LC_COLLATE=en_US LANG=en_US thunderbird" and evaluated the expressions above in the error console and got the same result.

I don't know why I am seeing different results from y'all. I'm frankly not sure it matters. If I'm seeing different results, then other people are going to see different results, so if the intention is for the sorting to be case-insensitive, then I think you need to explicitly make it case-insensitive in the code.

Comment 13

[:aceman]

Attachment: patch

OK, so can you try this patch?

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to Jonathan Kamens from comment #12)
> I don't know why I am seeing different results from y'all. I'm frankly not
> sure it matters. If I'm seeing different results, then other people are

Well it would be good to know what the difference is, and if it's a bug, get it fixed (eventually at least). 

Are you using the nightly binaries from mozilla.org?
I'm on ubuntu, and it's not a problem for me either.

Comment 15

[:aceman]

Yes, I would also like to know what is wrong here. If it is a problem in localeCompare or something. We can add the .toLocaleLowercase() as in the patch, but it may be wasted CPU cycles.

But as I said, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare does not seem to guarantee case insensitiveness. The default "sensitivity" is "variant" for usage "sort" (but we do not yet request "usage: sort").

Jonathan, can you try:
"Inbox".localeCompare("archive", "", {usage:"sort", sensitivity: "accent"})
"Inbox".localeCompare("archive", "C", {usage:"sort", sensitivity: "accent"})
"Inbox".localeCompare("archive", "en-US", {usage:"sort", sensitivity: "accent"})

Comment 16

[Jonathan Kamens]

(In reply to :aceman from comment #13)
> OK, so can you try this patch?

Patch works, thank you.

(In reply to Magnus Melin from comment #14)
> Well it would be good to know what the difference is, and if it's a bug, get
> it fixed (eventually at least). 

I don't disagree that it would be useful to understand why it's behaving differently for me than it is for others. But given that whether or not a locale sorts case-insensitively by default is defined by the locale, if indeed the desired behavior in Thunderbird is for the list to always be sorted insensitively regardless of the locale setting, then that's how it should be coded, independent of figuring out the anomaly we're discussing here..
 
> Are you using the nightly binaries from mozilla.org?
> I'm on ubuntu, and it's not a problem for me either.

I was using a build I did myself, but I went ahead and downloaded and tested a nightly build yesterday and it has the same problem.

(In reply to :aceman from comment #15)
> Jonathan, can you try:
> "Inbox".localeCompare("archive", "", {usage:"sort", sensitivity: "accent"})
> "Inbox".localeCompare("archive", "C", {usage:"sort", sensitivity: "accent"})
> "Inbox".localeCompare("archive", "en-US", {usage:"sort", sensitivity:
> "accent"})

All of these return -24 when I evaluate them in the error console.

Comment 17

[:aceman]

(In reply to Jonathan Kamens from comment #16)
> I don't disagree that it would be useful to understand why it's behaving
> differently for me than it is for others. But given that whether or not a
> locale sorts case-insensitively by default is defined by the locale, if
> indeed the desired behavior in Thunderbird is for the list to always be
> sorted insensitively regardless of the locale setting, then that's how it
> should be coded, independent of figuring out the anomaly we're discussing
> here..
>  
> (In reply to :aceman from comment #15)
> > Jonathan, can you try:
> > "Inbox".localeCompare("archive", "", {usage:"sort", sensitivity: "accent"})
> > "Inbox".localeCompare("archive", "C", {usage:"sort", sensitivity: "accent"})
> > "Inbox".localeCompare("archive", "en-US", {usage:"sort", sensitivity:
> > "accent"})
> 
> All of these return -24 when I evaluate them in the error console.

But if I read the docs page correctly, we should be using ["Inbox".localeCompare("archive", "", {usage:"sort", sensitivity: "accent"})] instead of forcing .toLocaleLowerCase, but it is still not working for you.

The page says the options arguments are there since Gecko 29 so maybe they are not polished yet.
Neil, do you know who the right people are to look at this? Maybe some more extensive tests should be added to http://mxr.mozilla.org/comm-central/source/mozilla/js/src/jit-test/tests/basic/testLocaleCompare.js .

Comment 18

[Magnus Melin [:mkmelin]]

Adding Norbert from bug 769871. Any ideas, or is it just a bug?

Comment 19

[:aceman]

Looking at bug 853301, do we actually have this properly enabled in TB?

Comment 20

[Norbert Lindenberg]

Adding Jeff Walden, the current owner of the ECMAScript Internationalization API in Mozilla. He's more familiar with the current state of the build options for this API than I.

Comment 16 tells me that this functionality is not enabled in the builds you're using: If localeCompare followed the specification for the ECMAScript Internationalization API, it would throw exceptions for the language tags "" and "C" because they're not valid BCP 47 language tags ("en-US" is valid). This means you're using an older implementation.

Also, please check what the file systems you work with do for case-insensitive comparison before relying on localeCompare or localeToUpperCase. I'm not sure whether file systems track locales and, for example, treat i and İ as equivalent, and i and I as different, in Turkish.

Comment 21

[:aceman]

(In reply to Norbert Lindenberg from comment #20)
> Comment 16 tells me that this functionality is not enabled in the builds
> you're using: If localeCompare followed the specification for the ECMAScript
> Internationalization API, it would throw exceptions for the language tags ""
> and "C" because they're not valid BCP 47 language tags ("en-US" is valid).
> This means you're using an older implementation.

How should we skip the second argument (meaning use the current app locale). The docs page says to leave it "undefined".

> Also, please check what the file systems you work with do for
> case-insensitive comparison before relying on localeCompare or
> localeToUpperCase. I'm not sure whether file systems track locales and, for
> example, treat i and İ as equivalent, and i and I as different, in Turkish.
Why? The function should be filesystem independent and observe the spec (ICU or something). What would be the point of it depending on the filesystem?

Comment 22

[Norbert Lindenberg]

(In reply to :aceman from comment #21)

> How should we skip the second argument (meaning use the current app locale).
> The docs page says to leave it "undefined".

Depends on what you mean by "app": If you mean the locale of the JavaScript app, it would have to know that by its BCP 47 language tag and pass it in. If you mean the containing app (Thunderbird), pass in undefined, and make sure that the default locale for the ECMAScript Internationalization API is determined by the locale of the containing app (as it is in Firefox).

> > Also, please check what the file systems you work with do for
> > case-insensitive comparison before relying on localeCompare or
> > localeToUpperCase. I'm not sure whether file systems track locales and, for
> > example, treat i and İ as equivalent, and i and I as different, in Turkish.
> Why? The function should be filesystem independent and observe the spec (ICU
> or something). What would be the point of it depending on the filesystem?

I had the impression that you want to achieve behavior that's compatible with that of the file system. If that's not the case, then please ignore my comment. But then the entire discussion of case sensitive vs. case insensitive seems irrelevant - a reasonable locale sensitive sort order will always sort strings next to each other if they only differ in case, and for the user interface that's all that matters.

Comment 23

[:aceman]

(In reply to Norbert Lindenberg from comment #22)
> (In reply to :aceman from comment #21)
> 
> > How should we skip the second argument (meaning use the current app locale).
> > The docs page says to leave it "undefined".
> 
> Depends on what you mean by "app": If you mean the locale of the JavaScript
> app, it would have to know that by its BCP 47 language tag and pass it in.
> If you mean the containing app (Thunderbird), pass in undefined, and make
> sure that the default locale for the ECMAScript Internationalization API is
> determined by the locale of the containing app (as it is in Firefox).
> 
> > > Also, please check what the file systems you work with do for
> > > case-insensitive comparison before relying on localeCompare or
> > > localeToUpperCase. I'm not sure whether file systems track locales and, for
> > > example, treat i and İ as equivalent, and i and I as different, in Turkish.
> > Why? The function should be filesystem independent and observe the spec (ICU
> > or something). What would be the point of it depending on the filesystem?
> 
> I had the impression that you want to achieve behavior that's compatible
> with that of the file system. If that's not the case, then please ignore my
> comment. But then the entire discussion of case sensitive vs. case
> insensitive seems irrelevant - a reasonable locale sensitive sort order will
> always sort strings next to each other if they only differ in case, and for
> the user interface that's all that matters.
We want locale sensitive sort order that ignores case (if that is appropriate for the locale).
And we are trying to determine whether the reporter's locale is somehow "unreasonable" or whether .localeCompare actually works correctly. It now looks like we may not have the correct locale support compiled in TB yet for .localeCompare to work according to http://mxr.mozilla.org/comm-central/source/mozilla/js/src/jit-test/tests/basic/testLocaleCompare.js.

Does bug 853301 contain the changes that we need to apply to TB to use the new API? Is Firefox already using the new API?

Comment 24

[:aceman]

Attachment: patch v2

OK, I'll consolidate the sorting into a function so we can switch it all later when we find out how we can make use of the new .localeCompare features.

Comment 25

[[e-mail]]

Comment on attachment 8398137 [details] [diff] [review]
patch v2

>     let sortKey = a._folder.compareSortKeys(b._folder);
>     if (sortKey)
>       return sortKey;
>-    return a.text.localeCompare(b.text);
>+    return localeCompare(a.text, b.text);
...
>               return a.compareSortKeys(b) ||
>-                     a.prettyName.localeCompare(b.prettyName);
>+                     this.localeCompare(a.prettyName, b.prettyName);
compareSortKeys already compares both the special folder type and the name (case-insensitively, to boot). So we shouldn't be attempting a secondary sort...

>           recentFolders.sort(function sort_folders_by_name(a, b) {
>-            return a.label.localeCompare(b.label);
>+            return this.localeCompare(a.label, b.label);
"this" isn't what you think it is...

Comment 26

[:aceman]

(In reply to [e-mail] from comment #25)
> >               return a.compareSortKeys(b) ||
> >-                     a.prettyName.localeCompare(b.prettyName);
> >+                     this.localeCompare(a.prettyName, b.prettyName);
> compareSortKeys already compares both the special folder type and the name
> (case-insensitively, to boot). So we shouldn't be attempting a secondary
> sort...

So why is the secondary sort there? Maybe there are account types where comparesortkey is unimplemented? Or should I just remove it?

Comment 27

[[e-mail]]

I would just remove it.

Comment 28

[:aceman]

Attachment: patch v3

The 'this' inside of .sort() seems to work for me (it fetches the right function localeCompare) so I left it in.

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8398762 [details] [diff] [review]
patch v3

Review of attachment 8398762 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/folderUtils.jsm
@@ +225,5 @@
> + *
> + * @param aString1  first string to compare
> + * @param aString2  second string to compare
> + */
> +function localeCompare(aString1, aString2) {

Could we name it something that doesn't collide with an existing name, like folderCompare?

@@ +228,5 @@
> + */
> +function localeCompare(aString1, aString2) {
> +  // Ultimately we want to use something like this:
> +  // aString1.localeCompare(aString2, undefined, {usage:"sort", sensitivity: "accent"})
> +  // but for that TB needs to switch to the new internationalization API, see bug 981405.

It's unclear to me if we needed to do some kind of switchover. If we need some change, file a bug and reference that.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 8398762 [details] [diff] [review]
patch v3

Review of attachment 8398762 [details] [diff] [review]:
-----------------------------------------------------------------

Other than that, r=mkmelin I suppose. Seems to do what it should.

Comment 31

[:aceman]

Attachment: patch v4

Did you mean this?

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 8402342 [details] [diff] [review]
patch v4

Review of attachment 8402342 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, though re bug 992651 I'm still confused about why we'd need a config change, or rather if we do, how come not everyone of us need one?

Comment 33

[:aceman]

Actually the patch here works around the problem that we don't know why somebody needs the toLowerCase forced. Maybe it is because .localeCompare doesn't work 100% correctly, because TB does not yet use all the new functionality (the new API). Once that is done, we can do bug 992651 and .localeCompare should do everything needed automatically for us.

Comment 34

[[e-mail]]

Comment on attachment 8402342 [details] [diff] [review]
patch v4

>-          var aLabel = a.prettyName;
>-          var bLabel = b.prettyName;
>+          let aLabel = a.prettyName;
>+          let bLabel = b.prettyName;
Bah, I wondered what the noise was.

>+              return a.compareSortKeys(b)
Nit: missing ;

Comment 35

[:aceman]

Attachment: patch v4.1 [checked in in comment 36]

Thanks.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3e6e4aee7b4c

Comment 37

[:aceman]

(In reply to [e-mail] from comment #25)
> >           recentFolders.sort(function sort_folders_by_name(a, b) {
> >-            return a.label.localeCompare(b.label);
> >+            return this.localeCompare(a.label, b.label);
> "this" isn't what you think it is...

Maybe you were right. The function works fine when I tried Recent menu in the message list (context menu -> Move to). But it breaks the filter editor (move/copy to actions)...

Comment 38

[:aceman]

Attachment: patch for wrong 'this'

Comment 39

[[e-mail]]

Why not use .bind()?

Comment 40

[:aceman]

Attachment: patch for wrong 'this' v2

OK, like this?

Comment 41

[:aceman]

Attachment: patch for wrong 'this' v3

Or if you'd like it fancier :)

Comment 42

[:aceman]

Neil, can you please r- the other patch and comment why the chosen one is better?

Comment 43

[[e-mail]]

Comment on attachment 8405689 [details] [diff] [review]
patch for wrong 'this' v3

I'm not used to their semantics yet, that's all. (Admittedly I didn't try very hard to find out definitively what they are anyway.)

Comment 44

[[e-mail]]

Comment on attachment 8405689 [details] [diff] [review]
patch for wrong 'this' v3

Actually they used to do the equivalent of .bind() but that doesn't get optimised by whatever JIT we're using these days. However they've very recently been given special treatment so you should now prefer them to .bind().

Comment 45

[:aceman]

Yes, http://www.jandemooij.nl/blog/2014/04/11/fast-arrow-functions-in-firefox-31/

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c7590f41f8c3