851440 - [Contact] Contacts with the same names and numbers are randomly sorted.

Status: RESOLVED FIXED

Description

[leo.bugzilla.gecko]

Issue cases>
case I
Incoming call, Dialer app can't display the contact published first among the same contacts.
case II
The same contacts are not listed by published-orderd in Contact App.

Reason>
If contacts have same values, they are sorted by "id" value randomly created.

Other OS scenario>
In Android and Mac OS, the same contacts are sorted by published value. 
So, Incoming call, Dialer app showes up the first published contact.

Comment 1

[Kay Lee]

In addtion to bug851440 issue, there is other issue.
If each contact has same text in other name field, it is not able to be listed regularly.

For example, there are 4 contacts, as below.
Contact A {GivenName: "mozilla", FamilyName: " "}
Contact B {GivenName: "mozilla", FamilyName: " "}
Contact C {GivenName: " ", FamilyName: "mozilla"}
Contact D {GivenName: " ", FamilyName: "mozilla"}

sort result example>
mozilla (--> contact C)
mozilla (--> Contact A)
mozilla (--> Contact B)
mozilla (--> Contact D)

So, I'll attach the proposed patch to fix these issues and two image file(before and after patch).
Please, check this patch.

Comment 2

[Kay Lee]

Attachment: Image file of bug851330 before patch.

Number is published-orderd.

Comment 3

[Kay Lee]

Attachment: Image file of bug851330 after patch.

Contacts are sorted by Given or Family name and then by published-order.

Comment 4

[Kay Lee]

Attachment: It's patch to fix these issues.

Please check this proposed patch.
But This patch(bug851440_v1.patch) inclued the patch for Bug 852057 (https://bugzilla.mozilla.org/show_bug.cgi?id=852057).

Already covering Bug 852057, you'd better to apply patch v2.(bug851440_v2.patch)

Comment 5

[Kay Lee]

Attachment: It's patch ver.2 to fix only bug 851440,(except bug 852057)

It's patch ver.2 to fix only bug 851440,(except bug 852057)

You'd better apply this patch(bug851440_v2.patch, if bug 852057 is already fixed(https://bugzilla.mozilla.org/show_bug.cgi?id=852057)

Comment 6

[Kay Lee]

(In reply to Kay Lee from comment #2)
> Created attachment 735031 [details]
Image file of bug851330 before patch.
> Number is published-orderd.

bug851330 -> bug851440(mistake)

Comment 7

[Kay Lee]

(In reply to Kay Lee from comment #3)
> Created attachment 735032 [details]
Image file of bug851330 after patch.
> Contacts are sorted by Given or Family name and then by published-order.

bug851330 -> bug851440 (mistake)

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Kay Lee from comment #1)
> In addtion to bug851440 issue, there is other issue.
> If each contact has same text in other name field, it is not able to be
> listed regularly.
> 
> For example, there are 4 contacts, as below.
> Contact A {GivenName: "mozilla", FamilyName: " "}
> Contact B {GivenName: "mozilla", FamilyName: " "}
> Contact C {GivenName: " ", FamilyName: "mozilla"}
> Contact D {GivenName: " ", FamilyName: "mozilla"}
> 
> sort result example>
> mozilla (--> contact C)
> mozilla (--> Contact A)
> mozilla (--> Contact B)
> mozilla (--> Contact D)
> 
> So, I'll attach the proposed patch to fix these issues and two image
> file(before and after patch).
> Please, check this patch.

Well sorting a list with repeating words will always result in a random result. If we want the API to fix this problem we have to include it in the API specification and it has to go in standardization process.
My opinion is that we are solving a non-problem here and make the sorting function more complicated for no reason but if people want this in the standard we can add it.
The right place for discussion:
http://lists.w3.org/Archives/Public/public-sysapps/2013Apr/subject.html

Comment 9

[Gregor Wagner [:gwagner]]

I looked at your patch again but you don't look at the published field for comparison if the names are not sortable. A testcase would be nice as well :)
So I think if we can't sort just sort based on the 'published' field.

Comment 10

[Gregor Wagner [:gwagner]]

Ok ignore my earlier comment :) I was looking at the wrong patch.
Can you add a testcase for it?

Comment 11

[Kay Lee]

Sorry for late response.
Please let me know how to run the contact testcase on my target device.(In reply to Gregor Wagner [:gwagner] from comment #10)
> Ok ignore my earlier comment :) I was looking at the wrong patch.
> Can you add a testcase for it?

Comment 12

[Kay Lee]

Attachment: [patch] Same contacts are sorted by published-order.

It's patch for bug851440.

Comment 13

[Kay Lee]

Attachment: [testcase] The testcase for this patch

I'd like you to check the testcase for this patch.

Comment 14

[Kay Lee]

How is the process going with this issue?

Comment 15

[Gregor Wagner [:gwagner]]

Comment on attachment 768116 [details] [diff] [review]
[patch] Same contacts are sorted by published-order.

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +763,5 @@
>        const sortBy = aFindOptions.sortBy == "familyName" ? [ "familyName", "givenName" ] : [ "givenName" , "familyName" ];
>  
>        aResults.sort(function (a, b) {
> +        let x, y, px, py;
> +        let x, y, published_priority = 0;

Please clean up this part. Don't redefine x and y.

@@ +784,5 @@
>              if (y) {
>                y = y.join("").toLowerCase();
>              }
>              yIndex++;
>            }

Please fix the indentation for the rest of the patch.

@@ +796,5 @@
> +		  }
> +		  if (!x) {
> +			  if (!y) {
> +				  if (name_priority !=0) {
> +					  return name_priority;

Can't we move the name_priority computation from above to here? I think the code becomes simpler and easier to read.

@@ +804,5 @@
> +				  py = JSON.stringify(b.published);
> +
> +				  if (px && py) {
> +					  published_priority = px.localeCompare(py);
> +					  return published_priority;

No need to use a extra variable here. Just do
return px.localeCompare(py);

Comment 16

[Gregor Wagner [:gwagner]]

Comment on attachment 768118 [details] [diff] [review]
[testcase] The testcase for this patch

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

::: dom/contacts/tests/test_contacts_basics.html
@@ +1337,5 @@
> +    var options = {sortBy: "familyName",
> +                   sortOrder: "descending"};
> +    req = navigator.mozContacts.find(options);
> +    req.onsuccess = function () {
> +      is(req.result.length, 3, "3 results");

Please fix indentation.

@@ +1340,5 @@
> +    req.onsuccess = function () {
> +      is(req.result.length, 3, "3 results");
> +  	  ok(true, req.result[0].published);
> +	  ok(true, req.result[1].published);
> +	  ok(true, req.result[2].published);

This doesn't test anything.
You want something like
ok(req.result[0].published < req.result[1].published, "Right sorting order");

'ok' only evaluates the first argument and ok(true, ...) will always pass.

Comment 17

[Kay Lee]

Attachment: [new_patch] Same contacts are sorted by published-order.

[new_patch] Same contacts are sorted by published-order. 

After review, I attached new patch.
I guess I attached wrong patch before. Please check this patch.

Comment 18

[Kay Lee]

Attachment: [new_testcase] The testcase for this patch

[new_testcase] The testcase for this patch.
After your review, I changed the testcase code.
please check it again.

Comment 19

[Gregor Wagner [:gwagner]]

Comment on attachment 778370 [details] [diff] [review]
[new_patch] Same contacts are sorted by published-order.

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +821,5 @@
>        const sortOrder = aFindOptions.sortOrder;
>        const sortBy = aFindOptions.sortBy == "familyName" ? [ "familyName", "givenName" ] : [ "givenName" , "familyName" ];
>  
>        aResults.sort(function (a, b) {
> +        let x, y, px, py;

no need to add px and py here.

@@ +844,5 @@
>            }
> +          if (!x) {
> +            if (!y) {
> +              px = JSON.stringify(a.published);
> +              py = JSON.stringify(b.published);

declare px and py locally here.

Comment 20

[Kay Lee]

Attachment: bug851440_ContactDB_v2.patch

[patch_v2] Same contacts are sorted by publishsed-order.
it declares px and py locally.

Comment 21

[Gregor Wagner [:gwagner]]

Comment on attachment 782881 [details] [diff] [review]
bug851440_ContactDB_v2.patch

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

Thanks!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/1a68749afae0
https://hg.mozilla.org/projects/birch/rev/d47539d6c15f

Thanks for the patch, Kay! One request - please make sure that you have Mercurial configured per the directions below so that your patches have all the necessary commit information in them needed for checkin. It makes life much easier for those checking in on your behalf :-)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a68749afae0
https://hg.mozilla.org/mozilla-central/rev/d47539d6c15f