118624 - AB quick search and contacts side bar does not search/match Nickname field, but should (ux-inconsistent with autocomplete); make quicksearch & autocomplete queries customizable prefs (mail.addr_book.quicksearchquery.format / autocompletequery.format)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Ninoschka Baca]

Trunk build 2002-01-07-03: WinMe

Overview: In the Address Book the quick search feature should work on the Nick 
Name field (it currently works with the First Name, Last Name and Email 
Address). As stated in bug# 117320 and bug# 79014.

Comment 1

[scottputterman]

*** Bug 118627 has been marked as a duplicate of this bug. ***

Comment 2

[scottputterman]

moving the dups nsbeta1 nomination over.

Comment 3

[(not reading, please use [e-mail] instead)]

is this really desired?  NickName is not listed in the spec:

http://www.mozilla.org/mailnews/specs/addressbook/#Advanced

"The search bar performs a search based on Display Name, First Name, Last Name, 
and Email. If the user wants to perform a search on other criteria, they can 
perform an advanced search."

Comment 4

[kmurray1115]

Future is fine with me. I don't see a need for this anytime soon, unless we
begin to see a trend in the feedback.

Comment 5

[R.K.Aa.]

*** Bug 183566 has been marked as a duplicate of this bug. ***

Comment 6

[R.K.Aa.]

*** Bug 150138 has been marked as a duplicate of this bug. ***

Comment 7

[u32858]

I think search by nickname is essential, i even thought I had not added a user
to my address book because if this problem. I don't think it would take much to
implement, could it be assigned a higher priority?

Could it be assigned to all OS? (occurs on linux)

Regards

JG

Comment 8

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 9

[Cavin Song]

Reassigning.

Comment 10

[Wayne Mery (:wsmwk)]

remove milestone
mod summary to proper name "nickname"
(similar TB Bug 224863)

Comment 11

[Mark Banner (:standard8)]

*** Bug 224863 has been marked as a duplicate of this bug. ***

Comment 12

[Ben]

Regarding a couple of comments whether this is necessary, here is the part that I wrote in bug #224863 that is closed as a dup:

Take for instance somebody called William who is commonly called Bill. One would have William as the official name and Bill as nickname. Typing Bill in the bar should also present our friend Bill.

Comment 14

[Matěj Cepl]

Perhaps should be closed as a duplicate of bug 298438?

Comment 15

[Thomas D. (:thomas8)]

(In reply to Matej Cepl from comment #14)
> Perhaps should be closed as a duplicate of bug 298438?

Thanks Matej for the pointer, I think until bug 298438 gets implemented (which would solve this bug afasics), we should keep this open as a dependant. Perhaps just adding a single field (nickname) to what AB quicksearch goes through is easier than adding the whole range, so somebody might pick this up, and nick should definitely work even if we don't get the rest of the set with selectors etc. as discussed in bug 298438.

Surely forms part of bug 298438 -> setting dependency.

Comment 17

[Thomas D. (:thomas8)]

This is clearly violating ux-consistency, so it's a bug. This somewhat defeats the very purpose of nicknames: No reason why this highly efficient, explicitly user-defined way of searching should fail for AB quicksearch, much less for contacts side bar, while it works only in composition's recipient autocomplete. And even there we don't get it right as we don't give nicks the highest precedence within autocomplete results (bug 325458; as a sidenote, without autocomplete, you'll even get nothing for nicks per bug 295428).

We can't expect users to remember that searching for nicks works in recipient field, but will fail just a few more centimeters to the left in contacts side bar, which however serves the very same function of adding recipients to your composition. In fact, all of AB quicksearch, contacts side bar, and recipient autocomplete share the same function of searching/finding addresses, so they should be as consistent as possible.

Comment 18

[Thomas D. (:thomas8)]

The next step for this bug is to find starting point in code.
After that, it should not be too hard as we'll just add the nickname field in analogy to other fields already searched. Hopefully :)

Comment 19

[Thomas D. (:thomas8)]

I'll have mercy on this popular little pain in the neck (6 dupes, 4 votes).

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Search more fields per mail.addr_book.quicksearchquery.format pref: NickName, SecondEmail, Notes of Mailing Lists, etc.

(In reply to Thomas D. from comment #19)
> I'll have mercy on this popular little pain in the neck (6 dupes, 4 votes).

One-line patch - pls review asap to meet TB31 deadline April 28th

Dear reviewers, for AB QuickSearch and Contacts Side Bar, this *one-line patch* adds searching in Nickname field and some more fields which imo are eligible as a minimal consens fix as part of bug 298438; for the full list of available fields, see Bug 298438 Comment 20.

I've added some more ui-reviewers to speed things up because this has strings so it's probably bound to the branching deadline for TB31 on April 28th, and I'd love to see this land.

Fields added by this patch to mail.addr_book.quicksearchquery.format pref:

NickName, SecondEmail, Notes of Mailing Lists, Company, Department, JobTitle, WebPage1, Webpage2

NickName definitely needs to be searched per this bug.
Notes of Mailing Lists are labelled "Description" in the UI, so that's worth searching, too (it's also included in autocomplete search).
Company (aka "Organisation"), Department, JobTitle, WebPage1, WebPage2 are all "name-ish" things which users might reasonably want to search for in a search box currently labelled "Name or Email". In the interest of simplicity (and lack of time and ideas), I didn't change that label but we could if we wanted.

If anybody thinks/observes this causing performance issues (Gerv?), we can remove some, even most of these fields again. I'd definitely want to add NickName, SecondEmail, and Organisation (we even show Organisation in the AB Name column if that's not otherwise filled).

Comment 21

[Thomas D. (:thomas8)]

FYI, here's the change proposed in patch attachment 8411461 [details] [diff] [review] (line breaks added for presentation):

-mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)
(LastName,c,@V))
+mail.addr_book.quicksearchquery.format=?(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)
(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)(and(IsMailList,=,TRUE)(Notes,c,@V))
(Company,c,@V)(Department,c,@V)(JobTitle,c,@V)(WebPage1,c,@V)(WebPage2,c,@V))

This makes AB quicksearch / contacts side bar more consistent with autocomplete quicksearch per current patch just landing on bug 558931 (attachment 8411330 [details] [diff] [review]):

+      let modelQuery = "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)" +
+                       "(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)" +
+                       "(and(IsMailList,=,TRUE)(Notes,c,@V)))";

Comment 22

[Thomas D. (:thomas8)]

Reviewers, can you pls test if this *breaks* LDAP searches (I think not); note that result sets of LDAP searches won't be consistent with this due to their different field nature (we could look at that in another bug).

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8411461 [details] [diff] [review]
Patch V.1: Search more fields per mail.addr_book.quicksearchquery.format pref: NickName, SecondEmail, Notes of Mailing Lists, etc.

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +262,5 @@
>  # note, changing this might require a change to SearchNameOrEmail.label
>  # in messenger.dtd
>  #
>  # LOCALIZATION NOTE - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true
>  # "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))"

i guess this should be LOCALIZATION NOTE (mail.addr_book.quicksearchquery.format) and adjusted

@@ +269,1 @@
>  

need to bump mail.addr_book.quicksearchquery.format when we change the value

the fields doesn't seem to be the same as autocomplete. and on that topic, can we simply use the same pref then? (if it's a pref, didn't look)

Comment 24

[Gervase Markham [:gerv]]

(In reply to Thomas D. from comment #20)
> If anybody thinks/observes this causing performance issues (Gerv?), we can
> remove some, even most of these fields again. I'd definitely want to add
> NickName, SecondEmail, and Organisation (we even show Organisation in the AB
> Name column if that's not otherwise filled).

I think it's your responsibility as patch author to make sure it doesn't regress performance - particularly as this code is known to already have poor performance, and you are adding more data for it to search. See bug 984875 for leads on where you could get a large address book to test with. If all else fails, you can use mine, but I'd obviously prefer not.

Gerv

Comment 25

[Josiah Bruner [:jsbruner]]

Comment on attachment 8411461 [details] [diff] [review]
Patch V.1: Search more fields per mail.addr_book.quicksearchquery.format pref: NickName, SecondEmail, Notes of Mailing Lists, etc.

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

I personally think this is fine from a UI-perspective (I haven't tested the patch, I'm just looking at the code), but I'd still like Blake's feedback since I'm rushing things. As I will note below, I am quite worried about perf here, but don't have time to test myself.

(In reply to Thomas D. from comment #22)
> Reviewers, can you pls test if this *breaks* LDAP searches (I think not);
> note that result sets of LDAP searches won't be consistent with this due to
> their different field nature (we could look at that in another bug).

Why haven't you tested it? You really shouldn't be making reviewers parent your code.

(In reply to Thomas D. from comment #20)
> If anybody thinks/observes this causing performance issues (Gerv?), we can
> remove some, even most of these fields again. I'd definitely want to add
> NickName, SecondEmail, and Organisation (we even show Organisation in the AB
> Name column if that's not otherwise filled).

Same thing here, you should test yourself. However, I myself am *very* worried about perf here and would not be pleased with ANY significant perf drop (Anything more than 5 milliseconds) and personally I think it's risky to change something perf-sensitive this close to TB 31 (It's hard to change performance things later AND uplift them, especially with the minimal user-test base.

I myself follow a "If you *think* it will cause perf regressions, you didn't do it right" line of thinking.

Comment 26

[Thomas D. (:thomas8)]

Fwiw, this bug is a ridiculous design failure which should have been fixed long ago, and there was a lot of noise from users about Nicknames not working as they should on getsatisfaction.

Unfortunately, I won't finish this by tomorrow's branching deadline for TB31 given that I only started 4 days ago and there's a number of issues to consider to get this right (breaking addons if we bump the property for localizers; but localization might not even be needed for the quicksearchquery.format if we find better way to handle phonetic search; performance).

Comment 27

[:aceman]

(In reply to Magnus Melin from comment #23)
> need to bump mail.addr_book.quicksearchquery.format when we change the value
> 
> the fields doesn't seem to be the same as autocomplete. and on that topic,
> can we simply use the same pref then? (if it's a pref, didn't look)

In autocomplete the string is hardcoded. But it could be retrieved from a pref (http://hg.mozilla.org/comm-central/file/a9c18429042c/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#l339).

(In reply to Gervase Markham [:gerv] from comment #24)
> I think it's your responsibility as patch author to make sure it doesn't
> regress performance - particularly as this code is known to already have
> poor performance, and you are adding more data for it to search. See bug
> 984875 for leads on where you could get a large address book to test with.
> If all else fails, you can use mine, but I'd obviously prefer not.

Yes, but I'd consider this search less performance sensitive. When filling a term into the search field the user expects a search to be done and it to take some time.

On the other hand autocomplete happens without express user consent and therefore should be transparent enough to not interfere with typing.

Comment 28

[Magnus Melin [:mkmelin]]

So maybe it doesn't have to be a pref here either?

Comment 29

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #28)
> So maybe it doesn't have to be a pref here either?

We should certainly keep this as a customizable pref because there's plenty of evidence on bugs (e.g. bug 298438, 3 dupes, 12 votes, and discussion on the bug) that different users have different needs for searching, e.g. which fields to search or not. Allowing easy customization of AB default search without addons adds a lot of value esp. for business deployments. Getting the search algorithm URL from pref doesn't create any problems that we'd have to solve here.

OT: So I'd rather follow Magnus' original idea and try if we can make autocomplete to observe a pref, too. However, it's not as straightforward as it looks, because autocomplete has entirely independent search algorithms (different technique, same search pattern), one for the first search (using search URL which we can easily read from external pref, see aceman's comment 27), and one for narrowing down existing result sets as you type along (manually coded in JS). Suyash has just tweaked/optimized the latter which doesn't even use the search URL syntax. So we'd have to look into that again and see if it can be done with search URL. However, that's not needed for this bug, so we can discuss it elsewhere.

For this bug here, some thoughts:

1) AB search URL is currently realized as localizable pref. That's only because we currenly require locales which want/need phonetic search to manually tweak the search URL by doing both of the follwing:
- set "mail.addr_book.show_phonetic_fields" to true (if applicable for their locale)
- AND manually insert "(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V)" (non-localized!) into the otherwise non-localized default URL:

> # LOCALIZATION NOTE - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true
> # "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))"
> mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V))

2) Asking localizers to manually tweak the search URL for just inserting two fixed, non-localized fields is unnecessary and error-prone. Worse, each time we change the default (non-phonetic) part of the search URL, we'll have to bother localizers again without any need (and I suspect we'll have some more iterations on search algorithms in the near future). I think this is unfortunate design which should be improved.

3) Here's my PROPOSAL for a better design which
- eliminates the useless and error-prone need for localization of the search URL
- thus makes future changes of the default search URL, as in this bug, easier (no localization needed)
- yet still keeps search URL customizable in a safe and simple manner:
(Note: This design change will facilitate implementation of this bug, but it's not required; therefore, the new search fields proposed by this bug are not yet included in this proposal).

a) Make current mail.addr_book.quicksearchquery.format a non-localizable default pref for non-phonetic search (because essentially there's nothing to localize, really), defined centrally in mailnews.js [1]:

> pref("mail.addr_book.quicksearchquery.format", "chrome://messenger/locale/messenger.properties")
This should become:
pref("mail.addr_book.quicksearchquery.format", "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V))")

b) Have a new, separate, non-localizable default pref for phonetic search, defined centrally in mailnews.js [1]:
pref("mail.addr_book.quicksearchquery.format.phonetic", "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V))")

c) Keep current mail.addr_book.show_phonetic_fields as (the only) localizable pref (so each locale can still decide if they need phonetic search or not). At runtime, read "show_phonetic_fields" boolean pref and then read the non-phonetic pref (a) OR the phonetic pref (b) accordingly. To improve performance, we could even do that only once at each AB or composition start and then remember the relevant search URL as a constant.

[1] http://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#147

Comment 30

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #29)

> 3) Here's my PROPOSAL for a better design which
> - eliminates the useless and error-prone need for localization of the search
> URL

My proposal assumes that except for the phonetic part, localizers do NOT (and should not) touch the default search URL for other reasons. That assumption should be verified, but I don't know where the non-English locales live, so I can't check.

Comment 31

[Thomas D. (:thomas8)]

Curse bug 540...

(In reply to Thomas D. from comment #29)
> 3) Here's my PROPOSAL for a better design which
> [...]
> b) Have a new, separate, non-localizable default pref for phonetic search,
> defined centrally in mailnews.js [1]:
> pref("mail.addr_book.quicksearchquery.format.phonetic",
> "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V))")

Sorry, copy & paste failure, this should obviously be

pref("mail.addr_book.quicksearchquery.format.phonetic",
"?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))")

Comment 32

[Thomas D. (:thomas8)]

And while we are here, when we helpfully continue to allow customization of the search URL via pref, we should also allow customization of the placeholder text attribute of the search input box, as requested in [1]:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/addressbook/abContactsPanel.dtd#9 (also in some other files):
> <!ENTITY SearchNameOrEmail.label "Name or Email">
This should be become a pref in mailnews.js:
> pref("mail.addr_book.quicksearchbox.placeholder.label", "Name or Email")

[1] http://mozilla.6506.n7.nabble.com/mail-addr-book-quicksearchquery-Format-amp-Label-td182177.html

Comment 33

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #30)
> My proposal assumes that except for the phonetic part, localizers do NOT
> (and should not) touch the default search URL for other reasons. That
> assumption should be verified, but I don't know where the non-English
> locales live, so I can't check.

http://lxr.mozilla.org/l10n-central/ (or maybe l10n-mozilla-aurora)

Comment 34

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #33)
> http://lxr.mozilla.org/l10n-central/ (or maybe l10n-mozilla-aurora)

Thanks! A new world there, and lots of translation bugs around mail.addr_book.quicksearchquery.format and some boolean prefs, which would break AB search for affected locales (but we probably don't ship those anyway because they aren't up to date). I filed some initial bugs for that.

> (In reply to Thomas D. from comment #30)
> > My proposal assumes that except for the phonetic part, localizers do NOT
> > (and should not) touch the default search URL for other reasons. That
> > assumption should be verified

Findings wrt this bug (from Trunk) -> which confirms my assumption from comment 30:

http://lxr.mozilla.org/l10n-central/search?string=quicksearchquery.format

Japanese locales are the only ones using phonetic search as a default:
mail.addr_book.show_phonetic_fields=true
(And a couple of other locales have incorrectly localized that flag to things like தவறு, which I can't read and which won't work)
(Note that users in setups who communicate with contacts from such countries might also want to use phonetic search pref as a per-installation setting).

Locales with phonetic search (Japanese) only added the two phonetic fields and did not change any other aspect of the default search; nor did any other locale change the default search algorithm.

http://lxr.mozilla.org/l10n-central/search?string=phoneticlastname
(scan for double result lines)

Japanese example:
http://lxr.mozilla.org/l10n-central/source/ja-JP-mac/suite/chrome/mailnews/messenger.properties#249
mail.addr_book.quicksearchquery.format = ?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)

Comment 35

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #34)
> Locales with phonetic search (Japanese) only added the two phonetic fields
> and did not change any other aspect of the default search; nor did any other
> locale change the default search algorithm.
>
> Japanese example:
> http://lxr.mozilla.org/l10n-central/source/ja-JP-mac/suite/chrome/mailnews/
> messenger.properties#249
> mail.addr_book.quicksearchquery.format =
> ?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,
> @V)(PhoneticFirstName,c,@V)

Sorry, copy&paste truncation, the complete line is:

mail.addr_book.quicksearchquery.format	= ?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))

Comment 36

[:aceman]

I wonder why we fetch the pref as Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields",
Components.interfaces.nsIPrefLocalizedString).data . That implies we expect the data to be a localized UTF8 (or something) text. But we wanted it to only contain "true"/"false" so can't we simplify it to getCharPref() ? Or would it fail if there would be those localized strings in the pref? I can try that.

Anyway, Thomas can you get some comments on your proposals in the various comments?

Comment 37

[:aceman]

Comment on attachment 8411461 [details] [diff] [review]
Patch V.1: Search more fields per mail.addr_book.quicksearchquery.format pref: NickName, SecondEmail, Notes of Mailing Lists, etc.

I'd be OK with this extension of the query as long as we test the perf impact (I can do that later. I would also agree to using this same value in the autocomplete code.

Comment 38

[:aceman]

Attachment: patch v2

This implements most of the proposals in this bug so now you can decide what to chop off and what to improve yet ;)

Comment 39

[Thomas D. (:thomas8)]

Comment on attachment 8437857 [details] [diff] [review]
patch v2

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

Thanks a lot Aceman HERO for the WIP patch which helps to speed this up and make some of the proposals more tangible...
This looks like a great start, although it won't work consistently for autocomplete yet (as I tried 2 explain b4).

The good (feedback+ for going in the right directions):
* AB QS and Autocomplete now more consistent
* Autocomplete now customizable (albeit incompletely, see "the bad")
* Customizability of AB quick searches preserved, important for many usecases including enterprise
* Localization comments improved, avoid translation errors (albeit still error-prone, this shouldn't be localizable, only customizable, see "the ugly").

The bad (feedback- wins):
* For autocomplete, only what you type fast before first pausing will use the search URL from pref. Anything typed after that will still run through the hard-coded internal js search algorithm [1] (just streamlined by Suyash in Bug 558931), which just reduces the inital result set from the URL-based query. So when users/admins actually customize the search URL, it will be different from the hard-coded version and we'll end up with unpredictable results which are different depending on the speed of typing search terms. I'm not sure what could be possible and performance-efficient solution here, but I'm quite sure it needs a separate bug because it involves ripping out the secondary js algorithm and reinventing that to use the URL somehow, depending on performance tests for the new design etc.

[1]http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#178

The ugly:
* As explained and proposed in my comment 29, I still think there's no need to continue exposing those prefs to localization errors, given that there's nothing to localize except adding phonetic fields which we can easily offer in the form of another default pref (...format.phonetic). Both prefs should be non-localizable, i.e. their values stored directly in mailnews.js. More so as we are already seeing several localization errors in this area. Making the prefs non-localizable will also make it easier for us to further improve them in the future without involving localizers and breaking addons each time.
* (Optional) Although default algorithms for AB quicksearch and Recipient Autocomplete should definitely be identical, I'd consider offering separate prefs for them. Enterprise admins might want to optimize performance of recipient autocomplete according to the actual design/contents of the company AB database, while allowing broader searches in AB quick search (although it would be bit odd for contacts side bar, right next to autocomplete input) I expect others to disagree... well then, shared pref is better than no pref, so I can live with that, too.

::: mail/base/content/ABSearchDialog.js
@@ -193,1 @@
>             attrs = ["DisplayName","FirstName","LastName","NickName","_AimScreenName"];

OT: For Name searches, searching _AimScreenName but not its siblings (all the other chat names) looks wrong. We should search either all of them or none. Probably just an omission when the new chat name fields were introduced. If we agree on this, could easily be fixed here, otherwise new bug.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +136,1 @@
>        Components.interfaces.nsIPrefLocalizedString).data;

(see above, the ugly, and comment 29) This is where we break addons without need (due to that horrible localization bug which forces us to change the variable names each time we change content). I also agree with aceman it doesn't seem necessary to use nsIPrefLocalizedString here given that only plain English must be used in this pref, assuming that addon authors would rather use English than Hebrew for naming potential new AB fields).

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +253,5 @@
>  mailnews.send_default_charset=UTF-8
>  mailnews.view_default_charset=ISO-8859-1
>  
>  # generate display names in last first order
> +# LOCALIZATION NOTE (mail.addr_book.displayName.lastnamefirst): valid values are: "true" or "false" (keep the English words)

Yes, much better. How about this:

# LOCALIZATION NOTE (mail.addr_book.displayName.lastnamefirst): Do not translate
# anything in the line below, just choose between the English words "true" or
# "false". Set to "true" if your localization prefers "LastName, FirstName" over
# "FirstName LastName".

@@ +262,5 @@
>  #
>  # note, changing this might require a change to SearchNameOrEmail.label
>  # in messenger.dtd
>  #
>  # LOCALIZATION NOTE - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true

This localization note would also require updating, or, better still, remove the entire pref from here (see next code comment).

@@ +263,5 @@
>  # note, changing this might require a change to SearchNameOrEmail.label
>  # in messenger.dtd
>  #
>  # LOCALIZATION NOTE - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true
> +# "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))"

All this complicated/error-prone instruction for localizers which is irrelevant for most localizations anyway could be eliminated here by transferring this into 2 non-localizable prefs in mailnews.js, as explained in "the ugly" above / proposal of comment 29. (Otherwise  the phonetic model query in the comment would need updating according to the normal query).

@@ +270,2 @@
>  
> +# LOCALIZATION NOTE (mail.addr_book.show_phonetic_fields: valid values are: "true" or "false" (keep the English words)

Much better, closing bracket missing. Perhaps rewrite like this:

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +15,5 @@
>    this._searchResults = []; // final results
>    this.searchString = aSearchString;
>    this._collectedValues = new Map();  // temporary unsorted results
> +  this._modelQuery =
> +    Services.prefs.getComplexValue("mail.addr_book.quicksearchquery.format2",

The bad (see above): This is correct and good in itself, but creates inconsistent results as user types more characters of the search phrase, depending on speed and pauses, as the hard-coded ´checkEntry´ function will be used to narrow down results, thus ignoring the model query. Converting ´checkEntry` to use the pref takes a lot more, better done in a separate bug if there's none yet.

http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#178

Plus (see "the ugly" above), it's really awful to break addons like that and add numbers to general prefs just because they are localizable but don't need to be.

I'd also consider  separate pref for autocomplete, even if it has the same default value as the AB quick search pref.

::: mailnews/mailnews.js
@@ +143,5 @@
>  // @V == the escaped value typed in the quick search bar in the addressbook
>  //
>  // note, changing this might require a change to SearchNameOrEmail.label in
>  // messenger.dtd or searchNameAndEmail.emptytext in abMainWindow.dtd
> +pref("mail.addr_book.quicksearchquery.format2", "chrome://messenger/locale/messenger.properties");

Dito, this is where we break addons. Instead, we could keep quicksearchquery.format here, add quicksearchquery.format.phonetic pref and then add the plain search URL values here, instead of linking to localization.

::: suite/locales/en-US/chrome/mailnews/messenger.properties
@@ +240,5 @@
>  mailnews.send_default_charset=UTF-8
>  mailnews.view_default_charset=ISO-8859-1
>  
>  # generate display names in last first order
> +# LOCALIZATION NOTE (mail.displayName.lastnamefirst): valid values are: "true" or "false" (keep the English words)

# LOCALIZATION NOTE (mail.displayName.lastnamefirst): Do not translate
# anything in the line below, just choose between the English words "true" or
# "false". Set to "true" if your localization prefers "LastName, FirstName" over
# "FirstName LastName".

@@ +249,5 @@
>  #
>  # note, changing this might require a change to SearchNameOrEmail.label
>  # in messenger.dtd
>  #
>  # LOCALIZATION NOTE - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true

Dito for suite here (same as in TB), note would need updating, and the model query in the comment too. Or just simplify by transfering into mailnews.js, see "the ugly" / comment 29.

Comment 40

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #39)
> Comment on attachment 8437857 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8437857 [details] [diff] [review]:
> -----------------------------------------------------------------
[SNIP]
> @@ +270,2 @@
> >  
> > +# LOCALIZATION NOTE (mail.addr_book.show_phonetic_fields: valid values are: "true" or "false" (keep the English words)
> 
> Much better, closing bracket missing. Perhaps rewrite like this:

# LOCALIZATION NOTE (mail.addr_book.show_phonetic_fields): Do not translate
# anything in the line below, just choose between the English words "true" or
# "false". Set to "true" if your localization wants the fields PhoneticFirstname
# and PhoneticLastname to be shown on the "Edit contact" properties dialogue.

Comment 41

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #29)

> c) Keep current mail.addr_book.show_phonetic_fields as (the only)
> localizable pref (so each locale can still decide if they need phonetic
> search or not). At runtime, read "show_phonetic_fields" boolean pref and
> then read the non-phonetic pref (a) OR the phonetic pref (b) accordingly. To
> improve performance, we could even do that only once at each AB or
> composition start and then remember the relevant search URL as a constant.

Correction: To improve performance, we could read the "show_phonetic_fields" boolean pref only once at each AB or composition start (or perhaps even only at TB start) and then remember that as a constant.
We currently read the search URL from pref (...quicksearchquery.format) each time user does a search, which is correct and required behaviour to allow both users and addons to change the search algorithm at any time (e.g. similar to quick filter bar's sender/recipient/subject/body facets).

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 8437857 [details] [diff] [review]
patch v2

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

Agreed it's probably better to have only a pref for phonetic or not and not have all localizer set a format2.

(As it was brought up, I really doubt admins really would tweak prefs like this.)

Comment 43

[Tony Mechelynck [:tonymec]]

(In reply to Thomas D. from comment #30)
> (In reply to Thomas D. from comment #29)
> 
> > 3) Here's my PROPOSAL for a better design which
> > - eliminates the useless and error-prone need for localization of the search
> > URL
> 
> My proposal assumes that except for the phonetic part, localizers do NOT
> (and should not) touch the default search URL for other reasons. That
> assumption should be verified, but I don't know where the non-English
> locales live, so I can't check.

You can "unofficially" get SeaMonkey langpacks (and Linux executables) for 18 languages including en-US from http://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-central-linux64/?C=M;O=D (where the query string at the end sorts them newest-first for your convenience; see also other subdirectories of the parent directory). SeaMonkey doesn't "officially" build languages other than en-US ATM AFAIK. Don't know about Thunderbird.

These langpacks ought to include all strings for the languages included there, so that's one way of checking, either by unzipping the langpack XPIs or by installing them (on any platform) and opening the address book. If you run Linux you can also just download, install and run the prebuilt .tar.bz2, of course. Otherwise, there ought to be .../locale/ab-CD/... paths (replacing ab-CD by the language code) in the source.

Comment 44

[Tony Mechelynck [:tonymec]]

P.S. If you install all langpacks in a single executable you can test them in succession thanks to the -UILocale command-line option (which takes a locale parameter) or to the "Edit → Preferences → Appearance → User Interface Language" rolldown (which needs a restart AFAIK).

Comment 45

[Josiah Bruner [:jsbruner]]

Comment on attachment 8437857 [details] [diff] [review]
patch v2

I'm trying to catch up on reviews and feedback, so I'm cancelling this until work progresses again (and all other feedback was addressed)

Comment 46

[Mike Conley (:mconley) (:⚙️)]

I'm not sure I can add any useful information to this bug.

Comment 47

[Thomas D. (:thomas8)]

Attachment: Patch v.3

This was carefully crafted in close cooperation with aceman as part of the Summit hackathon, addressing a whole range of issues mentioned. Thanks to Richard (:paenglab) who introduced me to TortoiseHg which makes patches so much easier...

- add some search fields (per comment 20, has ui-r+ on attachment 8411461 [details] [diff] [review])
- remove model queries from localization (never used, and error-prone) by introducing separate default pref for phonetic search
- synchronize AB quicksearch/contacts side bar and recipient autocomplete by default
- allow independent customization of recipient autocomplete query (per popular demand, functional difference, and coders' deep consideration)
- make it easier for addons to change things in this corner

Enjoy! Imagine nickname searches now actually finding things in AB/contacts side bar...

Comment 48

[:aceman]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

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

I support these changes. And making the autocomplete search query be a pref makes it possible to customize the search for users that are complaining about the recent changes to autocomplete. It allows to make it search in less fields (e.g. Nickname only), or change from "contains" to "begins with" matching. So this would be a big improvement in user customizability without any big code changes.

I have not yet tested the patch fully, but I welcome the features it is implementing.

Comment 49

[Thomas D. (:thomas8)]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

(In reply to Thomas D. from comment #47)
> Created attachment 8509796 [details] [diff] [review]
> Patch v.3
> 
> This was carefully crafted in close cooperation with aceman as part of the
> Summit hackathon, addressing a whole range of issues mentioned. Thanks to
> Richard (:paenglab) who introduced me to TortoiseHg which makes patches so
> much easier...
> 
> - add some search fields (per comment 20, has ui-r+ on attachment 8411461 [details] [diff] [review]
> [details] [diff] [review])
> - remove model queries from localization (never used, and error-prone) by
> introducing separate default pref for phonetic search
> - synchronize AB quicksearch/contacts side bar and recipient autocomplete by
> default
> - allow independent customization of recipient autocomplete query (per
> popular demand, functional difference, and coders' deep consideration)
> - make it easier for addons to change things in this corner
> 
> Enjoy! Imagine nickname searches now actually finding things in AB/contacts
> side bar...

(In reply to :aceman from comment #48)
> Comment on attachment 8509796 [details] [diff] [review]
> Patch v.3
> 
> Review of attachment 8509796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I support these changes. And making the autocomplete search query be a pref
> makes it possible to customize the search for users that are complaining
> about the recent changes to autocomplete. It allows to make it search in
> less fields (e.g. Nickname only), or change from "contains" to "begins with"
> matching. So this would be a big improvement in user customizability without
> any big code changes.
> 
> I have not yet tested the patch fully, but I welcome the features it is
> implementing.

Comment 50

[:aceman]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

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

I have now tested it properly. The changes in addressbook search seem to work. But just opening the compose window cases TB to crash (with just JS code!). I need to investigate that more.

::: mail/components/addrbook/content/abCommon.js
@@ +780,5 @@
> +  } else {
> +    modelQuery = Services.prefs.getCharPref(basePrefName);
> +  }
> +  // remove leading "?" to migrate existing customized values for mail.addr_book.quicksearchquery.format
> +  if (modelQuery.beginsWith("?"))

This should be .startsWith().

@@ +796,5 @@
> +    return Services.prefs.prefHasUserValue(basePrefName +".phonetic");
> +  } else {
> +    return Services.prefs.prefHasUserValue(basePrefName);
> +  }
> +}

We need both functions in the /suite version of abCommon.js too, as nsAbAutoCompleteSearch.js is shared between TB and SM so this code must be in both. I wonder if you also need to change /suite addressbook.js code because we changed the pref for both programs, but code is changed only in TB. If SM does not want the addressbook search changes, at least it needs to adapt to the now missing "?" in the search query.

::: mailnews/mailnews.js
@@ +150,5 @@
> +// mail.addr_book.quicksearchquery.format.phonetic will be used if mail.addr_book.show_phonetic_fields is "true"
> +pref("mail.addr_book.quicksearchquery.format.phonetic", "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)(and(IsMailList,=,TRUE)(Notes,c,@V))(Company,c,@V)(Department,c,@V)(JobTitle,c,@V)(WebPage1,c,@V)(WebPage2,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))");
> +
> +// mail.addr_book.autocompletequery.format will be used if mail.addr_book.show_phonetic_fields is "false"
> +pref("mail.addr_book.autocompletequery.format", "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)(and(IsMailList,=,TRUE)(Notes,c,@V))(Company,c,@V)(Department,c,@V)(JobTitle,c,@V)(WebPage1,c,@V)(WebPage2,c,@V))");

We could keep the autocomplete query string in the current state as "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)" +		
"(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)" +		
"(and(IsMailList,=,TRUE)(Notes,c,@V)))";
so that this patch has no perf impact. We should not slow down autocomplete even more at this stage. I am not worried about the AB searches, where some waiting is expected by the user.

Comment 51

[Magnus Melin [:mkmelin]]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

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

::: mail/components/addrbook/content/abCommon.js
@@ +770,5 @@
> + * query URL part from prefs, allowing users to customize these searches.
> + * @param aQueryName  the name of a model query, e.g. "quicksearch" for AB quicksearch
> +                      and contacts side bar, or "autocomplete" for recipient autocomplete
> + */
> +function getModelQuery(aQueryName) {

I think it it be saner to just pass in the base pref name a parameter

@@ +775,5 @@
> +  let modelQuery = "";
> +  let basePrefName = "mail.addr_book." + aQueryName + "query.format";
> +  if (Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields",
> +      Components.interfaces.nsIPrefLocalizedString).data == "true") {
> +    modelQuery = Services.prefs.getCharPref(basePrefName +".phonetic");

space after plus

@@ +786,5 @@
> +  return modelQuery;
> +}
> +
> +/**
> + * Check if the currently used pref which has the model query was customized by user

add a period

@@ +794,5 @@
> +  if (Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields",
> +      Components.interfaces.nsIPrefLocalizedString).data == "true") {
> +    return Services.prefs.prefHasUserValue(basePrefName +".phonetic");
> +  } else {
> +    return Services.prefs.prefHasUserValue(basePrefName);

no else after return please

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +310,5 @@
>  
>      if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
>          aSearchString.startsWith(aPreviousResult.searchString) &&
> +        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
> +        !aPreviousResult._modelQueryHasUserValue)

so what's the purpose of the hasuservalue check?

Comment 52

[:aceman]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

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

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +15,5 @@
>    this._searchResults = []; // final results
>    this.searchString = aSearchString;
>    this._collectedValues = new Map();  // temporary unsorted results
> +  this._modelQuery = getModelQuery("autocomplete"); // reads mail.addr_book.autocompletequery.format | mail.addr_book.autocompletequery.format.phonetic
> +  this._modelQueryHasUserValue = modelQueryHasUserValue("autocomplete"); // check if the currently active model query has been modified

I have found out why autocomplete is not returning anything after this patch. The functions from abCommon.js like getModelQuery are not accessible here. I was mistaken that abCommon.js is somehow included here. We need to find a way how to do it.

@@ +310,5 @@
>  
>      if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
>          aSearchString.startsWith(aPreviousResult.searchString) &&
> +        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
> +        !aPreviousResult._modelQueryHasUserValue)

I have found the reason for the crash. The line here has lost the opening curly brace.

Comment 53

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #51)
> Comment on attachment 8509796 [details] [diff] [review]
> Patch v.3
> 
> Review of attachment 8509796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/addrbook/content/abCommon.js
> @@ +770,5 @@
> > + * query URL part from prefs, allowing users to customize these searches.
> > + * @param aQueryName  the name of a model query, e.g. "quicksearch" for AB quicksearch
> > +                      and contacts side bar, or "autocomplete" for recipient autocomplete
> > + */
> > +function getModelQuery(aQueryName) {
> 
> I think it it be saner to just pass in the base pref name a parameter

Aceman thought otherwise, claiming this would somehow make it easier for addons, but with second thoughts, that's doubtful (e.g. you can't use a fully customized pref name) . Maybe pref name parameter as suggested by Magnus is better?

> @@ +794,5 @@
> > +  if (Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields",
> > +      Components.interfaces.nsIPrefLocalizedString).data == "true") {
> > +    return Services.prefs.prefHasUserValue(basePrefName +".phonetic");
> > +  } else {
> > +    return Services.prefs.prefHasUserValue(basePrefName);
> 
> no else after return please

Why not?

> ::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
> @@ +310,5 @@
> >  
> >      if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
> >          aSearchString.startsWith(aPreviousResult.searchString) &&
> > +        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
> > +        !aPreviousResult._modelQueryHasUserValue)
> 
> so what's the purpose of the hasuservalue check?

As explained in comment:
>        // We have successful previous matches, therefore iterate through the
>        // list and reduce as appropriate
>+      // Todo: However, if autocomplete model query has been customized, we fall
>+      // back to using the full query again instead of reducing result list in js,
>+      // which might be less performant so we should try morphing the query for js.
>+      // At least we now allow users to customize their autocomplete model query...

Iow, if hasuservalue is true (user defined his own search), then we can't use the default js algorithm which reduces the initial result set, so we use the full c++ url query instead (ignoring the initial result set).

Comment 54

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #52)
> Comment on attachment 8509796 [details] [diff] [review]
> Patch v.3
> 
> Review of attachment 8509796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
> @@ +15,5 @@
> >    this._searchResults = []; // final results
> >    this.searchString = aSearchString;
> >    this._collectedValues = new Map();  // temporary unsorted results
> > +  this._modelQuery = getModelQuery("autocomplete"); // reads mail.addr_book.autocompletequery.format | mail.addr_book.autocompletequery.format.phonetic
> > +  this._modelQueryHasUserValue = modelQueryHasUserValue("autocomplete"); // check if the currently active model query has been modified
> 
> I have found out why autocomplete is not returning anything after this
> patch. The functions from abCommon.js like getModelQuery are not accessible
> here. I was mistaken that abCommon.js is somehow included here. We need to
> find a way how to do it.

I'm surprised to hear that, because I think we checked it at the summit and it was indirectly referenced through some other file which was referenced, and also: You tested at the summit and it worked!?

Don't forget there's also the problem of beginsWith vs. startsWith, so beginsWith can also break things because it's invalid syntax.

> @@ +310,5 @@
> >  
> >      if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
> >          aSearchString.startsWith(aPreviousResult.searchString) &&
> > +        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
> > +        !aPreviousResult._modelQueryHasUserValue)
> 
> I have found the reason for the crash. The line here has lost the opening
> curly brace.

Comment 55

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #53)
> > no else after return please
> 
> Why not?

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices item #1

Besides indention concerns it also let's your brain "close that chapter" of code, so you only have to think about small chunks at a time.

Comment 56

[:aceman]

(In reply to Thomas D. from comment #54)
> (In reply to :aceman from comment #52)
> > Comment on attachment 8509796 [details] [diff] [review]
> > Patch v.3
> > 
> > Review of attachment 8509796 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
> > @@ +15,5 @@
> > >    this._searchResults = []; // final results
> > >    this.searchString = aSearchString;
> > >    this._collectedValues = new Map();  // temporary unsorted results
> > > +  this._modelQuery = getModelQuery("autocomplete"); // reads mail.addr_book.autocompletequery.format | mail.addr_book.autocompletequery.format.phonetic
> > > +  this._modelQueryHasUserValue = modelQueryHasUserValue("autocomplete"); // check if the currently active model query has been modified
> > 
> > I have found out why autocomplete is not returning anything after this
> > patch. The functions from abCommon.js like getModelQuery are not accessible
> > here. I was mistaken that abCommon.js is somehow included here. We need to
> > find a way how to do it.
> 
> I'm surprised to hear that, because I think we checked it at the summit and
> it was indirectly referenced through some other file which was referenced,
I inferred that via the availability of the encodeABTermValue() but now I found the autocomplete has its own copy of the function, so there is no inclusion.

> and also: You tested at the summit and it worked!?
No, AB search worked at the summit, autocomplete did NOT.
 
> Don't forget there's also the problem of beginsWith vs. startsWith, so
> beginsWith can also break things because it's invalid syntax.
Yes, I have that fixed in my local copy.

> > @@ +310,5 @@
> > >  
> > >      if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
> > >          aSearchString.startsWith(aPreviousResult.searchString) &&
> > > +        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
> > > +        !aPreviousResult._modelQueryHasUserValue)
> > 
> > I have found the reason for the crash. The line here has lost the opening
> > curly brace.

You should upload a new version of the patch at least with these syntax errors fixed :)

Comment 57

[Thomas D. (:thomas8)]

Sorry, turns out I don't have time to finish this, but the patch is at an advanced stage so maybe someone else can take over (start by addressing review comments, comment 48 ff).

Aceman?

Comment 58

[:aceman]

Comment on attachment 8509796 [details] [diff] [review]
Patch v.3

I think you can finish this, just fix the syntax errors as said earlier and wait for bug 1100801 to be solved so that autocomplete starts working too.

Comment 59

[:aceman]

Attachment: patch v.4

As bug 550411 solved the phonetic and lastname prefs, I have cleaned up the patch here to not contain those parts. So now this patch contains really only the features Thomas is developing for this bug. I haven't tested it after the cleanup, but it still has to wait on bug 1100801 anyway.

Comment 60

[Thomas D. (:thomas8)]

Attachment: WIP patch v.5 (various nitfixes)

(In reply to :aceman from comment #56)
> You should upload a new version of the patch at least with these syntax
> errors fixed :)

OK, here's a better WIP patch (untested / not checked against bitrot) which addresses all of comment 50, comment 51, comment 52, except the following:

1) patch breaks autocomplete because new patch functions are not yet accessible
Aceman, could you pls suggest a good way of fixing that?

(In reply to :aceman from comment #52)
> Comment on attachment 8509796 [details] [diff] [review]
> Patch v.3
> ::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
> @@ +15,5 @@
> >    this._searchResults = []; // final results
> >    this.searchString = aSearchString;
> >    this._collectedValues = new Map();  // temporary unsorted results
> > +  this._modelQuery = getModelQuery("autocomplete"); // reads mail.addr_book.autocompletequery.format | mail.addr_book.autocompletequery.format.phonetic
> > +  this._modelQueryHasUserValue = modelQueryHasUserValue("autocomplete"); // check if the currently active model query has been modified
> 
> I have found out why autocomplete is not returning anything after this
> patch. The functions from abCommon.js like getModelQuery are not accessible
> here. I was mistaken that abCommon.js is somehow included here. We need to
> find a way how to do it.

2) suite part of patch is still needed

(In reply to :aceman from comment #50)
> Comment on attachment 8509796 [details] [diff] [review]
> Patch v.3
> 
> Review of attachment 8509796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/addrbook/content/abCommon.js
> @@ +796,5 @@
> > +    return Services.prefs.prefHasUserValue(basePrefName +".phonetic");
> > +  } else {
> > +    return Services.prefs.prefHasUserValue(basePrefName);
> > +  }
> > +}
> 
> We need both functions in the /suite version of abCommon.js too, as
> nsAbAutoCompleteSearch.js is shared between TB and SM so this code must be
> in both. I wonder if you also need to change /suite addressbook.js code
> because we changed the pref for both programs, but code is changed only in
> TB. If SM does not want the addressbook search changes, at least it needs to
> adapt to the now missing "?" in the search query.

And, according to aceman, this is somehow waiting for bug 1100801.

Comment 61

[:aceman]

Comment on attachment 8607004 [details] [diff] [review]
WIP patch v.5 (various nitfixes)

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

Thomas, it seems now you can finish this feature. Bug 1100801 added the shared module at mailnews/base/util/ABQueryUtils.jsm . Please put your new functions of getModelQuery() and modelQueryHasUserValue() into that new file (and not abCommon.js). nsAbAutoCompleteSearch.js already includes that file so autocomplete should now also work with your changes as it now will see the functions (the older patches didn't).

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +15,5 @@
>    this._searchResults = []; // final results
>    this.searchString = aSearchString;
>    this._collectedValues = new Map();  // temporary unsorted results
> +  this._modelQuery = getModelQuery("mail.addr_book.autocompletequery.format"); // will return mail.addr_book.autocompletequery.format.phonetic if mail.addr_book.show_phonetic_fields == true
> +  this._modelQueryHasUserValue = modelQueryHasUserValue("mail.addr_book.autocompletequery.format"); // check if the currently active model query has been modified

Please put these comments on separate lines before the respective code lines so keep lines short.

Comment 62

[Wayne Mery (:wsmwk)]

I'm hoping thomas can finish this good work

Comment 63

[Thomas D. (:thomas8)]

I'm not sure if Wayne is trying to pull me away from elsewhere or just trying to pull me back here ;)
But anyway, thanks for the encouraging and appreciative reminder.
So I looked into this today and I've just finished the new patch. TortoiseHg is my friend! :)

Comment 64

[Thomas D. (:thomas8)]

Attachment: Patch v. 6: Implement prefs for AB quicksearch, recipient autocomplete; add nickname & other fields to AB quicksearch, contacts side bar & SM's Select Addresses dialogue

New patch. SeaMonkey also benefits from our work here.

(In reply to Thomas D. from comment #47)
* add some search fields to AB quicksearch, Contacts Side Bar, and SM's Select Addresses dialogue (via existing mail.addr_book.quicksearchquery.format pref)

NickName, SecondEmail, Notes of Mailing Lists, Company, Department, JobTitle, WebPage1, Webpage2

* remove model queries from localization (never used, and error-prone) by introducing separate default pref for phonetic search
* thereby make mail.addr_book.quicksearchquery.format pref customizable
* [not yet implemented] synchronize AB quicksearch/contacts side bar and recipient autocomplete by default
* allow independent customization of recipient autocomplete query via pref (per popular demand, functional difference, and coders' deep consideration)
* make it easier for addons to change things in this corner

> Enjoy! Imagine nickname searches now actually finding things in AB/contacts
> side bar...

...and imagine being able to customize your Recipient Autocomplete Search according to your own personal needs, as well as AB quicksearch!

Comment 65

[:aceman]

Comment on attachment 8674446 [details] [diff] [review]
Patch v. 6: Implement prefs for AB quicksearch, recipient autocomplete; add nickname & other fields to AB quicksearch, contacts side bar & SM's Select Addresses dialogue

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

I tested it and it does work now as advertised, also in the compose autocomplete. I tested searching via nickname and custom1 field (after editing the query to contain custom1).
Just fix the syntax errors I describe in the comments ;)

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +398,4 @@
>        // Use helper method to split up search query to multi-word search
>        // query against multiple fields.
>        let searchWords = getSearchTokens(fullString);
> +      let searchQuery = generateQueryURI(this._modelQuery, searchWords);

You have to use result._modelQuery here. That is the object that contains the variable.

@@ +398,5 @@
>        // Use helper method to split up search query to multi-word search
>        // query against multiple fields.
>        let searchWords = getSearchTokens(fullString);
> +      let searchQuery = generateQueryURI(this._modelQuery, searchWords);
> +      

You add trailing spaces.

::: mailnews/base/util/ABQueryUtils.jsm
@@ +59,5 @@
> + *                       If phonetic search is used, corresponding pref must exist:
> + *                       e.g. mail.addr_book.quicksearchquery.format.phonetic
> + * @return               depending on mail.addr_book.show_phonetic_fields pref,
> + *                       the value of aBasePrefName or aBasePrefName + ".phonetic"
> +*/

Please align this * symbol under the preceding lines (add one leading space).

@@ +60,5 @@
> + *                       e.g. mail.addr_book.quicksearchquery.format.phonetic
> + * @return               depending on mail.addr_book.show_phonetic_fields pref,
> + *                       the value of aBasePrefName or aBasePrefName + ".phonetic"
> +*/
> +function getModelQuery(aBasePrefName) {

You also need to add these function names into the this.EXPORTED_SYMBOLS array at the top of this file.

@@ +84,5 @@
> + *                       e.g. mail.addr_book.quicksearchquery.format.phonetic
> + * @return               true or false
> + */
> +function modelQueryHasUserValue(aBasePrefName) {
> +  if (Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields",

You need to include Services.jsm in this file otherwise Services object is unknown. Add "Components.utils.import("resource://gre/modules/Services.jsm");" after the this.EXPORTED_SYMBOLS line.

::: mailnews/mailnews.js
@@ +155,5 @@
>  //
>  // note, changing this might require a change to SearchNameOrEmail.label in
>  // messenger.dtd or searchNameAndEmail.emptytext in abMainWindow.dtd
> +// mail.addr_book.quicksearchquery.format will be used if mail.addr_book.show_phonetic_fields is "false"
> +pref("mail.addr_book.quicksearchquery.format", "(or(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(NickName,c,@V)(PrimaryEmail,c,@V)(SecondEmail,c,@V)(and(IsMailList,=,TRUE)(Notes,c,@V))(Company,c,@V)(Department,c,@V)(JobTitle,c,@V)(WebPage1,c,@V)(WebPage2,c,@V))");

Please add some comments that quicksearchquery.format is used in the AB quick search and the autocompletequery.format is for the compose autocomplete.

::: suite/mailnews/addrbook/addressbook.js
@@ +354,5 @@
>    ClearCardViewPane();  
>  
>    if (!gQueryURIFormat)
> +    // Get model query from pref, without preceding "?", so we need to add it again
> +    gQueryURIFormat = "?" + getModelQuery("mail.addr_book.quicksearchquery.format");

Please add the {} brackets similar to abSelectAddressesDialog.js for safety.

Comment 66

[Thomas D. (:thomas8)]

Attachment: Patch v. 7 (incl. nitfixes of comment 65)

Nitfixes per comment 65.

Aceman, anyone can do ui-reviews according to official documentation, so I'm asking you... otherwise pls pass on.

I made one more small change in mailnews/addrbook/src/nsAbAutoCompleteSearch.js:

     if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
         aSearchString.startsWith(aPreviousResult.searchString) &&
-        aPreviousResult.searchResult == ACR.RESULT_SUCCESS) {
+        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
+        !result._modelQueryHasUserValue) {

I changed the last line, before we had:
> +        !aPreviousResult._modelQueryHasUserValue) {

I think we must use !result (the newly generated result object for the current search), because:
User can change pref between first (cached) search, and subsequent (current) search (e.g. via addons, e.g. when he is not happy with initial search results, set some different filters, then continue typing without clearing search). So we must check _modelQueryHasUserValue with the current result object, not the stale aPreviousResult object. If there's a user pref, we must bail out and do new search, and not try to narrow down the stale search result using the stale pref.

Comment 67

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #66)
> Created attachment 8675532 [details] [diff] [review]
> Patch v. 6 (incl. nitfixes)
> I think we must use !result (the newly generated result object for the
> current search), because:
> User can change pref between first (cached) search, and subsequent (current)
> search (e.g. via addons, e.g. when he is not happy with initial search
> results, set some different filters, then continue typing without clearing
> search). So we must check _modelQueryHasUserValue with the current result
> object, not the stale aPreviousResult object. If there's a user pref, we
> must bail out and do new search, and not try to narrow down the stale search
> result using the stale pref.

It we want to protect against this case, then you also must check if the query string itself has changed between runs. Before your patch the string (query) was hardcoded so that could not happen.

Comment 68

[:aceman]

Comment on attachment 8675532 [details] [diff] [review]
Patch v. 7 (incl. nitfixes of comment 65)

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

Ok, think this works now. I give you ui-r+ based on comments from Josiah and Bwinton which I think you implemented.
I give r+ for the TB parts. Please fix the nit and the other comment about the change of quqery string between runs.
Then try also a SM reviewer for the suite parts.
Thanks.

::: mailnews/base/util/ABQueryUtils.jsm
@@ +6,4 @@
>   * This file contains helper methods for dealing with addressbook search URIs.
>   */
>  
> +this.EXPORTED_SYMBOLS = ["getSearchTokens", "getModelQuery", "modelQueryHasUserValue", "generateQueryURI", "encodeABTermValue"];

Please wrap this into 80 columns and align the next line after the [ bracket. Like 
this.EXPORTED_SYMBOLS = ["getSearchTokens", "getModelQuery", "generateQueryURI",
                         "modelQueryHasUserValue", "encodeABTermValue"];

Comment 69

[:aceman]

(In reply to Josiah Bruner [:JosiahOne] (needinfo for responses) from comment #25)
> (In reply to Thomas D. from comment #20)
> > If anybody thinks/observes this causing performance issues (Gerv?), we can
> > remove some, even most of these fields again. I'd definitely want to add
> > NickName, SecondEmail, and Organisation (we even show Organisation in the AB
> > Name column if that's not otherwise filled).
> 
> Same thing here, you should test yourself. However, I myself am *very*
> worried about perf here and would not be pleased with ANY significant perf
> drop (Anything more than 5 milliseconds)

The current patch does not change the query string for autocomplete so there should be no perf change.

I tested the patch with 2 ABs of 20000 contacts combined on a 3Ghz AMD Phenom II.
In both quick search in AB and autocomplete, the results are returned within seconds.
Yes, IF the user changes the autocomplete query string from the default, then we avoid the cache and then when typing subsequent characters is much slower than if we were using the cache (even with a longer default query).
But that is not a regression, just the price if somebody opts to use the new feature this patch provides (autocomplete configurability).

Comment 70

[Thomas D. (:thomas8)]

Attachment: patch v. 7.1 (nitfix linewrap & comment on hardcoded _checkentry)

(In reply to :aceman from comment #68)

I fixed the nit (linewrap exported symbols).

(In reply to :aceman from comment #67)
> > So we must check _modelQueryHasUserValue with the current result
> > object, not the stale aPreviousResult object. If there's a user pref, we
> > must bail out and do new search, and not try to narrow down the stale search
> > result using the stale pref.
> 
> If we want to protect against this case, then you also must check if the
> query string itself has changed between runs. Before your patch the string
> (query) was hardcoded so that could not happen.

I think my patch already handles any changes of the model query pref between a previous search ["foo"] and the current search ["foobar"] correctly:

At the beginning of the startSearch function (which triggers whenever search terms are entered/changed), we always create a new result object, which will automatically have the most current _modelQuery as a property...:

  startSearch: function startSearch(aSearchString, aSearchParam,
                                    aPreviousResult, aListener) {
    let params = JSON.parse(aSearchParam);
    var result = new nsAbAutoCompleteResult(aSearchString);

If new search string starts with old search string (user added more at the end), and we still have the previous result, AND model query is NOT CUSTOMIZED (i.e. if model query uses pref default value)...

    if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
        aSearchString.startsWith(aPreviousResult.searchString) &&
        aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
        !result._modelQueryHasUserValue) {

...use _checkentry function to reduce the old result set accordingly (assuming that will be more efficient than searching against the whole AB again).
_checkentry still uses the hardcoded search which is equivalent with the default model query pref value (I added a comment to explain and ensure that).

    }
    else
    {
      // Construct the search query from pref; using a query means we can
      // optimise on running the search through c++ which is better for string
      // comparisons (_checkEntry is relatively slow).
      [...]

      let searchWords = getSearchTokens(fullString);
      let searchQuery = generateQueryURI(result._modelQuery, searchWords);

...otherwise (new search terms, no previous result, or user has customized model query, always or changed between searches) generate the new query from the current (new) result, which is holding the most recent _modelQuery as currently set by user when the search (re-)started.

Comment 71

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #70)
> If new search string starts with old search string (user added more at the
> end), and we still have the previous result, AND model query is NOT
> CUSTOMIZED (i.e. if model query uses pref default value)...
> 
>     if (aPreviousResult instanceof nsIAbAutoCompleteResult &&
>         aSearchString.startsWith(aPreviousResult.searchString) &&
>         aPreviousResult.searchResult == ACR.RESULT_SUCCESS &&
>         !result._modelQueryHasUserValue) {
> 

I think aSearchString and aPreviousResult.searchString contain only the user entered word. So it does not say anything about whether the model query has changed.

Comment 72

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #71)
> I think aSearchString and aPreviousResult.searchString contain only the user
> entered word. So it does not say anything about whether the model query has
> changed.

From my reading, my patch already works correctly for all cases where user is always using customized model query, or has changed the model query between previous and current search while just expanding on previous search words (e.g. foo -> foo bar).
This is because whenever the pref has a customized value, we will re-construct the full c++ query using the current object called "result", which is newly created including the most up-to-date user-defined _modelQuery property each time searching fires.

For new search words (where the beginning of the new search words is not same as old), we fire a completely new query using all customized values anyway, so that case also works.

So the only case where it will actually fail is when after a successful previous search, you add more search words AND revert from customized model query pref back to *default* model query pref.
In that case because you're no longer using customized pref (!result._modelQueryHasUserValue), current patch wrongly assumes we can narrow down your previous search result, which is wrong because that result was created from a different model query, so it might be bigger or smaller set compared to new model query. So yeah, for that case and that case only, I must change it.

Comment 73

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #72)
> From my reading, my patch already works correctly for all cases where user
> is always using customized model query, or has changed the model query
> between previous and current search 
to another customized model query (!= default model query)
> while just expanding on previous search
> words (e.g. foo -> foo bar).

Comment 74

[Thomas D. (:thomas8)]

Attachment: Patch v. 7.2 (nitfix Comment 67, per Comment 72)

The nitfix of this patch ensures that we always get the very correct result set, no matter which way the user tweaks the model query pref back and forth between custom value(s) and default value.

Forward Aceman's ui-r+ from Comment 68.

Comment 75

[Thomas D. (:thomas8)]

> aPreviousResult._modelQuery == result._modelQuery

This should be inverted for better readability (after the next review):

result._modelQuery == aPreviousResult._modelQuery

Comment 76

[:aceman]

Comment on attachment 8687674 [details] [diff] [review]
Patch v. 7.2 (nitfix Comment 67, per Comment 72)

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

Thanks.

Comment 77

[Thomas D. (:thomas8)]

Comment on attachment 8687674 [details] [diff] [review]
Patch v. 7.2 (nitfix Comment 67, per Comment 72)

Neil, we are offering the benefits of this RFE to SeaMonkey for free, just say yes and it's all ready to land ;)

This bug doesn't actually change much for SM except making AB quicksearch and AutoComplete Search more configurable with hidden prefs and adding a couple of relevant fields like NickName and SecondEmail to AB search.

1) mail.addr_book.quicksearchquery.format: existing pref, removed from localization error zone, now user-configurable; search some more fields in AB and ABSelectAddressesDialog:
current:  PrimaryEmail, DisplayName, FirstName, LastName
added:  + NickName, SecondEmail, Notes of Mailing Lists, Company, Department, JobTitle, WebPage1, WebPage2 (all of which can obviously be helpful to find the right people in your AB...)

2) mail.addr_book.quicksearchquery.format.phonetic: new pref, removed from localization error zone, now user-conigurable. Intelligently preset to all of the above fields + PhoneticFirstName, PhoneticLastName

3)  mail.addr_book.autocompletequery.format: new pref, now user-configurable instead of hardcoded, fieldset unchanged. Advanced users will love this.

4) mail.addr_book.autocompletequery.format.phonetic; new pref: dito, see 3).

Comment 78

[:aceman]

For completeness, each localization can still choose whether it wants the phonetic variant of the query or not. So this feature is not lost.

Comment 79

[Thomas D. (:thomas8)]

Neil, ping? This bug has been waiting for your greenlight to comment 77 since 2015-11-15, which unfortunately has also prevented landing in TB because code intersects. Tia.

Comment 80

[Kent James (:rkent)]

Thomas, please prepare a patch that removes the suite portion, and give it checkin-needed.

Comment 81

[:aceman]

(In reply to Kent James (:rkent) from comment #80)
> Thomas, please prepare a patch that removes the suite portion, and give it
> checkin-needed.

We still need review for the /mailnews part. Then if we wanted to land /mail + /mailnews part only it would need deep thought whether SM does not stay broken referencing some old prefs/code no longer existing in /mailnews.

Comment 82

[Kent James (:rkent)]

(In reply to :aceman from comment #81)
> (In reply to Kent James (:rkent) from comment #80)
> > Thomas, please prepare a patch that removes the suite portion, and give it
> > checkin-needed.
> 
> We still need review for the /mailnews part.

I saw your r+ and assumed it covered the /mail and /mailnews parts. Review coverage of this code is very thin, and I'm happy to accept a review by Aceman or Magnus on this code. Are you unable to confirm your review?

> Then if we wanted to land /mail
> + /mailnews part only it would need deep thought whether SM does not stay
> broken referencing some old prefs/code no longer existing in /mailnews.

It is not reasonable to withhold features from Thunderbird because we cannot get a review of string changes in /suite. The reasonable approach is to provide the /suite portion of the change, wait a week for a review, then land the breaking change in /mailnews. Fixing /suite then becomes trivial for the SM people, they just have to review and land the patch. In this case, we are well past that already. There are no string changes here for /mail so there is no reason this could not land for TB 45, if we can just get it finished.

All of this assumes that the /mail and /mailnews code has been reviewed by either aceman or magnus.

Comment 83

[:aceman]

(In reply to Kent James (:rkent) from comment #82)
> (In reply to :aceman from comment #81)
> > (In reply to Kent James (:rkent) from comment #80)
> > > Thomas, please prepare a patch that removes the suite portion, and give it
> > > checkin-needed.
> > 
> > We still need review for the /mailnews part.
> 
> I saw your r+ and assumed it covered the /mail and /mailnews parts. Review
> coverage of this code is very thin, and I'm happy to accept a review by
> Aceman or Magnus on this code. Are you unable to confirm your review?

According to the current designation of TB peers I think my review can be considered valid for the /mail part.
Of course I tested the /mailnews part by running the whole feature inside TB. If you can give the patch r+ just based on my testing, you can do it. Just please mark it officially so there is no problem.

> It is not reasonable to withhold features from Thunderbird because we cannot
> get a review of string changes in /suite. The reasonable approach is to
> provide the /suite portion of the change, wait a week for a review, then
> land the breaking change in /mailnews. Fixing /suite then becomes trivial
> for the SM people, they just have to review and land the patch. In this
> case, we are well past that already. There are no string changes here for
> /mail so there is no reason this could not land for TB 45, if we can just
> get it finished.

I can split the patch and we can proceed with this plan.

Comment 84

[Kent James (:rkent)]

The suite people generally want to do their own reviews. For the /mailnews part, when you have a patch that has separated out the /suite portions, I'll do a rs=rkent (or you can do it) based on aceman's review.

Comment 85

[:aceman]

Attachment: patch v7.2 - TB part [checked in - comment 89][checkin Aurora - comment 90]

Comment 86

[:aceman]

Attachment: patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora[checked in - comment 95][checkin Aurora - comment 115]

Comment 87

[:aceman]

I have split the patch as discussed.

Comment 88

[Kent James (:rkent)]

Comment on attachment 8705803 [details] [diff] [review]
patch v7.2 - TB part [checked in - comment 89][checkin Aurora - comment 90]

I did a smoke test of this, confirmed that LDAP still works and that it searches nickname fields, but did not do a detailed review of the code. But there have been enough eyes on this already, so I'm going to rs=rkent

It is important for this to land and start getting tested for TB 45. I'll do an aurora landing without the deletion from the l10n file so that we don't confuse localizers.

Comment 89

[Kent James (:rkent)]

Comment on attachment 8705803 [details] [diff] [review]
patch v7.2 - TB part [checked in - comment 89][checkin Aurora - comment 90]

https://hg.mozilla.org/comm-central/rev/4f4f6a3674c9

(I'm not sure where those funny characters came from at the front of the bug description).

This is a breaking change for SeaMonkey. We hate to do that, but it was important to get this landed. The SM patch needs review and landing.

I'll keep this open for now, but if the SM patch does not get landed soon we'll break it into a separate bug.

Comment 90

[Kent James (:rkent)]

Comment on attachment 8705803 [details] [diff] [review]
patch v7.2 - TB part [checked in - comment 89][checkin Aurora - comment 90]

Landing into comm-aurora, but without the .properties deletion so that no localization files are changed.

http://hg.mozilla.org/releases/comm-aurora/rev/90416ee7dce2

Comment 91

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #90)
> Landing into comm-aurora, but without the .properties deletion so that no
> localization files are changed.
> http://hg.mozilla.org/releases/comm-aurora/rev/90416ee7dce2

Did you see the test failures this causes?
https://treeherder.mozilla.org/#/jobs?repo=comm-aurora&revision=90416ee7dce2

TEST-UNEXPECTED-FAIL | mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js | checkInputItem - [checkInputItem : 182] 3 == 4
TEST-UNEXPECTED-FAIL | mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js | checkReductionSearch - [checkReductionSearch : 171] 2 == 1

Comment 92

[[e-mail]]

Comment on attachment 8705804 [details] [diff] [review]
patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora[checked in - comment 95][checkin Aurora - comment 115]

Sorry, I've been busier recently, and I've been struggling to find time to keep up with bugmail let alone reviews :-(

Comment 93

[:aceman]

(In reply to Jorg K (GMT+1) from comment #91)
> Did you see the test failures this causes?
> https://treeherder.mozilla.org/#/jobs?repo=comm-aurora&revision=90416ee7dce2

No, thanks for noticing.

> TEST-UNEXPECTED-FAIL |
> mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js | xpcshell
> return code: 0
> TEST-UNEXPECTED-FAIL |
> mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js | checkInputItem
> - [checkInputItem : 182] 3 == 4
> TEST-UNEXPECTED-FAIL |
> mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js | xpcshell
> return code: 0
> TEST-UNEXPECTED-FAIL |
> mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js |
> checkReductionSearch - [checkReductionSearch : 171] 2 == 1

I have analyzed the failures and I suspect a mistake in this patch uncovered a problem of the autocomplete cache, that is not caused by the patch.

I try to prepare a quick patch to please the tests and then file a new bug for the issue found.

Comment 94

[Jorg K (CEST = GMT+2)]

I can't see the failure on C-C, so I'm wondering what's going on. Anyway, great if we can fix failures.

Comment 95

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/164ef58967d47b2e4c6473fcb641c97242cf9b95
Bug 118624 - Implement customizable prefs: mail.addr_book.quicksearchquery.format, autocompletequery.format; add Nickname & other fields to AB quick search, contacts side bar and SM's Select Addresses dialogue. Seamonkey part. r=Neil a=Neil SM CLOSED TREE

Comment 96

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #94)
> I can't see the failure on C-C, so I'm wondering what's going on. Anyway,
> great if we can fix failures.

possibly bug 1238369

Comment 97

[Ian Neal]

Comment on attachment 8705804 [details] [diff] [review]
patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora[checked in - comment 95][checkin Aurora - comment 115]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: SM will be broken on c-a
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): none
As the TB part has been approved for c-a, so does the c-c part.

Comment 98

[:aceman]

Attachment: fix the tests [checked in - comment 106][checkin Aurora - comment 109]

It seems we need to add the modelQuery attribute to the interface otherwise it is lost when shuffling the previousResult object around the calls.

Comment 99

[Kent James (:rkent)]

Ian, I don't understand the SM release cycle well enough to approve SM-only uplifts. I've seen you listed as a= under SM comm-aurora patches. Is there some reason you did not do the a+ on this one?

Comment 100

[Philip Chee]

Comment on attachment 8705804 [details] [diff] [review]
patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora[checked in - comment 95][checkin Aurora - comment 115]

a=me. Sorry can't change the field to + since this patch is in MailNews.

Comment 101

[Kent James (:rkent)]

Comment on attachment 8706142 [details] [diff] [review]
fix the tests  [checked in - comment 106][checkin Aurora - comment 109]

rs of Philip Chee's a+

Comment 102

[Kent James (:rkent)]

Comment on attachment 8706142 [details] [diff] [review]
fix the tests  [checked in - comment 106][checkin Aurora - comment 109]

Oops wrong patch and field

Comment 103

[:aceman]

(In reply to :aceman from comment #93)
> (In reply to Jorg K (GMT+1) from comment #91)
> I try to prepare a quick patch to please the tests

The patch "fix the tests" here.

> I have analyzed the failures and I suspect a mistake in this patch uncovered
> a problem of the autocomplete cache, that is not caused by the patch.
> 
> ...and then file a new bug for the issue found.

Filed as bug 1238728.

Comment 104

[Kent James (:rkent)]

Ratty approved the patch, so no more question for Ian.

Comment 105

[Kent James (:rkent)]

Comment on attachment 8706142 [details] [diff] [review]
fix the tests  [checked in - comment 106][checkin Aurora - comment 109]

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

This sort of makes sense, but I did not attempt to fully understand the underlying issues. I'll trust it though for a test fix.

We'll do the aurora uplifts after c-c works, but need to check if the base patch has been uplifted first.

Comment 106

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/5f24035f8e56c8963b4a989ef8a88e5cc3c7afb4
Bug 118624 - fix tests. r=rkent a=aleth

Comment 107

[Ian Neal]

(In reply to Kent James (:rkent) from comment #104)
> Ratty approved the patch, so no more question for Ian.

Well, just for completeness sake, the answer is the same as for Ratty, it is in MailNews and I cannot + there.

Comment 108

[Jorg K (CEST = GMT+2)]

patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora
still needs landing.

Landings required for Aurora:
patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora
fix the tests (after approval).

Comment 109

[Jorg K (CEST = GMT+2)]

Aurora: Patch "fix the tests":
https://hg.mozilla.org/releases/comm-aurora/rev/93a6aabccfbb

Checkin and uplift still needed for "patch v7.2 - SM part".

Comment 110

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #109)
> Checkin and uplift still needed for "patch v7.2 - SM part".

This seems to have bitrotted.

Comment 111

[Jorg K (CEST = GMT+2)]

Badly so, Aceman, please rebase.

Comment 112

[:aceman]

(In reply to aleth [:aleth] from comment #110)
> (In reply to Jorg K (GMT+1) from comment #109)
> > Checkin and uplift still needed for "patch v7.2 - SM part".
> 
> This seems to have bitrotted.

No, it was already checked in to c-c:
https://hg.mozilla.org/comm-central/rev/164ef58967d4

Please land in aurora when possible.

Comment 113

[Jorg K (CEST = GMT+2)]

Comment on attachment 8705804 [details] [diff] [review]
patch v7.2 - SM part r=Neil a=Ratty a=comm-aurora[checked in - comment 95][checkin Aurora - comment 115]

(In reply to :aceman from comment #112)
> No, it was already checked in to c-c:
> https://hg.mozilla.org/comm-central/rev/164ef58967d4
Damn, I didn't see that, sorry.
 
> Please land in aurora when possible.
Will land there with the next lot. Sorry, I didn't spot the C-C landing and therefore didn't uplift with the other 11 patches I uplifted yesterday.

Comment 114

[Jorg K (CEST = GMT+2)]

This bug is fixed, all patches have landed on TB46. One Aurora uplift pending.

Comment 115

[Jorg K (CEST = GMT+2)]

Aurora for SM 2.42:
https://hg.mozilla.org/releases/comm-aurora/rev/3eb75c3af991