Closed
Bug 851440
Opened 12 years ago
Closed 12 years ago
[Contact] Contacts with the same names and numbers are randomly sorted.
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: leo.bugzilla.gecko, Assigned: km.lee009)
References
Details
Attachments
(4 files, 5 obsolete files)
31.15 KB,
image/png
|
Details | |
30.85 KB,
image/png
|
Details | |
3.11 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Component: General → Gaia::Contacts
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.
Contacts are sorted by Given or Family name and then by published-order.
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)
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)
(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)
(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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Ok ignore my earlier comment :) I was looking at the wrong patch.
Can you add a testcase for it?
Assignee | ||
Comment 11•12 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•12 years ago
|
||
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•12 years ago
|
||
I'd like you to check the testcase for this patch.
Attachment #768118 -
Flags: review?(anygregor)
Updated•12 years ago
|
Assignee: nobody → km.lee009
Assignee | ||
Comment 14•12 years ago
|
||
How is the process going with this issue?
Updated•12 years ago
|
Attachment #768118 -
Attachment is patch: true
Comment 15•12 years ago
|
||
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 16•12 years ago
|
||
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•12 years ago
|
||
[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•12 years ago
|
||
[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)
Updated•12 years ago
|
Attachment #778371 -
Flags: review?(anygregor) → review+
Comment 19•12 years ago
|
||
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•12 years ago
|
||
[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 21•12 years ago
|
||
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+
Updated•12 years ago
|
Whiteboard: checkin-needed
Comment 22•12 years ago
|
||
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
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a68749afae0
https://hg.mozilla.org/mozilla-central/rev/d47539d6c15f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•