Closed
Bug 913976
Opened 11 years ago
Closed 7 years ago
Contacts: Refactor search results sorting code
Categories
(Core Graveyard :: DOM: Contacts, defect)
Core Graveyard
DOM: Contacts
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: reuben, Assigned: reuben)
Details
Attachments
(2 files, 4 obsolete files)
3.60 KB,
patch
|
cpeterson
:
review+
|
Details | Diff | Splinter Review |
5.78 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #801306 -
Flags: review?(anygregor)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #801307 -
Flags: review?(cpeterson)
Comment 3•11 years ago
|
||
Comment on attachment 801307 [details] [diff] [review]
2- Refactor contacts search result sorting code on Android
Review of attachment 801307 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM!
::: mobile/android/base/ContactService.java
@@ +829,5 @@
> mSortBy = sortBy.toLowerCase();
> mSortOrder = sortOrder.toLowerCase();
> }
>
> + private String flattenJSONArray(JSONArray array) {
The flattenJSONArray() method can be static because it is a stateless helper function.
@@ +830,5 @@
> mSortOrder = sortOrder.toLowerCase();
> }
>
> + private String flattenJSONArray(JSONArray array) {
> + StringBuilder result = new StringBuilder();
Micro-optimization time! :) StringBuilder's default capacity is 16 characters. If the JSONArrays of family and given names we flatten are typically larger than 16 characters, we can give a better capacity hint. Maybe something like `array.length() * 20` with a comment?
Attachment #801307 -
Flags: review?(cpeterson) → review+
Comment 4•11 years ago
|
||
Comment on attachment 801306 [details] [diff] [review]
1- Refactor contacts search results sorting code
Review of attachment 801306 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/contacts/fallback/ContactDB.jsm
@@ +934,5 @@
> +
> + // Accumulate a single string from the contacts' sort properties, eg.:
> + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith"
> + // when sorting by "givenName", and "smithmaryann" when sorting by
> + // "familyName".
I don't think that works.
How about
given: ["Test1", "Test2"], family: ["Test3"]
and given: ["Test1"], family: ["Test2", "Test3"]
The sort string would always look like "Test1Test2Test3"
We used this approach in the beginning but it didn't support all use-cases.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #4)
> Comment on attachment 801306 [details] [diff] [review]
> 1- Refactor contacts search results sorting code
>
> Review of attachment 801306 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +934,5 @@
> > +
> > + // Accumulate a single string from the contacts' sort properties, eg.:
> > + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith"
> > + // when sorting by "givenName", and "smithmaryann" when sorting by
> > + // "familyName".
>
> I don't think that works.
> How about
> given: ["Test1", "Test2"], family: ["Test3"]
> and given: ["Test1"], family: ["Test2", "Test3"]
I can just add a space between given and family name. Also, we should have tests for this.
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #3)
> Comment on attachment 801307 [details] [diff] [review]
> 2- Refactor contacts search result sorting code on Android
>
> Review of attachment 801307 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> LGTM!
Thanks for the review!
> ::: mobile/android/base/ContactService.java
> @@ +829,5 @@
> > mSortBy = sortBy.toLowerCase();
> > mSortOrder = sortOrder.toLowerCase();
> > }
> >
> > + private String flattenJSONArray(JSONArray array) {
>
> The flattenJSONArray() method can be static because it is a stateless helper
> function.
Inner classes cannot define static methods or attributes.
> @@ +830,5 @@
> > mSortOrder = sortOrder.toLowerCase();
> > }
> >
> > + private String flattenJSONArray(JSONArray array) {
> > + StringBuilder result = new StringBuilder();
>
> Micro-optimization time! :) StringBuilder's default capacity is 16
> characters. If the JSONArrays of family and given names we flatten are
> typically larger than 16 characters, we can give a better capacity hint.
> Maybe something like `array.length() * 20` with a comment?
Will do.
Comment 7•11 years ago
|
||
(In reply to Reuben Morais [:reuben] from comment #5)
> (In reply to Gregor Wagner [:gwagner] from comment #4)
> > Comment on attachment 801306 [details] [diff] [review]
> > 1- Refactor contacts search results sorting code
> >
> > Review of attachment 801306 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: dom/contacts/fallback/ContactDB.jsm
> > @@ +934,5 @@
> > > +
> > > + // Accumulate a single string from the contacts' sort properties, eg.:
> > > + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith"
> > > + // when sorting by "givenName", and "smithmaryann" when sorting by
> > > + // "familyName".
> >
> > I don't think that works.
> > How about
> > given: ["Test1", "Test2"], family: ["Test3"]
> > and given: ["Test1"], family: ["Test2", "Test3"]
>
> I can just add a space between given and family name. Also, we should have
> tests for this.
Yes we should have :)
Assignee | ||
Comment 8•11 years ago
|
||
Updated to work in the case you mentioned, plus tests.
Attachment #801306 -
Attachment is obsolete: true
Attachment #801306 -
Flags: review?(anygregor)
Attachment #812124 -
Flags: review?(anygregor)
Comment 9•11 years ago
|
||
Comment on attachment 812124 [details] [diff] [review]
1- Refactor contacts search results sorting code, v2
We found that we have to make some changes here.
Attachment #812124 -
Flags: review?(anygregor)
Updated•11 years ago
|
Component: DOM: Device Interfaces → DOM: Contacts
Assignee | ||
Comment 10•11 years ago
|
||
We'll have to do something about this eventually, specially because the Android sorting code doesn't match ContactDB exactly. For now let's just land the tests.
Attachment #812124 -
Attachment is obsolete: true
Attachment #8364655 -
Flags: review?(anygregor)
Comment 11•11 years ago
|
||
Comment on attachment 8364655 [details] [diff] [review]
Just the tests
Review of attachment 8364655 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/contacts/tests/test_contacts_basics2.html
@@ -438,5 @@
> - next();
> - };
> - req.onerror = onFailure;
> - },
> - function () {
Well you are removing the test-case that deals with contacts with identical fields and you replace it with tests that do the sorting based on the names. I don't think we want this.
Attachment #8364655 -
Flags: review?(anygregor)
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Well you are removing the test-case that deals with contacts with identical
> fields and you replace it with tests that do the sorting based on the names.
> I don't think we want this.
What do you mean? The removed test made sure tests with the same sort property (in this case, givenName), were sorted by the published field. The first test I added does the same.
Comment 13•11 years ago
|
||
(In reply to Reuben Morais [:reuben] from comment #12)
> (In reply to Gregor Wagner [:gwagner] from comment #11)
> > Well you are removing the test-case that deals with contacts with identical
> > fields and you replace it with tests that do the sorting based on the names.
> > I don't think we want this.
>
> What do you mean? The removed test made sure tests with the same sort
> property (in this case, givenName), were sorted by the published field. The
> first test I added does the same.
It does something similar but not the same. The original test does sorting with identical contacts. The new test does it with empty sort string fields.
Assignee | ||
Comment 14•11 years ago
|
||
OOOOOH, I see what you mean. This was definitely not intended :)
Interdiff:
diff --git a/dom/contacts/tests/test_contacts_basics2.html b/dom/contacts/tests/test_contacts_basics2.html
--- a/dom/contacts/tests/test_contacts_basics2.html
+++ b/dom/contacts/tests/test_contacts_basics2.html
@@ -435,17 +435,17 @@ var steps = [
{ name: ["8"], givenName: ["a"] },
];
saveContacts(contacts, function() {
- var options = {sortBy: "familyName",
+ var options = {sortBy: "givenName",
sortOrder: "ascending"};
req = navigator.mozContacts.find(options);
req.onsuccess = function () {
is(req.result.length, 8, "8 results");
Attachment #8364655 -
Attachment is obsolete: true
Attachment #8364857 -
Flags: review?(anygregor)
Comment 15•11 years ago
|
||
Comment on attachment 8364857 [details] [diff] [review]
Just the tests, v2
Review of attachment 8364857 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/contacts/tests/test_contacts_basics2.html
@@ +435,5 @@
> + // props, but they're equal.
> + { name: ["5"], givenName: ["a"] },
> + { name: ["6"], givenName: ["a"] },
> + { name: ["7"], givenName: ["a"] },
> + { name: ["8"], givenName: ["a"] },
Now there is just one more difference :) We iterate over all properties with the old version. We take given and family name afaik. With the new version we just compare one single property.
Assignee | ||
Comment 16•11 years ago
|
||
Oh wow, I completely forgot about this patch :(
Attachment #8364857 -
Attachment is obsolete: true
Attachment #8364857 -
Flags: review?(anygregor)
Attachment #8410949 -
Flags: review?(anygregor)
Updated•11 years ago
|
Attachment #8410949 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 17•7 years ago
|
||
This code no longer exists.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•