Open Bug 586101 Opened 14 years ago Updated 2 years ago

Quickfiltering for "Recipients" fails on BCC recipients (does not find/return messages with matching recipients in Bcc: header)

Categories

(Thunderbird :: Search, defect)

defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: thomas8, Assigned: aceman)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #213318 +++ STR 1 have messages in ANY folder that have visible-for-you BCC recipients header, e.g. Bcc: blind@asdf.com - e.g. in your SENT folder, because you sent message(s) to BCC recipients - or in any folder, with "Tools > Account Settings > Your Account > Copies & Folders > Place replies in the folder of the message being replied to" enabled - or because you manually copied such sent messages into any folder 2 Ctrl+F, enter "blind" into the filter bar search box (with recipient filter ON, as is the default) Actual result: - matching message containing "blind" as BCC-recipient is not found Expected result: - matching message containing "blind" as BCC-recipient should be found (clearly, BCC recipients are a legitimate and not infrequent variant of "Recipients", and there is no indication whatsoever that we exlude Bcc header) This problem was initially reported by Wayne Mery (:wsmwk) as early as 2003 (7 years ago) as bug 213318, of which this an updated clone. This is non-trivial for users who regularly use BCC recipients, e.g. as an anti-spam measure. It's about time to fix it. Please set wanted-thunderbird+ flag for this bug, and a target milestone, if possible.
Flags: in-testsuite?
Summary: Quickfiltering for "Recipients" fails on BCC recipients (does not find/return messages with matching BCC recipients) → Quickfiltering for "Recipients" fails on BCC recipients (does not find/return messages with matching recipients in Bcc: header)
I use the quick filter window on the upper right side of the Thunderbird windows. Since several months I send all my messages using BCC: instead of To: using the MailTweak plug-in (BTW, it would be interesting to let the user choose if the BCC is the default mode). I see this error is still happening, so when I want to locate a sent message to a certain e-mail, I need to open the advanced search windows (CTR + CAPS + F), and select to find in the From, To, CC and BCC field.
What is the official procedure to request wanted-thunderbird+ flag? This is basic quicksearch/filter functionality that's known broken since 2003. Shame. I am failing to understand how new features like quick filter bar can be implemented without ensuring that they actually work for expectable use cases.
The functionality for "recipient" does not include BCC recipients, only TO and CC, as of version 12. Is there a hidden config switch that would engage BCC in the Quick Filter, so as to avoid using the cumbersome advanced find tool ?
Flags: in-testsuite?
It's simple to change the search predicate used by the quick filter: http://mxr.mozilla.org/comm-central/source/mail/base/modules/quickFilterManager.js#1217 The problem is there is no to/cc/bcc predicate; just a from/to/cc/bcc predicate (AllAddresses): http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#52 The manager is oriented so that the predicates happen in parallel, so it's not entirely trivial to realize when sender and recipients are both flagged and then upgrade to AllAddresses. I think an additional predicate would need to be added for similar semantics; e-mail address predicates actually match against the display name and e-mail address separately. A work-around would be to use the HdrProperty predicate on bcc.
I am perplexed by comment #5. A user modification to the searches, to add "BCC", was simple enough [and appears to work...]. Why is it difficult to add an underlying predicate search [To/CC/BCC] to the search list, so that the recipient button could rely on it? Make #53, for to/cc/bcc, and start the custom stack at 54-99, instead ? Or move custom to 53, and tick upwards the stack to 54-99, placing to/cc/bcc at 52. or...am I missing something
(In reply to john ruskin from comment #6) > or...am I missing something Granted, I probably am . . .
It shouldn't be difficult; just more difficult than I was expecting! The C++ mods required should be minimal and just minor variations on the existing code. It's just more than changing a constant. Aceman seems interested in this bug, so I'm hoping he'll bring some of his skills to bear! Can you clarify what you mean by user modification? If you change it to nsMsgSearchAttrib.AllAddresses, it will work, but cast too wide a net. If you change it to nsMsgSearchAttrib.BCC, it might do something wacky because the constant should not exist and then it's possible a much different search than expected gets run. I see the search dialog (control-shift-F) does not include an explicit BCC option, so this makes me think my analysis is probably correct.
bienvenu, I assume there's nothing stopping us from adding a recipients search predicate (to/cc/bcc) in addition to our all addressses one (from/to/cc/bcc)?
By "User Modification", I meant that I added a custom search term "BCC" [within the control-shift-F] which executed the search on BCC addresses as I would expect .... From which I assumed adding/creating a "To/CC/BCC" to the predicate list wouldn't be a massively messy step. Andrew's solution path in comment #9 also moots bug 763631
No longer depends on: 213318
Hey, I was interested in this just as a bug triager :) But yes, if you decide I can just copy "TOorCC" to a new nsMsgSearchAttrib.Recipient and add BCC matching into it (copy the part from "AllAddresses"), then I think I am able to do it. It looks like the new option would also appear in the search dialog and as a Filter condition. That may not be a bad thing. Is that what you meant?
1. Need to amend test at :: http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_searchAddressInAb.js 2a. Add after line 73 in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchTerm.cpp :: 2b. Add after liine 1104 in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchAdapter.cpp :: {nsMsgSearchAttrib::ToOrCCorBCC, "to or cc or bcc"}, 3. Add in the region of http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl :: an attribute type for TOorCCorBCC, and a number 4. Modify line 1217 in http://mxr.mozilla.org/comm-central/source/mail/base/modules/quickFilterManager.js to the new attribute type TOorCCorBCC However...where is the underlying code for managing the TOorCC attribute stored -- where one might describe/define a new TOorCCorBCC? I am definitely missing something....
In the SEARCH AREA (Control-Shift-F) I note that some folks might want, with good reason, to retain the TOorCC functionality, independent of one for TOorCCorBCC, leaving -both- accessible from the drop down search . . . also retaining the existing "AllAddresses" (from/to/cc/bcc) . . . but still mapping your suggested new TOorCCorBCC to the recipient button in the QuickFilter area. aceman, is that what you meant ?
This is where ToOrCC is used: http://mxr.mozilla.org/comm-central/ident?i=ToOrCC . All those places would need a copy of ToOrCC to be ToOrCCOrBCC. Yes, Ctrl-shift-F is the Search dialog I mentioned. The new SearchAttrib would appear there as a new option under "To or CC". I just ask if that is what asuth wants. Make it a full blown new nsMsgSearchAttrib. I am just not sure if it is possible to insert it here http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#65 as a new number 9 and shift the rest.
A> I thought this.... ...shift/renumber -and- reset the range of 'other' ...or alternatively, put it down by 52, moving 53 other and the range.... B> I need a learning curve, as I'm still thinking densely: I had looked there, in that cross reference URL, for all the places where ToOrCC is used, and identified my list, above, in comment #13, of places for changes. What I still don't understand is which code makes the actual search of address fields [ie, -does- the boolean: "To or CC"]. All the code snippets I saw seem to either refer to or index that search. I would have thought that I would have found a code snippet that -did- the search, from which one could copy/modify/add a new snippet for TOorCCorBCC What am I missing ?
5. Add new blocks of code, to http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgImapSearch.cpp at lines 214, 368 and so on, to add blocks of TOorCCorBCC, to the existing online and offline tables
is there also code, for OnLine IMAP searches? that one, at line 453, and in that file, has comments which imply that it is for POP and OffLine IMAP
never mind...brain viscosity, i see it, the substance of my comment #18, item 5
Yes, it looks like that could be enough in mailnews/base/search/src/nsMsgImapSearch.cpp. Are you going to make the patch? You should first wait reply from asuth and bienvenu if this is what they wanted.
(In reply to Andrew Sutherland (:asuth) from comment #9) > bienvenu, I assume there's nothing stopping us from adding a recipients > search predicate (to/cc/bcc) in addition to our all addressses one > (from/to/cc/bcc)? No, nothing technical stopping us. We're not talking about exposing it in the UI, right? There's always grief when people find there is no bcc header in e-mail they receive :-(
David, I assumed in comment 12 the new search match will appear in the UI if we just fully copy an existing ToOrCC definition...
I'm laughing: despite other spectacular programming efficiencies that go back to IBM 1620s, and move forward through Fortran, VBA HTML/CSS, and the like, I could write out what I -think- might work, but I am only reading, now, how to use pymake , making diff files, and etc, just to make the patch, not even getting close to making a working TBird with the patch, nor the modifications to what I think might be a test, per my item 1 in comment #13. Plus, I think I would have to find what would be item 6, amend the nsMsgSearch.... reference for the Recipient button. I probably could write code, and where it should go - but beyond that, I might melt something, either in my head, or the world at large. Learning curve needed, first. So, I laugh, asking: when you say "going to make the patch" . . . what do you mean, uhm....precisely. I was merely ecstatic that the bug might be fixed, and looking through the code. Be glad to help, now, how I can.
Yes, your comments and looking through the code was highly spot on. So I thought you want to write the patch when you are already so far.
(In reply to David :Bienvenu from comment #22) > (In reply to Andrew Sutherland (:asuth) from comment #9) > > bienvenu, I assume there's nothing stopping us from adding a recipients > > search predicate (to/cc/bcc) in addition to our all addressses one > > (from/to/cc/bcc)? > > No, nothing technical stopping us. We're not talking about exposing it in > the UI, right? There's always grief when people find there is no bcc header > in e-mail they receive :-( Blake? My personal opinion is that our current phrasing of "recipients" that (should) cover to/cc/bcc seems good and is a sensible use-case. Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI. I think that surfacing just BCC on its own in advanced search (control-shift-f) and message filters may be asking for trouble since people might think we are able to logically deduce something about the bcc list even if it's not present. constants-wise, are you fine with renumbering or would you prefer that new constants get added at the end?
er, blake, question for you in comment 26. I mid-aired and forgot I was cc'ing...
(In reply to Andrew Sutherland (:asuth) from comment #26) > > constants-wise, are you fine with renumbering or would you prefer that new > constants get added at the end? renumbering is OK - the days where we persisted the constants are long gone.
Similar bug (which could perhaps be fixed here, too): Bug 535879
See Also: → 535879
(In reply to Andrew Sutherland (:asuth) from comment #26) > (In reply to David :Bienvenu from comment #22) > > No, nothing technical stopping us. We're not talking about exposing it in > > the UI, right? There's always grief when people find there is no bcc header > > in e-mail they receive :-( > Blake? My personal opinion is that our current phrasing of "recipients" > that (should) cover to/cc/bcc seems good and is a sensible use-case. +1. > Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI. > I think that surfacing just BCC on its own in advanced search > (control-shift-f) and message filters may be asking for trouble since people > might think we are able to logically deduce something about the bcc list > even if it's not present. +1.
In reply to comment #30 > Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI. > I think that surfacing just BCC on its own in advanced search > (control-shift-f) and message filters may be asking for trouble since people > might think we are able to logically deduce something about the bcc list > even if it's not present. There is a logical problem: at first I thought these two suggestions might be appropriate: 1) replacing "...when available", with "To/Cc plus BCC you send", as more accurate 2) Also, since BCC filtering is useful, include a BCC UI case in the search, which could be titled similarly, as: "BCC you send". However, BCC for logical "recipients" filters from two source: as mail SENT by the user, and also as mail RECEIVED as a BCC for just that user. I wonder if the proposed changes will catch both BCC groups
(In reply to :aceman from comment #25) > Yes, your comments and looking through the code was highly spot on. So I > thought you want to write the patch when you are already so far. Thanks, but....after looking through the steps to creating a patch [over and above suggesting coding], I note that I don't have a linux box, and have a broad learning curve to get a Try done, or a build, and on.... Can I just . . . watch and learn?
Yes, no problem. I also do not guarantee I'll surely implement this all to finish. But I have done something like this already, so I'll try. The exact wording of the option visible in UI can be bashed on in parallel :) David, what about this comment: // 34 - 43, reserved for ab / LDAP Do I need to somehow shift some constants in ab / LDAP too? I couldn't find any.
Assignee: nobody → acelists
I don't have time to spell out all the reasons in detail right now, but I strongly recommend the following: 1) make quick filter bar's "Recipients" button include BCC header for search results (without changing its caption) 2) in the code, name the new search term "Recipients", consistent with the existing nsMsgSearchAttribValues for "Subject" and "Sender" (not "From"), and also consistent with the quick filter buttons: > const nsMsgSearchAttribValue Recipients = 10 3) While we are here, fix this: Bug 535879 - filters: add "reply-to" header to the "all addresses" ("from, to, cc or bcc") filter search term 4) While we are here, add this: > const nsMsgSearchAttribValue BCC = 8 5a) For Tools > Message Filters and Advanced Search (Ctrl+Shift+F), surface the following in the UI (existing entries unmarked, new entries marked with *, modified entries marked with #): - BCC* (we definitely *do* want that exposed, and I don't see any potential for confusion here, it's no different or more confusing than CC which we already have, and both message filters and advanced search are for advanced users anyway) - To or CC - To, CC or BCC * - From, Reply-To, To, Cc or Bcc # (Bug 535879) (if we want less entries, we might skip "To or CC", but it might help to keep it, and it doesn't hurt) 5b) After fixing this bug, we might even consider natural language terms like "Recipients" for Message Filters and Advanced search, but not sure about that (is it possible to have a tooltip on dropdown list items? We could then use natural language terms and include the details in the tooltip)
(In reply to :aceman from comment #33) > David, what about this comment: > // 34 - 43, reserved for ab / LDAP > Do I need to somehow shift some constants in ab / LDAP too? I couldn't find > any. I couldn't find them either - perhaps that's an obsolete comment. Mark might remember, or be able to point to ab search code.
(In reply to Thomas D. from comment #34) > I don't have time to spell out all the reasons in detail right now, but I > strongly recommend the following: > > 1) make quick filter bar's "Recipients" button include BCC header for search > results (without changing its caption) > > 2) in the code, name the new search term "Recipients", consistent with the > existing nsMsgSearchAttribValues for "Subject" and "Sender" (not "From"), > and also consistent with the quick filter buttons: > > const nsMsgSearchAttribValue Recipients = 10 This is what I am going to do here. > 3) While we are here, fix this: > Bug 535879 - filters: add "reply-to" header to the "all addresses" ("from, > to, cc or bcc") filter search term This is not as trivial, I couldn't yet find out what code to just copy and tweak :) Let's keep that to bug 535879. > 4) While we are here, add this: > > > const nsMsgSearchAttribValue BCC = 8 > > 5a) For Tools > Message Filters and Advanced Search (Ctrl+Shift+F), surface > the following in the UI (existing entries unmarked, new entries marked with > *, modified entries marked with #): > > - BCC* (we definitely *do* want that exposed, and I don't see any potential > for confusion here, it's no different or more confusing than CC which we > already have, and both message filters and advanced search are for advanced > users anyway) I think BCC is doable from existing code. Is there a bug for it? > - To or CC > - To, CC or BCC * > - From, Reply-To, To, Cc or Bcc # (Bug 535879) > (if we want less entries, we might skip "To or CC", but it might help to > keep it, and it doesn't hurt) When the previous 2 items are in place. > 5b) After fixing this bug, we might even consider natural language terms > like "Recipients" for Message Filters and Advanced search, but not sure > about that (is it possible to have a tooltip on dropdown list items? We > could then use natural language terms and include the details in the tooltip) Maybe a tooltip is possible, I'll try.
> In reply to comment #30 and comment #31 On a little research / test case, I note that incoming mail, addressed to the USER via a BCC, does not appear to include any header information reflecting the USER, merely ";undisclosed recipients". On the assumption that is driven by the sender's server, and not TBird, I amend my wondering to: I wonder how, or if, TBird would/should regard mail, addressed to a USER via a BCC -- it doesn't appear to be addressed TO:, or searchable as such, nor as a BCC. I assume it would not be caught by an AllAddressee search, in its current incarnation; while not a part of this bug #586101, I wonder if it is plausible to include RECEIVED-AS-BCC mail as a change to the boolean search for AllAddresses
(In reply to :aceman from comment #33) > Yes, no problem. I also do not guarantee I'll surely implement this all to > finish. But I have done something like this already, so I'll try. The exact > wording of the option visible in UI can be bashed on in parallel :) > > David, what about this comment: > // 34 - 43, reserved for ab / LDAP > Do I need to somehow shift some constants in ab / LDAP too? I couldn't find > any. I think they just reserved 10 empty slots after the last nsMsgSearchAttribValue pertaining to AB card properties. My reading is that the values (numbers) of these slots are completely irrelevant, and we are shifting all the numbers anyway including > const nsMsgSearchAttribValue OtherHeader which must be the last entry, but the number is irrelevant as commented in the code. I suppose that the order of dropdown entries is derived from the numbers, so we can just keep a set of 10 empty slots numbers for AB at the current position, starting with whatever happens to be the number of > const nsMsgSearchAttribValue Department increased by +1 after our changes here. Just my reading, no guarantees.
aceman: re the renumbering.... This doesn't seem to impact your bug patch, but you're further in to the renumbering issue, than I am, and you might think of something. check out this commentary: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgLocalSearch.cpp#629
(In reply to john ruskin from comment #37) > I assume it would not be caught by an AllAddressee search, in its current > incarnation; while not a part of this bug #586101, I wonder if it is > plausible to include RECEIVED-AS-BCC mail as a change to the boolean search > for AllAddresses If that's possible at all, it's certainly non-trivial, and not part of this bug. Here, we only look at explicit BCC headers found in source of mail, usually found in sent mail which might be moved/copied/fcc'ed to any folder including inbox.
Attached patch WIP patchSplinter Review
OK, here is some preliminary patch for those eager to try something out and see how the whole feature looks in a patch:) It does not yet update the test. I named the SearchAttribute::Recipient, which is open to discussion. Also the UI (search and filters) show "Recipient" which can easily be changed. I have tested it on a local POP3 account. It appeared to me when using the "Recipients" in the quick filter bar that it matched also the "From" field, which would be strange. I had a test draft message having From: aceman@server, and BCC: aceman@server. Searching for "aceman" found this message even without the patch. I had to edit the message so that the From is somebody else. I am not sure why that it. Asuth?
Attachment #632438 - Flags: feedback?(bugmail)
Attachment #632438 - Flags: feedback?(bwinton)
Attachment #632438 - Flags: feedback?(dbienvenu)
Unfortunately, I'll be away for a week, so will not work on the patch. But I will collect feedback after that :) Thanks for all the useful hints here.
Status: NEW → ASSIGNED
shot in the dark: possible that the code uses 'recipients', and there is confusion between our "newer" meaning, and the code use at OLD LINE # 466 et seq [as was used in the AllAddresses] -- what is "getter_Copies(recipients) ... getting ?
In reply to comment #41 I think i mis-understood your comment When you did your test search, did you test for BCC=aceman, with a search for some BODY=fooBar? But, even if not: If you searched for "Aceman", it may have found both the SENT folder copy, and the INBOX copy --> validly discovering FROM=aceman and BCC=aceman
(In reply to john ruskin from comment #43) > shot in the dark: possible that the code uses 'recipients', and there is > confusion between our "newer" meaning, and the code use at OLD LINE # 466 et > seq [as was used in the AllAddresses] -- what is "getter_Copies(recipients) > ... getting ? I hope "recipients" in this case is just an arbitrary variable getting the results of GetRecipients(). It is used the same in ToOrCC so it should be correct. I assume it is the "To" field. (In reply to john ruskin from comment #44) > In reply to comment #41 > I think i mis-understood your comment > When you did your test search, did you test for BCC=aceman, with a search > for some BODY=fooBar? But, even if not: > If you searched for "Aceman", it may have found both the SENT folder copy, > and the INBOX copy --> validly discovering FROM=aceman and BCC=aceman I was using the quick filter bar above the message list. So I could not have 2 conditions like BCC=aceman OR BODY=fooBar. It was only Recipients=aceman, with recipients being the old meaning (=ToOrCC). It still found the msg having From=aceman and BCC=aceman. I searched only on Inbox, where I moved a draft message I composed for the test.
Attached file Testcase1.eml
(In reply to :aceman from comment #45) > I was using the quick filter bar above the message list. So I could not have > 2 conditions like BCC=aceman OR BODY=fooBar. It was only Recipients=aceman, > with recipients being the old meaning (=ToOrCC). It still found the msg > having From=aceman and BCC=aceman. I searched only on Inbox, where I moved a > draft message I composed for the test. Aceman: - could u pls include detailed STR for the false positive - could u pls add that test msg as attachment, after doing exactly this to anonymize if you need to: - text editor, replace all instances of "aceman" with "spiderman" - replace yourmailserver.com with asdf.com - replace other private details, but ensure each different detail ends up with a different replacement (so yourserver1.com always becomes asdf1.com, etc.) fwiw, I also had a false positive when testing quickfilter, but it was for "bodytext" where another msg was found which did not contain any trace of the word "bodytext" anywhere in source. but testcase of attachment 632594 [details] behaves as expected.
(In reply to Thomas D. from comment #47) > (In reply to :aceman from comment #45) Aceman, or simply forward as attachment to my private email address the original test msg which is found as false positive when recipient-filtering for aceman.
Attached file testcase 2
Attached file testcase 3
Thomas, STRs here: Import testcase 2 and testcase 3 into TB. Both messages are tweaked versions of your testcase. Testcase 2 is: BCC: blindjohn@asdf.com From: John Doe <blindjohn@asdf.com> Testcase 3 is: BCC: blindjohn@asdf.com From: John Doe <fromjohn@asdf.com> (no other recipient headers like CC, reply-to etc.) Now use the Quick filter bar in the folder where you imported the messages. Select only to match on "Recipients". Write string to search as "blindjohn". Result: Testcase 2 message is matched. WHY??? Expected result: No match, "blindjohn" is not a recipient in the current base Thunderbird matching on "To or CC" only. He is the sender. Test 2: Search on "blindjohn" and choose on the "Sender" button (not "Recipients"). The same testcase 2 msg is matched. That is actually correct. Thomas, can you confirm that?
Thomas and aceman: I have noted false positives, in the past few days, when testing my BCC issues. I posit that they may be connected to database issues [which seemed to cure some issues on folder compaction and repair in my case]. This idea mitigates in favor of test cases on nearly empty accounts? Also, since IMAP/Server and POP/LocalIMAP are two different programming areas, I wonder what kind of account you all run the test cases suggested; equally, I wonder about the general test bed, if it tests into both areas... Also: just curious....was the only copy of the test message in the single folder where the email was found? ie....you didn't send an email to yourself, merely imported the test eml to a single folder.
Attached file Test2, with added TO: field (obsolete) —
I added TO: asdf@junk.com to Test2, creating this Test2a Placed Test2 and Test2a into a new folder in LocalFolders, to avoid any issue on database. I ran this, obviously, in the code on POP/LocalIMAP Searched for "asdf" in w/ RECIPIENT and retrieved BOTH emails -- expected ONE Searched for "blindjohn" w/ RECIPIENT and retrieved only ONE email, TEST2 -- expected none From this, I would suggest that the routine that locates recipients [deep beyond where aceman and I have been toying with code], looks for BCCs if there are no TOs. I also think we all just discovered a fault in the RECIPIENT search, deep beyond where we're coding. My bet is that both this fault, and the behavior in the prior paragraph, are in the same logic run, and that the original author contemplated the utility of looking for BCCs if there was no TO, but miscoded the logic -- just my guess. Someone have a way to test these two test cases on SERVER-IMAP ?
AH....! Using the search function, From/To/CC/BCC, for "asdf", also returns both emails. So the defect occurs both with To/CC [currently mapped to Recipients button] and AllAddresses [mapped within the search drop down list] See lines: 453 to 487 in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgLocalSearch.cpp#453 From this, I am thinking there is a defect in the logic that both rely on, which could either be the boolKeepGoing" and the coding that surrounds/uses it, or the logic deeper in, mentioned in comment #53. My bet is the logic at lines 453 to 488, for the two cases where there are searches of multiple fields...
Learning Curve: msgToMatch->GetAuthor(getter_Copies(matchString)); msgToMatch->GetCcList(getter_Copies(ccList)); msgToMatch->GetBccList(getter_Copies(bccList)); msgToMatch->GetRecipients(getter_Copies(recipients)); I am having trouble deciphering what gets executed and why: I am looking at http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#640 lines 640 to 663 Also looking at the likes of this range of code: http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIdentity.cpp#184 thru line 196 and line 193 piques my interest, but the coding is lost to me. I take in inferences that the code looks further than mere BCClists, which makes me look at our error and wonder ...
Should Comment #53 and testcase Test2a [Test2 with added TO: field] be identified as a new bug ?
(In reply to :aceman from comment #51) > Testcase 2 is: > BCC: blindjohn@asdf.com > From: John Doe <blindjohn@asdf.com> > > Testcase 3 is: > BCC: blindjohn@asdf.com > From: John Doe <fromjohn@asdf.com> > > Result: > Testcase 2 message is matched. WHY??? > Thomas, can you confirm that? Yes, confirming for both tests as described in comment 51. I think analysis of John's comment 53 is right: If case of: To: tojohn AND BCC: blindjohn ...searching for Recipients-only: blindjohn returns nothing, because To: header takes precedence (and searching bcc isn't fully implemented, this bug) If case of: BCC: blindjohn (without To-header etc.) ...searching for Recipients-only: blindjohn returns this case, if To: header is missing, there seems to be some fallback code logic (or bug) to look into BCC header instead. I would try to get some experts on board and make this consistent here, not much point of filing that as a bug, because we *want* this behaviour, only more reliable and consistent. It's a bit less surprising if you consider that Thunderbird is a wild and untamed bird, and the birdmen are few ;)
(In reply to :aceman from comment #50) > Created attachment 632645 [details] > testcase 3 Aceman, thanks for these useful testcases. They would be even greater if the subject of test mails indicated the testcase # and some abbreviated hints about the specifics of that testcase (but ensure hints in subjects are not creating false positives that spoil the test). It's kinda hard right now to tell testcases apart.
see comment #53 This File effects the same text case, 2a, as obsolete one This time: Uploaded in plain text, with BODY explaining use
Attachment #632682 - Attachment is obsolete: true
Comment on attachment 632438 [details] [diff] [review] WIP patch looks like a good start. I'm still not clear on the ldap/addrbook gap comment
Attachment #632438 - Flags: feedback?(dbienvenu) → feedback+
In reply to Comment #60: > I'm still not clear on the ldap/addrbook gap comment David: The code [for the drop down list in search] included a gap of 10 numbers. See: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#93 Our collective assumption is that -no- code, anywhere else in the TBird code, expects those numbers to be reserved/unused; if that's true, then aceman's code is ok. Otherwise, it needs massaging.
(In reply to john :ruskin from comment #61) > Our collective assumption is that -no- code, anywhere else in the TBird > code, expects those numbers to be reserved/unused; if that's true, then > aceman's code is ok. Otherwise, it needs massaging. Right, that's what I'm not sure about, sorry to be unclear.
Comment on attachment 632438 [details] [diff] [review] WIP patch The UI seems good to me. f+. Later, Blake.
Attachment #632438 - Flags: feedback?(bwinton) → feedback+
Attachment #632438 - Flags: feedback?(bugmail) → feedback+
Comment on attachment 632438 [details] [diff] [review] WIP patch rkent, what do you think of the patch here? And what about the concerns raised in previous comments? Bienvenu and john ruskin.
Attachment #632438 - Flags: feedback?(kent)
(In reply to :aceman from comment #64) > Comment on attachment 632438 [details] [diff] [review] > WIP patch > > rkent, what do you think of the patch here? And what about the concerns > raised in previous comments? Bienvenu and john ruskin. (In reply to David :Bienvenu from comment #62) > (In reply to john :ruskin from comment #61) > > > See: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public > > /nsMsgSearchCore.idl#93 > > > > Our collective assumption is that -no- code, anywhere else in the TBird > > code, expects those numbers to be reserved/unused; if that's true, then > > aceman's code is ok. Otherwise, it needs massaging. > > Right, that's what I'm not sure about, sorry to be unclear. An observation to confirm that those 10 slots currently reserved for AB/LDAP are random (and we can use them as we please): http://hg.mozilla.org/releases/comm-1.9.1/rev/6bdc9d138c65#l3.31 Before first introduction of AllAddresses nsMsgSearchAttribValue, we had: // 33 - 43, reserved for ab / LDAP; After introduction of AllAddresses, we had: // 33 - 42, reserved for ab / LDAP; Now, we have: // 34 - 43, reserved for ab / LDAP; Also, AllAddresses was introduced as 43, and now it's 9. The highest value for "Customize" changed from 49 to 52. The numbers really don't matter, nor the position of the free slots, nor their number, nor their existence.
Numbers (Values of nsMsgSearchAttrib) are relevant only for one purpose: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#47 > * Definitions of search attribute types. The numerical order > * from here will also be used to determine the order that the > * attributes display in the filter editor. So the purpose of the reserved slots is to reserve some numbers where AB/LDAP search expressions for future use can be easily inserted without tweaking all the other numbers, while getting them displayed in correct order.
Comment on attachment 632438 [details] [diff] [review] WIP patch I'm not quite sure what feedback+ or feedback- means, so I'll do a feedback+ to be cheery though I have some reservations. I did not do a thorough code review, only conceptual. In some of the work I did earlier on BCC search, one of the main reservations has always been that very few users really understand BCC. In particular, it is impossible to search for BCC on an incoming email by design, so a BCC search is really only valid in the very narrow case of messages that have been sent by you. Any explicit exposure therefore of "BCC" in a search term is likely to mislead the vast majority of users. Now you have not exactly done that here, but instead are now exposing both a "Recipients" as well as a "ToOrCc" search term in the UI. Those are identical, except in the narrow case of message sent by you. That is a subtle difference that will not be understood by the average user. I also have the secondary concern that an excessively long list of search terms is confusing to users, so we should be cautious about adding search terms exposed to the UI in the main code. Instead, use extensions (such as my FiltaQuilla) to expose rarely used search terms. That is the whole point of allowing custom search terms. What I would support is extending the "ToOrCc" search term to include Bcc as well, and rename it in the UI to "Recipients". That way you are not adding any new search terms to the UI. I can't imagine a common use for searching ToOrCc but not searching Bcc as well, but if such a case exists it could be handled in an extension. Unfortunately you then have the problem of backwards compatibility with existing saved searches and filters. You'll probably need to keep the search term label named "ToOrCc" to avoid breaking existing filters and saved searches.
That would simplify things a lot. Who should we ask for objections?
(In reply to :aceman from comment #68) > That would simplify things a lot. Who should we ask for objections? Just let the comment stand for a day or so, there are plenty of cc's to this bug already who can express opinions.
(In reply to Kent James (:rkent) from comment #67) > Comment on attachment 632438 [details] [diff] [review] > WIP patch > > Any explicit exposure therefore of > "BCC" in a search term is likely to mislead the vast majority of users. I think it also depends where and how you expose it, and I have some doubts if hiding "BCC" makes that problem go away or if we are actually manifesting the problem of uneducated users, while removing relevant information for advanced users. For example, a user who really doesn't understand or even know bcc might still think he can find *all* mail to himself with ToOrCC filter, even if we don't mention BCC. > Now you have not exactly done that here, but instead are now exposing both a > "Recipients" as well as a "ToOrCc" search term in the UI. Those are > identical, except in the narrow case of message sent by you. That is a > subtle difference that will not be understood by the average user. Both Advanced Search and Message Filters are for advanced users. I cannot imagine any average user trying to set up a message filter. Imo average users are also very unlikey to even find advanced search, and much less use it. More so after we'll be hiding all the menus in a menu button which will require even more steps to discover this search. Even as an advanced user, I rarely use advanced search, because Quick Filter and Global Search do the trick for 90% of my use cases. (Fwiw, I have a few saved searches and message filters set up). If Advanced Search was designed for the average user, I'd assume it would not be so blatantly neglected in ux and functionality as it currently is (no message preview, no context menu, missing actions for found messages etc.). > I also > have the secondary concern that an excessively long list of search terms is > confusing to users, so we should be cautious about adding search terms > exposed to the UI in the main code. Instead, use extensions (such as my > FiltaQuilla) to expose rarely used search terms. That is the whole point of > allowing custom search terms. Crucial functionality should not be left to addons. Anything search is crucial. If we drop ToOrCC from the list, there will be no way to add the same amount of functionality back without Addons. Due to our currently limited UI, this will prevent most advanced searches that for whatever reason want to *exclude* BCC from searching. Doing To and BCC as separate conditions is not the same, because you need OR for that, which means you cannot add any other conditions with AND. > What I would support is extending the "ToOrCc" search term to include Bcc as > well, and rename it in the UI to "Recipients". That way you are not adding > any new search terms to the UI. I can't imagine a common use for searching > ToOrCc but not searching Bcc as well, but if such a case exists it could be > handled in an extension. I'm a bit worried about UI consistency here. We already have the following strings in the UI for advanced search & message filters: From To To or CC From, To, CC or BCC Regardless if we keep To or CC in that list or not, just adding "Recipients" to that list (for advanced users!) is really odd (inconsistent) and fuzzy: Stringlist C (current patch) From To [To or CC] Recipients From, To, CC or BCC For the sake of consistency, either change all of those terms to natural language, or keep all of them technical, but don't switch language register at random. Stringlist B (natural terms): Sender To [To or CC] Recipients Sender or Recipients Btw I think Recipients (plural) is slightly better than Recipient (singular); it's also consistent with Quick filter buttons. Ideally (if it's technically possible - is it?), combine the best of both worlds: Use natural language terms for the list entries and add details in a tooltip for each list entry (combines . If that's not possible, stay with technical terms: Stringlist A: technical terms From To [To or CC] To, CC or BCC From, To, CC or BCC Given that both filters and advanced search are for more advanced users, I think it's helpful to have precise information what this filter does. > Unfortunately you then have the problem of backwards compatibility with > existing saved searches and filters. You'll probably need to keep the search > term label named "ToOrCc" to avoid breaking existing filters and saved > searches. To use ToOrCC where we really mean ToOrCCOrBCC is asking for trouble. I strongly advise against such foul compromises. In a few years time, coders will curse us for not knowing if ToOrCC includes BCC or not. Names should be telling, and they should do what they say, not more, not less. And well, I wouldn't remove nor alter the meaning of nsMsgSearchAttrib ToOrCC, because altering the meaning might also break the logic of existing filters and saved searches.
Thomas, I've given my feedback, so I think I will leave it to others to respond to your points. I do however take strong objection to this: "Crucial functionality should not be left to addons. Anything search is crucial." There is nothing about fine parsing of BCC search that is crucial. You can't both argue that this feature is only for advanced users, and it is also crucial. And a main value of addons is to prevent clutter of the main user interface by rarely used features. I would certainly say that wanting to search To and CC but not BCC is a rarely used feature. Just because it is "search" cannot be used to trump all arguments for its inclusion.
Comment on attachment 632438 [details] [diff] [review] WIP patch Forget to set the flag.
Attachment #632438 - Flags: feedback?(kent) → feedback+
So what do we do now?
I was to record a bug for this precise thread description. However, after browsing the old discussion, it's not clear to me what does "assigned" currently mean. I understand that comment 34 proposal was essentially implemented years ago, and in fact I've been using this feature (locating recipients, even in BCC, in the Sent folder with Quick Filter) until very recently, when it stopped including BCC recipients. So, I would like to propose to review this later change in behavior as a bug. I find a very useful feature to locate with "Recipients" even in BCC in the Sent folder, since because of the different interface using the alternative Filter is cumbersome and less practical in order to find a particular message. Of course my gratitude to all that work on your part as well as your attention. Best regards.
Flags: needinfo?(acelists)
Yes, the bug is still assigned to me and I could finish it. I don't think any part of it has landed yet. But it appears to me there were some disagreement on how the feature actually should work (or be labeled in the UI). So the work has stalled. If any body can say how to proceed now, I could continue on the patch.
Flags: needinfo?(acelists)
(In reply to :aceman from comment #75) > Yes, the bug is still assigned to me and I could finish it. I don't think > any part of it has landed yet. But it appears to me there were some > disagreement on how the feature actually should work (or be labeled in the > UI). So the work has stalled. If any body can say how to proceed now, I > could continue on the patch. If it helps, in my opinion it worked well until recently, so just including back BCC within Recipients for the Sent folder in Quick Filter would be fine!
(In reply to José Fortes from comment #76) > > If it helps, in my opinion it worked well until recently, so just including > back BCC within Recipients for the Sent folder in Quick Filter would be fine! Looking at the patch again, the discussion got complex because the proposal was to add an additional search term, then we argued over that, as that would also be exposed in the already-too-complex advanced search. If the only goal here is to add BCC to the recipients in quick filter search, there is no reason that the same term has to be added to the advanced search dialogs. I believe that you could add the Recipients term as in the patch, but set Available=false, and then it would not show in the dialog. I believe that as long as enabled is true, then the search term would still work in quick filter. I'd have to check the code carefully to be sure this is true, as this stuff has very little documentation, but that would allow you to fix Quick Filter, adding BCC to Recipients there (which I don't believe is controversial) while also avoiding adding the same term to the advanced search dialog (which has From, To, CC, or BCC available as a reasonable alternative). The other option would be to do quick search recipients with a custom search term. That would be easy to implement, but might have performance issues. C++ would be better, this is performance sensitive code.
Blocks: 1127267
(In reply to Kent James (:rkent) from comment #77) > (In reply to José Fortes from comment #76) > > > > If it helps, in my opinion it worked well until recently, so just including > > back BCC within Recipients for the Sent folder in Quick Filter would be fine! > > Looking at the patch again, the discussion got complex because the proposal > was to add an additional search term, then we argued over that, as that > would also be exposed in the already-too-complex advanced search. Yeah, I agree that the current dropdown list of filter criteria is complex, messy, and unintuitive. In bug 1127267, I propose a complete makeover of that list, also to incorporate this bug 586101 and its two siblings: Bug 586101 - Add "BCC" to "To or CC" Filter (=> "Recipient") Bug 600928 - Add "Reply-To" to "From" Filter (=> "Sender") Bug 535879 - Add "Reply-To" to "From, To, CC, or BCC" Filter (=> "Sender or Recipient") > If the only goal here is to add BCC to the recipients in quick filter > search, there is no reason that the same term has to be added to the > advanced search dialogs. I believe that you could add the Recipients term as > in the patch, but set Available=false, and then it would not show in the > dialog. I believe that as long as enabled is true, then the search term > would still work in quick filter. I'd have to check the code carefully to be > sure this is true, as this stuff has very little documentation, but that > would allow you to fix Quick Filter, adding BCC to Recipients there (which I > don't believe is controversial) +1, sounds good to get rolling again, thanks. > while also avoiding adding the same term to > the advanced search dialog (which has From, To, CC, or BCC available as a > reasonable alternative). My proposal of Bug 1127267 attachment 8556401 [details] [1] seeks to address Kent's concerns about adding more complexity to the criteria list in a complete structural makeover. It also creates ux-consistency of both search behaviour and labelling between "Quick Filter Bar" and Advanced Search / Filters, for the benefit of both simple and advanced users. [1] To view the XUL demo of attachment 8556401 [details] in FF: - Install Remote XUL Manager: https://addons.mozilla.org/en/firefox/addon/remote-xul-manager/ - add a permission for bugzilla.mozilla.org (and <file> for local xul files): about:addons > Remote XUL Manager > Settings button
See Also: → 600928, 522886
See also: [Bug 522886] Implement "Recipients" column that shows all recipients (To, CC, and BCC) I think that's currently stalled because we weren't sure how to transition the underlying database now that we want "Recipients" field to actually/correctly contain all recipient types instead of only to-Recipients.
(In reply to :aceman from comment #75) > Yes, the bug is still assigned to me and I could finish it. I don't think > any part of it has landed yet. But it appears to me there were some > disagreement on how the feature actually should work (or be labeled in the > UI). So the work has stalled. If any body can say how to proceed now, I > could continue on the patch. aceman, I'd say go for simplicity
Aceman is waiting for guidance. So getting back to comment 77, my only interest is in quickfiter being fixed to get some relief, and have zero interest in the side issues which generated so much discussion (and I don't plan to be involved in much more) and add complexity. Can we move on with the patch (which has several feedback+)? (I'm not sure who to ask here, so taking a shot in the dark with magnus)
Flags: needinfo?(mkmelin+mozilla)
Would the situation here be improved by bug 1127267, by changing the labels and reorganizing the list of terms?
Flags: needinfo?(rkent)
The patch looks ok. I would do what it has, and not fold the bcc case into ToOrCc.
Flags: needinfo?(mkmelin+mozilla)
(In reply to :aceman from comment #82) > Would the situation here be improved by bug 1127267, by changing the labels > and reorganizing the list of terms? I think so.
"Would the situation here be improved by bug 1127267, by changing the labels and reorganizing the list of terms?" I like the term "Recipients" for "To, CC, and BCC" but the whole proposal adds 4 additional search terms to an already complex table, plus directly exposes BCC as a search term, both of which I am against. It seems to be driven also by "Crucial functionality should not be left to addons. Anything search is crucial." which I have also opposed. So on the whole I don't like that. If you really wanted to implement something like bug 1127267, perhaps you could have an initial "Basic Search" list, and an "Advanced" button that pops up all of the other options. Merely reordering options helps a little but not enough to overcome the problems of adding 4 additional terms. Looking over the around 10 requests in FiltaQuilla for new search terms, none have mentioned BCC. Most of these would require significant backend work to implement, like "Search for the existence of a header" or "Search for messages in which any message in the thread has been starred". Broken Body search is also a critical problem, and that (plus exposing attachment type and name to search) are the important issues worth working on. This is only my opinion, and I do not have a veto over this. Please do whatever the majority of contributors want.
Flags: needinfo?(rkent)
(In reply to Kent James (:rkent) from comment #85) > If you really wanted to implement something like bug 1127267, perhaps > you could have an initial "Basic Search" list, and an "Advanced" button that > pops up all of the other options. This can be done easily, I have a WIP patch ready. It has only one cosmetic problem that when you click the "Show Advanced items" item (which will be an item in the field menulist, similar to "Customize"), the popup closes. You need to reopen it to see the advanced items. I couldn't find a way to keep the menupopup open after running the command handler. stopPropagation() nor preventDefault seem to work. But we can polish that in the other bug.
Sorry to jump in, but it is really hard to follow this comment thread and the referenced bugs. It appears that there is still no way to use the quickfilter to find an email with an email address in the Bcc field as of 52.1.1. Is there going to be a fix that will allow quick filtering to catch Bcc?

This is still happening in 78.3.1. When searching through the body aswell, bcc adresses are found. But in big folders Thunderbird can get really slow if it searches through the body aswell.

TotalQuickFilter implements filtering for All Addresses, which includes Bcc.

Alright, but we should not expect all users to install that to fix this bug :)

No, you shouldn't. But it is very unlikely any resources will be allocated to this bug, imo. So there you are ;)

Thanks, I will look into it for sure!

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: