734198 - Contacts API: Add Sorting

Status: RESOLVED FIXED

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

Description

[Gregor Wagner [:gwagner]]

We should support sorting by field for find results. Like { sortBy: familyName, sortOrder: descending}.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Gregor Wagner [:gwagner]]

(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.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 7

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad6cc527d5c

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3ad6cc527d5c