Closed Bug 874462 Opened 8 years ago Closed 8 years ago

[ContactsAPI] Consider renaming "contains" filterop to "startsWith"

Categories

(Core :: DOM: Device Interfaces, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bkelly, Assigned: bkelly)

References

()

Details

(Whiteboard: c= p=2 s=2013.06.14 ,)

Attachments

(1 file, 1 obsolete file)

Currently the ContactsAPI offers filtering using "equals" and "contains" operations.  For a number of applications a "startsWith" filter would also be useful.  For example:

  - Type ahead name completion in many apps
  - Initializing letter groups within jump lists such as used in the contacts app

It appears from talking to the Julien and Rick on the mms app that "contains" used to function this way.  See bug 874456.
From talking the Gregor, it appears "contains" is supposed to implement "startsWith" behavior.  The mms app may be running into a separate bug.

Updating this bug to rename "contains" to "startsWith".  Alternatively, we could make "startsWith" an alias in order to provide backward compatibility.
Summary: [ContactsAPI] Consider adding "startsWith" filter support to ContactsAPI → [ContactsAPI] Consider renaming "contains" filterop to "startsWith"
Whiteboard: c= → c= p=2
No longer blocks: 865747
This patch adds a "startsWith" alias to "contains".  We can then write a bug to fix "contains" if possible in the future.  This is backward compatible for the time being.
Attachment #753972 - Flags: review?(anygregor)
I've dropped "contains" from the ContactsAPI spec and added "startsWith" instead. I see no reason to keep "contains" in the *spec* for backwards compat. I'll leave it up to others to decided what to do with the implementation of "contains". Thanks. -t
Should be easy to support both contains/startsWith in gaia (so that we can use both gecko branches), so let's remove "contains" quickly in another bug, as soon as the gaia work to support both is done.

I'll file bugs for this.
Flags: needinfo?(felash)
(In reply to Tantek Çelik from comment #3)
> I've dropped "contains" from the ContactsAPI spec and added "startsWith"
> instead. I see no reason to keep "contains" in the *spec* for backwards
> compat. I'll leave it up to others to decided what to do with the
> implementation of "contains". Thanks. -t

Tantek, having just "startsWith" is not strictly correct either, since right now we do have actual contains search for telephone numbers.

The alias proposed in this bug helps clarify for users what the operation will actually do, but the long term solution seems to be either adding full text search to IDB, or waiting for the DataStore API and then deprecate "contains" altogether.
Comment on attachment 753972 [details] [diff] [review]
Add "startsWith" filterOp alias for "contains"

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

Stealing since gwagner is away.

::: dom/contacts/fallback/ContactDB.jsm
@@ +792,5 @@
>      if (DEBUG) debug("ContactDB:find val:" + aOptions.filterValue + " by: " + aOptions.filterBy + " op: " + aOptions.filterOp);
>      let self = this;
>      this.newTxn("readonly", STORE_NAME, function (txn, store) {
> +      let filterOps = ["equals", "contains", "match", "startsWith"];
> +      if (aOptions && (filterOps.indexOf(aOptions.filterOp) >= 0)) {

We should warn if "contains" is used and |filterBy != ["tel"]| instead of just silently ignoring it.

::: dom/contacts/tests/test_contacts_basics.html
@@ +510,1 @@
>                     filterValue: properties1.tel[1].value.substring(0,5)};

We should keep this as contains and change the substring bounds so we actually test the contains code path.

@@ +950,1 @@
>                     filterValue: properties2.tel[0].value.substring(0, 7)};

Same here.
Attachment #753972 - Flags: review?(anygregor) → review+
Thanks for the review Reuben!

I was just talking to Etienne at JSConf and he was telling me that we are phasing out the use of "contains" with "tel" in favor of "match".  Etienne, can you confirm?
Flags: needinfo?(etienne)
(In reply to Ben Kelly [:bkelly] (Vacation+JSConf till June 1) from comment #7)
> Thanks for the review Reuben!
> 
> I was just talking to Etienne at JSConf and he was telling me that we are
> phasing out the use of "contains" with "tel" in favor of "match".  Etienne,
> can you confirm?

I looked it up to confirm, and I was missing something.

We're using (and will be using for the foreseeable future) the "contains" search on ['tel'] to implement the suggestion bar feature in the dialer (when you type a number on the keypad it will autocomplete with matching contacts).

Sorry.
Flags: needinfo?(etienne)
Thanks Etienne.

So, what does this mean for the spec?  For our implementation it seems we need "contains", but currently only for "tel".  Is it reasonable to encode such a specific exceptional case or should we keep general "contains" in the spec?  Tantek, what do you think?
Flags: needinfo?(tantek)
Attached patch Updated patchSplinter Review
Updated patch based on feedback from :reuben.  Carrying forward r+ from attachment 753972 [details] [diff] [review].

Note, I used Console.jsm to provide the warning message.  From what I understand this is the new desired library for this sort of thing.
Attachment #753972 - Attachment is obsolete: true
Attachment #756989 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4adebbde763a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 884679
Whiteboard: c= p=2 → c= p=2 s=2013.06.14 ,
So I had a long due needinfo flag set by myself here.

I'm not sure of what has been done here now.

For everything except "tel", "contains" is really a "startsWith", and in Gaia code (on master only) we should use "startsWith" instead of "contains" for those cases ? Is that what was implemented here ?
Flags: needinfo?(felash) → needinfo?(bkelly)
(In reply to Julien Wajsberg [:julienw] from comment #13)
> For everything except "tel", "contains" is really a "startsWith", and in
> Gaia code (on master only) we should use "startsWith" instead of "contains"
> for those cases ? Is that what was implemented here ?

I believe that is correct.  You can now use "startsWith" instead of "contains".  The "contains" keyword will still work for non-"tel" fields, though.  It will generate a warning to the console log in those situations.  I would recommend converting to "startsWith" in gaia code since it best describes the operation being performed.

As far as I can tell, the different behavior of the tel field is not documented in the Contacts API spec.  Its unclear to me if the spec should be updated to match or if this is bug in our implementation of the spec that should be corrected in the long term.

Perhaps we should have a meeting at the next work week to discuss?
Flags: needinfo?(bkelly)
Depends on: 901685
Flags: needinfo?(tantek)
You need to log in before you can comment on or make changes to this bug.