Autocomplete newsgroup names

RESOLVED FIXED in Thunderbird 28.0

Status

MailNews Core
Composition
P3
enhancement
RESOLVED FIXED
18 years ago
4 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: tessarakt)

Tracking

({student-project})

Trunk
Thunderbird 28.0
student-project
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb31features])

Attachments

(2 attachments, 39 obsolete attachments)

46.35 KB, image/png
Details
53.31 KB, patch
tessarakt
: review+
tessarakt
: superreview+
Details | Diff | Splinter Review
feature enhancement

get autocomplete to work when I'm addressing a message to "Newsgroup:"

Comment 1

18 years ago
*** Bug 39626 has been marked as a duplicate of this bug. ***

Updated

18 years ago
QA Contact: esther → stephend

Comment 2

18 years ago
*** Bug 11046 has been marked as a duplicate of this bug. ***
*** Bug 65498 has been marked as a duplicate of this bug. ***
Summary: RFE: autocomplete when posting news articles → Autocomplete when posting news articles

Comment 4

17 years ago
*** Bug 101994 has been marked as a duplicate of this bug. ***
*** Bug 105558 has been marked as a duplicate of this bug. ***
*** Bug 174030 has been marked as a duplicate of this bug. ***
Product: MailNews → Core

Updated

14 years ago
Severity: normal → enhancement
Priority: P3 → --
Summary: Autocomplete when posting news articles → Autocomplete newsgroup names

Comment 7

14 years ago
*** Bug 194153 has been marked as a duplicate of this bug. ***

Comment 8

14 years ago
*** Bug 265164 has been marked as a duplicate of this bug. ***

Comment 9

13 years ago
*** Bug 273540 has been marked as a duplicate of this bug. ***

Comment 10

12 years ago
*** Bug 363563 has been marked as a duplicate of this bug. ***
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: stephend → composition
Product: Core → MailNews Core

Updated

10 years ago
Duplicate of this bug: 461671

Updated

10 years ago
Flags: wanted-thunderbird3+
Keywords: helpwanted
Priority: -- → P3
Target Milestone: --- → Thunderbird 3.0rc1
This could be a nice little student project. Here's a quick summary:

There are autocomplete modules implementing different types of autocomplete for compose [1]. The simplest is nsAbAutoCompleteMyDomain.js. There is also documentation on devmo [2].

To autocomplete for newsgroups would require implementing a module (js would be the easiest). It would have to iterate through the newsgroup names probably getting the list of accounts via nsIMsgAccountManager and then checking for newsgroup only-accounts.

[1] http://mxr.mozilla.org/comm-central/find?text=&kind=text&string=nsAbAutoCom
[2] https://developer.mozilla.org/en/How_to_implement_custom_autocomplete_search_component
Keywords: student-project
Duplicate of this bug: 515550

Comment 16

9 years ago
Can't believe this "little student project" has bugged for nearly 9 years! Maybe it's not as easy as it looks like?

Comment 17

9 years ago
I'm taking this, though it probably won't get done by the time 3.0 is released.
Assignee: nobody → squibblyflabbetydoo

Comment 18

9 years ago
Created attachment 403610 [details] [diff] [review]
(Very) rough patch

This sort of works. It causes a regression in that it autocompletes email and newsgroups no matter which type of address you're typing, but I don't know how to change the autocompletesearch value dynamically (trying to do so doesn't actually have any effect). There's probably a Better Way to do that...

There are lots of other issues too (mostly in that it brute-forces the problem instead of trying to be smart with narrowing down results).
Using the autocomplete search parameter would seem the obvious way, but we already use that for passing the selected identity to mydomain and addrbook. Maybe you could make it a duel-value parameter or something.
(In reply to comment #19)
> Using the autocomplete search parameter would seem the obvious way, but we
> already use that for passing the selected identity to mydomain and addrbook.
> Maybe you could make it a duel-value parameter or something.
Something like "identity,addr_type" perhaps, and split it up in the component?
Someone could then add an autocomplete for addr_other to make it easier to add values for other headers.

Don't forget that the newsgroup names should only be from the current server.
(Well, the server belonging to the same account as the selected identity.)

Comment 21

9 years ago
Created attachment 409156 [details] [diff] [review]
Cleaned up patch

Ok, this patch handles identities sensibly and only autocompletes for appropriate types (i.e. newsgroup autocompletion only happens when the field is Newsgroup: or Followup-To:). The autocompletion matching is a little bit braindead (it just does a simple substring search), but I'm not sure what the ideal search method is.

This is probably ready for review, too. Who should I assign it to?
Attachment #403610 - Attachment is obsolete: true
I can have a look, but it may not be for a week or two yet. Another expert in this area is Neil (one of us could do r, the other sr).

Comment 23

9 years ago
Created attachment 409189 [details] [diff] [review]
Patch for review

Let's try a patch that doesn't have my debug code stowing away inside it...
Attachment #409156 - Attachment is obsolete: true
Attachment #409189 - Flags: superreview?(bugzilla)
Attachment #409189 - Flags: review?(neil)

Comment 24

9 years ago
A simple substring search may suffice, just like "subscribe to groups" interface. Few people need RegEx I think?

Comment 25

9 years ago
Well, the issue is that, currently, if I type "e" to autocomplete a newsgroup, *every* newsgroup containing an "e" anywhere will show up. If I type "e" to autocomplete an email, it will only show addresses/names starting with "e".

I don't think I'd want to match just the beginning of the newsgroup either, though. The beginning of a newsgroup name is often the least useful part. For example, if I wanted to access the C++ newsgroup, I'd rather type "c++" than have to start typing "comp.lang.c++" until it gets narrowed down enough. Maybe matching at the beginning of each name piece would work.
Comment on attachment 409189 [details] [diff] [review]
Patch for review

You rather inconveniently failed to make the equivalent changes to the suite/mailnews/compose versions of the mail/components/compose files...

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam") || '{}');
You could put the "{}" in the XUL as the default attribute value.

>+    if(["addr_to", "addr_cc", "addr_bcc", "addr_reply"].indexOf(params.type) == -1)
Nit: make this array a global constant or member variable.

>+		nsNntpAutoCompleteSearch.js \
Nit: call this "News" perhaps, not "Nntp"?

>+ * Contributor(s):
>+ *   Mark Banner <bugzilla@standard8.plus.com>
You're a contributor too!

>+  getStyleAt: function getStyleAt(aIndex) {
>+    return "local-news";
I think the local/remote distinction only makes sense for abook, not news.
Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

>+    let numAccounts = msgAccountManager.accounts.Count();
Please put msgAccountManager.accounts in its own local variable.

>+    let ident = msgAccountManager.getIdentity(idKey);
Not used?

>+      let account = msgAccountManager.accounts.GetElementAt(i)
>+        .QueryInterface(Components.interfaces.nsIMsgAccount);
QueryElementAt(i, Components.interfaces.nsIMsgAccount);

>+        let numIdents = account.identities.Count();
Please cache identities too.

>+          if (account.identities.GetElementAt(j).QueryInterface(
>+                Components.interfaces.nsIMsgIdentity).key == idKey) {
QueryElementAt again.

>+      let groups = this.cachedServer.rootFolder.subFolders;
I wonder whether it's worth searching unsubscribed groups too? (Presumably as a follow-up, as I don't see an easy way to do it.)

>+            value: curr.prettiestName,
>+            comment: this.cachedServer.prettyName
Remind me what the difference between the two is please?

Comment 27

9 years ago
Ok, I addressed most of the issues, but I have a couple of questions before I upload the revised patch:

(In reply to comment #26)
> >+  getStyleAt: function getStyleAt(aIndex) {
> >+    return "local-news";
> I think the local/remote distinction only makes sense for abook, not news.
> Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

Would "local-news"/"remote-news" be a good distinction for subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better names for them, though. (This is aside from the fact that I don't look at unsubscribed groups yet).

> >+        let numIdents = account.identities.Count();
> Please cache identities too.

Is there a way of invalidating the cached identities when necessary? That function should only be executing if you changed the currently-selected identity.

> >+            value: curr.prettiestName,
> >+            comment: this.cachedServer.prettyName
> Remind me what the difference between the two is please?

The value is the name of the group, and the comment is the name of the server. I suppose the comment isn't strictly necessary, since we're only getting results from one server, but I thought it would help clarify which newsgroup you're actually sending to. I can remove it though; I'm not particularly attached to it.
I found an issue indirectly caused by your patch; minResultsForPopup currently depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when it's set to 3 you'll never see the second newsgroup if exactly two match.

(In reply to comment #27)
> (In reply to comment #26)
> > >+  getStyleAt: function getStyleAt(aIndex) {
> > >+    return "local-news";
> > I think the local/remote distinction only makes sense for abook, not news.
> > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.
> Would "local-news"/"remote-news" be a good distinction for
> subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better
> names for them, though. (This is aside from the fact that I don't look at
> unsubscribed groups yet).
Not sure; I'd have to ask Standard8 to chime in on that.

> > >+        let numIdents = account.identities.Count();
> > Please cache identities too.
I meant in a local variable. Sorry for the confusion.

> > >+            value: curr.prettiestName,
> > >+            comment: this.cachedServer.prettyName
> > Remind me what the difference between the two is please?
And I misread this - I thought the comment was the group's pretty name; the reason I mention this is that I wasn't sure which name you were completing. (Netscape 4 supported something called that it called the prettiest name which was a human-readable description of the newsgroup.)

Comment 29

9 years ago
(In reply to comment #28)
> I found an issue indirectly caused by your patch; minResultsForPopup currently
> depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when
> it's set to 3 you'll never see the second newsgroup if exactly two match.

Looking through the code, it seems that minResultsForPopup is only set to 3 when the field would autocomplete for your domain, presumably so that the autocompleteToMyDomain doesn't show up if you have exactly one match in your address book. The easy way to fix this would be to eliminate that entirely and just always set minResultsForPopup to 2. Granted, this is a change in behavior, but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly enable it, I'd suppose that you want it to show up all the time. If nothing else, it would simplify the code a fair bit. :)
Sorry for not getting to look at this earlier.

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > >+  getStyleAt: function getStyleAt(aIndex) {
> > > >+    return "local-news";
> > > I think the local/remote distinction only makes sense for abook, not news.
> > > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.
> > Would "local-news"/"remote-news" be a good distinction for
> > subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better
> > names for them, though. (This is aside from the fact that I don't look at
> > unsubscribed groups yet).
> Not sure; I'd have to ask Standard8 to chime in on that.

I'm not sure I see a real case for unsubscribed newsgroups, personally, I definitely wouldn't want it by default as there are many newsgroups I don't subscribe to on a server and wouldn't want to have my autocomplete 'polluted' by them. I guess there could be a hidden option though.

(In reply to comment #29)
> (In reply to comment #28)
> > I found an issue indirectly caused by your patch; minResultsForPopup currently
> > depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when
> > it's set to 3 you'll never see the second newsgroup if exactly two match.
> 
> Looking through the code, it seems that minResultsForPopup is only set to 3
> when the field would autocomplete for your domain, presumably so that the
> autocompleteToMyDomain doesn't show up if you have exactly one match in your
> address book. The easy way to fix this would be to eliminate that entirely and
> just always set minResultsForPopup to 2. Granted, this is a change in behavior,
> but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly
> enable it, I'd suppose that you want it to show up all the time. If nothing
> else, it would simplify the code a fair bit. :)

Could we modify it so that if a newsgroup is selected then we minResultsForPopup is 2 (otherwise 3)?


Also, this patch currently breaks the autocomplete tests for the address book (in mailnews/addrbook/test). You need to fix those up, and it would be good to get some tests for your newsgroup specific code as well - you should be able to base them on the existing address book tests and some of the newsgroup code (ask if you need help).
Attachment #409189 - Flags: superreview?(bugzilla) → superreview-

Comment 31

9 years ago
(In reply to comment #30)
> I'm not sure I see a real case for unsubscribed newsgroups, personally, I
> definitely wouldn't want it by default as there are many newsgroups I don't
> subscribe to on a server and wouldn't want to have my autocomplete 'polluted'
> by them. I guess there could be a hidden option though.

Non-subscribed groups could be pretty useful for setting follow-ups. I wonder if this couldn't be handled by simply giving the subscribed groups priority so they show up higher in the autocomplete results.
Jim, would you still be able to get a new patch up?

If you need help with the tests, just let me know.
Attachment #409189 - Flags: review?(neil) → review-
(Assignee)

Comment 33

8 years ago
(In reply to comment #28)
> (Netscape 4 supported something called that it called the prettiest name which
> was a human-readable description of the newsgroup.)

I think this is usually called "tagline" (the charter is AFAIK not available from the NNTP server).

Comment 34

8 years ago
(In reply to comment #32)
> Jim, would you still be able to get a new patch up?
> 
> If you need help with the tests, just let me know.

I've been tinkering with this again, and the situation has gotten worse. For reasons I haven't quite been able to figure out, the autocomplete search param is always the param for the first row. This makes changing the autocomplete type based on the selected address type difficult/impossible.

Updated

7 years ago
Duplicate of this bug: 683884

Updated

6 years ago
Duplicate of this bug: 208192

Comment 37

6 years ago
Unassigning myself from this, since I'm unlikely to work on it in the near future.
Assignee: squibblyflabbetydoo → nobody
(Assignee)

Comment 38

6 years ago
Would it be a good approach to take Jim's latest patch and try to get it working? I'd like to get involved into Thunderbird a bit, and this might be a good start.

Another thing: Is there a description of the expected visible behavior of this change? Where I work, we call it "system description", and it is quite useful.

Comment 39

6 years ago
Bwinton is the current UI guy for Thunderbird, you can ask him what he would expect it to look.
(Assignee)

Comment 40

6 years ago
Created attachment 661588 [details] [diff] [review]
Jim Porter's patch, de-bitrotted

Just de-bitrotted - not ready for review!
(Assignee)

Comment 41

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #26)
> >+ * Contributor(s):
> >+ *   Mark Banner <bugzilla@standard8.plus.com>
> You're a contributor too!

I have been told on #maildev that the new license header should be used now:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Do I have to ask someone (especially, Jim Porter) to do that? The license block in Jim's latest patch mentioned original code developed by Mark Banner and Mozilla Messaging. From a legal perspective, can these notes be safely removed?

Thanks!
(In reply to Jens Müller from comment #41)
> Do I have to ask someone (especially, Jim Porter) to do that? The license
> block in Jim's latest patch mentioned original code developed by Mark Banner
> and Mozilla Messaging. From a legal perspective, can these notes be safely
> removed?
> 
> Thanks!

No you do not need to ask, all of c-c was changed to the MPL 2 at one point. MPL 1.1 states you can use any later version.
(Assignee)

Comment 43

6 years ago
And to properly record the answer to the other part of the question, answered on IRC: The change to MPL 2.0 also removed the requirement to list the name of authors/contributors, so that part of the license block in the old patch is obsolete as well.
(Assignee)

Comment 44

6 years ago
Created attachment 661637 [details] [diff] [review]
De-bitrotted patch, with most review comments applied
Attachment #661588 - Attachment is obsolete: true
(Assignee)

Comment 45

6 years ago
I de-bitrotted the patch and applied most review comments.

Open issues are:
* narrow done search results more intelligently
* set minResultsForPopup to 2 everywhere?
* As Jim already noted, the _first_ row is used to determine the kind of autocomplete used. So when first row is "To:" and second is "Newsgroup:", newsgroup autocompletion won't work. The same probably applies vice versa. And that would be a really common case: Writing a newsgroup posting and putting someone on Cc: 
* equivalent changes to the suite/mailnews/compose
* possibly remove the server name. It is not _that_ useful indeed.

Related issues to be tackled in follow-up bugs:
 * Also autocomplete to names of unsubscribed newsgroup
 * Download taglines from the server, and use them at various places (subscription dialog, tooltip in the three-pane-window, and autocompletion).
 * possibly only match at start of newsgroup name components. I guess we should wait for user feedback on the solution with its current scope.
 * When pressing "Write" while on an newsgroup account, pre-set the first row in the compose window to "Newsgroup". This is something I noticed while testing.
(Assignee)

Comment 46

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #26)
> Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

All of 
mail/themes/pinstripe/mail/compose/messengercompose.css
mail/themes/qute/mail/compose/messengercompose.css
mail/themes/qute/mail/compose/messengercompose-aero.css
mail/themes/gnomestripe/mail/compose/messengercompose.css
?

(And when ported to suite: All of
suite/themes/classic/mac/messenger/messengercompose/messengercompose.css
suite/themes/classic/messenger/messengercompose/messengercompose.css
suite/themes/modern/messenger/messengercompose/messengercompose.css
?)
(Assignee)

Comment 47

6 years ago
(In reply to Jens Müller from comment #46)
> (In reply to neil@parkwaycc.co.uk from comment #26)
> > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

Umm, when exactly is that icon shown? I didn't manage to get any icons (for the other types) shown ... I guess they should be left of the autocomplete-result list entries?

And which icon to use here? Is there an existing icon where I can just enter the path? And is there a list of all available icons somewhere? I guess theme authors would need such a list as well ...

Updated

6 years ago
Assignee: nobody → blog
Target Milestone: Thunderbird 3.0rc1 → ---
(Assignee)

Comment 48

6 years ago
(In reply to Jim Porter (:squib) from comment #34)
> (In reply to comment #32)
> > Jim, would you still be able to get a new patch up?
> > 
> > If you need help with the tests, just let me know.
> 
> I've been tinkering with this again, and the situation has gotten worse. For
> reasons I haven't quite been able to figure out, the autocomplete search
> param is always the param for the first row. This makes changing the
> autocomplete type based on the selected address type difficult/impossible.

I found this in addressingWidgetOverlay.js:

       //this copies the autocomplete sessions list from recipient#1
       input[0].syncSessions(document.getElementById('addressCol2#1'));

and it looked kind of fishy to me. So I commented out the second line - now it works!

Does someone now what this is supposed to do? Should I do anything else beyond just removing it?

Best,

Jens
(Assignee)

Comment 49

6 years ago
Documentation:
https://developer.mozilla.org/en-US/docs/XUL/Method/syncSessions (very short)

Here it is explained/defined:
http://mxr.mozilla.org/comm-central/source/mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml

609       <!-- make this widget listen to all of the same autocomplete sessions 
610            from another autocomplete widget -->

I don't know why one would want that. Is it for performance or for functional purposes?
(In reply to Jim Porter from comment #34)
> I've been tinkering with this again, and the situation has gotten worse. For
> reasons I haven't quite been able to figure out, the autocomplete search
> param is always the param for the first row. This makes changing the
> autocomplete type based on the selected address type difficult/impossible.
This is because the standard autocomplete options (mydomain/addrbook) use the param to work out what they should be autocompleting.

(In reply to Jens Müller from comment #48)
> I found this in addressingWidgetOverlay.js:
> 
>        //this copies the autocomplete sessions list from recipient#1
>        input[0].syncSessions(document.getElementById('addressCol2#1'));
> 
> Does someone now what this is supposed to do?

syncSessions exists so that LDAP autocomplete works on all rows, not just the first. This is with old-style LDAP autocomplete of course; --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param instead, to match mydomain/addrbook autocomplete.

Note that these autocompletes expect param to be the identity key, and this is reset whenever you choose an identity from the From: dropdown (see LoadIdentity in MsgComposeCommands.js).
(Assignee)

Comment 51

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #50)
> (In reply to Jim Porter from comment #34)
> > I've been tinkering with this again, and the situation has gotten worse. For
> > reasons I haven't quite been able to figure out, the autocomplete search
> > param is always the param for the first row. This makes changing the
> > autocomplete type based on the selected address type difficult/impossible.
> This is because the standard autocomplete options (mydomain/addrbook) use
> the param to work out what they should be autocompleting.

Jim's patch set the param fine, IMO. I think it just had no effect due to the syncSessions.

> (In reply to Jens Müller from comment #48)
> > I found this in addressingWidgetOverlay.js:
> > 
> >        //this copies the autocomplete sessions list from recipient#1
> >        input[0].syncSessions(document.getElementById('addressCol2#1'));
> > 
> > Does someone now what this is supposed to do?
> 
> syncSessions exists so that LDAP autocomplete works on all rows, not just
> the first. This is with old-style LDAP autocomplete of course;
> --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param
> instead, to match mydomain/addrbook autocomplete.

Is that bug 452232? 

Will it be finished soon, or should I try a more complicated solution that works with the old LDAP autocomplete?

> Note that these autocompletes expect param to be the identity key, and this
> is reset whenever you choose an identity from the From: dropdown (see
> LoadIdentity in MsgComposeCommands.js).

I will look at home. The current patch adjusts autocompleteparams in different functions, so I hope that case is already handled as well.

 -- Jens
(In reply to Jens Müller from comment #51)
> (In reply to comment #50)
> > (In reply to Jim Porter from comment #34)
> > > I've been tinkering with this again, and the situation has gotten worse. For
> > > reasons I haven't quite been able to figure out, the autocomplete search
> > > param is always the param for the first row. This makes changing the
> > > autocomplete type based on the selected address type difficult/impossible.
> > This is because the standard autocomplete options (mydomain/addrbook) use
> > the param to work out what they should be autocompleting.
> Jim's patch set the param fine, IMO. I think it just had no effect due to
> the syncSessions.
You're right, there is a major bug in syncSessions that means that all the autocompletes use the param from the first one. This means bad news if you ever delete the first row. Oops.

> > (In reply to Jens Müller from comment #48)
> > > I found this in addressingWidgetOverlay.js:
> > > 
> > >        //this copies the autocomplete sessions list from recipient#1
> > >        input[0].syncSessions(document.getElementById('addressCol2#1'));
> > > 
> > > Does someone now what this is supposed to do?
> > syncSessions exists so that LDAP autocomplete works on all rows, not just
> > the first. This is with old-style LDAP autocomplete of course;
> > --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param
> > instead, to match mydomain/addrbook autocomplete.
> Will it be finished soon, or should I try a more complicated solution that
> works with the old LDAP autocomplete?
No, it won't be finished soon, unless you finish it (joke!)

> > Note that these autocompletes expect param to be the identity key, and this
> > is reset whenever you choose an identity from the From: dropdown (see
> > LoadIdentity in MsgComposeCommands.js).
> I will look at home. The current patch adjusts autocompleteparams in
> different functions, so I hope that case is already handled as well.
Indeed, this wasn't the problem after all.
Created attachment 662104 [details] [diff] [review]
Possible syncSessions fix

This might fix the syncSessions bug, but I'm not 100% sure it works with LDAP.
(Assignee)

Comment 54

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #53)
> Created attachment 662104 [details] [diff] [review]
> Possible syncSessions fix
> 
> This might fix the syncSessions bug, but I'm not 100% sure it works with
> LDAP.

I hope it does - that is the whole reason we cannot just drop the syncSessions ...

Do you know a (dummy) LDAP server I could use for testing?

 -- Jens
(In reply to Jens Müller from comment #54)
> Do you know a (dummy) LDAP server I could use for testing?
David Bienvenu used to use the University of Washington
Hostname: directory.washington.edu Base DN: O=University of Washington,C=US
(Assignee)

Comment 56

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #53)
> Created attachment 662104 [details] [diff] [review]
> Possible syncSessions fix
> 
> This might fix the syncSessions bug, but I'm not 100% sure it works with
> LDAP.

I applied it, enabled LDAP, and it produces very weird results. I might manage to write them down some time later ... Basically, newsgroup lines autocomplete to LDAP addresses in many cases. I have tried to achieve the opposite, but that doesn't seem to occur.

On stdout/stderr I get messages like "search "ldap" not found - skipping." and "###!!! ASSERTION: We don't know what went wrong with the ldap operation: 'Error', file /home/jens/devel/mozilla-comm-central/ldap/xpcom/src/nsLDAPConnection.cpp, line 661".

Any ideas?

Best,

Jens
(Assignee)

Comment 57

6 years ago
Scenario 1:
* Compose new message
* Change first address row to "Newsgroup"
* enter "John"
-> auto-completion to a John from the LDAP directory

Scenario 2:
* Compose new message
* leave first row at "To", enter John, choose an LDAP entry, press Enter
* In the second line, change to "Newsgroup"
* Enter "John"
 -> autocompleted to a name from LDAP
* enter a string that occurs in a newsgroup name
 -> newsgroup entries appear, but also a name from LDAP

My assumption is that LDAP autocomplete somehow gets "hardwired" to the elements and never gets disconnected.

So my questions now are:
 * How does the LDAP autocomplete functionality (listener or whatever) get connected to the textbox in the first place?
 * How is it possible to disconnect it?

If I know that, I can provide the appropriate explicit actions when address-row types are changed etc.
(Assignee)

Comment 58

6 years ago
OK, got it:

setupLdapAutocompleteSession() sets up an LDAP autocomplete listener for the first row (if LDAP autocompletion is enabled).

For each input element, the following happens:

let autoCompleteWidget = document.getElementById("addressCol2#" + i);
if (autoCompleteWidget)
{
  autoCompleteWidget.addSession(LDAPSession);
}

In ReleaseAutoCompleteState(),
for (let i = 1; i <= maxRecipients; i++)
    document.getElementById("addressCol2#" + i).removeSession(gLDAPSession);
is done.

So I have to just _create_ the LDAP session in those two functions (and cache it in a global variable, as is already done), and call addSession/removeSession in the same functions where the params are also set.

A call to syncSessions will no longer be necessary.
(In reply to Jens Müller from comment #58)
> So I have to just _create_ the LDAP session in those two functions (and
> cache it in a global variable, as is already done), and call
> addSession/removeSession in the same functions where the params are also set.
You already have gLDAPSession, no need for a new one.

> A call to syncSessions will no longer be necessary.
Presumably because you will manually manipulate the LDAP sessions in _awSetAutoComplete() and setupLdapAutocompleteSession() instead?
(Assignee)

Comment 60

6 years ago
Created attachment 662356 [details] [diff] [review]
working LDAP autocomplete

LDAP autocomplete now works properly. LDAP sessions are now added/removed manually.
Attachment #661637 - Attachment is obsolete: true
(Assignee)

Comment 61

6 years ago
Changes for SeaMonkey were straightforward.

Now I am wondering about the CSS (comment #46, comment #47).
(Assignee)

Comment 62

6 years ago
(In reply to Mark Banner (:standard8) from comment #30)
> > Looking through the code, it seems that minResultsForPopup is only set to 3
> > when the field would autocomplete for your domain, presumably so that the
> > autocompleteToMyDomain doesn't show up if you have exactly one match in your
> > address book. The easy way to fix this would be to eliminate that entirely and
> > just always set minResultsForPopup to 2. Granted, this is a change in behavior,
> > but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly
> > enable it, I'd suppose that you want it to show up all the time. If nothing
> > else, it would simplify the code a fair bit. :)
> 
> Could we modify it so that if a newsgroup is selected then we
> minResultsForPopup is 2 (otherwise 3)?


Currently, I have always set minResultsForPopup to 2. Is this ok? Or should I put back the old behavior?
(Assignee)

Comment 63

6 years ago
The icons are only shown on Mac in Thunderbird, and in all themes in Seamonkey. I add the style for treechildren::-moz-tree-image(subscribed-news) in those places.

Open issues are:

1. Is it ok to set minResultsForPopup to 2 in all cases?

2. https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAutoCompleteSearch says:

"Results can be sent to the listener either synchronously or asynchronously, depending on the implementation."

Do I read this correctly as requiring that the listener MUST (or at least should) be called in any case?

The original patch introduced checks at the beginning of several mailnews-related autocomplete components that just return, without calling the listener. IMO they should call the observer. I am not sure, however, whether RESULT_IGNORED or RESULT_FAILURE or even RESULT_NOMATCH should be used.

On a side note, https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAutoCompleteResult is very rudimentary ...
(In reply to Jens Müller from comment #63)
> 1. Is it ok to set minResultsForPopup to 2 in all cases?
[You'll need ui-review, but this is an edge case, as you would need to turn autocomplete my domain on, and then the difference is that when you have a single addressbook match, you would still get the popup.]

> Do I read this correctly as requiring that the listener MUST (or at least
> should) be called in any case?
I believe it must, otherwise for instance the red "no match" styling won't get applied.

> I am not sure, however, whether RESULT_IGNORED or RESULT_FAILURE or even
> RESULT_NOMATCH should be used.
Any of them will work. Returning no results also works. Even returning results for the wrong search string works. My preference is for RESULT_IGNORED.
(Assignee)

Comment 65

6 years ago
Created attachment 663102 [details] [diff] [review]
Jim Porter's patch improved a bit and working, ready for review

Jim, thanks for the original patch. It's now working, has unit tests, also addresses Seamonkey, and has CSS for Mac and Seamonkey for the new autocomplete result types.

Neil, you already reviewed the old patch. So can you do this review as well, please?
Attachment #662356 - Attachment is obsolete: true
Attachment #663102 - Flags: review?(neil)
(Assignee)

Comment 66

6 years ago
Autocomplete works in Seamonkey, too.

However, LDAP autocomplete works only for the first address row in Seamonkey. Any ideas why this is so? I have not found differences between my changes to Thunderbird and Semonkey ...

A followup bug for Seamonkey, btw, is that an address card is shown for all kinds of fields, including newsgroups (not in the autocomplete result list, but afterwards in the field).
Comment on attachment 663102 [details] [diff] [review]
Jim Porter's patch improved a bit and working, ready for review

I haven't looked at anything called mail/* or */test*.

>+    var applicable = (this.applicableHeaders.indexOf(params.type) != -1)
[Not sure this was worth its own variable. Anyway, you forgot the semicolon!]

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+
>+const ACR = Components.interfaces.nsIAutoCompleteResult;
>+const nsIAutoCompleteResult = Components.interfaces.nsIAutoCompleteResult;
What's going on here? Also, double blank line.

>+  getValueAt: function getValueAt(aIndex) {
>+    return this._searchResults[aIndex].value;
>+  },
There's a new method, getLabelAt, which we should implement. (Not sure if we actually use it yet.)

>+  /**
>+   * Find the newsgroup server associated with the current identity.
>+   *
>+   * @param idKey  The key of the current identity.
>+   * @return       The incoming news server (or null if one does not exist).
>+   */
We should just pass in the selected account in the params, like we do for the identity.

>+  // For component registration
>+  classDescription: "Newsgroup Autocomplete",
>+  classID: Components.ID("e9bb3330-ac7e-11de-8a39-0800200c9a66"),
>+  contractID: "@mozilla.org/autocomplete/search;1?name=news",
Only the classID is needed these days.

>+    if(["addr_newsgroups", "addr_followup"].indexOf(params.type) == -1) {
>+      result.searchResult = ACR.RESULT_SUCCESS;
Are you sure?

>+      let groups = this.cachedServer.rootFolder.subFolders;
[There is almost a way of searching all folders on the server, using the setSearchValue, rowCount and getCellText functions. Unfortunately you need a tree to be able to call getCellText...]

>+      result.defaultIndex = 0;
Is this a good idea? (Assuming it makes any difference.)

>diff --git a/run-tests.sh b/run-tests.sh
>new file mode 100755
>index 0000000..7db9eef
>--- /dev/null
>+++ b/run-tests.sh
>@@ -0,0 +1,6 @@
>+#!/bin/bash
>+cd obj-x86_64-unknown-linux-gnu
>+make -C mailnews/addrbook/test/ xpcshell-tests
>+make -C mailnews/news/test/ xpcshell-tests
>+cd ..
???

>+                  if (autoCompleteWidget && 
>+                    ["addr_newsgroups", "addr_followup"].indexOf(typeWidget.value) == -1 )
Nit: no space before )

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
>+          params.idKey = idKey;
>+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
>+          awGetInputElement(i).removeSession(gLDAPSession);
Why does the session get removed in this case?

>       //this copies the autocomplete sessions list from recipient#1 
>-      input[0].syncSessions(document.getElementById('addressCol2#1'));
>+      //input[0].syncSessions(document.getElementById('addressCol2#1'));
Don't just comment it out, delete it!

>+    if ( ["addr_newsgroups", "addr_followup"].indexOf(selectElem.value) == -1 ){
>+      inputElem.addSession(gLDAPSession);
>+    } else {
>+      inputElem.removeSession(gLDAPSession);
>+    }
I'm not sure that the element likes you adding the session multiple times. Probably safest to remove the session, then add it if necessary.

I don't expect you to agree but it might simplify the patch to split it into pieces - switching autocompletesearchparam to JSON, adding the address type to the JSON, adding the account key to the JSON, adding the LDAP session manually, adding the news autocomplete.
(Assignee)

Comment 68

5 years ago
I'm working on this patch again. It's quite hard to split it up to split it up into so many parts, and see which detail belongs where ... But maybe it's a good exercise.
(Assignee)

Comment 69

5 years ago
Mark, can you confirm that handling LDAP sessions manually here has become obsolete with the landing of bug 452232? I am currently debitrotting the patch here.
Flags: needinfo?(mbanner)
(Assignee)

Comment 70

5 years ago
Created attachment 785452 [details] [diff] [review]
Part 1: Switch autocompletesearchparam to JSON
Attachment #663102 - Attachment is obsolete: true
Attachment #663102 - Flags: review?(neil)
(Assignee)

Comment 71

5 years ago
Created attachment 785453 [details] [diff] [review]
Part 2: Add address type to JSON
(Assignee)

Comment 72

5 years ago
Created attachment 785455 [details] [diff] [review]
Part 3: Change minresultsforpopup
(Assignee)

Updated

5 years ago
Attachment #785455 - Attachment description: Part 3: Switch existing autocomplete modules to JSON, let them check address type instead of manually enabling/disabling autocompletion → Part 3: Change minresultsforpopup
(Assignee)

Comment 73

5 years ago
Created attachment 785457 [details] [diff] [review]
Part 4: Switch existing autocomplete modules to JSON, let them check address type instead of manually enabling/disabling autocompletion
(Assignee)

Comment 74

5 years ago
Created attachment 785458 [details] [diff] [review]
Part 5: Implement autocomplete module for news
(Assignee)

Comment 75

5 years ago
Created attachment 785459 [details] [diff] [review]
Part 6: Adjust existing tests; test for new functionality
(Assignee)

Comment 76

5 years ago
Hi Neil,

(In reply to neil@parkwaycc.co.uk from comment #67)
> Comment on attachment 663102 [details] [diff] [review]
> Jim Porter's patch improved a bit and working, ready for review
> 
> I haven't looked at anything called mail/* or */test*.
> 
> >+    var applicable = (this.applicableHeaders.indexOf(params.type) != -1)
> [Not sure this was worth its own variable. Anyway, you forgot the semicolon!]

I think it is much more readable with it. Do you think it is a big performance hit? Semicolon is fixed.

> >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> >+
> >+
> >+const ACR = Components.interfaces.nsIAutoCompleteResult;
> >+const nsIAutoCompleteResult = Components.interfaces.nsIAutoCompleteResult;
> What's going on here? Also, double blank line.

Only ACR remains now.

> >+  getValueAt: function getValueAt(aIndex) {
> >+    return this._searchResults[aIndex].value;
> >+  },
> There's a new method, getLabelAt, which we should implement. (Not sure if we
> actually use it yet.)

I added it, and have it return the same value as getValueAt.

> >+  /**
> >+   * Find the newsgroup server associated with the current identity.
> >+   *
> >+   * @param idKey  The key of the current identity.
> >+   * @return       The incoming news server (or null if one does not exist).
> >+   */
> We should just pass in the selected account in the params, like we do for
> the identity.

Done.

> >+  // For component registration
> >+  classDescription: "Newsgroup Autocomplete",
> >+  classID: Components.ID("e9bb3330-ac7e-11de-8a39-0800200c9a66"),
> >+  contractID: "@mozilla.org/autocomplete/search;1?name=news",
> Only the classID is needed these days.

I removed the contractID.

> >+    if(["addr_newsgroups", "addr_followup"].indexOf(params.type) == -1) {
> >+      result.searchResult = ACR.RESULT_SUCCESS;
> Are you sure?

Replaced with ACR.RESULT_IGNORED.

> >+      let groups = this.cachedServer.rootFolder.subFolders;
> [There is almost a way of searching all folders on the server, using the
> setSearchValue, rowCount and getCellText functions. Unfortunately you need a
> tree to be able to call getCellText...]

So no need to change anything?

> >+      result.defaultIndex = 0;
> Is this a good idea? (Assuming it makes any difference.)

"Index of the default item that should be entered if none is selected"/

Your point is that maybe there should be no default item at all?

> >diff --git a/run-tests.sh b/run-tests.sh
> [...]
> ???

removed.

> >+                  if (autoCompleteWidget && 
> >+                    ["addr_newsgroups", "addr_followup"].indexOf(typeWidget.value) == -1 )
> Nit: no space before )

Fixed.

> [stuff related to LDAP sessions]

This has become obsolete with bug 452232.

> 
> I don't expect you to agree but it might simplify the patch to split it into
> pieces - switching autocompletesearchparam to JSON, adding the address type
> to the JSON, adding the account key to the JSON, adding the LDAP session
> manually, adding the news autocomplete.

I did it similar to the structure you suggest here. However, adjusting existing autocomplete modules to the changes done in Part 1 and Part 2 is only done in Part 4. Tests for everything are fixed in Part 6.

If you want me to move the relevant changes from Part 4 and 6 into Part 1 and 2, I could do so as well.

Thanks for the review of the first patch.

Regards,

Jens
(Assignee)

Updated

5 years ago
Attachment #785452 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785453 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785455 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785457 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785458 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785459 - Flags: review?(mconley)
(Assignee)

Comment 77

5 years ago
As far as I can see, all this is Compose-related, so asking review from you, Mike.

Blake, what is the preferred way to do UI review? I see three relevant points:
 * The change of minresultsforpopup in all cases.
 * The search behaviour of the new autocomplete feature for newsgroup names. Currently, the typed text has to appear as a substring anywhere in the newsgroup name. Perhaps it should rather match only at the beginning?
 * As the icon for the newsgroup autocomplete entries, I used the existing folder-newsgroup.png.

BR, Jens
(Assignee)

Updated

5 years ago
Flags: needinfo?(bwinton)
(Assignee)

Comment 78

5 years ago
Created attachment 785465 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.
(Assignee)

Comment 79

5 years ago
Arxy, can you push attachment 785465 [details] [diff] [review] to tryserver for me, please?
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 80

5 years ago
Created attachment 785466 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts
(Assignee)

Updated

5 years ago
Attachment #785466 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 81

5 years ago
Ian,

I am giving this to you as the owner of the Composition sub-module in Seamonkey. Please reassign the review if appropriate. The patch I set the r? on contains only the changes in suite/. To understand things, it might be easier to look at the entire patches, which are split up into 6 parts.

BR, Jens
(Assignee)

Comment 83

5 years ago
(In reply to Archaeopteryx [:aryx] from comment #82)
> Pushed to Thunderbird-Try as
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=0a30e8488e9d

Thanks, aryx.

Build and tests have run through now. As far as I can see, the tests show only issues for which bugs have already been filed, i.e., that were not caused by this patch.
(Assignee)

Comment 84

5 years ago
Magnus, the wanted-thunderbird3+ is obviously obsolete. Can you check whether some other wanted flag or target should be set instead?
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jens Müller (:tessarakt) from comment #77)
> As far as I can see, all this is Compose-related, so asking review from you,
> Mike.
> 
> Blake, what is the preferred way to do UI review? I see three relevant
> points:

Set ui-r? someone, and mention the points you mentioned.  ;)

(mconley might be a decent choice, since he's already reviewing the patches…)

For what it's worth, I gave the try-build a test run, and couldn't get newsgroup names to autocomplete, so a set of steps that demonstrates the functionality would make it way easier to ui-review…
Flags: needinfo?(bwinton)
(Assignee)

Comment 86

5 years ago
I still have different issues :-(

- In the autocomplete popup list, no icons are shown.
- The address type for the first row seems to determine the applicable autocomplete modules for all rows

I hope that will not be too difficult to find.
(Assignee)

Comment 87

5 years ago
Created attachment 785543 [details] [diff] [review]
Part 1: Switch autocompletesearchparam to JSON
Attachment #785452 - Attachment is obsolete: true
Attachment #785543 - Flags: review?(mconley)
Attachment #785452 - Flags: review?(mconley)
(Assignee)

Comment 88

5 years ago
Created attachment 785544 [details] [diff] [review]
Part 2: Add address type to JSON
Attachment #785453 - Attachment is obsolete: true
Attachment #785453 - Flags: review?(mconley)
Attachment #785544 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785544 - Attachment description: 61491-part2-add-address-type-to-json.patch → Part 2: Add address type to JSON
(Assignee)

Comment 89

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #86)
> - The address type for the first row seems to determine the applicable
> autocomplete modules for all rows

See bug 452232 comment 90.

When I remove that line with the call to syncSessions, everything works fine. I think this was overlooked in bug 452232 (or one of its related cases).
(Assignee)

Updated

5 years ago
Depends on: 899822
(Assignee)

Comment 90

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #86)
> - In the autocomplete popup list, no icons are shown.

Apparently, the themes have changed considerably ...

Multiple icons are now combined into one .png and then chosen like this:

  list-style-image: url("chrome://messenger/skin/icons/folder-pane.png");
  -moz-image-region: rect(208px 16px 224px 0px);

I will have to go carefully through all the .css files again. Until then, please ignore them (especially the icon locations) in Part 5.
(In reply to Jens Müller (:tessarakt) from comment #69)
> Mark, can you confirm that handling LDAP sessions manually here has become
> obsolete with the landing of bug 452232? I am currently debitrotting the
> patch here.

Yes they are obsolete, as are any references to gLDAPSession.

(In reply to Jens Müller (:tessarakt) from comment #84)
> Magnus, the wanted-thunderbird3+ is obviously obsolete. Can you check
> whether some other wanted flag or target should be set instead?

We don't generally use the wanted-* flags any more - although they were set, they didn't really provide much use.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(mbanner)
(Assignee)

Updated

5 years ago
Flags: wanted-thunderbird3+
(Assignee)

Comment 92

5 years ago
Created attachment 785850 [details] [diff] [review]
Part 5: Implement autocomplete module for news

This now contains the proper icons in the CSS.

The CSS properties taken from the folderPane.css in the respective theme. The only exception is suite/themes/classic/mac/messenger/messengercompose/messengercompose.css, where no icon for newsgroups is defined in folderPane.css - therefore I have just copied the properties from the non-mac classic theme.
Attachment #785458 - Attachment is obsolete: true
Attachment #785458 - Flags: review?(mconley)
Attachment #785850 - Flags: review?(mconley)
(Assignee)

Comment 93

5 years ago
Created attachment 785854 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.
Attachment #785465 - Attachment is obsolete: true
(Assignee)

Comment 94

5 years ago
Created attachment 785855 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts
Attachment #785466 - Attachment is obsolete: true
Attachment #785466 - Flags: review?(iann_bugzilla)
Attachment #785855 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 95

5 years ago
Created attachment 785858 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.
Attachment #785854 - Attachment is obsolete: true
(Assignee)

Comment 96

5 years ago
Created attachment 785861 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts
Attachment #785855 - Attachment is obsolete: true
Attachment #785855 - Flags: review?(iann_bugzilla)
Attachment #785861 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 97

5 years ago
Aryx, can you push another try build for me?

again
try: -b do -p all -u all -t none

Patches to apply:
 1. https://bug899822.bugzilla.mozilla.org/attachment.cgi?id=783416
 2. https://bug61491.bugzilla.mozilla.org/attachment.cgi?id=785858

Thanks! BR, Jens
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 98

5 years ago
Created attachment 785877 [details]
Screenshot of autocompletion in action

The screenshot shows the icon used and that match is done on any substring.
Attachment #785877 - Flags: ui-review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #785455 - Flags: ui-review?(mconley)
Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=383dffee295c - Thunderbird-Trunk is closed due to bustage, you might want to check out Standard8's xpcshell debug Try runs.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 100

5 years ago
Comment on attachment 785861 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts

I need to make some small corrections for the SeaMonkey part (I was not able to build SeaMonkey before yesterday so could not test). I will update the individual parts of the Patch accordingly.
Attachment #785861 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 101

5 years ago
Created attachment 786441 [details] [diff] [review]
Part 1: Switch autocompletesearchparam to JSON
Attachment #785543 - Attachment is obsolete: true
Attachment #785543 - Flags: review?(mconley)
Attachment #786441 - Flags: review?(mconley)
(Assignee)

Comment 102

5 years ago
Created attachment 786442 [details] [diff] [review]
Part 5: Implement autocomplete module for news
Attachment #785850 - Attachment is obsolete: true
Attachment #785850 - Flags: review?(mconley)
Attachment #786442 - Flags: review?(mconley)
(Assignee)

Comment 103

5 years ago
Created attachment 786443 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.
Attachment #785858 - Attachment is obsolete: true
(Assignee)

Comment 104

5 years ago
Created attachment 786444 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts
Attachment #785861 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #786444 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 105

5 years ago
Created attachment 786480 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.
Attachment #786443 - Attachment is obsolete: true
Comment on attachment 786480 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.

Ok, now that I've examined each piece, I'm going to try reviewing just the combined patch.
Attachment #786480 - Flags: review?(mconley)
Attachment #785455 - Flags: ui-review?(mconley)
Attachment #785455 - Flags: review?(mconley)
Comment on attachment 786480 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.

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

I've identified mostly style nits, but I have to say that most of this looks very sane (although I've yet to actually test it).

I'll probably test it tomorrow evening, when I'm back in my TB environment, but in the meantime, these style nits need to be fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3787,4 @@
>          var idKey = identityElement.value;
>          gCurrentIdentity = MailServices.accounts.getIdentity(idKey);
>  
> +        var accountKey = null;

let, not var

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +774,4 @@
>  
>  function _awSetAutoComplete(selectElem, inputElem)
>  {
> +  var params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));

let, not var

::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
@@ +16,3 @@
>    cachedIdentity: null,
>  
> +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],

Maybe it's just me, but I prefer a Javascript Set for this sort of thing. That way, instead of using indexOf == -1, we can just do:

someSet.has(x)

Also, is there a common place we could put such a Set? I'm seeing the same 4 things defined in an Array several times.

@@ +17,5 @@
>  
> +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> +
> +  startSearch: function(aString, aSearchParam, aResult, aListener) {
> +    var params = JSON.parse(aSearchParam);

Use let, not var

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +344,4 @@
>     */
>    startSearch: function startSearch(aSearchString, aSearchParam,
>                                      aPreviousResult, aListener) {
> +    var params = JSON.parse(aSearchParam);

let, not var. Apply this to all of your changes - I'll stop mentioning this now.

::: mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js
@@ +173,4 @@
>      // result ignored.
>      // The comma check is so that we don't autocomplete against the user
>      // entering multiple addresses.
> +    if ( !applicable || !aSearchString || aSearchString.contains(",")) {

No space after (

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteMyDomain.js
@@ +37,5 @@
> +  // Set up autocomplete parameters
> +  let params = JSON.stringify({ idKey: identity.key, type: "addr_to" });
> +  let paramsNews = JSON.stringify({ idKey: identity.key, type: "addr_newsgroups" });
> +  let paramsFollowup = JSON.stringify({ idKey: identity.key, type: "addr_followup" });
> +  

Nit, trailing whitespace. There are more of these in here - use the Splinter view to see them all, and please omit those in your next patch.

@@ +96,5 @@
>    do_check_eq(obs._result.getImageAt(0), null);
>  
> +  // No autocomplete for addr_newsgroups!
> +  acs.startSearch("a@b", paramsNews, null, obsNews);
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );

Spaces on either side of == - no space before )

@@ +100,5 @@
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );
> +  
> +  // No autocomplete for addr_followup!
> +  acs.startSearch("a@b", paramsFollowup, null, obsFollowup);
> +  do_check_true(obsFollowup._result==null || obsFollowup._result.matchCount==0 );

Same as above

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +178,5 @@
>    do_check_eq(obs._result.getImageAt(0), "");
>  
> +  // quick-check that nothing is found for addr_newsgroups
> +  acs.startSearch("email", paramNews, null, obsNews);
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );

Spaces on either side of ==, no space before ). Same below.

::: mailnews/news/src/Makefile.in
@@ +21,4 @@
>  OS_CXXFLAGS += -DNOMINMAX
>  endif
>  
> +EXTRA_COMPONENTS += \

These need to go into the moz.build now.

::: mailnews/news/src/nsNewsAutoCompleteSearch.js
@@ +1,5 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 
> +

Let's define Cu, Ci, and Cc up here, for brevity's sake.

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const ACR = Components.interfaces.nsIAutoCompleteResult;

Either all consts should start with k, like kACR and kSupportedTypes, or be all caps. Please pick one and stick with it.

@@ +11,5 @@
> +
> +function nsNewsAutoCompleteResult(aSearchString) {
> +  // Can't create this in the prototype as we'd get the same array for
> +  // all instances
> +  this._searchResults = new Array();

this._searchResults = [];

@@ +74,5 @@
> +   * @param accountKey  The key of the account.
> +   * @return            The incoming news server (or null if one does not exist).
> +   */
> +  _findServer: function _findServer(accountKey) {
> +    let msgAccountManager =

MailServices.accounts should be used instead. Search mxr for usages of MailServices.

@@ +97,5 @@
> +  startSearch: function startSearch(aSearchString, aSearchParam,
> +                                    aPreviousResult, aListener) {
> +    var params = JSON.parse(aSearchParam);
> +    var result = new nsNewsAutoCompleteResult(aSearchString);
> +    if( !params.type || !params.accountKey || 

Space between if and (. No space after (.

@@ +104,5 @@
> +      aListener.onSearchResult(this, result);
> +      return;
> +    }
> +
> +    if(params.accountKey != this.cachedAccountKey) {

Space after if

@@ +117,5 @@
> +        if (curr.prettiestName.indexOf(aSearchString) != -1) {
> +          result._searchResults.push({
> +            value: curr.prettiestName,
> +            comment: this.cachedServer.prettyName
> +            });

Wrong indentation.

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +30,5 @@
> +  let server = makeServer(NNTP_RFC977_handler, daemon);
> +  server.start(NNTP_PORT);
> +  
> +  // create identity
> +  var acctmgr = Cc["@mozilla.org/messenger/account-manager;1"]

MailServices.accounts
Attachment #786480 - Flags: review?(mconley) → review-
(Assignee)

Comment 108

5 years ago
(In reply to Mike Conley (:mconley) from comment #107)

> ::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
> @@ +16,3 @@
> >    cachedIdentity: null,
> >  
> > +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> 
> Maybe it's just me, but I prefer a Javascript Set for this sort of thing.
> That way, instead of using indexOf == -1, we can just do:
> 
> someSet.has(x)


I just found https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set on this, and it says: 

"Warning: The SpiderMonkey Set implementation is a prototype and the Set API and semantics specifications are unstable. The SpiderMonkey implementation may not reflect the latest specification draft. It is subject to change any time. It is provided as an experimental feature. Do not rely on it for production code."

Is this really something that should be used already?
 
> Also, is there a common place we could put such a Set? I'm seeing the same 4
> things defined in an Array several times.

Yeah, in all the autocomplete components for mail (search, my domain, LDAP). They all import resource:///modules/mailServices.js, but I don't think that is a proper place ...

The values are defined in mail/components/compose/content/messengercompose.xul (and its suite counterpart) - 4 of them apply to mail and 2 apply to news.
(In reply to Jens Müller (:tessarakt) from comment #108)
> (In reply to Mike Conley (:mconley) from comment #107)
> 
> > ::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
> > @@ +16,3 @@
> > >    cachedIdentity: null,
> > >  
> > > +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> > 
> > Maybe it's just me, but I prefer a Javascript Set for this sort of thing.
> > That way, instead of using indexOf == -1, we can just do:
> > 
> > someSet.has(x)
> 
> 
> I just found
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Set on this, and it says: 
> 
> "Warning: The SpiderMonkey Set implementation is a prototype and the Set API
> and semantics specifications are unstable. The SpiderMonkey implementation
> may not reflect the latest specification draft. It is subject to change any
> time. It is provided as an experimental feature. Do not rely on it for
> production code."
> 
> Is this really something that should be used already?

I think so, yes. I can tell you that they're used quite liberally within Firefox's code.

>  
> > Also, is there a common place we could put such a Set? I'm seeing the same 4
> > things defined in an Array several times.
> 
> Yeah, in all the autocomplete components for mail (search, my domain, LDAP).
> They all import resource:///modules/mailServices.js, but I don't think that
> is a proper place ...
> 
> The values are defined in
> mail/components/compose/content/messengercompose.xul (and its suite
> counterpart) - 4 of them apply to mail and 2 apply to news.

Ok, I wouldn't push too hard on that then. It just seemed a bit silly to redefine the same collections over and over again.
(Assignee)

Comment 110

5 years ago
Created attachment 804114 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)
Attachment #409189 - Attachment is obsolete: true
Attachment #662104 - Attachment is obsolete: true
Attachment #785455 - Attachment is obsolete: true
Attachment #785457 - Attachment is obsolete: true
Attachment #785459 - Attachment is obsolete: true
Attachment #785544 - Attachment is obsolete: true
Attachment #786441 - Attachment is obsolete: true
Attachment #786442 - Attachment is obsolete: true
Attachment #786444 - Attachment is obsolete: true
Attachment #786480 - Attachment is obsolete: true
Attachment #786444 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 111

5 years ago
I addressed the review comments (except for removing the multiple definitions of the set with the address types) and created a new patch. I also pushed it to the tryserver: https://hg.mozilla.org/try-comm-central/rev/2bcf17850020
(Assignee)

Comment 112

5 years ago
Comment on attachment 804114 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

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

::: mailnews/test/resources/localAccountUtils.js
@@ +97,4 @@
>      }
>      server.valid = true;
>  
> +    return { server: server, account: account};

OK, I guess I found another nit myself ...

No space after the {, right?
(Assignee)

Updated

5 years ago
Attachment #804114 - Attachment is obsolete: true
(Assignee)

Comment 113

5 years ago
Created attachment 804897 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)
Attachment #804897 - Flags: review?(mconley)
Comment on attachment 804897 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

Do the results display in a predictable order?

>+        if (curr.prettiestName.indexOf(aSearchString) != -1) {
Nit: use .contains instead

>+  inputElem.setAttribute('autocompletesearchparam',JSON.stringify(params));
Nit: space after comma
(Assignee)

Comment 115

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #114)
> Comment on attachment 804897 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter)
> 
> Do the results display in a predictable order?

I tried it out: It is exactly the same order in which the newsgroups are shown in the folder tree (which is the order in the newsrc file).

> >+        if (curr.prettiestName.indexOf(aSearchString) != -1) {
> Nit: use .contains instead
> 
> >+  inputElem.setAttribute('autocompletesearchparam',JSON.stringify(params));
> Nit: space after comma

Fixed.
(Assignee)

Comment 116

5 years ago
Created attachment 804938 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)
Attachment #804897 - Attachment is obsolete: true
Attachment #804897 - Flags: review?(mconley)
Attachment #804938 - Flags: review?(mconley)
Comment on attachment 804938 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

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

Just some style nits, and you'll probably want a mailnews reviewer to give their blessing, but this is looking really good. r=me with the nits fixed and the tests all passing.

Can you please get this pushed to try to ensure that the tests pass on our infrastructure?[1]

Thanks,

-Mike

[1]: https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +114,5 @@
>  
> +  let param = JSON.stringify({ type: "addr_to"  });
> +  let paramNews = JSON.stringify({ type: "addr_newsgroups"  });
> +  let paramFollowup = JSON.stringify({ type: "addr_followup"  });
> +  

Trailing whitespace.

::: mailnews/news/src/nsNewsAutoCompleteSearch.js
@@ +1,4 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 

Trailing whitespace

@@ +36,5 @@
> +
> +  getValueAt: function getValueAt(aIndex) {
> +    return this._searchResults[aIndex].value;
> +  },
> +  

Trailing ws

::: mailnews/news/test/unit/head_server_setup.js
@@ +93,4 @@
>  const NNTP_PORT = 1024+119;
>  
>  var _server = null;
> +var _account = null;

let, not var

@@ +110,4 @@
>  function setupLocalServer(port) {
>    if (_server != null)
>      return _server;
> +  let serverAndAccount = 

trailing whitespace

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +8,5 @@
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +// The basic daemon to use for testing nntpd.js implementations
> +var daemon = setupNNTPDaemon();

let, not var. This is a global, and so it should be prefixed with a g, like gDaemon.

@@ +38,5 @@
> +  let acs = Components.classes["@mozilla.org/autocomplete/search;1?name=news"]
> +    .getService(Components.interfaces.nsIAutoCompleteSearch);
> +  let obs;
> +
> +  let paramsN = JSON.stringify({ 

Trailing whitespace on this line, 46 and 48

@@ +54,5 @@
> +
> +  // misc.test is not subscribed
> +  obs = new acObserver();
> +  acs.startSearch("misc", paramsN, null, obs);
> +  do_check_true(obs._result==null || obs._result.matchCount==0 );

Spaces on either side of the =='s on line 58, 62, 66, 80.

No spaces before ).
Attachment #804938 - Flags: review?(mconley) → review+
Comment on attachment 785877 [details]
Screenshot of autocompletion in action

I don't think this patch warrants a UI review, as it's really just re-using a pre-existing mechanism that is well understood - just making it include more stuff.
Attachment #785877 - Flags: ui-review?(mconley)
(Assignee)

Comment 119

5 years ago
Created attachment 806093 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)
(Assignee)

Comment 120

5 years ago
(In reply to Mike Conley (:mconley) from comment #117)
> Comment on attachment 804938 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter)
> 
> Review of attachment 804938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some style nits, and you'll probably want a mailnews reviewer to give
> their blessing, but this is looking really good. r=me with the nits fixed
> and the tests all passing.

All nits are fixed. I uploaded a new patch.

> Can you please get this pushed to try to ensure that the tests pass on our
> infrastructure?[1]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=764c3467d1b7
 
> Thanks,
> 
> -Mike

Thanks for the diligent review, and sorry for all the nits you had to point out.

 - Jens
(Assignee)

Comment 121

5 years ago
Comment on attachment 806093 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

Neil, you reviewed an earlier version of this patch.

Could you also review the (hopefully) final version, with the hats of both a Mail and News Backend and a SeaMonkey Peer?

The patch (without the nit fixes) already got r+ from mconley.
Attachment #806093 - Flags: review?(neil)
(Assignee)

Comment 122

5 years ago
Created attachment 806190 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v5
Attachment #806093 - Attachment is obsolete: true
Attachment #806093 - Flags: review?(neil)
Attachment #806190 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Attachment #806190 - Flags: review?(neil)
(Assignee)

Comment 124

5 years ago
Created attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

I had to add packaging specs for the new JS module ... 

Now tests seem to be running through:  https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9717acb524e9 (not finished yet on all platforms).
Attachment #806190 - Attachment is obsolete: true
Attachment #806402 - Flags: review?(neil)
(Assignee)

Comment 125

5 years ago
Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

mconley: As I had to add packaging declarations, you might want to have another look:  https://bugzilla.mozilla.org/attachment.cgi?oldid=804938&action=interdiff&newid=806402&headers=1
Attachment #806402 - Flags: review?(mconley)
Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

Still looks good. Thanks Jens.
Attachment #806402 - Flags: review?(mconley) → review+
Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

>-  cachedParam: "",
>+  cachedIdKey: "",
You renamed cachedParam to cachedIdKey...

>+      if (params.idKey != this.cachedParam) {
>+        this.cachedIdentity = MailServices.accounts.getIdentity(params.idKey);
>+        this.cachedParam = params.idKey;
... but you didn't rename the uses.

>+const kSupportedTypes = Set(["addr_newsgroups","addr_followup"]);
Nit: space after comma.

>+    for (let i = 0; i < numAccounts; i++) {
>+      let account = accounts.queryElementAt(i, Ci.nsIMsgAccount);
>+      if (account.key == accountKey)
What's wrong with MailServices.accounts.getAccount(accountKey)?

>+        let accountKey = null;
>+        if (identityElement.selectedItem)
>+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
When does the identityElement not have a selectedItem?

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
>+          params.idKey = idKey;
>+          params.accountKey = accountKey;
>+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
Please use the searchParam property.

>-    autoCompleteWidget.minResultsForPopup = 2;
You didn't change the default value for SeaMonkey.

>+  let params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));
>+  params.type = selectElem.value;
>+  inputElem.setAttribute('autocompletesearchparam', JSON.stringify(params));
searchParam again please.
(Assignee)

Comment 128

5 years ago
Hi Neil,

(In reply to neil@parkwaycc.co.uk from comment #127)
> Comment on attachment 806402 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter) - v6
> 
> >-  cachedParam: "",
> >+  cachedIdKey: "",
> You renamed cachedParam to cachedIdKey...
> 
> >+      if (params.idKey != this.cachedParam) {
> >+        this.cachedIdentity = MailServices.accounts.getIdentity(params.idKey);
> >+        this.cachedParam = params.idKey;
> ... but you didn't rename the uses.
> 
> >+const kSupportedTypes = Set(["addr_newsgroups","addr_followup"]);
> Nit: space after comma.

All fixed.

> >+    for (let i = 0; i < numAccounts; i++) {
> >+      let account = accounts.queryElementAt(i, Ci.nsIMsgAccount);
> >+      if (account.key == accountKey)
> What's wrong with MailServices.accounts.getAccount(accountKey)?

Nothing, presumably. I changed it and will test it.

> >+        let accountKey = null;
> >+        if (identityElement.selectedItem)
> >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> When does the identityElement not have a selectedItem?

If there are no accounts, maybe? Or is that not possible?

> >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> >+          params.idKey = idKey;
> >+          params.accountKey = accountKey;
> >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> Please use the searchParam property.

I beg your pardon? Could you explain a little bit more? Which searchParam property?

> >-    autoCompleteWidget.minResultsForPopup = 2;
> You didn't change the default value for SeaMonkey.

I do it now.
 
> >+  let params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));
> >+  params.type = selectElem.value;
> >+  inputElem.setAttribute('autocompletesearchparam', JSON.stringify(params));
> searchParam again please.

Same question as above ...

Thanks a lot for the quick review!

Jens
(In reply to Jens Müller from comment #128)
> (In reply to comment #127)
> > >+        let accountKey = null;
> > >+        if (identityElement.selectedItem)
> > >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> > When does the identityElement not have a selectedItem?
> If there are no accounts, maybe? Or is that not possible?
Well, what I thought happens, is that if there are no accounts then you get prompted to add an account, and if there are still no accounts then the window closes. But I don't know whether this code runs before or after that.

> > >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> > >+          params.idKey = idKey;
> > >+          params.accountKey = accountKey;
> > >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> > Please use the searchParam property.
> I beg your pardon? Could you explain a little bit more? Which searchParam
> property?
I meant this one, but it seems that MDN isn't quite up-to-date with textbox XPFE autocomplete properties so I need to add it and others in:
https://developer.mozilla.org/en-US/docs/XUL/Property/searchParam
(Assignee)

Comment 130

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #129)
> (In reply to Jens Müller from comment #128)
> > (In reply to comment #127)
> > > >+        let accountKey = null;
> > > >+        if (identityElement.selectedItem)
> > > >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> > > When does the identityElement not have a selectedItem?
> > If there are no accounts, maybe? Or is that not possible?
> Well, what I thought happens, is that if there are no accounts then you get
> prompted to add an account, and if there are still no accounts then the
> window closes. But I don't know whether this code runs before or after that.

I added an else branch with a dump, and no message was output. So apparently this code runs after the prompt.

> > > >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> > > >+          params.idKey = idKey;
> > > >+          params.accountKey = accountKey;
> > > >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> > > Please use the searchParam property.
> > I beg your pardon? Could you explain a little bit more? Which searchParam
> > property?
> I meant this one, but it seems that MDN isn't quite up-to-date with textbox
> XPFE autocomplete properties so I need to add it and others in:
> https://developer.mozilla.org/en-US/docs/XUL/Property/searchParam

Thanks - fixed.
(Assignee)

Comment 131

5 years ago
Created attachment 807888 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v7
Comment on attachment 807888 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v7

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

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +44,5 @@
> +    accountKey: _account.key,
> +    type: "addr_newsgroups" });
> +  let paramsF = JSON.stringify({
> +    idKey: identity.key,
> +    accountKey: _account.key, 

trailing whitespace
(Assignee)

Comment 134

5 years ago
Created attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8
Attachment #807888 - Attachment is obsolete: true
(Assignee)

Comment 135

5 years ago
Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

The xpcshell tests still seem to be fine: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=6acf55f2343c

Neil, I addressed all your review comments.

Mike, I changed two lines in mail/components/compose/content/MsgComposeCommands.js based on Neil's suggestions, so technically I need another Thunderbird review ...

Diff to the last version of the patch is here: https://bugzilla.mozilla.org/attachment.cgi?oldid=806402&action=interdiff&newid=808045&headers=1
Attachment #808045 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Attachment #808045 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #804938 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #806402 - Attachment is obsolete: true
Attachment #806402 - Flags: review?(neil)

Comment 136

5 years ago
Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

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

(In reply to Jens Müller (:tessarakt) from comment #135)
> Mike, I changed two lines in
> mail/components/compose/content/MsgComposeCommands.js based on Neil's
> suggestions, so technically I need another Thunderbird review ...

For such cases it's fine to just carry forward the review.
Attachment #808045 - Flags: review?(mconley) → review+
(Assignee)

Updated

5 years ago
Attachment #808045 - Flags: superreview?
(Assignee)

Updated

5 years ago
Attachment #808045 - Flags: superreview? → superreview?(neil)
Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

>+treechildren::-moz-tree-image(subscribed-news) {
>+  margin-top: 2px;
>+  margin-bottom: 2px;
>+  -moz-margin-start: 2px;
>+  -moz-margin-end: -3px;
>+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
>+}
So, you seem to have over-zealously copied and pasted here. In particular the existing icons are all over the shop when it comes to image size, and so the margins don't make sense for the newsgroup icon. In particular the top and bottom margins cause the image to get clipped. r=me with those removed.

(The start and end margins aren't right either, but that's the problem of the remote-abook settings that you copied from, so it's OK to keep those for consistency.)
Attachment #808045 - Flags: review?(neil) → review+

Updated

5 years ago
Attachment #808045 - Flags: superreview?(neil) → superreview?(mbanner)
(Assignee)

Comment 138

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #137)
> Comment on attachment 808045 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter) - v8
> 
> >+treechildren::-moz-tree-image(subscribed-news) {
> >+  margin-top: 2px;
> >+  margin-bottom: 2px;
> >+  -moz-margin-start: 2px;
> >+  -moz-margin-end: -3px;
> >+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
> >+}
> So, you seem to have over-zealously copied and pasted here. In particular
> the existing icons are all over the shop when it comes to image size, and so
> the margins don't make sense for the newsgroup icon. In particular the top
> and bottom margins cause the image to get clipped. r=me with those removed.

This block is identical in suite/themes/classic/mac/messenger/messengercompose/messengercompose.css and suite/themes/classic/messenger/messengercompose/messengercompose.css so I assume I have to make the change in both files.

In suite/themes/modern/messenger/messengercompose/messengercompose.css I use url("chrome://messenger/skin/icons/folder-newsgroup.gif"), with the same margin values. Both images have identical size (16x16 pixels), so I assume I have to remove margin-top and margin-bottom there as well.
(Assignee)

Comment 139

5 years ago
Created attachment 810010 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v9
(Assignee)

Comment 140

5 years ago
Comment on attachment 810010 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v9

Carrying forward review by Neil and mconley.

According to http://www.seamonkey-project.org/dev/review-and-flags, I also need moa from a SeaMonkey MailNews module owner or peer (unless one of them says otherwise) ....
Attachment #810010 - Flags: superreview?(mbanner)
Attachment #810010 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #808045 - Attachment is obsolete: true
Attachment #808045 - Flags: superreview?(mbanner)
(Assignee)

Comment 141

5 years ago
(Neil confirmed on IRC that his r+ also includes moa+.)
Comment on attachment 810010 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v9

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

Looks good, sr=Standard8
Attachment #810010 - Flags: superreview?(mbanner) → superreview+
(Assignee)

Updated

5 years ago
Keywords: helpwanted → checkin-needed
https://hg.mozilla.org/comm-central/rev/5ac1dfd37ebf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 27.0
Backed out for Linux debug xpcshell crashes.
https://hg.mozilla.org/comm-central/rev/723f9a787538

https://tbpl.mozilla.org/php/getParsedLog.php?id=29730060&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 27.0 → ---
(Assignee)

Comment 145

5 years ago
Thanks, Ryan.

I could not produce this locally until now.

When I upgraded the patch to recent c-c, I noticed it will not cleanly apply due to this one line: http://hg.mozilla.org/comm-central/annotate/cd7065b3430f/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js#l130 . Accordingly, I will provide a new patch file and carry forward the existing r+ and sr+. 

As I cannot reproduce locally, I will push the patch to try again ...
(Assignee)

Comment 147

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #146)
> Pushed to try again:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4c77a703edbe

Sorry, https://hg.mozilla.org/try-comm-central/rev/d0cfdb9d7787

Forgot to include xpcshell tests ...
(Assignee)

Comment 148

5 years ago
The problem appeared again on try, so apparently this is valid.


The NS_ASSERTION occurs here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgMailSession.cpp#68 - nsMsgMailSession::AddFolderListener is called with a listener that already exists.

This is called from here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1226 within nsMsgAccountManager::LoadAccounts(). What seems a bit strange to me is the way possible reentrant calls to this function are handled ... There is a quick-out depending on m_accountsLoaded at the very beginning of the method, but this flag is only set at the end of the function ... So one thing to try out is add some debug output indicating calls to and successful completions of this function ...

Next step up in the stacktrace is nsMsgAccountManager::FindServerByURI at http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1824 - I cannot see anything unusual here.


The test script test_newsAutocomplete.js seems to finish fine: 
TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (1)
TEST-INFO | (xpcshell/head.js) | exiting test
TEST-PASS | (xpcshell/head.js) | 11 (+ 0) check(s) passed
TEST-INFO | (xpcshell/head.js) | 0 check(s) todo


And according to these log entries:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mailnews/base/src/nsMsgAccountManager.cpp, line 1825
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mailnews/base/util/nsMsgDBFolder.cpp, line 3194
###!!! ASSERTION: tried to add duplicate listener: 'index == -1', file ../../../../mailnews/base/src/nsMsgMailSession.cpp, line 68

there apparently _are_ multiple calls to LoadAccounts() ... First, one is not successful, causing the NS_ENSURE_SUCCESS in line 1825 to fail. The call to nsMsgAccountManager::LoadAccounts() (which subsequently leads to a call to nsMsgMailSession::AddFolderListener and then the failed NS_ASSERTION) is already in line 1824, so this is a different call.

So this is the second thing to debug: Why does LoadAccounts() return an error code the first time? There is only one return statement not returning NS_OK, so it has to be that one:

1229   // If we have code trying to do things after we've unloaded accounts,
1230   // ignore it.
1231   if (m_shutdownInProgress || m_haveShutdown)
1232     return NS_ERROR_FAILURE;
(Assignee)

Comment 149

5 years ago
I added some debug output - let's see what happens ...

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=916a41329c94
(Assignee)

Comment 150

5 years ago
jcranmer provided some helpful information on IRC.

He pointed out that the legacy NNTP credentials migration is being triggered during shutdown. This should not happen normally, it does happen in the tests because the event loop is not executed throughout the whole test, only at the end when the fakeserver is shutdown.

He further gave the advice to not start up and shutdown the fakeserver if I am not querying it. In fact, I AM NOT querying it in the test - I just blindly subscribe to newsgroups without ever retrieving the newsgroup list from the server.

So what I did now is to remove "server.start(NNTP_PORT);" and "server.stop();" from the test.

I pushed another try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7c949c33ae4f - let's see what happens.
(Assignee)

Comment 151

5 years ago
P.S.:

I do not claim to completely understand the problem, but is this a real solution or just a workaround for a more complex underlying issue? 

In particular, I have not yet understood whether this is deterministic or a timing problem ...
(Assignee)

Comment 152

5 years ago
Created attachment 827603 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v10
(Assignee)

Comment 153

5 years ago
Diffing the patches shows this as the only "material" difference, in mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js:

    function checkSearch(element, index, array) {
--    acs.startSearch(element, null, null, obs);
+     print("Checking " + element);
+-    acs.startSearch(element, null, null, obs)
 +    acs.startSearch(element, JSON.stringify({ type: "addr_to"  }), null, obs);

diff also finds a fake difference which does not really exist ...

Well, I trust the person who will check in the patch again will be able to resolve that conflict manually :)
(Assignee)

Comment 154

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #150)
> I pushed another try build:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7c949c33ae4f - let's see
> what happens.

The first crash (failing assert) is gone - but now there is a new one:

###!!! ASSERTION: nsNSSComponent relies on profile manager to wait for synchronous shutdown of all network activity: 'mIsNetworkDown', file ../../../../../../mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 1869

Bug 819746 seems related.
(Assignee)

Comment 155

5 years ago
I got it narrowed down: The crash occurs while loading mailnews/news/test/unit/tail_news.js, so probably _in_ that file ...
(Assignee)

Comment 156

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #155)
> I got it narrowed down: The crash occurs while loading
> mailnews/news/test/unit/tail_news.js, so probably _in_ that file ...

Sorry, it's not the original crash that occurs there, it's the construction of the nsNSSComponent.

Apparently, it occurs within mailnews/test/resources/mailShutdown.js, which is loaded from there, in the call to Services.obs.notifyObservers(null, "profile-change-net-teardown", null).

More precisely:

postShutdownNotifications() started.
nsObserverService::NotifyObservers: Notifying observers about quit-application-requested
nsObserverList::NotifyObservers: Notiying 1 observers about quit-application-requested
nsObserverService::NotifyObservers: Notified observers about quit-application-requested
observers notified about quit-application-requested.
Notifying observers about quit-application
nsObserverService::NotifyObservers: Notifying observers about quit-application
nsObserverList::NotifyObservers: Notiying 1 observers about quit-application
nsObserverService::NotifyObservers: Notified observers about quit-application
Notified observers about quit-application
Notifying observers about profile-change-net-teardown
nsObserverService::NotifyObservers: Notifying observers about profile-change-net-teardown
nsObserverList::NotifyObservers: Notiying 1 observers about profile-change-net-teardown
nsObserverService::NotifyObservers: Notifying observers about ipc:network:set-offline
nsObserverService::NotifyObservers: Notified observers about ipc:network:set-offline
nsObserverService::NotifyObservers: Notifying observers about network:offline-about-to-go-offline
nsObserverList::NotifyObservers: Notiying 1 observers about network:offline-about-to-go-offline
Creating instance of {e6f68040-c7ec-11d3-8cda-0060b0fc14a3}
Creating instance of {6476378a-da09-11d3-8cda-0060b0fc14a3}
nsObserverService::NotifyObservers: Notified observers about network:offline-about-to-go-offline
nsObserverService::NotifyObservers: Notifying observers about network:offline-status-changed
nsObserverService::NotifyObservers: Notified observers about network:offline-status-changed
Creating instance of {138ad1b2-c694-41cc-b201-333ce936d8b8}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {338c8597-1e32-4682-b5c7-cf8142c0bd1d}
Creating instance of {6ffbb526-205b-49c5-ae3f-5959c084075e}
###!!! ASSERTION: nsNSSComponent::nsNSSComponent() started: 'false', file /home/jens/devel/releases-comm-central/mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 22

So whatever it is that is notified about profile-change-net-teardown creates 6 XPCOM component instances, the last of which is nsNSSComponent ...

Quite ironic that this happens exactly for the topic which nsNSSComponent would have needed to have observed itself ...
(Assignee)

Comment 157

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #156)
> So whatever it is that is notified about profile-change-net-teardown creates
> 6 XPCOM component instances, the last of which is nsNSSComponent ...

The observer called is nsIOService:
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#910
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#659

All 6 component instances are created within this call to mSocketTransportService->Shutdown():
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#748

mSocketTransportService is a nsPISocketTransportService ("@mozilla.org/network/socket-transport-service;1"), retrieved here:
 - mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#225
(Assignee)

Comment 158

5 years ago
Indeed the first creation of nsNSSComponent happens in mozilla::net::NetworkActivityMonitor::Shutdown(). 

More precisely: In the invocation of mThread->Shutdown().
(Assignee)

Comment 159

5 years ago
On IRC, Standard8 pointed me to this:

> http://mxr.mozilla.org/comm-central/source/mailnews/news/test/unit/test_nntpPassword.js#51 let any events cycle before getting into the tail

Indeed, this seems to work - I will prepare a new try build now.
(Assignee)

Comment 161

5 years ago
Created attachment 828885 [details] [diff] [review]
Followup: Fixes for shutdown problems in xpcshell test
Attachment #828885 - Flags: review?(Pidgeot18)
(Assignee)

Comment 162

5 years ago
Green X on Try for all platforms (except Windows, which does not build at all at the moment ...).
Attachment #828885 - Flags: review?(Pidgeot18) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Can you please attach a roll-up patch that contains proper commit information and mark the others as obsolete?
Keywords: checkin-needed
(Assignee)

Comment 164

5 years ago
Created attachment 831096 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v11
Attachment #810010 - Attachment is obsolete: true
Attachment #827603 - Attachment is obsolete: true
Attachment #828885 - Attachment is obsolete: true
Attachment #831096 - Flags: superreview+
Attachment #831096 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2523fe46615f
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0

Comment 166

5 years ago
Sweet! Nice job pulling this one through, Jens!
Whiteboard: [tb31features]

Updated

5 years ago
Depends on: 952735
You need to log in before you can comment on or make changes to this bug.