Closed Bug 734198 Opened 9 years ago Closed 9 years ago

Contacts API: Add Sorting

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: gwagner, Assigned: gwagner)

Details

Attachments

(1 file, 2 obsolete files)

We should support sorting by field for find results. Like { sortBy: familyName, sortOrder: descending}.
Assignee: nobody → anygregor
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #611097 - Attachment is obsolete: true
Attachment #611559 - Flags: review?(bent.mozilla)
Comment on attachment 611559 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +377,5 @@
>        }
>      }
>  
> +    // Sorting functions takes care of limit if set.
> +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;

Hm, should that be testing against 'undefined'? Looks like you always define it to '' if it wasn't passed in...

Also, any time you are going to do something based on both branches of an if statement (in this case the ? operator) I think it's nicer to test against a non-negated condition (e.g. |options.sortBy === 'undefined' ? options.filterLimit : null|).

@@ +413,5 @@
>      debug("ContactDB:_findAll:  " + JSON.stringify(options));
>      if (!txn.result)
>        txn.result = {};
> +    // Sorting functions takes care of limit if set.
> +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;

here too.
(In reply to ben turner [:bent] from comment #3)
> Comment on attachment 611559 [details] [diff] [review]
> patch
> 
> Review of attachment 611559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +377,5 @@
> >        }
> >      }
> >  
> > +    // Sorting functions takes care of limit if set.
> > +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;
> 
> Hm, should that be testing against 'undefined'? Looks like you always define
> it to '' if it wasn't passed in...
> 
> Also, any time you are going to do something based on both branches of an if
> statement (in this case the ? operator) I think it's nicer to test against a
> non-negated condition (e.g. |options.sortBy === 'undefined' ?
> options.filterLimit : null|).

Actually the options object isn't created by the constructor. So the values are undefined.
Attached patch patchSplinter Review
Attachment #611559 - Attachment is obsolete: true
Attachment #611622 - Flags: review?(bent.mozilla)
Attachment #611559 - Flags: review?(bent.mozilla)
Comment on attachment 611622 [details] [diff] [review]
patch

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

::: dom/contacts/ContactManager.js
@@ +18,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
>  
> +XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {

This change doesn't seem strictly related, but ok.
Attachment #611622 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/3ad6cc527d5c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.