1000775 - For AB Quicksearch / contacts side bar, implement split multiword search pattern (*foo* AND *bar* for XXL search power; port bug 558931 to AB)

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #558931 +++

Bug 558931, for given search string "foo bar" (without quotes), introduces *foo* AND *bar* multiword search pattern to avoid search failures and to provide XXL search power to *recipient autocomplete*, where typing more phrases will rapidly reduce the result set in a Google style search (just type anything you remember, even partial phrases, in random order and independent of existing field data structure).

This bug seeks to introduce/port the same behaviour to AB Quicksearch / Contacts side bar (both using same algorithm iirc). It's a bug, and a major one for that, because many legitimate scenarios currently fail, even simple ones (say you have a card with Display Name: "Doe, John", and you search for "John Doe", it won't find anything). That's because we currently search for *foo bar* only, i.e. "contains full search string" instead of "contains each search word".

I'll add STR later, or perhaps we can get away without...

Comment 1

[Thomas D. (:thomas8)]

STR

1) have AB contact card with
- Display name: "Doe, John" (without quotes)
- email: "[e-mail]"
2) compose new msg, F9 to open contacts side bar for step 3b)
3a) in recipient pane with autocompletion, type:
John Doe
3b) in AB quick search (contacts side bar "Search for", or main AB "Name or Email"), type:
John Doe

Actual result

3a) recipient autocomplete uses *john* AND *doe* algorithm, hence successfully finds "Doe, John" (good)
3b) AB quick search, e.g contacts side bar, uses *john doe* algorithm, hence finds nothing (this bug)

Among other things, this also violates ux-consistency between contacts side bar and recipient pane:

Expected result

3b) As in 3a, "Doe, John" contact should be found and offered in the AB quick search results list. E.g. for intuitive basic searches like "John Doe", all matching cards should be found regardless where exactly in any field these words are present or contained.
As contacts side bar and recipient area both serve the same function of selecting recipients, they should also use same/similar search algorithm return the same/similar results.
Iow, *john* AND *doe* algorithm should be used for AB quicksearch, too (same as in recipient autocomplete).

So this bug wants to port autocomplete Bug 558931 to AB quick search.

For all the reasons spelled out in the discussion on bug 558931 (see Bug 558931 Comment 109), this bug is a major search failure even for simple cases and preventing loads of complete, efficient and flexible searches. This is especially significant for contacts side bar, which is the only way of adding multiple recipients from AB in one go short of creating a mailing list which isn't effective for many criteria-based searches and due to the design bugs in AB. So this bug also enables e.g. searches based on multiple "tags" which users might keep in some AB field for systematic recipient selection, which provides an excellent workaround for the lack of explicit tagging in AB.

Comment 2

[Thomas D. (:thomas8)]

Suyash, Aceman, or somebody else, any volunteers for completing/complementing the great work of bug 558931?

I suggest that we try to port the search goodness of bug 558931 to AB quick search asap; it's really odd and somewhat unprofessional when search terms that work in recipient autocomplete will still fail in contacts side bar a bit more to the left in the same scenario, and that even in the main AB, all flexible multi-word searches fail unless search words are directly adjacent in some field.

Also effective and comprehensive searches in AB (contacts side bar or otherwise) are arguably even more important than in autocomplete, because AB and contacts side bar can handle contacts in bulk and AB is the main data storage where successful searches based on whichever multiple criteria are really expected. (Whereas autocomplete by definition selects only one contact at a time; so you could use Contacts side bar to work around insufficiencies of autocomplete, but not vice versa, much harder/impossible to use autocomplete to work around insufficiencies of contacts side bar/AB quick search.)

Comment 3

[Suyash Agarwal (:sshagarwal)]

Hi,

I would enjoy doing it. But as you are aware, currently I am involved in my project
so I can't get to this soon, sry.

Thanks.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

I hope this suffices the contacts side panel requirement of this bug.

Thanks.

Comment 5

[Thomas D. (:thomas8)]

Comment on attachment 8479348 [details] [diff] [review]
Patch v1

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

Awesome! LGTM.

I particularly like the smart way this keeps using the quicksearchquery.format pref...

Perhaps we can ask Aceman sir to do a bit of performance testing for shorter and longer words just to get an idea of the impact. There'll be some, but I'm not concerned because we get a rocking powerful google style search in exchange. As has been said before, AB and contacts side panel can more easily afford delay before showing results because they will actually be perceived as a search, which is also triggered only once and explicitly by user pressing Enter (as opposed to incremental autocomplete search).

When's the deadline for TB31 ESR? This would be awesome to have for internal ux-consistency with the new algorithm from autocomplete twin bugs... As contacts side bar and recipient autocomplete are located side by side in the UI with very similar purpose, it would be really great to have same search behaviour on both...

::: mail/components/addrbook/content/abContactsPanel.js
@@ +143,5 @@
>    var searchInput = document.getElementById("peopleSearchInput");
>  
> +  if (searchInput.value != "") {
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);

I'd add a comment above this line (wrap lines as appropriate):
// Split up multiple searchwords to create a *foo* and *bar* search against searchfields, using the OR-search template from gQueryURIFormat for each word.

@@ +148,5 @@
> +    let fSearchURI = "";
> +    for (let searchWord of searchWords)
> +      fSearchURI += gQueryURIFormat.replace(/@V/g, encodeABTermValue(searchWord));
> +
> +    searchURI += "?(and" + fSearchURI + ")";

// searchURI now has all the (or(...)) searches, link them up with (and(...))

Comment 6

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #5)
> Comment on attachment 8479348 [details] [diff] [review]
> Patch v1
> > +    searchURI += "?(and" + fSearchURI + ")";
> 
> // searchURI now has all the (or(...)) searches, link them up with (and(...))

Ops, that proposed comment was meant to be:
> // fSearchURI now has all the (or(...)) searches, link them up with (and(...))

Btw, what does the "f" in fSearchURI stand for?

Comment 7

[Thomas D. (:thomas8)]

Suyash, can the awesomeness of the new algorithm from patch attachment 8479348 [details] [diff] [review] be applied to the main AB quicksearch in a similar way? :o :p
They are hooked to the same quicksearchquery.format pref iirc...

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Thanks for the feedback.
So, now this patch tries to address both the issues viz.
AB quicksearch and Contacts sidebar panel.
Please let me know if this works for you.

(In reply to Thomas D. from comment #6)
> (In reply to Thomas D. from comment #5)
> > Comment on attachment 8479348 [details] [diff] [review]
> > Patch v1
> > > +    searchURI += "?(and" + fSearchURI + ")";
> > 
> > // searchURI now has all the (or(...)) searches, link them up with (and(...))
> 
> Ops, that proposed comment was meant to be:
> > // fSearchURI now has all the (or(...)) searches, link them up with (and(...))
> 
> Btw, what does the "f" in fSearchURI stand for?
I don't know :P Maybe, "former" or "first". Anything to denote pre- stage
stuff. I just didn't want to change the used searchURI so instead of modifying
it I used this.

Thanks.

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 8481212 [details] [diff] [review]
Patch v2

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

Excellent. From my reading of the code, this should work (can't test).

I'm a bit surprised existing code mentions LDAP here, I thought LDAP is a different animal handled elsewhere. Anyway, to err on the safe side, please test if your new algorithm breaks LDAP lookups from contacts side bar or AB quick search, and report back if LDAP searches return correct results or results at all. I think a how to get a test LDAP was mentioned on some bug recently... does somebody remember? LDAP is one of TB's painpoints, see bug 86405 with an impressive 400 votes...

::: mail/components/addrbook/content/abContactsPanel.js
@@ +146,5 @@
> +  // searchfields, using the OR-search template from gQueryURIFormat for each word.
> +  if (searchInput.value != "") {
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);
> +    let fSearchURI = "";

For some reason I don't like fSearchURI...
Perhaps tmpSearchURI or innerSearchURI or partialSearchURI or buildSearchURI.
Aceman, ideas/preferences for the variable name?
Let's pick one and then please remember to change all instances accordingly in both files.

::: mail/components/addrbook/content/addressbook.js
@@ +505,5 @@
> +    // Split up multiple searchwords to create a *foo* and *bar* search against
> +    // searchfields, using the OR-search template from gQueryURIFormat for each word.
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);
> +    let fSearchURI = "";

dito for fSearchURI see above, and fix all around

Comment 10

[Suyash Agarwal (:sshagarwal)]

Thanks for the feedback.

LDAP was mentioned in a TODO comment so maybe it isn't addressed already.
Also, should I port these changes to SM too?

Comment 11

[Thomas D. (:thomas8)]

Attachment 8481212 [details] [diff]

Magnus, I know you are more than busy these days, but if you could spare a bit of time to review this smart patch by Suyash, that would be great. This would be awesome to land asap for ux-consistency of result sets between contacts side bar and the new autocomplete match pattern of bug 558931. Patch is straightforward and short, lgtm (see my comment 5 and comment 9 reviews). When is the deadline for TB31 ESR (and is there any public, up-to-date timetable for Thunderbird releases)?

Aceman,
(In reply to Thomas D. from comment #5)
> Perhaps we can ask Aceman sir to do a bit of performance testing for shorter
> and longer words just to get an idea of the impact. There'll be some, but
> I'm not concerned because we get a rocking powerful google style search in
> exchange. As has been said before, AB and contacts side panel can more
> easily afford delay before showing results because they will actually be
> perceived as a search, which is also triggered only once and explicitly by
> user pressing Enter (as opposed to incremental autocomplete search).

Comment 12

[Magnus Melin [:mkmelin]]

Sorry for the delay here. I'll get to it eventually...

I don't think this is ESR material though. AFAIK this calendar is up2date: https://mail.mozilla.com/home/[e-mail]/Thunderbird%20Schedules.html but doesn't say specifically which release. Builds get created a week or so in advance though.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8481212 [details] [diff] [review]
Patch v2

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

Finally got to this. Just a few minor things I'd like changed

::: mail/components/addrbook/content/abContactsPanel.js
@@ +145,5 @@
> +  // Split up multiple searchwords to create a *foo* and *bar* search against
> +  // searchfields, using the OR-search template from gQueryURIFormat for each word.
> +  if (searchInput.value != "") {
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);

nicer to do it all in one, so you don't need the intermediaries
let searchWords = searchInput.value.trim().split(/\s+/);

@@ +146,5 @@
> +  // searchfields, using the OR-search template from gQueryURIFormat for each word.
> +  if (searchInput.value != "") {
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);
> +    let fSearchURI = "";

agreed fSearchURI is an odd name
"queries" maybe?

@@ +147,5 @@
> +  if (searchInput.value != "") {
> +    let input = searchInput.value.trim();
> +    let searchWords = input.split(/\s+/);
> +    let fSearchURI = "";
> +    for (let searchWord of searchWords)

please use braces for all for loops.
and why the forEach instead in the other location? please use the same in both places

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Okay, fixed the nits.

Thanks.

Comment 15

[:aceman]

Just for info, this will affect patch in bug 118624.

I remember complaints that some users do not like the multiword searches. Would you mind adding a way to search for the whole string, e.g. if it is inside quotes? If there isn't a bug already I can create it.

Comment 16

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #15)
> Just for info, this will affect patch in bug 118624.
> 
> I remember complaints that some users do not like the multiword searches.

I'm not aware of such complaints... (users are more concerned about startsWith vs. contains) multiword definitely wins over single-string because multiword can still find single-string, but single-string fails for a lot of legitimate queries and unreasonably limits the search to exact multiword strings which kinda defeats the purpose of searching because you already have to know the exact string. That said...

> Would you mind adding a way to search for the whole string, e.g. if it is
> inside quotes? If there isn't a bug already I can create it.

Yes, quoted strings should definitely be observed as single strings, which is useful and legitimate functionality for certain scenarios.

"foo bar" --> search for single string *"foo bar"*
"foo bar" baz --> search for *"foo bar"* AND *baz*
foo bar --> search for *foo* AND *bar*

Not sure how to deal with
"foo bar (missing trailing quote)

I think in quick search we automatically parse that same as
"foo bar" (treat as if trailing quote at end of search string was present)
which looks acceptable to me without deeper consideration (single word with quote seems less likely)

Comment 17

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #15)
> Just for info, this will affect patch in bug 118624.
Oh.
So, should this wait, or, is that waiting?
I think, that's gonna take some time. Should we slide this in?

> I remember complaints that some users do not like the multiword searches.
> Would you mind adding a way to search for the whole string, e.g. if it is
> inside quotes? If there isn't a bug already I can create it.
Ya cool! Please let me know the bug (I am unable to find one), I'll complete it.

Thanks.

Comment 18

[:aceman]

Comment on attachment 8514121 [details] [diff] [review]
Patch v3

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

::: mail/components/addrbook/content/addressbook.js
@@ +490,5 @@
>  
> +    // Remove the preceeding '?' as we have to prefix "?and" to this format.
> +    gQueryURIFormat = gQueryURIFormat.slice(1);
> +  }
> +

This is needed now, but will be removed again in bug 118624 if it is updated on top of this patch.

@@ +509,5 @@
> +    searchWords.forEach(searchWord =>
> +      queryURI += gQueryURIFormat.replace(/@V/g, encodeABTermValue(searchWord)));
> +
> +    // queryURI has all the (or(...)) searches, link them up with (and(...)).
> +    searchURI += "?(and" + queryURI + ")";

It looks like this patterns is repeating in the 2 files (and probably in mailnews/addrbook/src/nsAbAutoCompleteSearch.js too).
See in bug 118624 how we centralized some common code into abCommon.js. Can you make a common function for building the query too? You will have to implement bug 1091675 somewhere too, so one central function would be a good place for it.

Comment 19

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #17)
> (In reply to :aceman from comment #15)
> > Just for info, this will affect patch in bug 118624.
> Oh.
> So, should this wait, or, is that waiting?
> I think, that's gonna take some time. Should we slide this in?

Yes, it's fine to land this before bug 118624. I guess it's first come, first served for whichever bug ;)

> > I remember complaints that some users do not like the multiword searches.
> > Would you mind adding a way to search for the whole string, e.g. if it is
> > inside quotes? If there isn't a bug already I can create it.

FTR: Filed as bug 1091675.

Comment 20

[:aceman]

(In reply to Thomas D. from comment #19)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #17)
> > (In reply to :aceman from comment #15)
> > > Just for info, this will affect patch in bug 118624.
> > Oh.
> > So, should this wait, or, is that waiting?
> > I think, that's gonna take some time. Should we slide this in?
> 
> Yes, it's fine to land this before bug 118624. I guess it's first come,
> first served for whichever bug ;)
Sometimes we try to find the order yielding the least work :) But in this case none of the patches have reviews yet and this one seems smaller so more likely to get done faster.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8514121 [details] [diff] [review]
Patch v3

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

LGTM! r=mkmelin
Thanks for the patch, Suyash!

Comment 22

[Suyash Agarwal (:sshagarwal)]

Thanks.

Comment 23

[aleth [:aleth]]

This patch has bitrotted, sorry.

Comment 24

[Suyash Agarwal (:sshagarwal)]

Attachment: Updated patch for check-in

Sorry, updated the patch now.
Carrying over review from mkmelin.

Thanks.

Comment 25

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/fa75ea17aa0e