localeCompare() returns incorrect values when invoked in a notifier

[fbennett]

I think there is a problem in itemTreeView.js, in the madeChanges block at lines 500 to 584 of the current revision (rr8425). The function invokes this.sort(), which ultimately invokes localeCompare() on strings for comparison. When invoked from the main thread (?), localeCompare() returns correct values, so "An Apple" comes before "A Pear" in a sort. But when invoked via the notifier, the sort comes out the other way.

The attached screenshot traces the relevant functions. Everything after the == SORT AFTER CHANGES === line comes from the notifier invocation. The string comparisons outlined in yellow are for the same two strings, first in one sequence ordering, then in the other. One of the comparisons should be 1, but both are -1.

In the notifier invocation, localeCompare() is apparently (judging from what I see on the screen) performing a straight binary string sort, so "A Pear" would come out before "An Apple" (i.e. the space is treated as significant).

The difference bubbles up into the UI when an item is dropped onto a collection. The center pane then gets resorted in binary string order, so the user sees a change in the listing even though the operation hasn't touched the content of entries.

As a workaround (possibly a fix, but I don't know the code well enough to be sure), I've replaced all this.sort() invocations inside the notifier with this_needsSort = true. This seems to trigger a sort operation (there is a pause after dropping the item from a largeish library), and the ordering returned is the same as we get from a fresh visit to the target collection or library (so the user sees no change).

I'm not sure if this has any bad side effects, but it seems promising as a starting point.

(I've tested this only in the multilingual branch, but I can't see anything in the code that would otherwise account for this; and the comparison values shown in the screenshot come directly from localeCompare(), so there doesn't seem to be a mistake there.)

[dstillman]

Can you provide steps to reproduce? I'm not clear on what the issue is.

[fbennett]

... and the lesson for the day is "Never, never, ever trust localeCompare()".

First point: No problem in the trunk, and so closing this ill-fated ticket.

Second point: In the multilingual branch, when I shift back to using collation.compareString(1 , a, b) as the trunk does, the problem also goes away.

Sorry for the extra traffic; I'm relieved now that you were skeptical.