Address Book's autocomplete (and the compose implementation that uses it) is still based on xpfe's version. This will currently cause problems with Thunderbird and SeaMonkey running on xulrunner due to bug 263042 (which is currently blocked at the front-end due to ui problems). SeaMonkey is looking at picking up as much as possible from toolkit which its transfer to being an xul app - the autocomplete switch is the main blocker to this. Whilst we could fix bug 263042, fixing this (and any other related areas that I've forgotten) would mean we don't have to ship two autocomplete implementations in our apps. Additionally, I think that with SeaMonkey switching from xpfe to toolkit, and TB about to release 2.0 and then start thinking about 3.0 its an idea time to get this started and implemented.

Patch for the initial build changes to drop xpfe autocomplete and build toolkit version. This will obviously break our existing autocomplete items until I've written the new code.

Comment on attachment 320701 [details] [diff] [review] Build Config patch >+#ifdef XPFE_AUTOCOMPLETE > { "Address Book Auto Complete Session", NS_ABAUTOCOMPLETESESSION_CID, > NS_ABAUTOCOMPLETESESSION_CONTRACTID, nsAbAutoCompleteSessionConstructor }, >+#endif You might find it easier to convert the backend to the toolkit interfaces first - xpfe autocomplete should be able to handle them OK but so far we're only using them for history so if you have any problems please let me know.

Here is a WIP patch that I started writing the other weekend ago. It basically starts to re-write the autocomplete code to use the new toolkit APIs (and cleans up some crusty logic). I'm just going to hand this off - and not work on it anymore. I hope it helps some.

(In reply to comment #4) > Created an attachment (id=320721) [details] > WIP Patch > > Here is a WIP patch that I started writing the other weekend ago. It basically > starts to re-write the autocomplete code to use the new toolkit APIs (and > cleans up some crusty logic). > > I'm just going to hand this off - and not work on it anymore. I hope it helps > some. Nick, thanks for this patch, it certainly gives me a useful starting point.

Having taken a look at this, I've decided to do as Neil suggested and start implementing the toolkit autocomplete interfaces using xpfe autocomplete, and do the switch when I've got something working (I'll probably check it out in between though to check for missing/incorrect implementations of things). The patch I'm attaching is a very simple proof-of-concept. I want to go for this in javascript, at least initially (i.e. assuming performance is ok). It only returns hard-coded results, but we have the layout of the the two required classes and it returns results. I'm intending that the search class be as generic as possible with respect to the address books - i.e. if we can, all searching (remote and local) will be done from the one class. I think this should be possible, though we may need some tweaks to the address book code.

> The patch I'm attaching is a very simple proof-of-concept. I want to go for > this in javascript, at least initially (i.e. assuming performance is ok). It > only returns hard-coded results, but we have the layout of the the two required JS would probably work ok - but you would probably already have to add a C++ layer to wrap sqlite (or native AB, like AddressBook.framework on Mac OS X) searches if addrbook goes that way? I only reference this because places used C++: http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.h#133 http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#426 Just my 2 cents...

(In reply to comment #7) > > The patch I'm attaching is a very simple proof-of-concept. I want to go for > > this in javascript, at least initially (i.e. assuming performance is ok). It > > only returns hard-coded results, but we have the layout of the the two required > > JS would probably work ok - but you would probably already have to add a C++ > layer to wrap sqlite (or native AB, like AddressBook.framework on Mac OS X) > searches if addrbook goes that way? Good point, however the address book interfaces (nsIAbDirectory and friends) already have search functionality available within them. I would not want to mix autocomplete with the separate implementations of address book that we have, as that would mean we end up with n implementations of virtually the same autocomplete functionality. The difference with history is that there is only one format/file/protocol to pull data from, with address book we have many...

Note: this needs a) a new nsIAbAutoCompleteSearch.idl b) change nsAbAutoCompleteSession to not complete the default domain c) change addressingWidgetOverlay.xul to include this autocomplete d) change MsgComposeCommands to set the domain into this service

Current WIP patch - this basically implements the initial checks for preferences and items based on the search string. Also includes unit tests for those items.

This also removes the special-casing for autocomplete to my domain from the addrbook session and updates the compose window to use the new search.

Comment on attachment 322571 [details] [diff] [review] [checked in] Split off autocomplete to my domain into a separate search + gAutocompleteSearch = Components.classes["@mozilla.org/autocomplete/search;1?name=mydomain"].getService(Components.interfaces.nsIAbAutoCompleteSearch); nit: some of these lines are a bit long. + // if the pref is set to turn on the comment column, honor it here. + // this element then gets cloned for subsequent rows, so they should + // honor it as well + // + try + { + if (getPref("mail.autoComplete.highlightNonMatches")) + autoCompleteWidget.highlightNonMatches = true; nit: incorrect indentation r=me with those fixed.

(In reply to comment #13) > (From update of attachment 322571 [details] [diff] [review]) > + if (getPref("mail.autoComplete.highlightNonMatches")) > + autoCompleteWidget.highlightNonMatches = true; > > nit: incorrect indentation That's not fair, I just outdented the block, warts and all...

Don't forget the part of splitting off autocomplete to my domain where you put the new file in the various packages(-static) files, so Tb/Win and (I'm betting) SM/Win and SM/Linux don't break and spew "Error: Components.classes['@mozilla.org/autocomplete/search;1?name=mydomain'] is undefined" while typing addresses.

I'll commit the relevant part of this as I get reviews.

Comment on attachment 323200 [details] [diff] [review] [checked in] Add nsAbAutoCompleteMyDomain.js to packages files. D'oh!

Current WIP for the main AB autocomplete. On TB's Compose window (I haven't changed the other callers yet), this patch can: - work out when not to try to autocomplete - autocomplete on all address books enabled for local search This patch cannot: - sort out duplicates - search for mailing lists - search LDAP directories (offline or online). The building blocks are mostly in place now, and the unit test is helping to know I've got it right. Next steps will be to define the full unit test cases and implement the rest of the main AB search code to the current standard.

Comment on attachment 323379 [details] [diff] [review] Implement Toolkit interfaces v0.3 (WIP) >- setupLdapAutocompleteSession(); >+ // setupLdapAutocompleteSession(); You don't need to comment this out until you start writing LDAP search ;-) >+ var dirName = directory.dirName; Shouldn't you check for this._commentColumn here rather than later? >+ // This is the regular expression we'll use for matching. >+ var searchExp = new RegExp("^" + result.searchString, "i"); This won't work when result.searchString contains (e.g.) "." >+ if (card.nickName.search(searchExp) != -1 || >+ card.displayName.search(searchExp) != -1 || >+ card.firstName.search(searchExp) != -1 || >+ card.lastName.search(searchExp) != -1 || >+ card.primaryEmail.search(searchExp) != -1) if (searchExp.test(card.nickName) || searchExp.text(card.displayName) || searchExp.text(card.firstName) || searchExp.text(card.lastName) || searchExp.text(card.primaryEmail)) >+ // Find out what fields we're searching against I don't see you using this? Or is this LDAP-only?

Updated version of this patch. Not quite ready for review. This patch reimplements most of local AB search with toolkit interfaces, the following items are currently outstanding: * matching strings like "ma ba" against "Mark Banner". * Searching replicated (offline) LDAP autocomplete directories (I think this would be best with the identity parameter and changing some of the nsIAbDirectory interfaces, more to come). * Searching only the previous results if a search has already been successful and the next one is a sub-set search (this will require a extension of nSIAbAutoCompleteSearch so we can get back the nsIAbCards). The offline LDAP autocomplete directories I'll probably separate the API changes into a separate bug. The previous results I'll do here as its just an extension of this patch. Also included is an updated test case which is probably more intensive than it needs to be, but at least it should cover everything implemented so far. The nsAbAutoCompleteMyDomain.js change is to make it work consistently alongside nsAbAutoCompleteSearch.js as I found one difference when I was writing the test.

Comment on attachment 323118 [details] [diff] [review] [checked in] autocompletesearchparam Neil, I think we really do want this patch. One thing I don't understand though is how does this change really affect the code: -top.MAX_RECIPIENTS = 0; +top.MAX_RECIPIENTS = 1; /* for the initial listitem created in the XUL */ and is it really relevant to adding the autocompletesearchparam?

Comment on attachment 324323 [details] [diff] [review] Implement Toolkit interfaces v0.5 (WIP) /me wonders what happened to v0.4 >- var address = this.defaultDomain && /^[^@,]+$/.test(aString) && >+ var address = this.defaultDomain && aString && /^[^@,]+$/.test(aString) && Why the extra empty string test? /^[^@,]+$/ needs at least one character.

Comment on attachment 323118 [details] [diff] [review] [checked in] autocompletesearchparam >+ for (var i = 1; i <= awGetMaxRecipients(); i++) >+ awGetInputElement(i).setAttribute("autocompletesearchparam", idKey); This code gets called in three places: 1. When the compose window is opened 2. When the compose window is reopened 3. When somebody selects a different identity from the dropdown (obviously) In case 3) the number of rows in the addressing widget depends of course on the number of recipients. In cases 1) and 2) however the receipients have not yet been loaded. Now in case 2) the previous addresses have been removed from the addressing widget and only the one blank entry remains. However currently in case 1) the addressing widget code has not been initialised and awGetMaxRecipients() currently returns zero, although there is actually one row defined in the XUL, and we want to set the autocompleteparam attribute on it! Note that this value is subsequently overwritten anyway when the recipients finally get loaded.

Comment on attachment 323118 [details] [diff] [review] [checked in] autocompletesearchparam Thanks for the explanation, this works well.

This patch provides a unit test for nsAbAutoCompleteMyDomain, and fixes one minor bug (if null was passed as the string, then it would return success, rather than failure). Also I've included some Makefile.in improvements - I've moved the EXTRA_COMPONENTS from mailnews/addrbook/Makefile.in to mailnews/addrbook/src/Makefile.in as that is where we'd normally put them. I almost did this in separate bug, but seeing as I had this patch and it is semi related to this bug...

Comment on attachment 324601 [details] [diff] [review] Provide test case for MyDomain (inc 1 fix) and sort out EXTRA_COMPONENTS in Makefile.in files Cancelling reviews, I'm going to update some more of the MyDomain code which will require reworking of the tests. So may as well do that all at the same time.

Needs attachment 323118 [details] [diff] [review] too of course.

Additions: - All existing items that use address book autocomplete now are set up to use the new search - The new component has been added to the packages* files - New nsIAbAutoCompleteResult interface - We can now do a reduced search within a previous set of results (assuming bug 438861 is fixed). To do: - Check sorting on popularity - Implement and test search for "First Last"

Updated unit test for nsAbAutoCompleteMyDomain, including changes to check with an identity passed via the search param.

Comment on attachment 325246 [details] [diff] [review] [checked in] Provide test case for MyDomain and sort out EXTRA_COMPONENTS in Makefile.in files v2 addrbook autocomplete also special-cases @ so you might want to test that too.

Comment on attachment 325246 [details] [diff] [review] [checked in] Provide test case for MyDomain and sort out EXTRA_COMPONENTS in Makefile.in files v2 Checked in, I added a test for "a@b" to return ACR.RESULT_FAILURE as suggested by Neil.

This patch implements everything to the same (or maybe better) standard as the existing address book interfaces. Unit tests hopefully cover everything or at least the significant points. I'm not removing nsAbAutoCompleteSession yet because lightning still uses that. Until we switch over completely to toolkit's API I do not believe there is a sensible way around that for them to work with both trunk and branch.

Comment on attachment 325263 [details] [diff] [review] Implement Toolkit interfaces for addrbook search v0.9 (WIP) >+ // Private/set-up items >+ classDescription: "Address Book Autocomplete", >+ classID: Components.ID("2f946df9-114c-41fe-8899-81f10daf4f0c"), >+ contractID: "@mozilla.org/autocomplete/search;1?name=addrbook", "For component registration"? >+ // Private functions methods ;-) >+ // ssa = search string array (first element: full string, >+ // second element: first word, third element: anything after first word. This is too confusing, I think separate parameters would be easier. >+ // ssla = search string length array: lengths of strings in ssa. You can avoid the lengths by using card.displayName.toLocaleLowerCase.lastIndexOf(string, 0) == 0 >+ return true; >+ else if (ssla[1] > 0 && ssla[2] > 0 && Don't use else after return. >+ _checkDuplicate: function _checkDuplicate(card, emailAddress, commentColumn, >+ currentResults) { You don't use commentColumn. >+ // Its a duplicate, is the new one is more popular? It's >+ card.notes != "" ? >+ card.notes : >+ card.displayName); card.notes || card.displayName >+ } >+ else { >+ emailAddress = this._parser.makeFullAddress(card.displayName, >+ card.primaryEmail); Although if mailing lists have a blank primaryEmail you can use card.primaryEmail || card.notes || card.displayName >+ if (emailAddress == "") if (!emailAddress) >+ // Find out where to insert the card. >+ var insertPosition = 0; >+ >+ // First sort on popularity index. >+ var cardPopularityIndex = card.popularityIndex; >+ for (; insertPosition < result._searchResults.length; ++insertPosition) { >+ if (cardPopularityIndex >= >+ result._searchResults[insertPosition].card.popularityIndex) >+ break; >+ } >+ >+ // Next sort on full address >+ for (; insertPosition < result._searchResults.length && >+ (cardPopularityIndex == >+ result._searchResults[insertPosition].card.popularityIndex); >+ ++insertPosition) { >+ if (emailAddress < result._searchResults[insertPosition].value) >+ break; >+ } I think these would be more readable as while loops i.e. while (insertPosition < result._searchResults.length && cardPopularityIndex == ... && emailAddress < ...) ++insertPosition; Actually should we be comparing the lower case email addresses? (Should we be storing the lower case email addresses in the results?) >+ style: "local-abook", What are we doing for replicated LDAP directories? >+ // If the search string isn't value, or contains a comma, or the user >+ // hasn't enabled autocomplete, then just return no matches / or the >+ // result ignored. >+ // The comma check is so that we don't autocomplete against the user >+ // entering multiple addresses. Are we allowing @ signs? >+ if (!aSearchString || aSearchString == "" || !aSearchString already implies aSearchString != "" >+ aSearchString.indexOf(",") != -1) { I'd prefer /,/.test(aSearchString) {or /[@,]/} >+ var result = new nsAbAutoCompleteResult(); >+ result.searchString = aSearchString; Prefer coding in constructor style i.e. var result = new nsAbAutoCompleteResult(aSearchString); I'd also create this earlier, as you need a result whatever happens. >+ result.searchResult = ACR.RESULT_SUCCESS_ONGOING; You're hoping to implement async results? ;-) >+ // Craft this by hand - we want the first item to contain the full string, >+ // the second item with just the first word, and the third item with >+ // anything after the first word. var [first, rest] = aSearchString.split(/\s+/, 1); perhaps? >+ if (aPreviousResult instanceof nsIAbAutoCompleteResult && >+ (aSearchString.substr(0, aPreviousResult.searchString.length) == >+ aPreviousResult.searchString) && aSearchString.lastIndexOf(aPreviousResult.searchString, 0) == 0 again

(In reply to comment #34) > Although if mailing lists have a blank primaryEmail you can use > card.primaryEmail || card.notes || card.displayName I thought about this, but I think I'd like to keep that separate. > I think these would be more readable as while loops i.e. > while (insertPosition < result._searchResults.length && > cardPopularityIndex == ... && > emailAddress < ...) > ++insertPosition; > Actually should we be comparing the lower case email addresses? > (Should we be storing the lower case email addresses in the results?) The original code didn't do anything if the popularity index was the same. To be honest, I think this is a unlikely situation, but I wanted to cover it anyway. The way I've covered it is to sort it like we do all our tree views. > >+ style: "local-abook", > What are we doing for replicated LDAP directories? This is the way we do it currently, so nothing different. > >+ // If the search string isn't value, or contains a comma, or the user > >+ // hasn't enabled autocomplete, then just return no matches / or the > >+ // result ignored. > >+ // The comma check is so that we don't autocomplete against the user > >+ // entering multiple addresses. > Are we allowing @ signs? Yes, that's what local search does at the moment. > >+ // Craft this by hand - we want the first item to contain the full string, > >+ // the second item with just the first word, and the third item with > >+ // anything after the first word. > var [first, rest] = aSearchString.split(/\s+/, 1); perhaps? Unfortunately for "a b c" that just returns ["a","b"]

Updated per comments

(In reply to comment #35) > (In reply to comment #34) > > Although if mailing lists have a blank primaryEmail you can use > > card.primaryEmail || card.notes || card.displayName > I thought about this, but I think I'd like to keep that separate. Well, maybe you could use isMailingList ? notes || displayName : primaryEmail > > I think these would be more readable as while loops i.e. > > while (insertPosition < result._searchResults.length && > > cardPopularityIndex == ... && > > emailAddress < ...) > > ++insertPosition; > > Actually should we be comparing the lower case email addresses? > > (Should we be storing the lower case email addresses in the results?) > The original code didn't do anything if the popularity index was the same. To > be honest, I think this is a unlikely situation, but I wanted to cover it > anyway. The way I've covered it is to sort it like we do all our tree views. Actually the popularity index wasn't the point here, the point was that the body of the for loop was a conditional break statement so you were effectively writing two loop exit conditions. > > >+ style: "local-abook", > > What are we doing for replicated LDAP directories? > This is the way we do it currently, so nothing different. So could getStyleAt() simply return "local-abook" all the time?

Fixed mail list, style is now just returned consistently all the time and not stored separately.

With corrected test, of course!

Comment on attachment 325783 [details] [diff] [review] [checked in] MyDomain special @ handling r=me. It'd be useful if you could see why we're getting just one item in the drop down though (when we enter email@abc for something that doesn't exist and MyDomain is on).

Comment on attachment 325781 [details] [diff] [review] Implement Toolkit interfaces for addrbook search v1.1 >+ if (firstWord.length > 0 && rest.length > 0 && Nit: could use just firstWord && rest && here >+ style: "local-abook", Unnecessary :-)

Addressed Neil's comments, carrying forward his r, requesting sr.

Comment on attachment 325961 [details] [diff] [review] [checked in] Implement Toolkit interfaces for addrbook search v1.2 + * Portions created by the Initial Developer are Copyright (C) 1999 + * the Initial Developer. All Rights Reserved. Was this file copied from an old file? Or should this be 2008? @$$}7}@ \ No newline at end of file I don't think Mork would care about adding a new line here, not that it matters...

(In reply to comment #43) > (From update of attachment 325961 [details] [diff] [review]) > + * Portions created by the Initial Developer are Copyright (C) 1999 > + * the Initial Developer. All Rights Reserved. > > Was this file copied from an old file? Or should this be 2008? I had copied the file from nsIAbAutoCompleteSearch.idl originally, and just changing it where necessary. However given the changes you could probably say I could have written it from scratch and no-one would be able to tell. > @$$}7}@ > \ No newline at end of file > > I don't think Mork would care about adding a new line here, not that it > matters... > I think that's possibly because its in mac format? Displays fine in Emacs in mac mode.

Comment on attachment 325961 [details] [diff] [review] [checked in] Implement Toolkit interfaces for addrbook search v1.2 Checked in. LDAP next...

I was trying out some toolkit things earlier and noticed the defaultIndex implementation in the new search code is different to the old nsAbAutoCompleteSession, this patch matches them up.

Comment on attachment 326676 [details] [diff] [review] [checked in] Fix default index setting. >+ else >+ result.searchResult = ACR.RESULT_NOMATCH; I don't suppose you'd mind making the default result ACR.RESULT_NOMATCH? (The if block makes the else statement look lonely!)

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

Neil and I realised (after chatting on irc) that we have broken the current functionality that looks up addresses in the background when you copy and paste (or drag and drop) names onto the addressing widget. As we really need everything on toolkit interfaces before we fix this, these changes just disable the code (but still allows correct drag and drop separation of names) and bug 456550 has been raised to put them back later.

Comment on attachment 339947 [details] [diff] [review] [checked in] Disable background-autocomplete on paste for now >+ We should fix it see bug 456550. Nit: tab

Comment on attachment 339947 [details] [diff] [review] [checked in] Disable background-autocomplete on paste for now Checked in, changeset id: 408:afc9faafb312

This isn't making beta 1... moving out.

I'm marking this as fixed. The remaining parts to do are: - Move LDAP over to toolkit interfaces (bug 452232) - Move TB over to toolkit autocomplete (bug 360648)

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/c5c58619fe8f Move the "my domain" autocomplete suggestion to a separate JS component b=370306 r=Standard8 sr=bienvenu