883269 - [contacts] characters with non-ascii characters (like umlauts) are sorted always into first positions for letter

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Contacts, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Unagi with Gaia 1.1.0.0-prerelease 20130614070213

Contacts with non-ASCII character like umlauts (ä, ö, ü), accents (á) etc. are always sorted in front of the other contacts with the same leading letter of the contact name and only containing ascii characters.

Example:

Actual sort order:
Jörg Amigo
Julian Schönwetter
Jana Prima

Expected sort order:
Jana Prima
Jörg Amigo
Julian Schönwetter

Comment 1

[Gregor Wagner [:gwagner]]

What's the language on your device?
The sorting depends on your language selection. If you have selected a language where the 'umlauts' are not part of the language you will get the strange sorting behavior.

Comment 2

[Gregor Wagner [:gwagner]]

I just tried it with german and I get your expected sort order.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The language is set to German. Even after switching to English, closing the Contacts app, launching it, closing it, switching back to German and opening the Contacts, the issue persists.

Comment 4

[Gregor Wagner [:gwagner]]

The sort order is cached. We have to clear the sorting-cache if we change the language.

Comment 5

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Attachment: Pull Request #10764 - Sort contacts in list

Add contacts to the group list in order and not just append them.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 770735 [details]
Pull Request #10764 - Sort contacts in list

Tried on the phone, working fine.

Thanks Mihai, could you squash your commits and land this?

Comment 7

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Thanks Francisco! Landed on master:
https://github.com/mozilla-b2g/gaia/commit/b068bef2bfbb759271d4446afa5d4b2207c44ae9

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry I didn't see this bug until now, but I have some concerns about the final solution:

1) We're sorting twice on the device; once in gecko and once in gaia.  We now ignore the gecko sorting.

2) This is going to add a lot of CPU usage when loading a large list.  The addToGroup() function does a very simple bubble sort which is effectively O(n^2).  This is ok for adding one contact here and there, but is probably inadequate for loading a large list.  This change now uses this sort for the bulk loading of contacts at start.

Mihai, have you tested this with the reference workloads?  Try "make reference-workload-heavy" to install 1000 contacts on the phone.  You may need to add debug to see how long the load takes.

Out of curiousity, is there a reason we did not pursue fixing the sort caching issue in gecko mentioned in comment 4?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I just tested master with the reference-workload-heavy data set.  With this change scrolling while data is loading is now janky.

I'm very sorry, but I really think this should be backed out and the sort caching issue should be fixed in gecko.

Comment 10

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

After chatting with Ben on IRC, we agreed that it's best if this patch is reverted due to performance concerns and the gecko approach is pursued first.

Thanks for checking all this, Ben!

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Commit reverted:

  https://github.com/mozilla-b2g/gaia/commit/6d79318e980613b65838dfb990a2cd75a9c0e1ff

Thanks for understanding my concerns Mihai!

Gregor, Reuben, what are your thoughts on fixing the sort cache for language change issue?  I can take a look on Monday if you don't have the bandwidth.

Comment 12

[Reuben Morais [:reuben]]

Huh, wth. This bug fell of the cracks, I'm pretty sure I had a patch for it before I lost my data…

Comment 13

[Reuben Morais [:reuben]]

So I have a patch to invalidate our sorting cache when the language changes, but it doesn't help, because we're using String.prototype.localeCompare in ContactsDB and the locale in the parent is always set to en_us/C. (Yay!)

Comment 14

[Alex Keybl [:akeybl]]

No longer blocking for 1.1, we agreed in triage that this is only a major issue if the contact starts with a non-ascii character, which will be uncommon.

Comment 15

[Reuben Morais [:reuben]]

Unassigning until this is actionable again.

Comment 16

[Julien Wajsberg [:julienw]]

Is there already a bug to support the 2 additional arguments to localeCompare (see MDN [1]) ?

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare

Comment 17

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Is there already a bug to support the 2 additional arguments to
> localeCompare (see MDN [1]) ?
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/String/localeCompare

That would be Bug 866301.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Clearing the perf keyword since this is currently a functional problem.  The perf keyword was only added because the attempted solution created a perf problem.  Any final solution should be cognizant of performance impact (like all changes), but this is not directly a performance issue.

Comment 19

[Julien Wajsberg [:julienw]]

Looks like we have localeCompare with the additional arguments now. This bug could be actionable now ?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Contacts API is gone.