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

RESOLVED FIXED

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gecko, Assigned: Kay Lee)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Component: General → Gaia::Contacts
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 735031 [details]
Image file of bug851330 before patch.

Number is published-orderd.
(Assignee)

Comment 3

5 years ago
Created attachment 735032 [details]
Image file of bug851330 after patch.

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

Comment 4

5 years ago
Created attachment 735035 [details] [diff] [review]
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)
Attachment #735035 - Flags: review?(anygregor)
(Assignee)

Comment 5

5 years ago
Created attachment 735037 [details] [diff] [review]
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)
Attachment #735037 - Flags: review?(anygregor)
(Assignee)

Comment 6

5 years ago
(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)
(Assignee)

Comment 7

5 years ago
(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)
(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
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.
Ok ignore my earlier comment :) I was looking at the wrong patch.
Can you add a testcase for it?
(Assignee)

Comment 11

5 years ago
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?
(Assignee)

Comment 12

5 years ago
Created attachment 768116 [details] [diff] [review]
[patch] Same contacts are sorted by published-order.

It's patch for bug851440.
Attachment #735035 - Attachment is obsolete: true
Attachment #735037 - Attachment is obsolete: true
Attachment #735035 - Flags: review?(anygregor)
Attachment #735037 - Flags: review?(anygregor)
Attachment #768116 - Flags: review?(anygregor)
(Assignee)

Comment 13

5 years ago
Created attachment 768118 [details] [diff] [review]
[testcase] The testcase for this patch

I'd like you to check the testcase for this patch.
Attachment #768118 - Flags: review?(anygregor)
Assignee: nobody → km.lee009
(Assignee)

Comment 14

5 years ago
How is the process going with this issue?
Attachment #768118 - Attachment is patch: true
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);
Attachment #768116 - Flags: review?(anygregor)
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.
Attachment #768118 - Flags: review?(anygregor) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 778370 [details] [diff] [review]
[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.
Attachment #768116 - Attachment is obsolete: true
Attachment #778370 - Flags: review?(anygregor)
(Assignee)

Comment 18

5 years ago
Created attachment 778371 [details] [diff] [review]
[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.
Attachment #768118 - Attachment is obsolete: true
Attachment #778371 - Flags: review?(anygregor)
Attachment #778371 - Flags: review?(anygregor) → review+
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.
(Assignee)

Comment 20

5 years ago
Created attachment 782881 [details] [diff] [review]
bug851440_ContactDB_v2.patch

[patch_v2] Same contacts are sorted by publishsed-order.
it declares px and py locally.
Attachment #778370 - Attachment is obsolete: true
Attachment #778370 - Flags: review?(anygregor)
Attachment #782881 - Flags: review?(anygregor)
Comment on attachment 782881 [details] [diff] [review]
bug851440_ContactDB_v2.patch

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

Thanks!
Attachment #782881 - Flags: review?(anygregor) → review+
Whiteboard: checkin-needed
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
Flags: in-testsuite+
Whiteboard: checkin-needed

Updated

5 years ago
Depends on: 906004
You need to log in before you can comment on or make changes to this bug.