278799 - Unifinder: Sorting events should use collator, precomputed keys

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20050111 Mozilla Sunbird/0.2RC2

Sorting events by strings should use locale collation order, not javascript
string comparison.  It should also compute expensive keys just once per sort,
rather than for every comparison.


Reproducible: Always

Steps to Reproduce:
1. Add events titled:  "Event1", "event2", "Meeting1", "meeting2"
2. sort events by title

Actual Results:  
Sorted by unicode character:  Event1 < Meeting1 < event2 < meeting2
Sort by calendar name (with patch from Bug 278621) is much slower than sort by
title 

Expected Results:  
Sort upper and lowercase next to each other: Event1 < event2 < Meeting1 < meeting2
Sort by calendar name should be nearly as fast as by title.

Comment 1

[gekacheka]

Attachment: unifinder.js patch: precompute sort keys, use collator from nsICollationFactory and locale

(patch --dry-run -l -p 2 -i file.patch)

Reorganize sorting to used precomputed sort keys and locale collator:

* modify treeView.cycleHeader to call treeView.sortCalendarEvents to sort.
* modify refreshEventTree to call treeView.sortCalendarEvents to sort.
* Add treeView.sortCalendarEvents which precomputes 
  collator (from LocaleService and CollationFactory), 
  sort keys (using getEventSortKey), 
  and start time for sort, 
  then sorts, then forgets sortKeys.
* Add treeView.sortKeyByEvent_cache_put, sortKeyByEvent_cache_get which
  index values, using event UID as hash code.  The UID may not be unique
  since an event may appear in more than one file, and may be modified in
  one of the files.   So if there is a collision, creates a linked list of
  entries.  But for speed does not create an entry for the common case
  where there is just one value.
* Add getEventSortKey(event) which computes the sort key of event based on
  selectedColumn.  String keys should not be null.
* modify compareEvents to get precomputed sort keys and use collator for
strings.
* eliminate compareStrings and nullToEmpty, as string collator is used instead.


Tested on 0.2 branch.  
* Titles now sort in collation order.
* Sorting events by calendar name is nearly as fast as by titles.

Comment 2

[Mostafa Hosseini]

gekacheka: Development on the branch has stopped. RC2 is out and if there are no
major complaints, we will make it the official 0.2 release. The patches might
work on the trunk code but there is a high possibility for them needing to be
rewritten because of the changes to the interfaces.
Meanwhile, we can use your help on getting the trunk code working again.

Comment 3

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 4

[Bas van den Bosch]

Both aspects of this bug (sorting by calendar and sorting by unicode) is still present with calendar0.8pre (2007111604) 

Comment 5

[Simon Paquet [:sipaq]]

Gekacheka, is this patch still needed or can the bug be closed?

Comment 6

[gekacheka]

Attachment: v2: unifinder.js patch: precompute sort keys, use collator from nsICollationFactory and locale

(patch -l -p 1 -i file.patch)

Updated patch.

i18n: Collation is also important for languages that use non-ASCII letters 
(e.g., javascript '<' sorts by unicode order, which puts accented chars after all ASCII chars, not next to the unaccented form.)

Comment 7

[Michael Büttner (no reviews TFN)]

Comment on attachment 295575 [details] [diff] [review]
v2: unifinder.js patch: precompute sort keys, use collator from nsICollationFactory and locale

I couldn't find anything to complain about. Thanks for the patch. r=mickey.

Comment 8

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 10

[Andreas Treumann]

Checked in nightly build 2008021718 -> task is fixed and verified.