do AND of multiple words in newsgroups' subscribe filter

RESOLVED FIXED in Thunderbird 27.0

Status

MailNews Core
Networking: NNTP
--
enhancement
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Brian Rogers, Assigned: tessarakt)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 27.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 12 obsolete attachments)

2.01 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
1.71 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
5.64 KB, patch
tessarakt
: review+
Details | Diff | Splinter Review
2.95 KB, patch
tessarakt
: review+
Details | Diff | Splinter Review
3.42 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All

Comment 2

17 years ago
This would be a nice feature to have. Nominating for Mozilla 1.1
Keywords: mozilla1.1
Product: Browser → Seamonkey

Updated

13 years ago
Assignee: sspitzer → mail

Updated

11 years ago
Blocks: 176238

Updated

10 years ago
Component: MailNews: Subscribe → MailNews: Message Display
QA Contact: stephend → search

Comment 3

9 years ago
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
Status: NEW → UNCONFIRMED

Comment 4

9 years ago
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

Comment 5

9 years ago
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

Comment 6

9 years ago
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

Comment 7

9 years ago
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

Comment 8

9 years ago
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

Comment 9

9 years ago
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
Assignee: mail → nobody
QA Contact: search → message-display

Comment 10

8 years ago
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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → EXPIRED
(Assignee)

Comment 11

8 years ago
What to do to get this in Thunderbird? File again?
(Reporter)

Comment 12

8 years ago
I'll just update the product and reopen.
Status: RESOLVED → UNCONFIRMED
Component: MailNews: Message Display → Folder and Message Lists
Product: SeaMonkey → Thunderbird
QA Contact: message-display → folders-message-lists
Resolution: EXPIRED → ---

Comment 13

8 years ago
Can newsgroup names never contain spaces? I can't find an RFC saying this. If I did I'd confirm the bug.
(Assignee)

Comment 14

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

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

8 years ago
Summary: [RFE] AND Search for subscribe window. → do AND of multiple words in newsgroups' subscribe filter

Comment 15

8 years ago
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. ;-)
Component: Folder and Message Lists → Backend
Product: Thunderbird → MailNews Core
QA Contact: folders-message-lists → backend
(Assignee)

Comment 16

6 years ago
assign to me, please

Updated

6 years ago
Assignee: nobody → blog
Status: NEW → ASSIGNED
(Assignee)

Comment 17

6 years ago
(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.
(Assignee)

Comment 18

6 years ago
Created attachment 663654 [details] [diff] [review]
Proposed patch. Splits the search string into parts and looks for each of them.
Attachment #663654 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #663654 - Flags: review?
(Assignee)

Comment 19

6 years ago
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.
(Assignee)

Comment 20

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

Comment 21

6 years ago
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.
Assignee: blog → nobody
Component: Backend → Networking: NNTP

Comment 22

6 years ago
Kent, I don't think you intended to change the assignee (bugzilla will bite you that way sometimes)
Assignee: nobody → blog
(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.
Attachment #663654 - Flags: feedback-
(Assignee)

Comment 25

6 years ago
Created attachment 663856 [details] [diff] [review]
Part 1: Functionality

Just the functionality, addressing review comments. I'll take a look into Mozmill tests and prepare another patch with tests.
Attachment #663654 - Attachment is obsolete: true
Attachment #663856 - Flags: review?(Pidgeot18)

Updated

6 years ago
Duplicate of this bug: 370889
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.
Attachment #663856 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 28

5 years ago
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 ...
(Assignee)

Updated

5 years ago
Depends on: 874690
(Assignee)

Comment 29

5 years ago
(In reply to Jens Müller from comment #28)
>  (alternatively 1a: Make fakeservers work in Mozmill ...)

That is probably bug 874690 ...
(Assignee)

Comment 30

5 years ago
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?
(Assignee)

Comment 31

5 years ago
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?
(Assignee)

Comment 32

5 years ago
(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 ...
(Assignee)

Updated

5 years ago
Depends on: 903804
No longer depends on: 874690
(Assignee)

Updated

5 years ago
Depends on: 874690
(Assignee)

Comment 33

5 years ago
I hacked up a working Mozmill test, including a running fakeserver. It still requires quite some polishing, though.
(Assignee)

Comment 34

5 years ago
Created attachment 789731 [details] [diff] [review]
Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started
Attachment #789731 - Flags: feedback?(Pidgeot18)
(Assignee)

Comment 35

5 years ago
Created attachment 789745 [details] [diff] [review]
Part 3: Provide a subscribe window helper
(Assignee)

Comment 36

5 years ago
Created attachment 789762 [details] [diff] [review]
Part 4: Actual test
(Assignee)

Updated

5 years ago
Attachment #789731 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Attachment #789745 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Attachment #789762 - Flags: review?(mbanner)
Attachment #789762 - Flags: feedback?(Pidgeot18)
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?
(Assignee)

Comment 38

5 years ago
(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?
(Assignee)

Comment 39

5 years ago
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.
Attachment #789762 - Flags: feedback?(Pidgeot18) → feedback+
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.
Attachment #789731 - Flags: feedback?(Pidgeot18) → feedback+
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.
Attachment #789731 - Flags: review?(mbanner) → review?(Pidgeot18)
Attachment #789745 - Flags: review?(mbanner) → review?(Pidgeot18)
Attachment #789762 - Flags: review?(mbanner) → review?(Pidgeot18)
(Assignee)

Comment 44

5 years ago
Created attachment 804942 [details] [diff] [review]
Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started
Attachment #789731 - Attachment is obsolete: true
Attachment #789731 - Flags: review?(Pidgeot18)
Attachment #804942 - Flags: review?(Pidgeot18)
(Assignee)

Comment 45

5 years ago
Created attachment 804944 [details] [diff] [review]
For convenience: Part 1 to 4 and the patch from Bug 903804 in one patch
(Assignee)

Updated

5 years ago
Attachment #804944 - Attachment description: For convenience: Part 1 to 4 in one patch → For convenience: Part 1 to 4 and the patch from Bug 903804 in one patch
(Assignee)

Comment 46

5 years ago
Created attachment 804946 [details] [diff] [review]
Part 3: Provide a subscribe window helper
Attachment #789745 - Attachment is obsolete: true
Attachment #789745 - Flags: review?(Pidgeot18)
Attachment #804946 - Flags: review?(Pidgeot18)
(Assignee)

Comment 47

5 years ago
Created attachment 804947 [details] [diff] [review]
Part 2: Extend nntp-helpers Mozmill module so that the fakeserver can be started
Attachment #804942 - Attachment is obsolete: true
Attachment #804942 - Flags: review?(Pidgeot18)
Attachment #804947 - Flags: review?(Pidgeot18)
(Assignee)

Comment 48

5 years ago
Created attachment 804949 [details] [diff] [review]
For convenience: Part 1 to 4 and the fix from bug 903804 in one patch
Attachment #804944 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #789762 - Flags: review?(Pidgeot18)
(Assignee)

Updated

5 years ago
Attachment #804946 - Flags: review?(Pidgeot18)
(Assignee)

Comment 49

5 years ago
(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.
(Assignee)

Comment 50

5 years ago
Created attachment 804956 [details] [diff] [review]
Part 3: Provide a subscribe window helper
Attachment #804946 - Attachment is obsolete: true
Attachment #804956 - Flags: review?(Pidgeot18)
(Assignee)

Comment 51

5 years ago
Created attachment 804957 [details] [diff] [review]
Part 4: Actual test
Attachment #789762 - Attachment is obsolete: true
Attachment #804957 - Flags: review?(Pidgeot18)
(Assignee)

Comment 52

5 years ago
Created attachment 804958 [details] [diff] [review]
For convenience: Part 1 to 4 and the fix from bug 903804 in one patch
Attachment #804949 - Attachment is obsolete: true
Attachment #804947 - Flags: review?(Pidgeot18) → review+
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).
Attachment #804956 - Flags: review?(Pidgeot18) → review+
Attachment #804957 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 55

5 years ago
Created attachment 809136 [details] [diff] [review]
Part 3: Provide a subscribe window helper (v2) [carrying forward r=Pidgeot18@gmail.com]

Carrying forward Joshua's review because only the review comments were addressed.
Attachment #804956 - Attachment is obsolete: true
Attachment #809136 - Flags: review+
(Assignee)

Comment 56

5 years ago
Created attachment 809139 [details] [diff] [review]
Part 4: Actual test (v2) [carrying forward r=Pidgeot18@gmail.com]
Attachment #804957 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #809136 - Attachment description: Part 3: Provide a subscribe window helper (v2) → Part 3: Provide a subscribe window helper (v2) [carrying forward r=Pidgeot18@gmail.com]
(Assignee)

Updated

5 years ago
Attachment #809139 - Attachment description: Part 4: Actual test (v2) → Part 4: Actual test (v2) [carrying forward r=Pidgeot18@gmail.com]
Attachment #809139 - Flags: review+
(Assignee)

Updated

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

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [checkin AFTER blocking bug 903804]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin AFTER blocking bug 903804]
Target Milestone: --- → Thunderbird 27.0

Comment 58

5 years ago
Created attachment 811505 [details] [diff] [review]
external api fix

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

Comment 60

5 years ago
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)'

Updated

5 years ago
Attachment #811505 - Flags: review?(neil) → review-

Comment 62

5 years ago
Created attachment 811733 [details] [diff] [review]
external api fix, v2

Ah what a trap :(
Using MsgFind then.
Attachment #811505 - Attachment is obsolete: true
Attachment #811733 - Flags: review?(neil)
(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.
Attachment #811733 - Flags: review?(neil) → review+

Comment 66

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

Comment 68

5 years ago
Yeah that should have worked. Seems my brain wasn't hooked up properly :(
(Assignee)

Comment 69

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

Comment 70

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