798128 - [email] search requires at least 3 letters before it finds anything and is case sensitive.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P3)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

## Environment :
Otoro phone, build 2012-10-03 us
Taken from default.xml in b2g-distro: 
* "platform_build" revision= 795261940c8b11fb7dddd7a8e9dd8561fdc4fb64
* "gaia" revision= a3339652136fde9e5207e1bc1263a410c84f55fc 
* "releases-mozilla-central" revision= aecce9686d0b0e6ec25bb31e837eadace251a951
* "gonk-misc" revision= 202d2c62443c098b125e5d9d7b42226d98230e44
  
## Repro :
1. set up email w/ a hotmail account
2. send an email with the title "test" for the subject to the hotmail account
3. hit the search button 
4. type in t
5. type in e
6. type in s

## Expected :
1. the email will be see in the search filter for step 4, 5, and 6

## Actual :
1. the email only shows up after the 6th step

## Note :
1. search is also case sensitive.  probably shouldn't be?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

The case-sensitive problem seems like a blocker.

Comment 2

[Andrea Marchesini [:baku]]

the search is not case sensitive and it starts when the input length is more than 2 chars.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I'm using indexOf, so I'm pretty sure we're case sensitive.  (Which was a stop-gap first step.  We definitely do not want to be case sensitive.)

But indeed, I do require 3 characters before we start searching right now.  That's inappropriate for CJK (and should/will be fixed), but seems reasonable for languages like English where it's unlikely anything useful will come of searching for two characters.  The rationale is that because the search is probably useless at 2 characters, it's a waste of resources and may slow us down servicing the useful 3+ character searches to do them.

Of course, I have search stop looking after it gets "enough" results (currently 15), and also stops searching a given message after it gets sufficient hits within the message, so it's not like we're going to wastefully look for the letter "t" in every message in the folder.

Casey, opinions on whether the 3-character (for non-CJK character sets) is reasonable, and/or whether we should have some UI feedback like displaying "type at least 3 letters to see search results"?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

This is a proposal for the case-sensitive issue.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 670397 [details] [diff] [review]
patch

Thanks very much for the patch!  I am very psyched to have some contributions to the back-end!

I would prefer that we construct regular expressions with the ignore flag.  The only real complication there is that we need to make sure to escape the string passed in so that the regular expression is searching for the actual literal string rather than interepreting the string as a regex.  Wholesale string lowercasing will take a lot of time and generate a lot of garbage.

Also, please provide the patch against the contents of the gaia-email-libs-and-more repo which is used to generate gaia-email-opt.js.  The file of interest is: https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/searchfilter.js

If you do a full git clone with "--recursive" to get all the submodules, you should be able to easily build a gaia-email-opt.js build with your changes using make.  There is also a install-gaia-email-opt target if you create a "gaia-symlink" that points to the root of the gaia directory.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 2

Can you give me a feedback about this approach?
What this patch does is:

1. xpcshell test for the search filters
2. the input can be a string OR a regexp. The string uses indexOf, regexp uses the exec().

I just need a feedback because the test is passed, but I didn't test it on gaia/b2g.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 671699 [details] [diff] [review]
patch 2

This looks fantastic.  Many thanks for going the extra mile and writing the functional unit test!

Things that would be great to fix for the next rev:
- The RegExp constructor takes an optional second argument which is the flags.  We should pass 'i' in order to get case-ignoring behaviour.

- Right now you are using slice() to approximate the start-search-from-a-given-offset functionality.  I believe that if we pass the 'y' = sticky flag as well as 'i', then we can simply manipulate lastIndex instead of needing to call slice and potentially generate extra garbage.  The one main thing is that we will need to set lastIndex to 0 ourselves if no offset is specified or things will be wonky for us.

- If you could add another false test to the unit tests for author and recipient that fail to match when there is a string to compare against, I think that would be useful coverage-wise.


Notes:

- I'm not sure we really need or want to support the phrase-is-a-string-not-a-regexp use-case.  I think it's reasonable for us to just require that a regexp be passed in.  Having said that, I don't think there's really any need to change the code right now, but if it makes sense to take that bit out, I'm fine with it.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 3

- 'i' has been added to new RegExp

- a couple of new unit-test check more

- about 'y': I tried but I failed :) I'm not expert of regexp and /foo/yi is not enough.

If you review this patch and you think it's good enough, I'll appreciate if you can test it on target or with the emulator.

Comment 9

[Casey Yee]

"Casey, opinions on whether the 3-character (for non-CJK character sets) is reasonable, and/or whether we should have some UI feedback like displaying "type at least 3 letters to see search results"?"

Ideally we start showing search results from the first letter entered.  In particular with the To and From fields this would be important.  Less so with Subject or All filters.

The idea is to minimize the number of keystrokes required to get to whatever they are looking for.

Comment 10

[Casey Yee]

"Casey, opinions on whether the 3-character (for non-CJK character sets) is reasonable, and/or whether we should have some UI feedback like displaying "type at least 3 letters to see search results"?"

Ideally we start showing search results from the first letter entered.  In particular with the To and From fields this would be important.  Less so with Subject or All filters.

The idea is to minimize the number of keystrokes required to get to whatever they are looking for.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 671887 [details] [diff] [review]
patch 3

I ran with this and it works under b2g-desktop, but there appears to be a regression in the e-mail address snippet generation with regards to the "matchRuns" instances.  Although the 'start' is provided, according to the debug 'length' is ending up undefined (specifically, in the debug which uses JSON.stringify, it's missing, which implies that it was undefined).  Looking at the code, it appears that the code is still using phrase.length in those cases (where phrase is a Regexp lacking a length), as opposed to the subject matcher which uses match[0].length instead, which is the correct thing.

If you could fix that and update the unit tests to check the matchRuns, that would be great and I think then I can land it.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Casey Yee [:cyee] from comment #10)
> Ideally we start showing search results from the first letter entered.  In
> particular with the To and From fields this would be important.  Less so
> with Subject or All filters.
> 
> The idea is to minimize the number of keystrokes required to get to whatever
> they are looking for.

Makes sense, we will eliminate the constraint and just require 1 character to start search.  We can always revisit if/when it causes a responsiveness problem and other tweaks to how many messages we search at a time and timeout delays does not improve things sufficiently.

This is really a separate issue from the case-sensitivity issue, so we might need to spin that aspect off, or we can land it as a tiny separate patch on this bug too.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 4

Comment 14

[Andrea Marchesini [:baku]]

This patch should work in this direction.
Hard for me to try it on target, so please, if you can do this check for me, I'll appreciate :)

> If you could fix that and update the unit tests to check the matchRuns, that
> would be great and I think then I can land it.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Ack, this fell off my radar.  I will review and ideally land this evening.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 673235 [details] [diff] [review]
patch 4

Looks good, I am landing with a few minor changes stacked on top as a separate commit.  See my next comment for details.

Thanks very much for the patch and tests!

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/70
  merged into gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/0d0771dd5a31b4a8f27be99ee978bd2e7eb4767a

https://github.com/mozilla-b2g/gaia/pull/6046
  merged into gaia/master:
https://github.com/mozilla-b2g/gaia/commit/4be82f91d752da6bf2d25a1a682a0062c4fba7fa

I made the constant change so we start searching with a single character as a part of the gaia landing, so there is no need to spin-off another bug or anything like that.

Comment 18

[Mila Davydova]

Verified as fixed with Build ID 20121231070201