Closed
Bug 452232
Opened 16 years ago
Closed 11 years ago
Move LDAP autocomplete over to toolkit interfaces
Categories
(MailNews Core :: LDAP Integration, defect)
MailNews Core
LDAP Integration
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 25.0
People
(Reporter: standard8, Assigned: standard8)
References
(Depends on 1 open bug, Blocks 1 open bug, Regressed 1 open bug)
Details
(Keywords: addon-compat, memory-footprint)
Attachments
(7 files, 18 obsolete files)
29.25 KB,
patch
|
standard8
:
review+
jhorak
:
feedback+
|
Details | Diff | Splinter Review |
733 bytes,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
6.90 KB,
patch
|
jhorak
:
review+
jhorak
:
superreview+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
49.11 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
88.16 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
Just like we've done for local address book autocomplete in bug 370306 we need to move LDAP autocomplete over to toolkit interfaces.
The proposal to do this:
- Implement a new service to interface directly to toolkit autocomplete, and provide a wrapper around the existing sessions.
- Move the identity set-up functions (see MsgComposeCommands.js) into the sessions, there is now a parameter which specifies which identity to use.
- The new service will load (and cache) a session for an identity.
- Note the sessions must be unloaded on shut-down (or potentially when the compose window is closed). This could be done via an extra interface to the new service.
Flags: wanted-thunderbird3+
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Assignee | ||
Updated•16 years ago
|
Whiteboard: [at risk]
Updated•16 years ago
|
Flags: blocking-thunderbird3+
Updated•16 years ago
|
Assignee: nobody → bugzilla
Assignee | ||
Comment 1•16 years ago
|
||
A very WIP version, still sorting out error handling, and xpfe autocomplete doesn't seem to like it as much as toolkit autocomplete.
Assignee | ||
Comment 2•16 years ago
|
||
This isn't making beta 1... moving out.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Comment 3•16 years ago
|
||
+ Components.classes["@mozilla.org/observer-service;1"]
+ .getService(Components.interfaces.nsIObserverService)
+ .notifyObservers(gMsgCompose, "compose-window-close", "");
What will we call this when we support LDAP completed address lists? ;-)
+ errorDescription: null,
IIRC this is what you're supposed to set when there's an error. The xpfe autocomplete should convert this internally into a fake (unselectable) result... I don't know about toolkit though.
+ // The cache is indexed by the address book URI. Each item in the cache
+ // then has three items:
+ // book - the address book associated with the URI.
+ // query - the nsAbLDAPDirectoryQuery in use for the URI.
+ // context - the search context for the URI (used for cancelling the search).
Out of interest, is changing the LDAP settings live? (I'd want to be able to fix a "broken" autocomplete without closing the compose window.)
+ // Find out where to insert the card.
+ var insertPosition = 0;
+
+ // Next sort on full address
+ while (insertPosition < this._result._searchResults.length &&
+ emailAddress > this._result._searchResults[insertPosition].value)
+ ++insertPosition;
+
+ this._result._searchResults.splice(insertPosition, 0, {
+ value: emailAddress,
+ card: card,
+ });
Autocomplete doesn't expect search results to mutate as autocomplete progresses. Specifically, this code could change the data for a row that the user had already selected.
+ if (topic == "compose-window-close") {
+ // Force the individual query items to null, so that the memory
+ // gets collected straight away.
+ for (var item in this._cachedQueries) {
+ this._cachedQueries[item].query = null;
+ this._cachedQueries[item].book = null;
+ }
+ this._cachedQueries = {};
+ Components.classes["@mozilla.org/observer-service;1"]
+ .getService(Components.interfaces.nsIObserverService)
+ .removeObserver(this, "compose-window-close");
This is the wrong time to remove the observer. Alternatively you could implement nsISupportsWeakReference and add yourself as a weak observer.
+ if (this._result.matchCount) {
+ this._result.searchResult = ACR.RESULT_SUCCESS_ONGOING;
+ this._result.defaultIndex = 0;
This will make autocomplete default to the first found index... if you want.
Comment 4•16 years ago
|
||
While we'd love to get this for 3.0b1, we wouldn't actually hold the release for it, so moving the target milestone out.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Assignee | ||
Comment 5•16 years ago
|
||
This doesn't need to block TB 3 any more, we have a way around it if we need it. Still very much wanted as it would make it easier for extensions.
Resetting assignee to default as I don't have time to work on this at the moment.
Assignee: bugzilla → nobody
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Priority: P1 → --
Whiteboard: [at risk] → [probably needs bug 343332 fixing to solve shutdown with ldap ssl issues]
Target Milestone: Thunderbird 3.0b2 → ---
Assignee | ||
Updated•16 years ago
|
Comment 6•14 years ago
|
||
Any chance to get this updated to trunk? The attached WIP fails to apply and I'd like to see how far we can compile with libxul right now. IMHO, doing that should be a top priority for mozilla-2.0-based releases, but we break in LDAP right now - is this patch alone supposed to fix that or do we need even more?
Assignee | ||
Comment 7•14 years ago
|
||
I'm going to try and get my head back around this patch now that bug 343332 is almost fixed.
Assignee: nobody → bugzilla
Assignee | ||
Updated•14 years ago
|
Whiteboard: [probably needs bug 343332 fixing to solve shutdown with ldap ssl issues]
Assignee | ||
Comment 8•14 years ago
|
||
Updated patch which needs the patch in bug 633150 to be able to apply and work.
This is basically an update to get the original WIP patch working again, and certainly needs work in the new ldap autocomplete component before its finished, as well as some general tidy up I expect. I also suspect there's some more code to be removed from other areas as well.
Attachment #338096 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
I've tried the WIPv2 patch with latest comm-central. I've managed to compile it successfully but autocomplete does not work in compose. Is there any way I can help? This patch is one of few that are missing to link comm-central against libxul.
Comment 10•13 years ago
|
||
Comment on attachment 511359 [details] [diff] [review]
WIP v2
+ args.expression = abMgr.convertQueryString(searchStr);
This got renamed to convertQueryStringToExpression but after fixing that I'm still getting an error: Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIAbDirectoryQuery.doQuery]
Comment 11•13 years ago
|
||
Comment on attachment 511359 [details] [diff] [review]
WIP v2
>+ var supportsString = Components.classes["@mozilla.org/supports-cstring;1"]
>+ .createInstance(Components.interfaces.nsISupportsCString);
>+
>+ supportsString.data = attributes;
>+
>+ args.typeSpecificArg = supportsString;
This needs to be an nsIAbLDAPAttributeMap. I was able to get search results to show up by using queryObject.book.attributeMap here.
Comment 12•13 years ago
|
||
This code snippet seems to create an appropriate map. I don't know whether we should cache the map; it doesn't seem worthwhile creating it each time:
attributes =
Components.classes["@mozilla.org/addressbook/ldap-attribute-map;1"]
.createInstance(Components.interfaces.nsIAbLDAPAttributeMap);
attributes.setAttributeList("DisplayName", "cn,commonname", false);
attributes.setAttributeList("PrimaryEmail", "mail", false);
Comment 13•13 years ago
|
||
(In reply to comment #11)
> (From update of attachment 511359 [details] [diff] [review])
> >+ var supportsString = Components.classes["@mozilla.org/supports-cstring;1"]
> >+ .createInstance(Components.interfaces.nsISupportsCString);
> >+
> >+ supportsString.data = attributes;
> >+
> >+ args.typeSpecificArg = supportsString;
> This needs to be an nsIAbLDAPAttributeMap. I was able to get search results
> to show up by using queryObject.book.attributeMap here.
Ah, I see now, I failed to apply the patch to nsAbLDAPDirectoryQuery.cpp
Comment 14•13 years ago
|
||
This seems to work in SeaMonkey.
This patch does not include any file removals or Thunderbird-specific changes.
Comment 15•13 years ago
|
||
It doesn't seems to work in Thunderbird. Searching in Address book works just fine, but when composing new message an ldap lookup is not performed during typing character in To: field. Only addresses from local address book are shown (I've got Preferences/Composition/Addressing/Address Autocompletion/Directory server enabled).
Comment 16•13 years ago
|
||
Including the bits of 511359 that I didn't include last time.
I built Thunderbird just to prove that it would work, which it did.
Attachment #577426 -
Attachment is obsolete: true
Comment 17•13 years ago
|
||
I managed to fix latest patch for me. It missed following code:
getLabelAt: function getLabelAt(aIndex) {
return this.getValueAt(aIndex);
},
Attaching patch as reference. It's working just fine now. What's required to finish this bug?
Assignee | ||
Comment 18•13 years ago
|
||
(In reply to jhorak from comment #17)
> Attaching patch as reference. It's working just fine now. What's required to
> finish this bug?
I believe there are two things:
1) Patch Lightning. This uses the LDAP autocomplete on the event attendees dialog, and we should get it hooked up there.
2) Teach nsAbLDAPAutoCompleteSearch to respect the ldap_2.<server>.autoComplete.filterTemplate preference. This seems to be frequently tweaked and known having done a google search (hence I don't think we can drop it).
This latter issue is probably where I didn't get time to complete the patch. For it, we need to do:
- a bit of work at the end of nsAbLDAPAutoCompleteSearch.startSearch to get the preference if it is set (I wonder if its worth adding a default pref at the same time). This work will also need to make sure all the parameters are in the attribute map that gets passed to the query.
- Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the preference (this may be the more difficult bit, as we either need to hook into the search capabilities of nsAbDirectoryQuery, or re-implement them ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query where we already have query results.
Also, I'm not sure if the current LDAP implementation supports returning results where the display name has matched, and the SecondEmail is listed. If it does, then we should implement that here. If not, then we can always do that later (or just move to supporting mulit-valued mail elements).
Assignee: mbanner → nobody
Updated•13 years ago
|
Severity: normal → major
Comment 19•13 years ago
|
||
Attaching patch which can be applied on current trunk.
Comment 20•13 years ago
|
||
Oh, sorry I forgot to hg add some files to patch.
Updated•13 years ago
|
Attachment #620703 -
Attachment is obsolete: true
Comment 21•13 years ago
|
||
This adds a configure option --enable-incomplete-toolkit-ldap-autocomplete and then there are ifdefs in all the core places so that only the selected components actually get built.
I could move the option out of the LDAP block if you like, it just means an extra ifdef in the manifest. (The other uses are already ifdef MOZ_LDAP_XPCOM.)
No ifdefs are required in the compose code as it is written to handle --disable-ldap anyway.
Attachment #620741 -
Flags: review?(mbanner)
Comment 22•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #21)
> Created attachment 620741 [details] [diff] [review]
> Add configure option
>
> This adds a configure option --enable-incomplete-toolkit-ldap-autocomplete
> and then there are ifdefs in all the core places so that only the selected
> components actually get built.
>
> I could move the option out of the LDAP block if you like, it just means an
> extra ifdef in the manifest. (The other uses are already ifdef
> MOZ_LDAP_XPCOM.)
>
> No ifdefs are required in the compose code as it is written to handle
> --disable-ldap anyway.
I'm a bit confused here, I can't see enable-incomplete-toolkit-ldap-autocomplete parameter in the patch. Do I miss something?
Comment 23•13 years ago
|
||
D'oh, you'd think that for a patch that adds a configure option that I would remember to include configure.in in my diff...
Attachment #620741 -
Attachment is obsolete: true
Attachment #622350 -
Flags: review?(mbanner)
Attachment #622350 -
Flags: feedback?(jhorak)
Attachment #620741 -
Flags: review?(mbanner)
Assignee | ||
Comment 24•13 years ago
|
||
Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]
Review of attachment 622350 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, I'm happy with this. I've not looked at nsAbLDAPAutoCompleteSearch.js as I basically wrote most of that and I think we've done some back and forth before over fixing it that it is good enough for the experimental option.
::: suite/mailnews/compose/addressingWidgetOverlay.xul
@@ +78,5 @@
> <textbox id="addressCol2#1" class="plain textbox-addressingWidget uri-element"
> aria-labelledby="addressCol1#1"
> type="autocomplete" flex="1" maxrows="4"
> newlines="replacewithcommas"
> + autocompletesearch="mydomain addrbook ldap" timeout="300"
This change needs to be done in calendar-event-dialog-attendees.xml as well (this can be included in my r+).
Attachment #622350 -
Flags: review?(mbanner) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #511359 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #578288 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #581283 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #620706 -
Attachment is obsolete: true
Comment 25•13 years ago
|
||
Try run for 2d9fc5ebbe42 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=2d9fc5ebbe42
Results (out of 38 total builds):
exception: 1
success: 23
warnings: 11
failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-2d9fc5ebbe42
Comment 26•13 years ago
|
||
Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]
Pushed changeset dff73c42414f to comm-central.
Comment 27•13 years ago
|
||
Try run for e28d9dcf40e6 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=e28d9dcf40e6
Results (out of 37 total builds):
exception: 1
success: 19
warnings: 15
failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-e28d9dcf40e6
Comment 28•13 years ago
|
||
This function is missing in nsAbLDAPAutoCompleteSearch.js and it's called during autocompletion of address in compose window.
Attachment #624752 -
Flags: review?(mbanner)
Comment 29•13 years ago
|
||
Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]
I can build TB with external linkages now. This seems to be for me very promising in order to build with external libxul.
Attachment #622350 -
Flags: feedback?(jhorak) → feedback+
Assignee | ||
Updated•13 years ago
|
Attachment #624752 -
Flags: review?(mbanner) → review+
Comment 30•13 years ago
|
||
Setting checkin-needed for attachment: https://bug452232.bugzilla.mozilla.org/attachment.cgi?id=624752
Keywords: checkin-needed
Comment 31•13 years ago
|
||
Attachment 624752 [details] [diff] committed to comm-central as https://hg.mozilla.org/comm-central/rev/490b4c844b34
Updated•13 years ago
|
Target Milestone: --- → Thunderbird 15.0
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 15.0 → ---
Updated•13 years ago
|
Attachment #624752 -
Attachment description: Adding missing function getLabelAt() → Adding missing function getLabelAt()
[Checked in: Comment 31]
Updated•13 years ago
|
Attachment #622350 -
Attachment description: Really add configure option → Really add configure option
[Checked in: Comment 26]
Comment 32•12 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #18)
> 2) Teach nsAbLDAPAutoCompleteSearch to respect the
> ldap_2.<server>.autoComplete.filterTemplate preference. This seems to be
> frequently tweaked and known having done a google search (hence I don't
> think we can drop it).
>
> This latter issue is probably where I didn't get time to complete the patch.
> For it, we need to do:
>
> - a bit of work at the end of nsAbLDAPAutoCompleteSearch.startSearch to get
> the preference if it is set (I wonder if its worth adding a default pref at
> the same time). This work will also need to make sure all the parameters are
> in the attribute map that gets passed to the query.
I'm looking at ldap_2.<server>.autoComplete.filterTemplate preference. In code I see that from filter string (like (or(PrimaryEmail,bw,%v)(DisplayName,bw,%v))) is created nsIAbBooleanExpression which is later used to create ldap filter expression: (|(sn=%v*)...). The ldap_2.<server>.autoComplete.filterTemplate variable is expected to be ldap filter expression string.
Current situation:
args.expression = abMgr.convertQueryStringToExpression(searchStr);
in:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js#298
conversion to ldap filter expression:
rv = nsAbBoolExprToLDAPFilter::Convert(map, expression, filter);
in:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp#433
I would rather produce ldap filter expression string in nsAbLDAPAutoCompleteSearch.startSearch (while there is a search term and default filter experssion can be easily overrided it by content of ldap_2.<server>.autoComplete.filterTemplate) and pass it to nsAbLDAPDirectoryQuery::DoQuery. However this would break nsIAbDirectoryQuery interface. Any hints?
Comment 33•12 years ago
|
||
Patch is using nsILDAPService::CreateFilter to create filter from filterTemplate and search string.
Attachment #656435 -
Flags: review?(mbanner)
Assignee | ||
Comment 34•12 years ago
|
||
Comment on attachment 656435 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v1
Somehow, I didn't see this when it came in, and unfortunately this is the same as the previous patch on this bug - attachment 624752 [details] [diff] [review].
Attachment #656435 -
Flags: review?(mbanner)
Comment 35•12 years ago
|
||
Oh dear, I've attached wrong patch, sorry about it. This is the right one.
Attachment #656435 -
Attachment is obsolete: true
Attachment #664381 -
Flags: review?(mbanner)
Assignee | ||
Comment 36•12 years ago
|
||
Comment on attachment 664381 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v2
Review of attachment 664381 [details] [diff] [review]:
-----------------------------------------------------------------
I think you're heading in the right direction, but the problem with doing the work of forming the filter in nsAbLDAPDirectoryQuery is that it will affect all LDAP queries, not just the autocomplete ones.
What I'd suggest you do is to move the filter formation into nsAbLDAPAutoCompleteSearch.startSearch replacing the existing searchStr values (which were just there temporarily anyway). You can then pass that filter via the query args.
You'll want to use the same default as we have now for that string as well (http://hg.mozilla.org/comm-central/annotate/246b6dfaf2e7/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp#l39).
Attachment #664381 -
Flags: review?(mbanner) → review-
Comment 37•12 years ago
|
||
Thanks for looking, I hope I got it right. Attaching new patch which moves creating of filter to nsAbLDAPAutoCompleteSearch.js.
Attachment #664381 -
Attachment is obsolete: true
Attachment #664906 -
Flags: review?(mbanner)
Comment 38•12 years ago
|
||
Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3
>+ var ldapSvc = Components.classes["@mozilla.org/network/ldap-service;1"]
>+ .createInstance(Components.interfaces.nsILDAPService);
[Not read the patch thoroughly but this should probably use getService]
Assignee | ||
Comment 39•12 years ago
|
||
Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3
A quick glance and this is what I was thinking of, so I'll try and take a look at it tomorrow.
Comment 40•12 years ago
|
||
Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3
>+ var filterTemplate;
>+ // Check if autoComplete.filterTemplate pref for selected ldap server exists
>+ try {
>+ var filterTemplate = prefSvc.getCharPref(acDirURI + ".autoComplete.filterTemplate");
>+ } catch (e) { }
>+
>+ // When filter template not found use default template
>+ if (!filterTemplate)
>+ filterTemplate = "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))";
It might be possible to replace this with
filterTemplate = book.getStringValue("autoComplete.filterTemplate",
"(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");
Comment 41•12 years ago
|
||
Thanks for comments, attaching better version (getService and usage of queryObject.book).
Attachment #664906 -
Attachment is obsolete: true
Attachment #664906 -
Flags: review?(mbanner)
Attachment #665324 -
Flags: review?(mbanner)
Comment 42•12 years ago
|
||
I'm looking at this one now:
(In reply to Mark Banner (:standard8) from comment #18)
> - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> preference (this may be the more difficult bit, as we either need to hook
> into the search capabilities of nsAbDirectoryQuery, or re-implement them
> ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> where we already have query results.
I'm not sure how to solve it. It might be possible to map filterTemplate content to list of card parameters (something like opposite of nsAbBoolExprToLDAPFilter::Convert) and then use _checkEntry to test if search string matches. This could be more complicated for ldap binary operators.
Could you please elaborate hooking into nsAbDirectryQuery a little further? Do you mean to somehow modify DoQuery to use cached search results?
Comment 43•12 years ago
|
||
Doesn't the XPFE code re-query each time? (This has the side-effect of working around the 100-card query result limit.) In which case, if you did want to add caching, perhaps you could just limit it to the common case of no template.
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #43)
> Doesn't the XPFE code re-query each time? (This has the side-effect of
> working around the 100-card query result limit.) In which case, if you did
> want to add caching, perhaps you could just limit it to the common case of
> no template.
Not sure about XPFE version of the LDAP backend, but the caching already works in the auto complete module we've implemented so far (with xpfe autocomplete).
Assignee | ||
Comment 45•12 years ago
|
||
Comment on attachment 665324 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v4
Review of attachment 665324 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/addrbook/public/nsIAbDirectoryQuery.idl
@@ +39,5 @@
> * A parameter which can be used to pass in data specific to a particular
> * type of addressbook.
> */
> attribute nsISupports typeSpecificArg;
> +
nit: drop the spaces from this line and the one just after the function definition.
::: mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js
@@ +290,5 @@
> Components.classes["@mozilla.org/addressbook/directory/query-arguments;1"]
> .createInstance(Components.interfaces.nsIAbDirectoryQueryArguments);
>
> + var filterTemplate = queryObject.book.getStringValue("autoComplete.filterTemplate",
> + "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");
nit: whitespace at end of line.
::: mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
@@ +428,5 @@
> +
> + if (filter.IsEmpty()) {
> + // Get the filter
> + nsCOMPtr<nsISupports> supportsExpression;
> + rv = aArguments->GetExpression (getter_AddRefs (supportsExpression));
nit: no space before open brackets (and for the expression line), these were there before, but please tidy as you're touching them.
@@ +435,5 @@
> + nsCOMPtr<nsIAbBooleanExpression> expression (do_QueryInterface (supportsExpression, &rv));
> +
> + // figure out how we map attribute names to addressbook fields for this
> + // query
> + rv = nsAbBoolExprToLDAPFilter::Convert(map, expression, filter);
Shouldn't we be checking rv here?
Assignee | ||
Comment 46•12 years ago
|
||
Comment on attachment 665324 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v4
This is looking good, but I'd like to see an updated version with comment 45 items fixed.
Attachment #665324 -
Flags: review?(mbanner) → review-
Comment 47•12 years ago
|
||
Fixed nits and throwing exception when filter is empty.
Attachment #665324 -
Attachment is obsolete: true
Attachment #669912 -
Flags: review?(mbanner)
Assignee | ||
Comment 48•12 years ago
|
||
Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5
I think you got the title wrong for this patch, so updating it.
Attachment #669912 -
Attachment description: Save uri for downloaded files patch v5 → Support for ldap_2.<server>.autoComplete.filterTemplate preference v5
Assignee | ||
Updated•12 years ago
|
Status: REOPENED → NEW
Assignee | ||
Comment 49•12 years ago
|
||
Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5
Review of attachment 669912 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the delay. This looks fine.
Attachment #669912 -
Flags: review?(mbanner) → review+
Assignee | ||
Comment 50•12 years ago
|
||
(In reply to jhorak from comment #42)
> I'm looking at this one now:
>
> (In reply to Mark Banner (:standard8) from comment #18)
> > - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> > preference (this may be the more difficult bit, as we either need to hook
> > into the search capabilities of nsAbDirectoryQuery, or re-implement them
> > ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> > where we already have query results.
>
> I'm not sure how to solve it. It might be possible to map filterTemplate
> content to list of card parameters (something like opposite of
> nsAbBoolExprToLDAPFilter::Convert) and then use _checkEntry to test if
> search string matches. This could be more complicated for ldap binary
> operators.
>
> Could you please elaborate hooking into nsAbDirectryQuery a little further?
> Do you mean to somehow modify DoQuery to use cached search results?
I think I was probably wondering if we could hook into nsAbDirectoryQuery::matchCard (or split that out to a separate module) and pass a boolean expression converted from the filter, and use those match routines rather than the ones in _checkEntry.
Comment 51•12 years ago
|
||
Is s-r required as long as nsIAbDirectoryQuery.idl was changed?
Assignee | ||
Comment 52•12 years ago
|
||
Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5
Oh yes, forgot about that, I'm sure Neil will take a quick look.
Attachment #669912 -
Flags: superreview?(neil)
Comment 53•12 years ago
|
||
Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5
>+ var filterTemplate = queryObject.book.getStringValue("autoComplete.filterTemplate",
>+ "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");
>+ // If autoComplete.filterTemplate is set to empty string use default value
>+ if (!filterTemplate)
>+ filterTemplate = "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))";
I'm not too keen on the duplication of the default string. If you want to guard against someone accidentally setting a zero-length preference then I would pass an empty default value, which is then handled by the subsequent check.
>+ // Create filter from filter template and search string
>+ var ldapSvc = Components.classes["@mozilla.org/network/ldap-service;1"]
>+ .getService(Components.interfaces.nsILDAPService);
>+ var filter = ldapSvc.createFilter(1024, filterTemplate, "", "", "", aSearchString);
>+ if (!filter)
>+ throw new Error("Filter string is empty, check if filterTemplate variable is valid in prefs.js.");
> args.typeSpecificArg = queryObject.attributes;
> args.querySubDirectories = true;
>+ args.filter = filter;
createFilter's return value is documented to be an AUTF8String, so the new filter attribute should be the same otherwise XPConnect will trash the high bytes. sr=me with that fixed.
Attachment #669912 -
Flags: superreview?(neil) → superreview+
Comment 54•12 years ago
|
||
Attaching fixed patch, copying positive reviews.
Attachment #669912 -
Attachment is obsolete: true
Attachment #678693 -
Flags: superreview+
Attachment #678693 -
Flags: review+
Comment 55•12 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #50)
> I think I was probably wondering if we could hook into
> nsAbDirectoryQuery::matchCard (or split that out to a separate module) and
> pass a boolean expression converted from the filter, and use those match
> routines rather than the ones in _checkEntry.
So we need code which will convert the filterTemplate to the boolean expression, right?
Updated•12 years ago
|
Keywords: checkin-needed
Comment 56•12 years ago
|
||
Assignee: nobody → jhorak
Status: NEW → RESOLVED
Closed: 13 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
Updated•12 years ago
|
Attachment #678693 -
Attachment description: Save uri for downloaded files patch v6 → Save uri for downloaded files patch v6 [Checked in: Comment 56]
Comment 57•12 years ago
|
||
Unfortunately, this caused build bustage. Backed out.
https://hg.mozilla.org/comm-central/rev/36b49398e65c
https://tbpl.mozilla.org/php/getParsedLog.php?id=16905255&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Resolution: FIXED → ---
Target Milestone: Thunderbird 19.0 → ---
Comment 58•12 years ago
|
||
Comment on attachment 678693 [details] [diff] [review]
Save uri for downloaded files patch v6 [Checked in: Comment 56]
>+ nsCAutoString filter;
...
>- nsAutoCString filter;
I didn't spot it before, but this change looks erroneous.
Comment 59•12 years ago
|
||
> I didn't spot it before, but this change looks erroneous.
But of course:
Bug 773151 - Rename nsCAutoString to nsAutoCString
Status: REOPENED → ASSIGNED
Comment 60•12 years ago
|
||
The patch compiles for me with that fixed.
Comment 61•12 years ago
|
||
Comment on attachment 678693 [details] [diff] [review]
Save uri for downloaded files patch v6 [Checked in: Comment 56]
Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.
Comment 62•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #61)
> Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.
Leave open or fixed?
Assignee | ||
Comment 63•12 years ago
|
||
(In reply to Takanori MATSUURA from comment #62)
> (In reply to neil@parkwaycc.co.uk from comment #61)
> > Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.
>
> Leave open or fixed?
Leave open. To be explicit there are two items remaining (which I know jhorak knows about, but commenting here to be explicit):
- The last couple of parts in comment 18, namely:
> - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> preference (this may be the more difficult bit, as we either need to hook
> into the search capabilities of nsAbDirectoryQuery, or re-implement them
> ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> where we already have query results.
>
>
> Also, I'm not sure if the current LDAP implementation supports returning
> results where the display name has matched, and the SecondEmail is listed.
> If it does, then we should implement that here. If not, then we can always
> do that later (or just move to supporting mulit-valued mail elements).
- Actually enabling the toolkit interfaces by default and full removal of the code (a switch to use toolkit autocomplete itself should be done probably in bug 360648 and/or others).
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 64•12 years ago
|
||
While working on patch, I've realized that ldap query doesn't return all records matching typed search term (this is probably set on Preferences/Composition/LDAP directory servers/Edit/Advanced/Don't return more than X results - by default 30). This of course breaks reusing already obtained LDAP results (because results are incomplete).
I've got working prototype but it's not useful, when there are missing records which wasn't found by last ldap search with limit 30.
Any hint or workaround? Maybe start to using it after we have less than maximum ldap results returned? And is it worth?
Updated•12 years ago
|
Flags: needinfo?(mbanner)
Comment 65•12 years ago
|
||
Current ldap autocomplete implementation in JS is broken. For example if I'm looking for "Andrew Someone" and type "Andrew " there's only one record left. When I press backspace more Andrews appear in drop down menu.
This is because we're trying to filter previous result of ldap query (here: http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js#188
Because this query doesn't have to contain all records stored on ldap server (there's query limit 30 I believe - maxHits) in the end we end up with missing records.
I found this issue only when building with --enable-incomplete-toolkit-ldap-autocomplete.
My LDAP server for some reason doesn't return maxHits records even when there should be is more than maxHits records returned so I can't use count of returned records to determine whenever server returned all remaining records (ie. I can use _checkEntry) or there might be more results (count == maxHits, ie. have to redo ldap query).
Assignee | ||
Comment 66•12 years ago
|
||
(In reply to jhorak from comment #64)
> While working on patch, I've realized that ldap query doesn't return all
> records matching typed search term (this is probably set on
> Preferences/Composition/LDAP directory servers/Edit/Advanced/Don't return
> more than X results - by default 30). This of course breaks reusing already
> obtained LDAP results (because results are incomplete).
>
> I've got working prototype but it's not useful, when there are missing
> records which wasn't found by last ldap search with limit 30.
> Any hint or workaround? Maybe start to using it after we have less than
> maximum ldap results returned? And is it worth?
Would it perhaps be better not to cache the ldap results? I'm not actually sure how much of this is going on server side versus client side.
Flags: needinfo?(mbanner)
Comment 67•12 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #66)
> > I've got working prototype but it's not useful, when there are missing
> > records which wasn't found by last ldap search with limit 30.
> > Any hint or workaround? Maybe start to using it after we have less than
> > maximum ldap results returned? And is it worth?
>
> Would it perhaps be better not to cache the ldap results? I'm not actually
> sure how much of this is going on server side versus client side.
I guess so, there's no caching in current implementation (without --enable-incomplete-toolkit-ldap-autocomplete) AFAIK.
Assignee | ||
Comment 68•11 years ago
|
||
Neil and I discussed this on irc the other day. Given the cache issues, and the fact that we didn't have a cache before, we agreed that we just shouldn't use the cache option.
Therefore this patch removes the partially implemented cache option. I'll add a patch in a bit for enabling the toolkit based search by default.
Attachment #772362 -
Flags: review?(neil)
Comment 69•11 years ago
|
||
Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]
>-
> // Here we need to do the real search, as we haven't got any previous
> // results to fall back on.
>
Nit: This comment is no longer relevant. (Although now I begin to wonder whether we need to cache the identity...)
This looks reasonable but jcranmer has broken LDAP on all of my external API builds so I can't test it right now.
Assignee | ||
Comment 70•11 years ago
|
||
For mailing lists, we don't have an identity, therefore we don't have the parameter. So for now, we should just handle it and seek out the default identity.
Attachment #772400 -
Flags: review?(neil)
Assignee | ||
Comment 71•11 years ago
|
||
This patch removes the build option, and switch to toolkit autocomplete interfaces by default. Requesting Joshua as build peer review, and Neil for sr.
Attachment #772402 -
Flags: superreview?(neil)
Attachment #772402 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 72•11 years ago
|
||
This removes all the old ldap set-up code (yay!), and with the previous patches, completes the switch.
Attachment #772403 -
Flags: review?(neil)
Attachment #772403 -
Flags: review?(Pidgeot18)
Comment 73•11 years ago
|
||
If I'm not mistaken, these changes mean we would no longer need to build mozilla/xpfe/components/autocomplete?
Assignee | ||
Comment 74•11 years ago
|
||
(In reply to Joshua Cranmer [:jcranmer] from comment #73)
> If I'm not mistaken, these changes mean we would no longer need to build
> mozilla/xpfe/components/autocomplete?
From what Neil has said before to me, we would still need some of the content based code until we completely switch to toolkit interfaces. We would be able to get rid of the binary parts that we have now (with some modification of the toolkit parts).
Comment 75•11 years ago
|
||
Comment on attachment 772402 [details] [diff] [review]
Remove the build option, switch to toolkit interfaces (build config parts)
Review of attachment 772402 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/addrbook/src/moz.build
@@ +73,5 @@
> EXTRA_COMPONENTS += [
> 'nsAbAutoCompleteMyDomain.js',
> 'nsAbAutoCompleteSearch.js',
> 'nsAbLDAPAttributeMap.js',
> + 'nsAbLDAPAutoCompleteSearch.js',
This change isn't quite right, it adds the component even when LDAP is disabled.
::: mailnews/addrbook/src/nsAddrbook.manifest
@@ -6,4 @@
> contract @mozilla.org/addressbook/ldap-attribute-map;1 {127b341a-bdda-4270-85e1-edff569a9b85}
> component {4ed7d5e1-8800-40da-9e78-c4f509d7ac5e} nsAbLDAPAttributeMap.js
> contract @mozilla.org/addressbook/ldap-attribute-map-service;1 {4ed7d5e1-8800-40da-9e78-c4f509d7ac5e}
> -#ifdef MOZ_INCOMPLETE_TOOLKIT_LDAP_AUTOCOMPLETE
Similarly, instead of being deleted, this should just change to MOZ_LDAP_XPCOM.
Attachment #772402 -
Flags: review?(Pidgeot18) → review+
Comment 76•11 years ago
|
||
(In reply to Mark Banner from comment #74)
> (In reply to Joshua Cranmer from comment #73)
> > If I'm not mistaken, these changes mean we would no longer need to build
> > mozilla/xpfe/components/autocomplete?
>
> From what Neil has said before to me, we would still need some of the
> content based code until we completely switch to toolkit interfaces. We
> would be able to get rid of the binary parts that we have now (with some
> modification of the toolkit parts).
To clarify, mozilla/xpfe/components/autocomplete has three parts:
1. src - only needed by LDAP C++ autocomplete and its consumers
2. public - needed by src; currently needed by resources/content
3. resources/content - needed by SeaMonkey; currently needed by Thunderbird
So once the LDAP C++ autocomplete is removed, the XPFE C++ can be removed, then I can write a patch to remove the legacy interface support, but meanwhile Thunderbird can also investigate switching to toolkit autocomplete XBL at which point resources/content can be moved to suite/common/bindings.
Comment 77•11 years ago
|
||
Comment on attachment 772400 [details] [diff] [review]
Handle undefined parameter - e.g. mailing lists
>+ if (!aParam) {
>+ try {
>+ this._cachedIdentity =
>+ MailServices.accounts.defaultAccount.defaultIdentity;
>+ }
>+ catch (ex) {
>+ Components.utils.reportError("LDAP autocomplete could not get" +
>+ "default identity");
I didn't even realise that Thunderbird had LDAP autocomplete on its mailing lists, so I looked at the code, and it currently seems to use the global LDAP preferences, which would make slightly more sense to me. Note that it's not strictly necessary to catch the exception as the autocomplete widget will ignore the search if startSearch throws.
>+ this._result.searchResult = ACR.RESULT_IGNORED;
>+ aListener.onSearchResult(this, this._result);
>+ return;
>+ }
>+ }
>+ else if (aParam != this._cachedParam) {
> this._cachedIdentity = MailServices.accounts.getIdentity(aParam);
> this._cachedParam = aParam;
> }
This breaks the cache because if you compose using a particular identity, then edit an address list, then compose using that identity again, you end up with the default identity instead. (Maybe just get rid of the cache?)
Attachment #772400 -
Flags: review?(neil) → review-
Updated•11 years ago
|
Attachment #772402 -
Flags: superreview?(neil) → superreview+
Comment 78•11 years ago
|
||
Comment on attachment 772403 [details] [diff] [review]
Switch to toolkit interfaces, remove old js code
- autocompletesearch="addrbook" timeout="300" maxrows="4"
+ autocompletesearch="addrbook ldap" timeout="300" maxrows="4"
[I guess I need to add this to suite/mailnews/addrbook/abListOverlay.xul too]
>-
>+
Go home Mercurial, you're drunk.
Attachment #772403 -
Flags: review?(neil) → review+
Comment 79•11 years ago
|
||
Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]
Oh yes, this is a world of improvement.
r=me with that comment fix as previously noted.
Attachment #772362 -
Flags: review?(neil) → review+
Assignee | ||
Comment 80•11 years ago
|
||
Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]
https://hg.mozilla.org/comm-central/rev/1474fbd5ab3d
Attachment #772362 -
Attachment description: Remove cache option → Remove cache option [checked in: comment 80]
Comment 81•11 years ago
|
||
Comment on attachment 772403 [details] [diff] [review]
Switch to toolkit interfaces, remove old js code
Review of attachment 772403 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not going to purport to be an expert on this stuff, but looks good to me. Sorry for the delay, I got sidetracked when reviewing this.
::: mail/components/addrbook/content/abMailListDialog.xul
@@ +67,4 @@
> <listcell class="addressingWidgetCell">
> <textbox id="addressCol1#1" class="plain textbox-addressingWidget uri-element"
> type="autocomplete" flex="1"
> + autocompletesearch="addrbook ldap" timeout="300" maxrows="4"
Remove trailing whitespace while you're here?
Attachment #772403 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #772403 -
Attachment is obsolete: true
Assignee | ||
Comment 83•11 years ago
|
||
With comments addressed
Assignee: jhorak → mbanner
Attachment #772402 -
Attachment is obsolete: true
Attachment #777039 -
Flags: superreview+
Attachment #777039 -
Flags: review+
Assignee | ||
Comment 84•11 years ago
|
||
Addressed review comments - removed the identity cache and use the global preference if appropriate.
Attachment #772400 -
Attachment is obsolete: true
Attachment #777045 -
Flags: review?(neil)
Assignee | ||
Comment 85•11 years ago
|
||
Correct patch.
Attachment #777037 -
Attachment is obsolete: true
Attachment #777047 -
Flags: review+
Assignee | ||
Comment 86•11 years ago
|
||
Correct version
Attachment #777049 -
Flags: superreview+
Attachment #777049 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #777039 -
Attachment is obsolete: true
Comment 87•11 years ago
|
||
Comment on attachment 777045 [details] [diff] [review]
Handle undefined parameter - e.g. mailing lists v2 [checked in comment 88]
Nice!
Attachment #777045 -
Flags: review?(neil) → review+
Assignee | ||
Comment 88•11 years ago
|
||
Landed the last three parts.
https://hg.mozilla.org/comm-central/rev/f0535a4d8727
https://hg.mozilla.org/comm-central/rev/c06df11f1abf
https://hg.mozilla.org/comm-central/rev/0ba38b41f77f
If there are any issues following this, please file them as new bugs.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Assignee | ||
Updated•11 years ago
|
Attachment #777045 -
Attachment description: Handle undefined parameter - e.g. mailing lists v2 → Handle undefined parameter - e.g. mailing lists v2 [checked in comment 88]
Assignee | ||
Updated•11 years ago
|
Attachment #777047 -
Attachment description: Switch to toolkit interfaces, remove old js code v3 → Switch to toolkit interfaces, remove old js code v3 [checked in comment 88]
Assignee | ||
Updated•11 years ago
|
Attachment #777049 -
Attachment description: Remove the build option, switch to toolkit interfaces (build config parts) v3 → Remove the build option, switch to toolkit interfaces (build config parts) v3 [checked in comment 88]
Comment 89•11 years ago
|
||
(PS: Should you remove "[leave open]" on this bug whiteboard ?)
Comment 90•11 years ago
|
||
In mail/components/compose/content/addressingWidgetOverlay.js there is still this code:
//this copies the autocomplete sessions list from recipient#1
input[0].syncSessions(document.getElementById('addressCol2#1'));
Is this still required?
Comment 91•11 years ago
|
||
(In reply to Jens Müller from comment #90)
> In mail/components/compose/content/addressingWidgetOverlay.js there is still
> this code:
>
> //this copies the autocomplete sessions list from recipient#1
> input[0].syncSessions(document.getElementById('addressCol2#1'));
>
> Is this still required?
No, see the dependency on bug 899822 that I recently added.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Updated•11 years ago
|
Keywords: addon-compat
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•