I would like to see the subscribe window do an 'and' search when you type in multiple words. For example, if I typed in "3d games" it would find 3dfx.games, alt.games.3d, and rec.games.video.3do. Currently (in build 2001090908), it just always searches for the exact string, and of course, newsgroups names can't contain spaces, so all results are eliminated.

No dupes found. Marking NEW. -> All/All.

This would be a nice feature to have. Nominating for Mozilla 1.1

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

MASS-CHANGE: This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago. Because of this, we're resolving the bug as EXPIRED. If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component. Query tag for this change: EXPIRED-20100420

What to do to get this in Thunderbird? File again?

I'll just update the product and reopen.

Can newsgroup names never contain spaces? I can't find an RFC saying this. If I did I'd confirm the bug.

(In reply to comment #13) > Can newsgroup names never contain spaces? I can't find an RFC saying this. If I > did I'd confirm the bug. http://tools.ietf.org/html/rfc5536#section-3.1.4 newsgroup-name = component *( "." component ) component = 1*component-char component-char = ALPHA / DIGIT / "+" / "-" / "_" (Indeed, I could not find anything in RFC 1036). So, I don't think there a newsgroup names with spaces out there. But even if, would it really hurt? They are still found, there might just be another newsgroup that contains the two strings.

The actual issue is a backend one, affecting both SM and TB. The overloaded setSearchValue functions for IMAP/NNTP need to be fixed. Shouldn't be too hard. ;-)

assign to me, please

(In reply to Karsten Düsterloh from comment #15) > The actual issue is a backend one, affecting both SM and TB. > The overloaded setSearchValue functions for IMAP/NNTP need to be fixed. > Shouldn't be too hard. ;-) Can you explain what needs to be done for IMAP and how to reproduce with IMAP? I opened the "Subscribe" window for an IMAP account, and the field for "filter list" is greyed out.

I have cancelled the review request because I still need to write unit tests. However, I wonder how to do this best. As far as I can see, the existing code does not test the filtering. nsNntpIncomingServer::SetSearchValue just fills in mSubscribeSearchResult (however, I think it works without setting a tree object first). There is no function to get _only_ the individual newsgroup names. The closest matches are nsNntpIncomingServer::GetCellText(int32_t row, nsITreeColumn* col, nsAString& _retval) and nsNntpIncomingServer::GetRowCount(int32_t *aRowCount). So to check the results, it would be necessary to create a nsITreeColumn mock object and pass that. How should that object look like? GetCellText just looks that the first character of the colId is 'n' ... In subscribe.xul, the column is called "nameColumn2". Is there documentation what exactly an nsISubscribableServer expects from the nsITreeBoxObject, i.e., what elements it might access etc.? Looking whether the first character is an "n" seems to be quite a hack to me. My proposal would be to use an extra attribute on the col object (like "contentType", and checking whether the value is "newsgroup" or "folder"). I guess further refactorings are totally out of scope here. Feedback is appreciated.

I think the general approach for testing is clear now. But I think I have to cause the nsNntpIncomingServer to fetch the group list from the server. I have no idea how to do this. There is nsISubscribableServer::startPopulating, but it is not documented at all. subscribe.xul has a button that calls Refresh(). This calls SetUpTree(true, newGroupsTab.selected). I.e., normally it wants all groups, but if the tabs for new groups is selected, it wants only new groups. SetUpTree then calls gSubscribableServer.startPopulating(msgWindow, forceToServer, getOnlyNew). This function sets gSubscribableServer.subscribeListener to an object which listens to the OnDonePopulating event. Then it calls gSubscribableServer.startPopulating(msgWindow, forceToServer, getOnlyNew). Seems easy enough to emulate in the test (which will have to be async), except for the msgWindow. I see that it is set up in SubscribeOnLoad(), don't understand it fully (e.g., what is msgWindow.rootDocShell.allowAuth doing?), and wonder what it is used for, and how to deal with it in the test. Again, feedback is appreciated.

The patch as written is entirely affecting NNTP code, so I am setting the component to reflect that. Perhaps jcranmer could give some hints on how you might test this with the nntp fake server.

Kent, I don't think you intended to change the assignee (bugzilla will bite you that way sometimes)

(In reply to Jens Müller from comment #19) > However, I wonder how to do this best. As far as I can see, the existing > code does not test the filtering. Subscription code is woefully undertested in our current architecture. Normally, since this is UI, I would suggest writing a mozmill test, but mozmill tests are unable to use the fakeserver frameworks at this point in time. If we go by offline search, it might be possible to stuff a hostinfo.dat file in the profile beforehand and use that to drive the UI for mozmill. You'd have to ask someone else how to write such a mozmill test... Looking things up indicates that this test is completely infeasible with xpcshell, since nsITreeColumn is unimplementable from JS (it has [noscript] methods that are used by the NNTP implementation), and the only way to get one of these objects appears to be from an XUL tree frame in the first place.

Comment on attachment 663654 [details] [diff] [review] Proposed patch. Splits the search string into parts and looks for each of them. Review of attachment 663654 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see another version of this patch before granting review. ::: mail/base/content/subscribe.js @@ +496,4 @@ > > if (!gSearchView && gSubscribableServer) { > gSearchView = gSubscribableServer.QueryInterface(Components.interfaces.nsITreeView); > + gSearchView.selection = null; Gratuitous whitespace change, unnecessary... ::: mailnews/news/src/nsNntpIncomingServer.cpp @@ +1644,3 @@ > mSearchValue = searchValue; > + mSearchValue.CompressWhitespace(true, true); > + Please delete trailing whitespace as well. @@ +1649,5 @@ > mTree->RowCountChanged(0, -mSubscribeSearchResult.Length()); > } > + > + // The search string can consist of several parts, > + // separated by whitespace. We determine these parts now. We have the method ParseString which looks like this: bool ParseString(const nsACString& aSource, char aDelimiter, nsTArray<nsCString>& aArray) Please convert the searchValue to UTF8 and then use ParseString instead of rewriting split routines yourself. It also does significantly less UTF8-to-UTF16 conversions (one string conversion instead of 1-to-N per newsgroup). ::: mailnews/news/src/nsNntpIncomingServer.h @@ +111,1 @@ > nsString mSearchValue; If this variable is only used in that one function, then just delete it.

Just the functionality, addressing review comments. I'll take a look into Mozmill tests and prepare another patch with tests.

Comment on attachment 663856 [details] [diff] [review] Part 1: Functionality Review of attachment 663856 [details] [diff] [review]: ----------------------------------------------------------------- I would have thought that the case-insensitive find would be easier, but apparently our string APIs suck.

Regarding the tests, apparently the following steps need to be taken: 1. Find a way to put a predefined hostinfo.dat into an account in the Mozmill tests (alternatively 1a: Make fakeservers work in Mozmill ...) 2. Start TB in offline mode in Mozmill 3. In Mozmill, perform a sequence of actions and tests involving the subscribe window ... Any hints for 1. and 2.? I guess for 3., I'll now start writing down the (manual) test script ...

(In reply to Jens Müller from comment #28) > (alternatively 1a: Make fakeservers work in Mozmill ...) That is probably bug 874690 ...

I found this in test-pref-window-helpers.js: > * Since the preferences window might be modal (it is currently modal on > * platforms without instantApply), it spins its own event loop. This means > * that you need to provide a callback to be executed when the window is loaded. Does the same hold for the subscribe window?

In a XUL tree element (in particular, an RDF-based one), how can I check for the existence of a certain entry? Get the tree, get the view, query nsITreeView, get rowCount, and then iterate over all rows using getCellText()? And is it possible to determine from Mozmill when a tree view has been fully populated?

(In reply to Jens Müller (:tessarakt) from comment #28) > Regarding the tests, apparently the following steps need to be taken: > 1. Find a way to put a predefined hostinfo.dat into an account in the > Mozmill tests > (alternatively 1a: Make fakeservers work in Mozmill ...) I tried out the stuff from test-nntp-helpers.js. setupLocalServer returns without error. The server it returns is a [nsIMsgIncomingServer: server5] and its rootFolder property is [xpconnect wrapped nsIMsgFolder @ 0x665a2b0 (native @ 0x67a0e10)]. But then, this folder object is apparently not found in mc.folderTreeView ...

I hacked up a working Mozmill test, including a running fakeserver. It still requires quite some polishing, though.

Comment on attachment 789731 [details] [diff] [review] Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started Review of attachment 789731 [details] [diff] [review]: ----------------------------------------------------------------- Fakeserver is now set up to install via testing-only modules. Have you tried using them?

(In reply to Joshua Cranmer [:jcranmer] from comment #37) > Fakeserver is now set up to install via testing-only modules. Have you tried > using them? No. Can you point me to a filename or a bug number for that change, please?

Result from a IRC discussion with jcranmer: As it is not trivially possible to load nntpd.js and imapd.js with Cu.import from resource://testing-common/..., I have filed bug 904812.

FYI I'm not going to get to look at this for a couple of weeks.

Comment on attachment 789762 [details] [diff] [review] Part 4: Actual test Review of attachment 789762 [details] [diff] [review]: ----------------------------------------------------------------- While I'm normally prefer the idea of small patches instead of large omnibus ones, for this kind of bug, it's really hard to follow what's going on when all of the test data is spread out across three patches. ::: mail/test/mozmill/subscribe/test-subscribe-news-filter.js @@ +27,5 @@ > +function test_subscribe_newsgroup_filter() { > + var daemon = setupNNTPDaemon(); > + var remoteServer = startupNNTPServer(daemon,NNTP_PORT); > + let server = setupLocalServer(NNTP_PORT). > + QueryInterface(Components.interfaces.nsINntpIncomingServer); You shouldn't need the QI here. If you do need a QI, roll it into the definition of setupLocalServer, not on the caller.

Comment on attachment 789731 [details] [diff] [review] Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started Review of attachment 789731 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/mozmill/shared-modules/test-nntp-helpers.js @@ +84,5 @@ > return daemon; > } > > +// Startup server > +function startupNNTPServer(daemon,port){ Please put a space after commas. It is *way* too easy to read these as periods instead.

Comment on attachment 789731 [details] [diff] [review] Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started Joshua can review these.

(In reply to Joshua Cranmer [:jcranmer] from comment #41) > ::: mail/test/mozmill/subscribe/test-subscribe-news-filter.js > @@ +27,5 @@ > > +function test_subscribe_newsgroup_filter() { > > + var daemon = setupNNTPDaemon(); > > + var remoteServer = startupNNTPServer(daemon,NNTP_PORT); > > + let server = setupLocalServer(NNTP_PORT). > > + QueryInterface(Components.interfaces.nsINntpIncomingServer); > > You shouldn't need the QI here. If you do need a QI, roll it into the > definition of setupLocalServer, not on the caller. Indeed, I don't need it.

Tests were successful on Linux: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=df3d8cc66c8a

Comment on attachment 804956 [details] [diff] [review] Part 3: Provide a subscribe window helper Review of attachment 804956 [details] [diff] [review]: ----------------------------------------------------------------- I don't pretend to understand Mozmill well enough to know if the helpers are totally correct, but seeing that it works on Try gives me confidence. ::: mail/test/mozmill/shared-modules/test-keyboard-helpers.js @@ +42,2 @@ > for (let i = 0; i < aStr.length; i++) > + aController.keypress(aElement, aStr.charAt(i), {}); I believe the standard idiom would be aElement || null here. @@ +56,5 @@ > } > > +/** > + * Emulates deleting the entire string by pressing Ctrl-A and DEL > + * nit: trailing whitespace ::: mail/test/mozmill/shared-modules/test-subscribe-window-helper.js @@ +27,5 @@ > +function installInto(module) { > + setupModule(); > + > + // Now copy helper functions > + module.open_subscribe_window_from_context_menu = Trailing whitespace @@ +41,5 @@ > + * @param aFunction Callback that will be invoked with a controller > + * for the subscribe dialogue as parameter > + */ > +function open_subscribe_window_from_context_menu(aFolder, aFunction) { > + folderDisplayHelper.right_click_on_folder(aFolder); There has been talk, IIRC, of removing "Subscribe..." from the context menu of all but the root folder in the tree view, but I don't follow enough UX changes to recall what the outcome of that discussion was. @@ +50,5 @@ > +} > + > +/** > + * Enter a string in the text box for the search value. > + * More trailing whitespace (indeed, every " *" line from here out as trailing whitespace).

Carrying forward Joshua's review because only the review comments were addressed.

https://hg.mozilla.org/comm-central/rev/a04a3bd1a95e https://hg.mozilla.org/comm-central/rev/90b7dd6d71c5 https://hg.mozilla.org/comm-central/rev/7dcbb0147e58 https://hg.mozilla.org/comm-central/rev/5bcac859f923

This broke external api builds. Also noticed the test module wasn't named the same as the file, which is what it's normally is, so i fixed that too.

Comment on attachment 811505 [details] [diff] [review] external api fix >+ if (!CaseInsensitiveFindInReadable(searchStringParts[j], mGroupsOnServer[i])){ [Not sure that works for nsCString, might have to use MsgFind instead.]

Well, it compiles and the test succeeds. I can change it if you want.

(In reply to Magnus Melin from comment #60) > Well, it compiles and the test succeeds. I can change it if you want. It might compile under the external API, but it doesn't compile under the internal API. nsNntpIncomingServer.cpp(1675) : error C2665: 'CaseInsensitiveFindInReadable' : none of the 3 overloads could convert all the argument types nsUnicharUtils.h(89): could be 'bool CaseInsensitiveFindInReadable(const nsAString_internal &, const nsAString_internal &)' while trying to match the argument list '(nsCString, nsCString)'

Ah what a trap :( Using MsgFind then.

(In reply to comment #59) > >+ if (!CaseInsensitiveFindInReadable(searchStringParts[j], mGroupsOnServer[i])){ > [Not sure that works for nsCString, might have to use MsgFind instead.] Although MsgFind works in the case where you specify an offset, I just realised that you can use Find if you omit the offset, as we have a #define CaseInsensitiveCompare true so that it works in the internal API case too.

Comment on attachment 811733 [details] [diff] [review] external api fix, v2 >+ if (MsgFind(mGroupsOnServer[i], searchStringParts[j], true, 0) == kNotFound) { I tried it and mGroupsOnServer[i].Find(searchStringParts[j], CaseInsensitiveCompare) compiles, so you could use that instead.

Ok, so i missed the negation there, but even with that fixed, it didn't work (test fails). So I pushed the MsgFind version - https://hg.mozilla.org/comm-central/rev/1a0ba6cf04a8

(In reply to Magnus Melin from comment #66) > Ok, so i missed the negation there, but even with that fixed, it didn't work Negation? I notice you removed the == kNotFound but that wasn't my intention, sorry if I confused you.

Yeah that should have worked. Seems my brain wasn't hooked up properly :(

As I have written the original patch which did not work with external API builds: Where can I find more information about external API, i.e., what it is at all, which functions are available there and how to test whether a change will work in an external API build? Just to avoid this kind of stuff the next time I might be doing something in C++ code ...

https://developer.mozilla.org/en-US/docs/Mozilla_external_string_guide Long story short, e.g. linux distros don't want to have serveral libxul copies, but would like to have the same libxul for firefox and thunderbird and what not. That's only possible with external linkage. So, to use it you add ac_add_options --enable-incomplete-external-linkage ... which is also a very nice win during development, as e.g. if you say make changes to cpp files in mailnews you only have to do a quick "make -C mailnews/" which is WAY faster (seconds) than rebuilding with internal linkage. The one downside is that the external builds are occasionally broken as there's no official support for it.