Last Comment Bug 737456 - Contacts API: Improve number lookup
: Contacts API: Improve number lookup
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 08:59 PDT by Gregor Wagner [:gwagner]
Modified: 2012-03-22 06:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.79 KB, patch)
2012-03-20 11:43 PDT, Gregor Wagner [:gwagner]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-03-20 08:59:40 PDT
Looking up contacts with numbers needs some improvements.
Currently we only support a substring lookup from the beginning.

For numbers we need more.
For example +1-234-56789 should be found with

234-567
1234
34567
Comment 1 Gregor Wagner [:gwagner] 2012-03-20 11:42:17 PDT
So with this patch we can lookup:
+1-234-567
Comment 2 Gregor Wagner [:gwagner] 2012-03-20 11:43:49 PDT
Created attachment 607644 [details] [diff] [review]
patch

We still can't find +1-234-567 with 234-567. Do we need this?
Comment 3 Philipp von Weitershausen [:philikon] 2012-03-20 17:40:21 PDT
(In reply to Gregor Wagner [:gwagner] from comment #2)
> We still can't find +1-234-567 with 234-567. Do we need this?

I would say we do.
Comment 4 Gregor Wagner [:gwagner] 2012-03-20 17:53:09 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Gregor Wagner [:gwagner] from comment #2)
> > We still can't find +1-234-567 with 234-567. Do we need this?
> 
> I would say we do.

actually this example works...
the thing that doesn't work is a combination like
+1-234-567-890 and you look for 234567-890
Comment 5 Philipp von Weitershausen [:philikon] 2012-03-20 18:01:58 PDT
(In reply to Gregor Wagner [:gwagner] from comment #4)
> (In reply to Philipp von Weitershausen [:philikon] from comment #3)
> > (In reply to Gregor Wagner [:gwagner] from comment #2)
> > > We still can't find +1-234-567 with 234-567. Do we need this?
> > 
> > I would say we do.
> 
> actually this example works...
> the thing that doesn't work is a combination like
> +1-234-567-890 and you look for 234567-890

Ah. Yeah that probably doesn't need to work.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-21 09:55:46 PDT
Comment on attachment 607644 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +226,5 @@
>        if (aContact.properties[field] && contact.search[field]) {
>          for (let i = 0; i <= aContact.properties[field].length; i++) {
> +          if (aContact.properties[field][i]) {
> +            if (field == "tel") {
> +              //Special case telephone number. 

Nit: Here and below add a space between // and first letter.

@@ +229,5 @@
> +            if (field == "tel") {
> +              //Special case telephone number. 
> +              // "+1-234-567" should also be found with 1234, 234-56, 23456
> +
> +              //Store "+1-234" as ["+1-234", "1-234", "-234"...]

Nit: maybe say that you're chopping a character from the front each time.

@@ +231,5 @@
> +              // "+1-234-567" should also be found with 1234, 234-56, 23456
> +
> +              //Store "+1-234" as ["+1-234", "1-234", "-234"...]
> +              let number = aContact.properties[field][i];
> +              for(let i=0; i<number.length; i++) {

Nit: spaces around = and <

@@ +235,5 @@
> +              for(let i=0; i<number.length; i++) {
> +                contact.search[field].push(number.substring(i, number.length));
> +              }
> +              // Store +1-234-567 as ["1234567", "234567"...]
> +              let tmp = aContact.properties[field][i].match(/\d/g);

Nit: Maybe rename tmp to digits? And you can do:

  let digits = number;

@@ +238,5 @@
> +              // Store +1-234-567 as ["1234567", "234567"...]
> +              let tmp = aContact.properties[field][i].match(/\d/g);
> +              if (tmp && number.length != tmp.length) {
> +                tmp = tmp.join('');
> +                for(let i=0; i<tmp.length; i++) {

Nit: spaces here too.
Comment 8 Etienne Segonzac (:etienne) 2012-03-22 05:01:22 PDT
Awesome, it also fixes the small "find contact from phone number" regression we had in the Gaia dialer app.
Comment 9 Marco Bonardo [::mak] 2012-03-22 06:30:03 PDT
https://hg.mozilla.org/mozilla-central/rev/bd34ce76aed2

Note You need to log in before you can comment on or make changes to this bug.