Closed
Bug 370306
Opened 18 years ago
Closed 16 years ago
Move Address Book's local autocomplete implementations to be based on toolkit's
Categories
(MailNews Core :: Address Book, defect, P1)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(10 files, 12 obsolete files)
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+
neil
:
superreview+
|
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+
neil
:
superreview+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
Bienvenu
:
review+
neil
:
superreview+
|
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.
Assignee | ||
Updated•17 years ago
|
Blocks: mailnews-libxul
Assignee | ||
Comment 2•17 years ago
|
||
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
Comment 3•17 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•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
(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.
Assignee | ||
Comment 6•17 years ago
|
||
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•17 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...
Assignee | ||
Comment 8•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
Assignee | ||
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Attachment #322571 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 13•17 years ago
|
||
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•17 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•17 years ago
|
||
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
I'll commit the relevant part of this as I get reviews.
Attachment #323200 -
Flags: review?(philringnalda)
Attachment #323200 -
Flags: review?(neil)
Comment 18•17 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+
Assignee | ||
Updated•17 years ago
|
Attachment #321385 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #321386 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
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
Assignee | ||
Updated•17 years ago
|
Attachment #320721 -
Attachment is obsolete: true
Assignee | ||
Comment 19•17 years ago
|
||
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•17 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?
Updated•17 years ago
|
Attachment #323200 -
Flags: review?(philringnalda) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #323200 -
Attachment description: Add nsAbAutoCompleteMyDomain.js to packages files. → [checked in] Add nsAbAutoCompleteMyDomain.js to packages files.
Assignee | ||
Comment 21•17 years ago
|
||
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
Assignee | ||
Comment 22•17 years ago
|
||
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•17 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•17 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.
Assignee | ||
Comment 25•17 years ago
|
||
Comment on attachment 323118 [details] [diff] [review]
[checked in] autocompletesearchparam
Thanks for the explanation, this works well.
Attachment #323118 -
Flags: review+
Assignee | ||
Comment 26•17 years ago
|
||
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)
Assignee | ||
Comment 27•17 years ago
|
||
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•17 years ago
|
Attachment #323118 -
Flags: superreview?(bienvenu)
Comment 28•16 years ago
|
||
Needs attachment 323118 [details] [diff] [review] too of course.
Attachment #324755 -
Flags: review?(bugzilla)
Assignee | ||
Comment 29•16 years ago
|
||
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•16 years ago
|
Attachment #323118 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Updated•16 years ago
|
Attachment #323118 -
Attachment description: autocompletesearchparam → [checked in] autocompletesearchparam
Assignee | ||
Updated•16 years ago
|
Attachment #324755 -
Flags: review?(bugzilla) → review+
Updated•16 years ago
|
Attachment #324755 -
Flags: superreview?(bienvenu)
Updated•16 years ago
|
Attachment #324755 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Updated•16 years ago
|
Attachment #324755 -
Attachment description: mydomain using autocompletesearchparam → [checked in] mydomain using autocompletesearchparam
Assignee | ||
Comment 30•16 years ago
|
||
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•16 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+
Assignee | ||
Comment 32•16 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
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
Assignee | ||
Comment 33•16 years ago
|
||
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•16 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-
Assignee | ||
Comment 35•16 years ago
|
||
(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"]
Assignee | ||
Comment 36•16 years ago
|
||
Updated per comments
Attachment #325263 -
Attachment is obsolete: true
Attachment #325749 -
Flags: review?(neil)
Comment 37•16 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?
Assignee | ||
Comment 38•16 years ago
|
||
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•16 years ago
|
||
With corrected test, of course!
Attachment #325783 -
Flags: superreview?(bienvenu)
Attachment #325783 -
Flags: review?(bugzilla)
Updated•16 years ago
|
Attachment #325783 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 40•16 years ago
|
||
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•16 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+
Assignee | ||
Updated•16 years ago
|
Attachment #325783 -
Attachment description: MyDomain special @ handling → [checked in] MyDomain special @ handling
Assignee | ||
Comment 42•16 years ago
|
||
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•16 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...
Assignee | ||
Comment 44•16 years ago
|
||
(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•16 years ago
|
Attachment #325961 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 45•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Assignee | ||
Comment 46•16 years ago
|
||
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•16 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+
Assignee | ||
Updated•16 years ago
|
Attachment #326676 -
Attachment description: Fix default index setting. → [checked in] Fix default index setting.
Updated•16 years ago
|
Product: Core → MailNews Core
Updated•16 years ago
|
Flags: blocking-thunderbird3.0b1? → blocking-thunderbird3.0b1+
Updated•16 years ago
|
Target Milestone: --- → Thunderbird 3.0b1
Assignee | ||
Updated•16 years ago
|
Whiteboard: [waiting for bug 452232]
Assignee | ||
Comment 48•16 years ago
|
||
3.0b1 flag is going away in favour of 3.0 flag and milestone combination.
Flags: blocking-thunderbird3.0b1+ → blocking-thunderbird3+
Assignee | ||
Comment 49•16 years ago
|
||
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•16 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•16 years ago
|
Attachment #339947 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 51•16 years ago
|
||
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
Assignee | ||
Comment 52•16 years ago
|
||
This isn't making beta 1... moving out.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Updated•16 years ago
|
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Assignee | ||
Comment 53•16 years ago
|
||
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
Closed: 16 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]
Comment 54•7 years 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.
Description
•