338753 - don't confuse "Sender:"-header with "From:" in quick search, thread columns

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Chris Moore]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.3) Gecko/20060513 Ubuntu/dapper Firefox/1.5.0.3
Build Identifier: the package version is '1.5.0.2-0ubuntu1' - it's from the ubuntu dapper beta

As reported in bug 338748, thunderbird mixes up "Sender" and "From" headers.  I have a bunch of messages which have both "Sender:" and "From:" headers.  I notice in the 'quick search' box that it's possible to search on "Sender".  So I typed in the value that's in the "Sender:" header of a lot of my email.  To my surprise no messages were found by the search.  I then entered a string which appears only in "From:" headers of some messages, and the "Sender" quick search matched those messages,

Reproducible: Always

Actual Results:  
When you "quick search" for "Sender", it doesn't search the "Sender" headers.  It does search the "From" headers.  There doesn't seem to be any way to do a quick search against the contents of the "Sender:" headers.

Expected Results:  
I would expect a quick search for "Sender" to match "Sender".  If I wanted to search the "From:" headers, I would expect that quick search to be called "From".

Comment 1

[Magnus Melin [:mkmelin]]

-> new

The main search and filter strings were fixed in bug 221724. Seems quick search was forgotten. (That and the thread column).

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Change all the remaining thunderbird "Sender" stuff (quick search, thread columns) to "From". From is also what Outlook uses.

Without this fix there is cause for confusion since bug 285474 (which I guess should be marked as fixed) has landed.

Comment 3

[Magnus Melin [:mkmelin]]

*** Bug 338748 has been marked as a duplicate of this bug. ***

Comment 4

[David :Bienvenu]

Comment on attachment 233465 [details] [diff] [review]
proposed fix

I'm pretty sure you'd need to change nsMsgDBView.cpp::GetCellText for this to work, since you've changed the name of the tree column. Other than that, I'm ok with this change (though "Subject or From" is a bit awkward)

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Yes, you're right, this patch fixes that. It also takes care of seamonkey.

Also, an unused property "sortBySenderOrRecipientMenuitem" I stumbled upon was removed:  http://lxr.mozilla.org/seamonkey/search?string=BySenderOrRecipientMenuitem

Comment 6

[Scott MacGregor]

Comment on attachment 234683 [details] [diff] [review]
proposed fix, v2

Renaming all of these variables, entity names, column ids, etc. seems like over kill. 

And most users don't know the difference between from and sender anyway.

Instead, why not make quick search on Sender (or From if we change the UI string) search both the Sender and the From header with an OR search.

Ditto for Subject or Sender.

Most users don't know the difference, quick search should just 'work' for them.

Comment 7

[Chris Moore]

If a quick search for messages containing a certain string in the Sender header returns messages which don't contain that string in their Sender header, then that's not "just working".

Just because you ignorant users doesn't mean you should make broken software.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to comment #6)
> (From update of attachment 234683 [details] [diff] [review] [edit])
> Renaming all of these variables, entity names, column ids, etc. seems like over
> kill. 

Well since the semantic meaning does change a bit, the entity names should change so localizers will notice, no? 

Ok, some variable names were changed just for code consistency. I can skip those changes of course, though I'd prefer changing them if you don't have a strong opinion about it... ;)

> Instead, why not make quick search on Sender (or From if we change the UI
> string) search both the Sender and the From header with an OR search.

Yeah, that sounds like a good idea!

---
Updating summary, quick search is really the smaller part of these changes.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Changes from v2: made search for From match "Sender:" also.
One caveat: For some reason matching "Sender:" doesn't work for online imap folders.

If you want me to keep the original names, how much of them should I keep?

Comment 10

[Scott MacGregor]

Comment on attachment 240930 [details] [diff] [review]
proposed fix, v3

I'm worried about backward compatibility issues with changing the tree col ID, as this will break folks who have hidden or otherwise re-ordered the senderCol.

That applies to messenger.xul and SearchDialog.xul.

David might have a comment about why searching the Sender header doesn't work for imap folders in quick search. I suspect it's because we don't fetch the Sender header for the  message when we are downloading just the header info for imap messages. So the msg hdr doesn't have a sender field.

Comment 11

[David :Bienvenu]

right, we don't fetch the "Sender" header unless you've added it to the list of custom headers by creating a mail filter that filters on the Sender header.

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

Updated patch, this doesn't touch the tree col ID. 

Quick search doesn't match also "Sender:" for online IMAP folders. I'm not sure how to make us fetch that header so it can be matched (if we want to do it in this bug). Besides that it works fine.

Comment 13

[Scott MacGregor]

Comment on attachment 242434 [details] [diff] [review]
proposed fix, v4

Hey David,

Does this code look ok to you? I wonder if we should make the back end search code treat nsMsgSearchAttribValue Sender searches as searches on the From or Sender header for everyone instead of doing a quick search specific thing. Would that make sense?

 
+      // make quick search for From also match the "Sender:" header
+      if (gSearchInput.searchMode == kQuickSearchFrom || gSearchInput.searchMode == kQuickSearchFromOrSubject)
+      {
+        term = gSearchSession.createTerm();
+        value = term.value;
+        value.str = termList[i];
+        term.value = value;
+        term.attrib = nsMsgSearchAttrib.OtherHeader;
+        term.op = nsMsgSearchOp.Contains;
+        term.booleanAnd = false;
+        term.arbitraryHeader = "Sender";
+        searchTermsArray.AppendElement(term);
+      }


Thanks for leaving the tree columns alone magnus.

I'm going to nominate this for tb2 approval, but we'll want to see some bake time on the trunk first. So I won't plus this right away.

Comment 14

[David :Bienvenu]

ugh, I worry that the arbitrary header Sender search is going to mean looking on disk at the headers of each message, which is going to slow things down. My vague recollection is that I'm storing the sender in the msg hdr, but I'm not sure the search code knows that.

Comment 15

[Scott MacGregor]

Comment on attachment 242434 [details] [diff] [review]
proposed fix, v4

pulling back the sr until we look into David's concerns. We might go back to taking this patch but without the From + Sender behavior for quick search...

Comment 16

[Magnus Melin [:mkmelin]]

*** Bug 364753 has been marked as a duplicate of this bug. ***

Comment 17

[Scott MacGregor]

Comment on attachment 242434 [details] [diff] [review]
proposed fix, v4

unfortunately this didn't make it in before the string freeze so we can't take this now.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v5

Unbitrotted the previous patch, and skipping the quicksearch From + Sender behavior. If we still want to do that, we can do it in another bug.

Comment 20

[Phil Ringnalda (:philor)]

mail/base/content/SearchDialog.xul 1.24
mail/base/content/mailWindowOverlay.js 1.167
mail/base/content/mailWindowOverlay.xul 1.208
mail/base/content/messenger.xul 1.72
mail/base/content/searchBar.js 1.42
mail/locales/en-US/chrome/messenger/SearchDialog.dtd 1.10
mail/locales/en-US/chrome/messenger/messenger.dtd 1.64
mail/locales/en-US/chrome/messenger/messenger.properties 1.48
mailnews/base/resources/content/commandglue.js 1.273
mailnews/base/resources/content/mailWindowOverlay.js 1.256
mailnews/base/resources/content/mailWindowOverlay.xul 1.330
mailnews/base/resources/content/threadPane.js 1.90
mailnews/base/resources/content/threadPane.xul 1.137
suite/locales/en-US/chrome/mailnews/messenger.dtd 1.215
suite/locales/en-US/chrome/mailnews/messenger.properties 1.141
suite/locales/en-US/chrome/mailnews/threadpane.dtd 1.28