Move Address Book's local autocomplete implementations to be based on toolkit's

RESOLVED FIXED in Thunderbird 3.0b2

Status

P1
normal
RESOLVED FIXED
12 years ago
11 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 3.0b2
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 12 obsolete attachments)

12.37 KB, patch
Details | Diff | Splinter Review
29.60 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.20 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.46 KB, patch
neil
: review+
philor
: review+
Details | Diff | Splinter Review
10.50 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
4.30 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.79 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
43.96 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.42 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.25 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 432161
Blocks: 432162
Blocks: 360648
Created attachment 320701 [details] [diff] [review]
Build Config patch

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.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Depends on: 433497

Comment 3

11 years ago
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.

Comment 4

11 years ago
Created attachment 320721 [details] [diff] [review]
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.
(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.
Created attachment 321247 [details] [diff] [review]
Implement Toolkit interfaces (WIP)

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.

Comment 7

11 years ago
> 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...

Comment 9

11 years ago
Created attachment 321385 [details]
Separate component for "my domain" autocomplete

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

Comment 10

11 years ago
Created attachment 321386 [details]
nsIAbAutoCompleteSearch.idl
Created attachment 322246 [details] [diff] [review]
Implement Toolkit interfaces v0.2 (WIP)

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.
Attachment #321247 - Attachment is obsolete: true

Comment 12

11 years ago
Created attachment 322571 [details] [diff] [review]
[checked in] Split off autocomplete to my domain into a separate search

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.
Attachment #322571 - Flags: superreview?(bienvenu)
Attachment #322571 - Flags: review?(bugzilla)

Updated

11 years ago
Attachment #322571 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #322571 - Flags: review?(bugzilla) → review+

Comment 14

11 years ago
(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...

Comment 15

11 years ago
Created attachment 323118 [details] [diff] [review]
[checked in] autocompletesearchparam
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.
Created attachment 323200 [details] [diff] [review]
[checked in] Add nsAbAutoCompleteMyDomain.js to packages files.

I'll commit the relevant part of this as I get reviews.
Attachment #323200 - Flags: review?(philringnalda)
Attachment #323200 - Flags: review?(neil)

Comment 18

11 years ago
Comment on attachment 323200 [details] [diff] [review]
[checked in] Add nsAbAutoCompleteMyDomain.js to packages files.

D'oh!
Attachment #323200 - Flags: review?(neil) → review+
Attachment #321385 - Attachment is obsolete: true
Attachment #321386 - Attachment is obsolete: true
Attachment #322571 - Attachment description: Split off autocomplete to my domain into a separate search → [checked in] Split off autocomplete to my domain into a separate search
Attachment #320721 - Attachment is obsolete: true
Created attachment 323379 [details] [diff] [review]
Implement Toolkit interfaces v0.3 (WIP)

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.
Attachment #322246 - Attachment is obsolete: true

Comment 20

11 years ago
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?
Attachment #323200 - Flags: review?(philringnalda) → review+
Attachment #323200 - Attachment description: Add nsAbAutoCompleteMyDomain.js to packages files. → [checked in] Add nsAbAutoCompleteMyDomain.js to packages files.
Created attachment 324323 [details] [diff] [review]
Implement Toolkit interfaces v0.5 (WIP)

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.
Attachment #323379 - Attachment is obsolete: true
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 23

11 years ago
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 24

11 years ago
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.
Attachment #323118 - Flags: review+
Depends on: 438333
Created attachment 324601 [details] [diff] [review]
Provide test case for MyDomain (inc 1 fix) and sort out EXTRA_COMPONENTS in Makefile.in files

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...
Attachment #324601 - Flags: superreview?(neil)
Attachment #324601 - Flags: review?(neil)
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.
Attachment #324601 - Attachment is obsolete: true
Attachment #324601 - Flags: superreview?(neil)
Attachment #324601 - Flags: review?(neil)

Updated

11 years ago
Attachment #323118 - Flags: superreview?(bienvenu)

Comment 28

11 years ago
Created attachment 324755 [details] [diff] [review]
[checked in] mydomain using autocompletesearchparam

Needs attachment 323118 [details] [diff] [review] too of course.
Attachment #324755 - Flags: review?(bugzilla)
Depends on: 438861
Created attachment 324815 [details] [diff] [review]
Implement Toolkit interfaces v0.6 (WIP)

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"
Attachment #324323 - Attachment is obsolete: true

Updated

11 years ago
Attachment #323118 - Flags: superreview?(bienvenu) → superreview+
Attachment #323118 - Attachment description: autocompletesearchparam → [checked in] autocompletesearchparam
Attachment #324755 - Flags: review?(bugzilla) → review+

Updated

11 years ago
Attachment #324755 - Flags: superreview?(bienvenu)

Updated

11 years ago
Attachment #324755 - Flags: superreview?(bienvenu) → superreview+
Attachment #324755 - Attachment description: mydomain using autocompletesearchparam → [checked in] mydomain using autocompletesearchparam
Created attachment 325246 [details] [diff] [review]
[checked in] Provide test case for MyDomain and sort out EXTRA_COMPONENTS in Makefile.in files v2

Updated unit test for nsAbAutoCompleteMyDomain, including changes to check with an identity passed via the search param.
Attachment #325246 - Flags: superreview?(neil)
Attachment #325246 - Flags: review?(neil)

Comment 31

11 years ago
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.
Attachment #325246 - Flags: superreview?(neil)
Attachment #325246 - Flags: superreview+
Attachment #325246 - Flags: review?(neil)
Attachment #325246 - Flags: review+
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.
Attachment #325246 - Attachment description: Provide test case for MyDomain and sort out EXTRA_COMPONENTS in Makefile.in files v2 → [checked in] Provide test case for MyDomain and sort out EXTRA_COMPONENTS in Makefile.in files v2
Created attachment 325263 [details] [diff] [review]
Implement Toolkit interfaces for addrbook search v0.9 (WIP)

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.
Attachment #324815 - Attachment is obsolete: true
Attachment #325263 - Flags: review?(neil)

Comment 34

11 years ago
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
Attachment #325263 - Flags: review?(neil) → review-
(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"]
Created attachment 325749 [details] [diff] [review]
Implement Toolkit interfaces for addrbook search v1

Updated per comments
Attachment #325263 - Attachment is obsolete: true
Attachment #325749 - Flags: review?(neil)

Comment 37

11 years ago
(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?
Created attachment 325781 [details] [diff] [review]
Implement Toolkit interfaces for addrbook search v1.1

Fixed mail list, style is now just returned consistently all the time and not stored separately.
Attachment #325749 - Attachment is obsolete: true
Attachment #325781 - Flags: review?(neil)
Attachment #325749 - Flags: review?(neil)

Comment 39

11 years ago
Created attachment 325783 [details] [diff] [review]
[checked in] MyDomain special @ handling

With corrected test, of course!
Attachment #325783 - Flags: superreview?(bienvenu)
Attachment #325783 - Flags: review?(bugzilla)

Updated

11 years ago
Attachment #325783 - Flags: superreview?(bienvenu) → superreview+
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).
Attachment #325783 - Flags: review?(bugzilla) → review+

Comment 41

11 years ago
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 :-)
Attachment #325781 - Flags: review?(neil) → review+
Attachment #325783 - Attachment description: MyDomain special @ handling → [checked in] MyDomain special @ handling
Created attachment 325961 [details] [diff] [review]
[checked in] Implement Toolkit interfaces for addrbook search v1.2

Addressed Neil's comments, carrying forward his r, requesting sr.
Attachment #325781 - Attachment is obsolete: true
Attachment #325961 - Flags: superreview?(bienvenu)
Attachment #325961 - Flags: review+

Comment 43

11 years ago
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.

Updated

11 years ago
Attachment #325961 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 325961 [details] [diff] [review]
[checked in] Implement Toolkit interfaces for addrbook search v1.2

Checked in. LDAP next...
Attachment #325961 - Attachment description: Implement Toolkit interfaces for addrbook search v1.2 → [checked in] Implement Toolkit interfaces for addrbook search v1.2
Priority: -- → P1
Created attachment 326676 [details] [diff] [review]
[checked in] Fix default index setting.

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.
Attachment #326676 - Flags: superreview?(neil)
Attachment #326676 - Flags: review?(neil)

Comment 47

10 years ago
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!)
Attachment #326676 - Flags: superreview?(neil)
Attachment #326676 - Flags: superreview+
Attachment #326676 - Flags: review?(neil)
Attachment #326676 - Flags: review+
Attachment #326676 - Attachment description: Fix default index setting. → [checked in] Fix default index setting.
Product: Core → MailNews Core
Depends on: 443370
Flags: blocking-thunderbird3.0b1?

Updated

10 years ago
Flags: blocking-thunderbird3.0b1? → blocking-thunderbird3.0b1+

Updated

10 years ago
Target Milestone: --- → Thunderbird 3.0b1
Depends on: 452232
Whiteboard: [waiting for bug 452232]
3.0b1 flag is going away in favour of 3.0 flag and milestone combination.
Flags: blocking-thunderbird3.0b1+ → blocking-thunderbird3+
Created attachment 339947 [details] [diff] [review]
[checked in] Disable background-autocomplete on paste for now

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.
Attachment #339947 - Flags: superreview?(neil)
Attachment #339947 - Flags: review?(bienvenu)

Comment 50

10 years ago
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
Attachment #339947 - Flags: superreview?(neil) → superreview+

Updated

10 years ago
Attachment #339947 - Flags: review?(bienvenu) → review+
Comment on attachment 339947 [details] [diff] [review]
[checked in] Disable background-autocomplete on paste for now

Checked in, changeset id: 408:afc9faafb312
Attachment #339947 - Attachment description: Disable background-autocomplete on paste for now → [checked in] Disable background-autocomplete on paste for now
This isn't making beta 1... moving out.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
No longer depends on: 433497
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: Move Address Book's autocomplete (and addressing widget) implementation to be based on toolkit's → Move Address Book's local autocomplete implementations to be based on toolkit's
Whiteboard: [waiting for bug 452232]
No longer depends on: 452232

Comment 54

11 months ago
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
You need to log in before you can comment on or make changes to this bug.