522886 - Implement "Recipients" column that shows all recipients (To, CC, and BCC)

Status: REOPENED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Thomas D. (:thomas8)]

As per bug 522885, folder views currently have a "Recipient" column that as matter of fact is just a "To" column with a wrong label (cf. screenshot,  attachment 406871 [details]).

Actual Behaviour:
- Recipient column shows only TO recipients

Expected Behaviour:
- "Recipients" column should show /all/ recipients in one single column (To, CC, and BCC).

Fixing this bug would be a more user-friendly and more useful (long-term) fix for bug 522885, too.
(Although we'd probably also want separate optional columns for each of TO, CC, and BCC, but that's another bug again.)

Comment 1

[Dave]

Hopefully, by "fixing" this bug it means that we will have *both* a "To" column and a "Receipients" column.

Losing a "To" (currently named "Receipient") column for a "Receipients" column will clutter up the message list by providing redundant receipients information that one could've seen in the header panel when selecting the message.

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

I have made the changes in mail/ but not in suite/ 
Should I make these changes in suite/ as well?

The multiple addresses in the recipient and To columns are separated by only a comma "," which doesn't look readable at the first look, can I make the separator to be ", " i.e. comma followed by a space?

Comment 3

[:aceman]

Let's way for reactions from the TB guys and then you can port the polished version to Seamonkey too, if the SM guys want it.

For the "," delimiter I would think it should also be made localizable.

Comment 4

[:aceman]

Comment on attachment 760130 [details] [diff] [review]
Patch

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

I do not like the duplication of column titles in SearchDialog.dtd and messenger.dtd. Maybe that should be merged in a follow-up bug, if the reviewers do not say there is a good reason for keeping the definitions duplicated.

But the patch is very nice, seems to work and also tests pass. You fixed the one that was unnecessarily hard-coding the position of the size column.

I observed just one problem, that the new toCol column has somehow received the same value of the "ordinal" attribute as the junkStatusCol (being 15). I don't know how to fix that but I feel it could cause problems.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +512,5 @@
> +      if (aRecipientsString.IsEmpty())
> +        CopyUTF8toUTF16(cachedRecipients, aRecipientsString);
> +      else
> +      {
> +        aRecipientsString.Append(NS_LITERAL_STRING(","));

Try .AppendLiteral() here.

::: mailnews/base/src/nsMsgDBView.h
@@ +145,5 @@
>    bool          mGoBackEnabled;
>    
>    virtual const char * GetViewName(void) {return "MsgDBView"; }
>    nsresult FetchAuthor(nsIMsgDBHdr * aHdr, nsAString &aAuthorString);
> +protected:

This is probably not needed here.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 760130 [details] [diff] [review]
Patch

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

Porting things to suite/ is optional. You only need to do it for cases where it's partly shared code that otherwise would break suite.

The To column should probably not be last (in the search dialog), but before From. Also in the main window columns i'd think maybe before From.

IN the main window the patch only seems to show a number (or blank) in the To column for me (works in search). Besides that i don't see any bigger issues with it.

::: mail/locales/en-US/chrome/messenger/SearchDialog.dtd
@@ +61,5 @@
>  <!ENTITY columnChooser.tooltip "Click to select columns to display">
>  <!ENTITY threadColumn.tooltip "Click to display message threads"> 
>  <!ENTITY fromColumn.tooltip "Click to sort by from">
>  <!ENTITY recipientColumn.tooltip "Click to sort by recipient">
> +<!ENTITY toColumn.tooltip "Click to sort by To recipient">

"Click to sort by to"

::: mailnews/base/public/nsIMsgDBView.idl
@@ +77,5 @@
>    const nsMsgViewSortTypeValue byAttachments = 0x20;
>    const nsMsgViewSortTypeValue byAccount = 0x21;
>    const nsMsgViewSortTypeValue byCustom = 0x22;
>    const nsMsgViewSortTypeValue byReceived = 0x23;
> +  const nsMsgViewSortTypeValue byToRecipient = 0x24;

Could we make it byTo throughout the patch. byToRecipent its a odd name

@@ +82,3 @@
>  };
>  
>  [scriptable, uuid(255d1c1e-fde7-11d4-a5be-0060b0fc04b7)]

should rev the uuid when you change idls. (use uuidgen to generate a new one)

::: mailnews/base/public/nsIMsgHdr.idl
@@ +84,5 @@
>      readonly attribute AString mime2DecodedAuthor;
>      readonly attribute AString mime2DecodedSubject;
>      readonly attribute AString mime2DecodedRecipients;
> +    readonly attribute AString mime2DecodedCCRecipients;
> +    readonly attribute AString mime2DecodedBCCRecipients;

please document using /// comments before each
(at least mime2DecodedRecipients leaves one wonder what it is)

::: mailnews/base/src/nsMsgDBView.cpp
@@ +509,5 @@
>      // was changed after cache.
>      if (!cachedRecipients.IsEmpty())
>      {
> +      if (aRecipientsString.IsEmpty())
> +        CopyUTF8toUTF16(cachedRecipients, aRecipientsString);

if one if-else have braces, they all should.

@@ +572,5 @@
>          }
>  
>          // add ',' and end of each recipient
> +        if (!aRecipientsString.IsEmpty() || i != 0)
> +          aRecipientsString.AppendLiteral(",");

agreed ", " would be more readable. i doubt it have to be localizable

@@ +583,5 @@
>    }
>    else
> +  {
> +    if (aRecipientsString.IsEmpty())
> +      aRecipientsString = unparsedRecipients;

if one if-else have braces, they all should.

Comment 6

[:aceman]

(In reply to Magnus Melin from comment #5)
> Porting things to suite/ is optional. You only need to do it for cases where
> it's partly shared code that otherwise would break suite.

In this case the shared implementation of Recipients column is changed (also for SM, but without the option of choosing the new To column) so we need a decision from SM on what to do and the solution probably need to be done in the same patch.

Comment 7

[Thomas D. (:thomas8)]

Suyash Agarwal, thanks for picking this up, that's great! :)

The current patch fixes bug 522885 so we'll then have an optional To-column (from column picker) and a shown-by-default Recipients-column which actually shows all recipients.

While we are here, can we complete the set by fixing bug 699588 to add the missing optional columns for CC and BCC, so that users can show them from column picker if they so wish?

----------------
Nit:

Also, while we are here, can we clean up those tooltips to get rid of that useless "Click here to" part of tooltips like:

"Click here to sort by recipient" etc., so that they become just plain vanilla
"Sort by recipient"?

Think about it, the "Click here to" part is just a nuisance:
- no informational value at all: we could add that to any tooltip in the whole UI: "Click here to get new messages", "Click here to print this message" and so on... so what's the point of having it here? (ux-consistency)
- "Click here to" just distracts from the real action which is "Sort by ..." (so it actually makes it harder to parse the tooltip for the relevant info)
- It's common knowledge that tables can be resorted by clicking their headers; users who don't have that knowledge are unlikely to hover the column header anyway ("Click here to" won't help them to find this click target)
- iow, tooltip will only show for users who have already found the click target (column header), and then we already have plenty of ux-feedback that you can "click here": column header is highlighted on hover, and the very fact that we show a tooltip with an action verb like "Sort by..." *always* implies that this an UI element where clicking will trigger that action...

(N.B. we've tried that road of being over-explicit in our UI before in old quick search where we had "Filter for To", "Filter for CC" etc. on the dropdown, which was equally useless when there's a clear context of magnifier icon and a filter searchbox... Alas, localizations didn't even bother to pick that up, and it's all gone now...)

Comment 8

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #2)
> I have made the changes in mail/ but not in suite/ 
> Should I make these changes in suite/ as well?

see comment 6

> The multiple addresses in the recipient and To columns are separated by only
> a comma "," which doesn't look readable at the first look, can I make the
> separator to be ", " i.e. comma followed by a space?

+1, it should be ", " with space as a default. I'd lean towards making it localizable / customizable as a pref, but I don't have a strong opinion on that. Having the pref would allow users to choose a delimiter which suits their local or individual needs best, e.g. to pick semicolon "; " instead of comma ", " or even to pick an entirely different separator like " | " for added salience.

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 760130 [details] [diff] [review]
Patch

clearing feedback request, see my comment 7 and comment 8

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #8)
> +1, it should be ", " with space as a default. I'd lean towards making it
> localizable / customizable as a pref, but I don't have a strong opinion on
> that. Having the pref would allow users to choose a delimiter which suits
> their local or individual needs best, e.g. to pick semicolon "; " instead of
> comma ", " or even to pick an entirely different separator like " | " for
> added salience.

Who's gonna change that pref, except for the sake if it? All prefs add a certain cost and if it's not needed lets just skip it.

Comment 11

[:aceman]

(In reply to Thomas D. from comment #7)
> While we are here, can we complete the set by fixing bug 699588 to add the
> missing optional columns for CC and BCC, so that users can show them from
> column picker if they so wish?
Then we can do that in bug 699588, after this patch is actually accepted.

> Also, while we are here, can we clean up those tooltips to get rid of that
> useless "Click here to" part of tooltips like:
Please file this as a new bug.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #10)
> (In reply to Thomas D. from comment #8)
> > +1, it should be ", " with space as a default. I'd lean towards making it
> > localizable / customizable as a pref, but I don't have a strong opinion ... 
> Who's gonna change that pref, except for the sake if it? All prefs add a
> certain cost and if it's not needed lets just skip it.

Yes, let's skip it *if* it's not needed. However, in terms of correct localization it might still be needed with the arrival of international email addresses (1), which are no longer limited to the classic ASCII character set of the English Alphabet. And looking at languages like Chinese, they don't use the same punctuation marks as English either (2)(3), so using comma for enumerations is actually odd for such languages. E.g. for Chinese:
> ， (U+FF0C FULLWIDTH COMMA) is the comma (,). It cannot be used for enumerating
> a list; see "enumeration comma"

Just my 2 cents. I acknowledge it's just dotting the i's and we can certainly ignore this for now and wait until we get a localization bug for that.

(1) http://en.wikipedia.org/wiki/International_email
(2) http://en.wikipedia.org/wiki/Comma_%28punctuation%29
(3) http://en.wikipedia.org/wiki/Chinese_punctuation

Comment 13

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #11)
> (In reply to Thomas D. from comment #7)
> > While we are here, can we complete the set by fixing bug 699588 to add the
> > missing optional columns for CC and BCC, so that users can show them from
> > column picker if they so wish?
> Then we can do that in bug 699588, after this patch is actually accepted.
> 
> > Also, while we are here, can we clean up those tooltips to get rid of that
> > useless "Click here to" part of tooltips like:
> Please file this as a new bug.

I thought both of these might be simple and straightforward enough to be included here if Blake agrees. Otherwise, I'm fine with doing them in their own bugs if it helps to get this landed faster.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 760130 [details] [diff] [review]
Patch

Notes:

", " seems better to me.  I'm -1 on adding a pref for it, but localizability seems like the right thing to do.
We should probably remove the "Click here to" from the tooltips, but I don't particularly care whether you do it here, or make Thomas file a new bug.  ;)
I don't think we need to add Cc and Bcc columns in this patch.  That seems like too much patch-bloat.  ;)

Thanks,
Blake.

Comment 15

[:aceman]

Yes, that is what I meant: localizability (hardcoded string per language), not preference.

The change of tooltips could be done in bug 881051 to keep this one on topic (otherwise due to the need of changing the string ID it would need to touch all columns here, not just the one being behaviour-changed).

Comment 16

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #15)
> Yes, that is what I meant: localizability (hardcoded string per language),
> not preference.

+1 if that means getting the string from some sort of language resource file (dtd etc.).

> The change of tooltips could be done in bug 881051 to keep this one on topic
> (otherwise due to the need of changing the string ID it would need to touch
> all columns here, not just the one being behaviour-changed).

Oh, I forgot we still have that nuisance localisation system which apparently forces us to change the string ID every time we're just tweaking the string. Grrr... (Any chance that somebody changes that system?) Yeah, so it's probably easier to review if we do tooltips separately. I'd rather not do that in bug 881051 (as I don't know if merging the strings is the right thing), so a separate bug would be best.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton) from comment #14)
> We should probably remove the "Click here to" from the tooltips, but I don't
> particularly care whether you do it here, or make Thomas file a new bug.  ;)

I voluntarily filed a new bug for tweaking the tooltips ;)

Bug 881073 Remove "Click here to..." clutter from message list column header tooltips ("Click here to sort by recipient" etc.) for ux-consistency with other tooltips

Comment 19

[:aceman]

I missed this line in the feedback. Please put it back (with 'column' as second argument), we need to save the cache.
-  UpdateCachedName(aHdr, "recipient_names", aRecipientsString);

Comment 20

[:aceman]

Oh, but do not save the concatenated aRecipientsString (To+CC+Bcc) but for each cache save only the proper addresses.

Comment 21

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

I have tried to address the comments.
mkmelin: I changed the uuid of the interface above the one you mentioned, as I changed the code in that. And, for placing the To column before From, I thought to take your opinion again, as another bug demands Cc and Bcc columns also, so current placement will make them in the order of From, Recipients, To, Bcc, Cc and I think this fits.
If you still favor the placement of To before From, please let me know that. I will make the change.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 760488 [details] [diff] [review]
Patch v2

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

I'm ok with the field order being as you suggested.

::: mail/locales/en-US/chrome/messenger/SearchDialog.dtd
@@ +61,5 @@
>  <!ENTITY columnChooser.tooltip "Click to select columns to display">
>  <!ENTITY threadColumn.tooltip "Click to display message threads"> 
>  <!ENTITY fromColumn.tooltip "Click to sort by from">
>  <!ENTITY recipientColumn.tooltip "Click to sort by recipient">
> +<!ENTITY toColumn.tooltip "Click to sort by To">

probably lowercase To for consistenty

::: mailnews/base/public/nsIMsgHdr.idl
@@ +86,5 @@
>      readonly attribute AString mime2DecodedRecipients;
> +    // fetches mime decoded cc recipients from the message database
> +    readonly attribute AString mime2DecodedCCRecipients;
> +    // fetches mime decoded bcc recipients from the message database
> +    readonly attribute AString mime2DecodedBCCRecipients;

/// style comment (with three slashes) is recognized as a documentation comment by doxygen, so please use that. (// is for non-documentation comments)

How about "MIME decoded recipients from the CC header." ?

Can we drop the Recipients form the name here too? mime2DecodedCC ?


Also, the uuid in nsIMsgHdr also needs to be changed

Comment 23

[Thomas D. (:thomas8)]

Comment on attachment 760488 [details] [diff] [review]
Patch v2

clearing feedback request - nothing to add from my side atm

Comment 24

[:aceman]

Comment on attachment 760488 [details] [diff] [review]
Patch v2

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

Looks fine to me, thanks.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +505,5 @@
>      GetCachedName(recipients, currentDisplayNameVersion, cachedRecipients);
>  
>      // recipients have already been cached,check if the addressbook
>      // was changed after cache.
>      if (!cachedRecipients.IsEmpty())

Not sure we actually check the addressbook here as the comment implies. But that is not your fault.

@@ +588,5 @@
> +    aRecipientsString.AppendLiteral(",");
> +
> +  aRecipientsString.Append(parsedRecipients);
> +
> +  UpdateCachedName(aHdr, column, parsedRecipients);

I would put the cache update above the line "if (!aRecipientsString.IsEmpty() && !parsedRecipients.IsEmpty())" .

Comment 25

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2 (revised)

I have tried to address all the changes suggested.

Comment 26

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 760584 [details] [diff] [review]
Patch v2 (revised)

Can you please suggest if these changes are to be made in suite/ as well?

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 760584 [details] [diff] [review]
Patch v2 (revised)

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

I still get (for some reason) strange order of columns, and just a number showing in To

::: mail/base/content/folderDisplay.js
@@ +396,5 @@
>      // recipient = To. You only care in outgoing folders.
>      recipientCol: function (viewWrapper) {
>        return viewWrapper.isOutgoingFolder;
>      },
> +    toCol: function (viewWrapper) {

nit: no space after "function"

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +663,5 @@
>  <!ENTITY columnChooser.tooltip "Click to select columns to display">
>  <!ENTITY threadColumn.tooltip "Click to display message threads">
>  <!ENTITY fromColumn.tooltip "Click to sort by from">
>  <!ENTITY recipientColumn.tooltip "Click to sort by recipient">
> +<!ENTITY toColumn.tooltip "Click to sort by To">

lowecase to here too

::: mailnews/base/src/nsMsgDBView.cpp
@@ +510,5 @@
>      {
> +      CopyUTF8toUTF16(cachedRecipients, parsedRecipients);
> +
> +      if (!aRecipientsString.IsEmpty())
> +        aRecipientsString.AppendLiteral(",");

we wanted ", " here and a few other places?

Comment 28

[Magnus Melin [:mkmelin]]

Attachment: screenshot of problem

Comment 29

[Magnus Melin [:mkmelin]]

BTW, was there a plan for a Recipient -> Recipients change?

Comment 30

[Suyash Agarwal (:sshagarwal)]

For the numbers being displayed in the column, can you please try deleting the .msf database files and see if the issue still exists?
And for ", " , I think the decision was to make it localizable (bug 881074), but if you want this space part to be done here, please let me know, and for Recipient->Recipients change, I don't think we talked over it, but I too have a +1 for it, so making this change.

Comment 31

[[e-mail]]

(In reply to Magnus Melin from comment #27)
> I still get (for some reason) strange order of columns
Is this with a new profile? Existing profiles will try to maintain your existing column order.

> just a number showing in To
There's a column 'total' with which this is being confused.

Comment 32

[Magnus Melin [:mkmelin]]

Its an existing profile.

Indeed blowing away the msf fixes it.

I'd make it ", " now unless it causes problems, and without a change to Recipients in this bug the landing would risk leaving it all in an confusing state.

Comment 33

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2 (revisited)

This patch also addresses agreed up points from comment 27.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 762243 [details] [diff] [review]
Patch v2 (revisited)

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

Unfortunately when we change strings we need to change the localization keys too, so localizers can keep up (need to make recipientColumn.label recipientsColumnn.label or something, and so on.)

it's also still just showing the numbers. please check if it's confused with 'total' like neil said.

Comment 35

[[e-mail]]

Comment on attachment 760584 [details] [diff] [review]
Patch v2 (revised)

Well, this all looks mostly reasonable, but I don't like the way the commonFetchRecipients method works, but that's just personal preference.

If I was writing it, I would leave the code to get the display names from the recipients in its own function, and have the caller deal with the cache i.e.

nsMsgDBView::FetchTo(nsIMsgDBHdr *aHdr, nsAString *aToString)
{
  int32_t currentDisplayNameVersion;
  GetDisplayNameVersion(&currentDisplayNameVersion);
  GetCachedName(aHdr, "to_names", currentDisplayNameVersion, aToString);

  if (to.IsEmpty())
  {
    nsString unparsedTo;
    aHdr->GetMime2DecodedTo(unparsedTo);
    GetDisplayNamesInAddressBook(unparsedTo, aToString);
    UpdateCachedName(aHdr, "to_names", currentDisplayNameVersion, aToString);
  }
}
Note that this is just pseudocode; it will not work as written for several reasons, for instance nsIMsgHdr's UTF16 support doesn't work.

>@@ -2013,16 +2044,20 @@ NS_IMETHODIMP nsMsgDBView::CellTextForCo
>           nsAutoString formattedCountString;
>           uint32_t numChildren;
>           thread->GetNumChildren(&numChildren);
>           formattedCountString.AppendInt(numChildren);
>           aValue.Assign(formattedCountString);
>         }
>       }
>     }
>+    else if (aColumnName[1] == 'o' && aColumnName[2] == 'C') // To column
>+    {
>+      rv = FetchToRecipients(msgHdr, aValue);
>+    }
This doesn't work because the 'total' column above only checks for 'to' - you need to change the 'total' check too.

Comment 36

[Thomas D. (:thomas8)]

(In reply to [e-mail] from comment #35)
> >+    else if (aColumnName[1] == 'o' && aColumnName[2] == 'C') // To column
> >+    {
> >+      rv = FetchToRecipients(msgHdr, aValue);
> >+    }
> This doesn't work because the 'total' column above only checks for 'to' -
> you need to change the 'total' check too.

Suyash, perhaps changing this line which seems to check for the Total column could fix the "showing total numbers in toCol" problem:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgGroupView.cpp#876

> 876     else if (aColumnName[0] == 't' && aColumnName[1] == 'o')

As an uninformed guess, this might work if you change it to match whatever the internal name is for the total column, e.g.

876     else if (aColumnName[0] == 't' && aColumnName[2] == 't')

It's really just a guess, pls verify the details yourself.

Comment 37

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #29)
> BTW, was there a plan for a Recipient -> Recipients change?

Good catch for the column header caption, thanks!
We really want "Recipients" (plural) because that's what we actually show, more so after this bug, and to be ux-consistent with that term as used in other parts of the application, e.g. Quick Filter which has "Recipients" facet referring to the same set of addresses we are showing in this column (and I wonder why we don't use the same collective natural language terms in Advanced Search and Filters, too...)

(In reply to Magnus Melin from comment #32)
> I'd make it ", " now unless it causes problems, and without a change to
> Recipients in this bug the landing would risk leaving it all in an confusing
> state.

+1

:sshagarwal, pls change it to ", " here in this bug, and please change the string for the column header caption to "Recipients" instead of current "Recipient".

(In reply to Magnus Melin from comment #34)
> Unfortunately when we change strings we need to change the localization keys
> too, so localizers can keep up (need to make recipientColumn.label
> recipientsColumnn.label or something, and so on.)

In this case, recipientsColumn.label (instead of recipientColumn.label) sounds good to me; for most other strings, we usually just add numbers to the keys, e.g.
somestring1.label instead of somestring.label

OT: I think that's an awful way of messing up the code for no reason, can't we change that?

Comment 38

[Thomas D. (:thomas8)]

Magnus, should we change this, too?

>  <!ENTITY recipientColumn.tooltip "Click to sort by recipient">

to become:

  <!ENTITY recipientsColumn.tooltip "Click to sort by recipients">

Comment 39

[Magnus Melin [:mkmelin]]

yes. 

There's been a lot of talk and promises over the years about fixing the entity update problem, but no solution yet.

Comment 40

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #39)
> yes. 

I understand that's a yes for comment 37 (entity update problem), or is it a yes to comment 38 (change tooltip), or both?

Comment 41

[Magnus Melin [:mkmelin]]

It's a yes to comment 38 - "recipient" should be "recipients" everywhere that it makes sense. Why wouldn't it?

Comment 42

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #35)
> Well, this all looks mostly reasonable, but I don't like the way the
> commonFetchRecipients method works, but that's just personal preference.
> 
> If I was writing it, I would leave the code to get the display names from
> the recipients in its own function, and have the caller deal with the cache
> i.e.
> 
> nsMsgDBView::FetchTo(nsIMsgDBHdr *aHdr, nsAString *aToString)
> {
>   int32_t currentDisplayNameVersion;
>   GetDisplayNameVersion(&currentDisplayNameVersion);
>   GetCachedName(aHdr, "to_names", currentDisplayNameVersion, aToString);
> 
>   if (to.IsEmpty())
>   {
>     nsString unparsedTo;
>     aHdr->GetMime2DecodedTo(unparsedTo);
>     GetDisplayNamesInAddressBook(unparsedTo, aToString);
>     UpdateCachedName(aHdr, "to_names", currentDisplayNameVersion, aToString);
>   }
> }

But why don't you like what I wrote? Did I miss some Object Oriented methodologies or something?
I mean, if I write the way you suggested, there will be a lot of repeated code because I will have to rewrite the same piece of code for To, Cc & Bcc (e.g. the for loop in the FetchRecipients()).

But, if you still think my way wasn't good enough, please let me know what I missed in it so that I can be comfortable with your way.

Thanks.

Comment 43

[[e-mail]]

I didn't say it wasn't good enough, I just said that I personally dislike it, however rewriting the code is probably beyond the scope of this bug (the rewrite would for instance try to share code with FetchAuthor too).

Comment 44

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2)

Sir,

Can you please tell me if this conforms with the style you suggested?

Comment 45

[:aceman]

I really hope this is not what Neil wants as there is so much unnecessary duplication...

Comment 46

[[e-mail]]

Comment on attachment 763224 [details] [diff] [review]
Patch (variant2)

No, as I said, what I was thinking of would really need to be separately covered by a rewrite.

Comment 47

[[e-mail]]

(Not sure why or how you managed to request both feedback and needinfo at the same time...)

Comment 48

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1)

Okay so, getting back to the original patch and problems.

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 763266 [details] [diff] [review]
Patch (variant1)

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

Seems to be working correctly now. The column order is odd for existing profiles, but i don't think we necessarily have to do something about it.

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +186,5 @@
>  <!ENTITY sortByFromCmd.accesskey "F">
> +<!ENTITY sortByRecipientsCmd.label "Recipients">
> +<!ENTITY sortByRecipientsCmd.accesskey "c">
> +<!ENTITY sortByToCmd.label "To">
> +<!ENTITY sortByToCmd.accesskey "t">

t is taken for Threaded. (and access keys should be given case sensitively, that it works with non-matching case is a fallback)

Might have to swap the O in order received to and "i"

Comment 50

[Thomas D. (:thomas8)]

sshagarwal:

can you attach new patch with these access keys modified in addition to what you have:

Order Received -> i
To -> o

Comment 51

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #49)
> Comment on attachment 763266 [details] [diff] [review]
> Patch (variant1)

Magnus, what's next to ensure that this bug can land in time for TB24?
It would be a pity if we miss this one.

* can we request checkin-needed based on your review+, or do we need a second review?

* access keys will still work with Enter even when two items have same access key - would it be ok to land this anyway if one duplicate access key is the only little nit left in the patch?

Comment 52

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1)

I changed the association of the accesskeys but I did not change the string ID this time, do I need to do that this time as well?

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 766265 [details] [diff] [review]
Patch (variant1)

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

That's ok - when changing accesskeys you don't need to bump the string. Every localization needs to check they don't end up with dupes manually.

Accesskey case should match that in the string though.

Thomas: This patch can't land until it has the mailnews/ review, and ui-r

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +186,5 @@
>  <!ENTITY sortByFromCmd.accesskey "F">
> +<!ENTITY sortByRecipientsCmd.label "Recipients">
> +<!ENTITY sortByRecipientsCmd.accesskey "c">
> +<!ENTITY sortByToCmd.label "To">
> +<!ENTITY sortByToCmd.accesskey "O">

lower case o (as it is in To)

@@ +191,4 @@
>  <!ENTITY sortByUnreadCmd.label "Read">
>  <!ENTITY sortByUnreadCmd.accesskey "R">
>  <!ENTITY sortByOrderReceivedCmd.label "Order Received">
> +<!ENTITY sortByOrderReceivedCmd.accesskey "I">

lower case i

Comment 54

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1)

Okay, made the change. Also, I would like to know if I am required to make the same for suite/

Comment 55

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 766268 [details] [diff] [review]
Patch (variant1)

mkmelin: I hope I can carry review from you. If you have any suggestions for this, please let me know.

Comment 56

[Magnus Melin [:mkmelin]]

Thats ok. There were some comments that suggest you would need to fix suite, but i think they were incorrect as you're not changing the current Recipient column functionality, just fixing the label to be what it actually already is. (That's the case isn't it?)

Comment 57

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #56)
> Thats ok. There were some comments that suggest you would need to fix suite,
> but i think they were incorrect as you're not changing the current Recipient
> column functionality, just fixing the label to be what it actually already
> is. (That's the case isn't it?)

This bug changes/adds/separates/corrects a recipient column functionality and a To-column functionality. It's more than labels.

Comment 58

[:aceman]

Yes Magnus, this patch changes functionality for suite in the Recipient column so we need a decision from the suite people. On the other hand, as they have releases more often, they will not need to wait for ~TB31 for the fix:)

Comment 59

[Magnus Melin [:mkmelin]]

Of course it's more than labels. I meant that as "Recipient" would easily be interpreted as both To+cc+bcc to begin with. That it used to show only Tos is an implementation detail. Anyone not intimately familiar with the column (in seamonkey) wouldn't care about the difference - and nothing is "broken" by the patch in that way.

Comment 60

[Suyash Agarwal (:sshagarwal)]

Okay, so if suite/ isn't required in this patch, then why should this wait?

Comment 61

[:aceman]

This waits on a review from somebody (e.g. currently selected Neil) for the mailnews parts. Also this changes (albeit fixes) behaviour of the "Recipient" column in Seamonkey too (without any way out, opposite to TB) so we need confirmation from SM if we can still continue with that.

Comment 62

[[e-mail]]

Sorry for the delay in getting back to this, but what change was made to fix the issue of the total and to column showing the same value?

Comment 63

[:aceman]

From the current patch:
   case 't':
     // total msgs in thread column
-    if (aColumnName[1] == 'o' && (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay))
+    if (aColumnName[1] == 'o' && aColumnName[2] == 't' &&
+       (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay))
     {
....
     }
+    else if (aColumnName[1] == 'o' && aColumnName[2] == 'C') // To column
+    {
+      rv = FetchToRecipients(msgHdr, aValue);
+    }

Neil, is this still not working?

Comment 64

[Thomas D. (:thomas8)]

Comment on attachment 762198 [details]
screenshot of problem

Per Comment 63, the problem seen in screenshot of Attachment 762198 [details] has been fixed, so let's make the screenshot obsolete to avoid the impression there's anything left to do here apart from Neil's review and BWinton's ui-review that we are waiting for.

Comment 65

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 766268 [details] [diff] [review]
Patch (variant1)

This seems to work the way I expect it to.  ui-r=me.

Thanks, Suyash!

Comment 66

[[e-mail]]

(In reply to aceman from comment #63)
> Neil, is this still not working?

That looks reasonable for threaded views (I hope to try once my build completes) but I am concerned that it won't work for grouped by sort views and I have no idea where to check for saved searches.

Comment 67

[[e-mail]]

Comment on attachment 766268 [details] [diff] [review]
Patch (variant1)

>+nsresult nsMsgDBView::commonFetchRecipients(nsIMsgDBHdr * aHdr, char * column,
>+                                            nsresult (nsIMsgDBHdr::*mimeDecoder)(nsAString&),
>+                                            nsAString &aRecipientsString)
Function pointers are funky because on MSVC the NS_IMETHOD makes them stdcall, not thiscall, so we need to change the declaration here too. Unfortunately being a member function pointer you can't just use NS_CALLBACK_(nsresult, mimeDecoder) so you have to use NS_STDCALL directly.

Comment 68

[[e-mail]]

Comment on attachment 766268 [details] [diff] [review]
Patch (variant1)

>-[scriptable, uuid(e8fdf9ca-9425-49de-b231-8d8fb51b8ee2)]
>+[scriptable, uuid(be96197f-7927-41c6-a4af-ff0c179ec469)]
> interface nsMsgViewSortType
> {
>   const nsMsgViewSortTypeValue byNone = 0x11; /* not sorted */
>   const nsMsgViewSortTypeValue byDate = 0x12;
>   const nsMsgViewSortTypeValue bySubject = 0x13;
>   const nsMsgViewSortTypeValue byAuthor = 0x14;
>   const nsMsgViewSortTypeValue byId = 0x15;
>   const nsMsgViewSortTypeValue byThread = 0x16;
>@@ -73,16 +73,17 @@ interface nsMsgViewSortType
>   const nsMsgViewSortTypeValue byRecipient = 0x1c;
>   const nsMsgViewSortTypeValue byLocation = 0x1d;
>   const nsMsgViewSortTypeValue byTags = 0x1e;
>   const nsMsgViewSortTypeValue byJunkStatus = 0x1f;
>   const nsMsgViewSortTypeValue byAttachments = 0x20;
>   const nsMsgViewSortTypeValue byAccount = 0x21;
>   const nsMsgViewSortTypeValue byCustom = 0x22;
>   const nsMsgViewSortTypeValue byReceived = 0x23;
>+  const nsMsgViewSortTypeValue byTo = 0x24;
I don't think you need to change the uuid just to add a new constant.

Comment 69

[[e-mail]]

Comment on attachment 766268 [details] [diff] [review]
Patch (variant1)

>             <treecol id="senderCol" persist="hidden swappedhidden ordinal width" flex="4" 
>                      hidden="false" swappedhidden="true"
>                      label="&fromColumn.label;" tooltiptext="&fromColumn.tooltip;"/>
>             <splitter class="tree-splitter"/>
>             <treecol id="recipientCol" persist="hidden swappedhidden ordinal width" flex="4" 
>                      hidden="true" swappedhidden="false"
>-                     label="&recipientColumn.label;" tooltiptext="&recipientColumn.tooltip;"/>
>+                     label="&recipientsColumn.label;" tooltiptext="&recipientsColumn.tooltip;"/>
>+            <splitter class="tree-splitter"/>
>+            <treecol id="toCol" persist="hidden swappedhidden ordinal width" flex="4"
>+                     hidden="true" swappedhidden="false"
>+                     label="&toColumn.label;" tooltiptext="&toColumn.tooltip;"/>
I'm not sure that you need both recipients and to shown by default on sent messages...

Comment 70

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1)(revised)

This patch addresses the last comments.

Comment 71

[[e-mail]]

(In reply to comment #66)
> That looks reasonable for threaded views (I hope to try once my build
> completes) but I am concerned that it won't work for grouped by sort views
> and I have no idea where to check for saved searches.

I just tried Group By Sort and I'm sad to say that the To column also displayed the Total in Thread.

Comment 72

[[e-mail]]

Comment on attachment 777387 [details] [diff] [review]
Patch (variant1)(revised)

>+nsresult nsMsgDBView::commonFetchRecipients(nsIMsgDBHdr * aHdr, char * column,
>+                                            nsresult (NS_STDCALL nsIMsgDBHdr::*mimeDecoder)
>+                                                       (nsAString&),
>+                                            nsAString &aRecipientsString)
...
>+  nsresult commonFetchRecipients(nsIMsgDBHdr * aHdr, char *,
>+                                 nsresult (nsIMsgDBHdr::*mimeDecoder)(nsAString&),
>+                                 nsAString&);
I forgot to mention you need to change the header file too.

Comment 73

[[e-mail]]

Comment on attachment 777387 [details] [diff] [review]
Patch (variant1)(revised)

>     <splitter class="tree-splitter"/>
>     <treecol id="senderCol" persist="ordinal width hidden swappedhidden" flex="4" hidden="false" swappedhidden="true"
>              label="&fromColumn.label;" tooltiptext="&fromColumn.tooltip;"/>
>     <treecol id="recipientCol" persist="ordinal width hidden swappedhidden" flex="4" hidden="true" swappedhidden="false"
>-             label="&recipientColumn.label;" tooltiptext="&recipientColumn.tooltip;"/>
>+             label="&recipientsColumn.label;" tooltiptext="&recipientsColumn.tooltip;"/>
>+    <treecol id="toCol" persist="ordinal width hidden swappedhidden" flex="4" hidden="true" swappedhidden="false"
>+             label="&toColumn.label;" tooltiptext="&toColumn.tooltip;"/>
>     <splitter class="tree-splitter"/>
The lack of a splitter between the sender and recipient appears to be a bug in SeaMonkey... probably dates back to the time when they were combined into a single column and nobody remembered to put one in when they split it up. It would be handy if you could put the two needed splitters in, thanks!

I was looking into the swappedhidden attribute and you need a bit of extra code in SetSentFolderColumns in commandglue.js to get it to work in SeaMonkey, and I'd also like the toCol to default to swappedhidden="true"; by comparison Thunderbird does something different with its columns and you probably don't need swappedhidden at all.

Comment 74

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1) v2

Okay, I hope things are covered now.

Comment 75

[[e-mail]]

Comment on attachment 778665 [details] [diff] [review]
Patch (variant1) v2

This is really close! Only 5 issues remaining; one regression, one previously reported, three new typos found.

>+    else if (aColumnName[1] == 'o') // To column
>+    {
>+      rv = FetchToRecipients(msgHdr, aValue);
>+    }
In your rush to fix the grouped view (which now displays correctly), you broke the flat view, which now shows the to instead of leaving the total unread blank.

>+  nsresult commonFetchRecipients(nsIMsgDBHdr * aHdr, char *,
>+                                 nsresult (nsIMsgDBHdr::*mimeDecoder)(nsAString&),
>+                                 nsAString&);
Still no NS_STDCALL

>     case nsMsgViewSortType.byRecipient:
>       columnID = "recipientCol";
>       break;
>+    case nsMsgViewSortType.byRecipient:
>+      columnID = "toCol";
>+      break;
Wrong case label?

>+          <menuitem id="sortByToMenuitem" type="radio" name="sortby" label="&sortByToCmd.label;" accesskey="&sortByToCmd.accesskey;" oncommand="MsgSortThreadPand('byTo')"/>
Typo: MsgSortThreadPand?

>                      recipientCol: 'byRecipient',
>+                     toCol: 'byRecipient',
Wrong sort order?

Comment 76

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1) v2 (revised)

I hope there are no loose ends left this time.

Comment 77

[[e-mail]]

Comment on attachment 778850 [details] [diff] [review]
Patch (variant1) v2 (revised)

Everything seems to be in order. Thanks!

Comment 78

[Suyash Agarwal (:sshagarwal)]

Thanks :)

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/73a55e607a3a

Comment 80

[:aceman]

Suyash, when did you change the "recipient_names" cache name to "torecipient_names" and why?
You know I specifically wanted you to keep the name of the cache as it already contained the correct data for "To" in existing users databases. Now you force the database to regenerate the "To" string, store it in a new cache field and the old one stays behind orphaned (nothing clears it so it uselessly takes up space in the db). Is there some plan with this? Or was this overturned by any of the reviewers?

Comment 81

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #80)
> Suyash, when did you change the "recipient_names" cache name to
> "torecipient_names" and why?

Sorry, It slipped in after patch v2 (revised) in patch v2(revisited). :(
But, I think I can fix this in bug 699588.

Comment 82

[:aceman]

But I understand the new name is better (if you change it to toRecipient_names to match the style of others) but if you'd want to keep it you should find a way to delete the contents of the old cache ("recipient_names").
We still have to think up a plan for these caches, as bug 881074 and bug 890683 will needs some mechanism to flush the caches and regenerate their contents.

Comment 83

[Suyash Agarwal (:sshagarwal)]

Maybe we can ask users to clear their caches. Or leave it as is, hoping in near future they will, at some point of time, clear their cache. :)

Comment 84

[Jonathan Protzenko [:protz]]

I just found out that the nsIMsgDBHdr interface had been changed without any notice (as far as I can tell) on a public newsgroup, or without any discussion of compatibility plans. That's uncool for addon developers, as pretty much anyone has code that mentions mime2DecodedRecipients, and chances are high that this is going to cause a gigantic pain.

I don't have the energy to read 83 comments. Has this been discussed before?

I'm going to raise the issue on newsgroups see if that impacts other people.

Comment 85

[Günter]

+1 !
Yes, this will hurt Reminderfox extension!

From .idl
    1.31 -    readonly attribute AString mime2DecodedRecipients;
    1.32 +    /// MIME decoded recipients from the To header.
    1.33 +    readonly attribute AString mime2DecodedTo;
    1.34 +    /// MIME decoded recipients from the CC header.
    1.35 +    readonly attribute AString mime2DecodedCC;
    1.36 +    /// MIME decoded recipients from the BCC header.
    1.37 +    readonly attribute AString mime2DecodedBCC;
could be more than a one liner to also support TB versions up to TB25 also.

Great move :->
& thanks to Jonathan to raise this issue!

Comment 86

[Suyash Agarwal (:sshagarwal)]

(In reply to Jonathan Protzenko [:protz] from comment #84)
> I just found out that the nsIMsgDBHdr interface had been changed without any
> notice.
(In reply to Günter from comment #85)
> +1 !
> Yes, this will hurt Reminderfox extension!


Sorry for not notifying.
If you want/suggest/agree I can place back the mime2DecodedRecipients as an alias for mime2DecodedTo (as it was before) or rather make it contain To + Cc + Bcc internally, as it should be. That's what is being fixed in this bug.

Thanks.

Comment 87

[Jonathan Protzenko [:protz]]

I think that it's easy enough to fix; code that manipulates foo.mime2DecodedRecipients will just have to switch to using (foo.mime2DecodedTo || foo.mime2DecodedRecipients) which is fine imho. I, for one, don't intend to maintain backwards compatibility, so that's fine for me.

Comment 88

[Thomas D. (:thomas8)]

I'd strongly recommend using correct names for functions etc., because maintaining wrong names for backwards compatibility is a recipe for spaghetti code which we are trying to avoid because it's not maintainable. So:

- mime2DecodedRecipients should point to all recipients per this bug, i.e. To, CC, and BCC, and
- mime2DecodedTo should point to only To-recipients

I don't think that'll technically break addons (assuming we haven't altered the name of that mime2DecodedRecipients function), bc we still fill with a list of recipients. For the logical problem (which subset of recipients you want to see), addons have to chose what they really want, per Jonathan's comment 87.
It's a bug that should never have been there, but if we don't fix that now, we'll have bigger maintenance problems in the future than a number of addons potentially seeing a bigger subset of recipients than expected.

Comment 89

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #86)
> (In reply to Jonathan Protzenko [:protz] from comment #84)
> > I just found out that the nsIMsgDBHdr interface had been changed ...
>
> If you want/suggest/agree I can place back the mime2DecodedRecipients as an
> alias for mime2DecodedTo (as it was before)

Per my comment 88, I think that's the worst possibility, highly confusing maintenance nightmare

> or rather make it contain To +
> Cc + Bcc internally, as it should be. That's what is being fixed in this bug.

Creating that alias and fill with the full triple set of recipients sounds reasonable to me to not technically breaking addons that are currently using this property (and we have no way of telling if they actually want To or the full triple set to+cc+bcc which just wasn't available due to this bug, so we really need to do the correct thing as per property name).
 
> Thanks.

Comment 90

[Jonathan Protzenko [:protz]]

> or rather make it contain To +
> Cc + Bcc internally, as it should be. That's what is being fixed in this bug.

That also sounds like the best solution to me. The situation was inconsistent before, and people who wanted to differentiate to / cc / bcc probably resorted to ugly hacks (like me). This can only improve the situation.

I'm unclear as to what the "recipients" field is supposed to contain, though.

Comment 91

[:aceman]

So it seems that all the comments converged to this solution:
Reinstate back mime2DecodedRecipients but make it return all of To+Cc+Bcc. In that way all callers still work and get now the more correct result. If they need To recipients only, then can switch to the new mime2DecodedTo function.

Comment 92

[Suyash Agarwal (:sshagarwal)]

Done sir!

So, those who have issues with the patch for this bug, comment 91 will be implemented in the patch for bug 699588; till then, stay tuned.

Thanks.

Comment 93

[Mark Banner (:standard8)]

Suyash, I'm sorry to have to do this, but I have backed this out from the tree.

https://hg.mozilla.org/comm-central/rev/615a816c04c8

The reason being is that this actually broke some of the xpcshell unit tests, but was hidden due to the mass unit test bustage that we had at the time. As a result of my investigations, I found some cases where the removed mime2DecodedRecipients was still in the code base:

/mail/base/test/unit/resources/viewWrapperTestUtils.js
    line 293 -- dump(" Recipients: " + aMsgHdr.mime2DecodedRecipients + "\n");
/mail/components/search/SpotlightIntegration.js
    line 163 -- this._msgHdr.mime2DecodedRecipients));
/mailnews/base/test/unit/test_nsMsgDBView.js
    line 448 -- ensure_view_ordering(SortType.byRecipient, SortOrder.ascending, 'mime2DecodedRecipients');
/mailnews/db/gloda/modules/fundattr.js
    line 640 -- aGlodaMessage._indexRecipients = aMsgHdr.mime2DecodedRecipients;
/mailnews/test/resources/logHelper.js
    line 374 -- to: aObj.mime2DecodedRecipients,

As we're rapidly trying to get the tree green again, and the follow-up patch is not yet in bug 699588 (plus it is merge day on Monday), I thought it best to back out, so that the issues can be fully resolved.

Comment 94

[:aceman]

OK, then we can modify the patch here to keep mime2DecodedRecipients.

Comment 95

[Suyash Agarwal (:sshagarwal)]

(In reply to Mark Banner (:standard8) from comment #93)
> Suyash, I'm sorry to have to do this, but I have backed this out from the
> tree.
> As we're rapidly trying to get the tree green again, and the follow-up patch
> is not yet in bug 699588 (plus it is merge day on Monday), I thought it best
> to back out, so that the issues can be fully resolved.

I am sorry that my patch created hell out there :(
I am going uploading the fix very quickly.

Comment 96

[Suyash Agarwal (:sshagarwal)]

Attachment: Previous patch + regression fix

I have restored mime2DecodedRecipients that now correctly works on (to+cc+bcc) instead of just (to) as it used to do before.

Comment 97

[Suyash Agarwal (:sshagarwal)]

Attachment: Previous patch + regression fix v2

I couldn't find the place in commonFetchRecipients where I could remove the check.

Comment 98

[[e-mail]]

Comment on attachment 785248 [details] [diff] [review]
Previous patch + regression fix v2

>-      CopyUTF8toUTF16(cachedRecipients, aRecipientsString);
>+      CopyUTF8toUTF16(cachedRecipients, parsedRecipients);
>+
>+      if (!aRecipientsString.IsEmpty())
>+        aRecipientsString.AppendLiteral(", ");
>+
>+      aRecipientsString.Append(parsedRecipients);
Don't need this change any more.

>         // add ',' and end of each recipient
>         if (i != 0)
>-          aRecipientsString.Append(NS_LITERAL_STRING(","));
>-
>-        aRecipientsString.Append(recipient);
>+          parsedRecipients.AppendLiteral(", ");
>+
>+        parsedRecipients.Append(recipient);
Or this change.

>       }
>     }
>     PR_FREEIF(names);
>     PR_FREEIF(emailAddresses);
>   }
>   else
>-    aRecipientsString = unparsedRecipients;
>-
>-  UpdateCachedName(aHdr, "recipient_names", aRecipientsString);
>-
>+  {
>+    parsedRecipients = unparsedRecipients;
>+  }
>+  UpdateCachedName(aHdr, column, parsedRecipients);
>+
>+  if (!aRecipientsString.IsEmpty() && !parsedRecipients.IsEmpty())
>+    aRecipientsString.AppendLiteral(", ");
>+
>+  aRecipientsString.Append(parsedRecipients);
Or this change.

>+  nsresult rv = m_mdb->RowCellColumnToMime2DecodedString(GetMDBRow(),
>+                                                         m_mdb->m_recipientsColumnToken, toRecipients);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!toRecipients.IsEmpty())
>+    resultRecipients.Assign(toRecipients);
Don't need to check this, just fetch directly into resultRecipients.

>+  if (!toRecipients.IsEmpty() && !ccRecipients.IsEmpty())
Check resultRecipients instead of toRecipients.

>+  if ((!toRecipients.IsEmpty() || !ccRecipients.IsEmpty()) && !bccRecipients.IsEmpty())
Check resultRecipients instead of to & cc.

Comment 99

[:aceman]

Comment on attachment 785248 [details] [diff] [review]
Previous patch + regression fix v2

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

Sorry, I do not like this change. I think you change more of the original patch than is needed.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +606,5 @@
> +  return commonFetchRecipients(aHdr, "recipient_names",
> +                               &nsIMsgDBHdr::GetMime2DecodedRecipients,
> +                               aRecipientsString);
> +}
> +

You can fix the cache name now as the patch was backed out. Use "recipient_names" for the "to" recipients as we discussed. There will be no "cache" for the recipient list (To+Cc+bcc). And use the FetchRecipients as it was in the patch, not sure why you change it now like this.

::: mailnews/db/msgdb/src/nsMsgHdr.cpp
@@ +700,5 @@
> +    resultRecipients.AppendLiteral(", ");
> +    resultRecipients.Append(bccRecipients);
> +  }
> +  return NS_OK;
> +}

Why doesn't this just call GetMime2DecodedTo, GetMime2DecodedCC, GetMime2DecodedBCC and concatenate the results?
I think this is the only change needed in this backed out patch. I am not sure why you change the logic in FetchRecipients.

Comment 100

[Suyash Agarwal (:sshagarwal)]

Attachment: Previous patch + regression fix v3

This patch addresses comments 98 and 99.
Okay and tested.

If any minor issues are left, I hope we can get them covered in the followup bug about the cc and bcc columns to be added so that I can get this in and start working on them.

Thanks.

Comment 101

[[e-mail]]

(In reply to aceman from comment #99)
> Why doesn't this just call GetMime2DecodedTo, GetMime2DecodedCC,
> GetMime2DecodedBCC and concatenate the results?
Why should it when it can just call GetMime2DecodedRecipients? Plus that way we can remove a bunch of code changes in commonFetchRecipients that deals with this edge case.

Comment 102

[:aceman]

Comment on attachment 786358 [details] [diff] [review]
Previous patch + regression fix v3

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

Yes, I like this one better. But I haven't built it yet.

Comment 103

[[e-mail]]

(In reply to comment #101)
> (In reply to aceman from comment #99)
> > Why doesn't this just call GetMime2DecodedTo, GetMime2DecodedCC,
> > GetMime2DecodedBCC and concatenate the results?
> Why should it when it can just call GetMime2DecodedRecipients? Plus that way
> we can remove a bunch of code changes in commonFetchRecipients that deals
> with this edge case.
Sorry, I realised that you weren't quoting the part of the code that I thought you were quoting.

Comment 104

[[e-mail]]

Comment on attachment 778850 [details] [diff] [review]
Patch (variant1) v2 (revised)

>-    aRecipientsString = unparsedRecipients;
>-
>-  UpdateCachedName(aHdr, "recipient_names", aRecipientsString);
>-
>+  {
>+    parsedRecipients = unparsedRecipients;
>+  }
>+  UpdateCachedName(aHdr, column, parsedRecipients);
>+
>+  if (!aRecipientsString.IsEmpty() && !parsedRecipients.IsEmpty())
>+    aRecipientsString.AppendLiteral(", ");
>+
>+  aRecipientsString.Append(parsedRecipients);
Unfortunately this change went missing, and you need it because you've changed FetchRecipients back to the version that fetches each header separately instead of using GetMime2DecodedRecipients.

Comment 105

[Suyash Agarwal (:sshagarwal)]

Attachment: Previous patch + regression fix v4

Fixed the appending of comma.

Comment 106

[Kent James (:rkent)]

Comment on attachment 797877 [details] [diff] [review]
Previous patch + regression fix v4

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

1) I can see one flaw in this which will result in a regression. You are assuming that "recipients" is a synonym for "to" but that is not strictly true in the existing code. This code in nsParseMailbox.cpp will set "recipients" to CC if TO is blank:

  recipient  = (to.length         ? &to :
  cc.length         ? &cc :
  m_newsgroups.length ? &m_newsgroups :
  0);

The effect of this is that, if you look at your new "recipients" for the case where there are only Cc: recipients, then your new recipients will contain the Cc list twice. Because this new code makes "To" and "Cc" both first-class items, the need to set "recipients" to "Cc" is less, so I would be comfortable with eliminating that, but I would like other opinions for this edge case, and I would have to look deeper to understand if there would be any undesirable side effects of that.

2) A grouped-by sort by "recipients" gives garbage (multiple group headers with the same "recipient" value) - but it seemed to do that before as well, so I am not sure this is a regression. Pity, a grouping by "recipient" with each person having their own header would actually be quite useful. Not sure what do to about that here. Perhaps the correct thing to do is to simply not allow grouped-by sorts of recipients?

I hate to r- this after there has been so much discussion, but you did ask for my comments, so I have to give them.

::: mail/base/content/folderDisplay.js
@@ +398,5 @@
>      // recipient = To. You only care in outgoing folders.
>      recipientCol: function (viewWrapper) {
>        return viewWrapper.isOutgoingFolder;
>      },
> +    toCol: function(viewWrapper) {

This code is not needed, as this list is only traversed if the folder is in DEFAULT_COLUMNS which this is not.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +487,5 @@
>  
> +nsresult nsMsgDBView::commonFetchRecipients(nsIMsgDBHdr * aHdr, char * column,
> +                                            nsresult (NS_STDCALL nsIMsgDBHdr::*mimeDecoder)
> +                                                       (nsAString&),
> +                                            nsAString &aRecipientsString)

Convention is that function arguments start with "a". These used to, but you did not. So "aColumn" and "aMimeDecoder"

You need to check for null pointer values with NS_ENSURE_ARG_POINTER for all incoming pointers, including aHdr, column, and mimeDecoder.

Like Neil, I am also uncomfortable with this nsIMsgDBHdr::*mimeDecoder  I have no idea if it will work, but I will rely on Neil. Adding complexity like this when not really necessary, to support a feature that is not really that important and does not rely on this complexity for success, is just making work for all of us. Coding has long-since gone past the days when we thought it was a good idea to make code as short as possible, even if the complexity made it hard to interpret. This is unusual code for the Mozilla platform, and this feature does not justify that innovation.

But that is more for the future, if you ask me to do future reviews. I'm not going to make you reverse it if Neil has already approved it.

@@ +3859,5 @@
>          case nsMsgViewSortType::byRecipient:
>              *pFieldType = kCollationKey;
>              *pMaxLen = kMaxRecipientKey;
>              break;
> +        case nsMsgViewSortType::byTo:

This should have been placed right below ::byRecipient so that the values, which are identical, were not duplicated.

::: mailnews/base/src/nsMsgDBView.h
@@ +468,5 @@
>                                           nsIMsgFolder *srcFolder,
>                                           bool &moveMessages,
>                                           bool &changeReadState,
>                                           nsIMsgFolder** targetFolder);
> +  nsresult commonFetchRecipients(nsIMsgDBHdr * aHdr, char *,

Normal convention, which you should follow here, is "nsIMsgDBHdr *aHdr" and to include the parameter names like "char *aColumn", "nsAString& aRecipientsString", and (nsAString& aRecipient) (if that is correct)

::: mailnews/base/src/nsMsgGroupView.cpp
@@ +178,5 @@
>      case nsMsgViewSortType::byRecipient:
>        (void) msgHdr->GetRecipients(getter_Copies(cStringKey));
>        CopyASCIItoUTF16(cStringKey, aHashKey);
>        break;
> +    case nsMsgViewSortType::byTo:

Why do you have separate byRecipient and byTo with identical hash keys? That does not seem correct.

::: mailnews/db/msgdb/src/nsMsgHdr.cpp
@@ +705,1 @@
>    return m_mdb->RowCellColumnToMime2DecodedString(GetMDBRow(), m_mdb->m_recipientsColumnToken, resultRecipients);

This is where you are making the assumption that leads to the bug I mentioned in the overview. "recipients" is not "to" always.

Comment 107

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #106)
> Comment on attachment 797877 [details] [diff] [review]
> Previous patch + regression fix v4
> Review of attachment 797877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 1) I can see one flaw in this which will result in a regression. You are
> assuming that "recipients" is a synonym for "to" but that is not strictly
> true in the existing code. This code in nsParseMailbox.cpp will set
> "recipients" to CC if TO is blank:
> 
>   recipient  = (to.length         ? &to :
>   cc.length         ? &cc :
>   m_newsgroups.length ? &m_newsgroups :
>   0);
> 
> Because this new code makes "To" and "Cc" both
> first-class items, the need to set "recipients" to "Cc" is less, so I would
> be comfortable with eliminating that

+1. I'm not in a position to understand all the details here, but if everybody could assist Suyash to clear up the existing mess once and for all so that where we say "Recipients", we actually mean that, and by analogy for "To", "CC", "Bcc" etc.
If recipients column now has all of "To", "CC", "Bcc", it seems indeed unnecessary to push "CC" into that because it will already be there.

I'm wondering about that &m_newsgroups thing:
- for messages with "normal" recipients and/or "newsgroup" recipients, do we need "newsgroup recipients" to be shown in our collective "Recipients" column? (probably yes; pls comment).

Comment 108

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

Possible fix addressing the comment.

Comment 109

[Suyash Agarwal (:sshagarwal)]

Attachment: side patch

I was trying to modify the recipient value in nsParseMailbox.cpp, this works but fails for some edge cases, so if this is desired, please let me know, I'll make it work.

Comment 110

[[e-mail]]

(In reply to Kent James from comment #106)
> 1) I can see one flaw in this which will result in a regression. You are
> assuming that "recipients" is a synonym for "to" but that is not strictly
> true in the existing code. This code in nsParseMailbox.cpp will set
So, this is going to give us a headache with existing profiles, where the CC, BCC and Newsgroup headers are correct, but the Recipient header will show the To if it's set, otherwise the CC if it's set, otherwise the first newsgroup. Unfortunately I can't think of a good approach here.

> (From update of attachment 797877 [details] [diff] [review])
> > +nsresult nsMsgDBView::commonFetchRecipients(nsIMsgDBHdr * aHdr, char * column,
> > +                                            nsresult (NS_STDCALL nsIMsgDBHdr::*mimeDecoder)
> > +                                                       (nsAString&),
> > +                                            nsAString &aRecipientsString)
> 
> You need to check for null pointer values with NS_ENSURE_ARG_POINTER for all
> incoming pointers, including aHdr, column, and mimeDecoder.

Well, that's not strictly true, because this is an internal method and the callers already null-check their variables before passing them in.

> But that is more for the future, if you ask me to do future reviews. I'm not
> going to make you reverse it if Neil has already approved it.

It was only grudging approval...

Comment 111

[Kent James (:rkent)]

I think this is really close. Thanks for all of the time you have put into this. Let's keep plodding to get it finished!

Looking at old messages on an existing profile, the Recipients and To columns seem to be blank. This is because you are pulling from the db property 'toList' but it is empty for existing messages. What is the plan for fixing that? (That would also be true for the other new db properties) If you don't see this, then increment mail.displayname.version for a test profile that has messages before this patch.

So somehow, you need a method that will populate the To values when it is missing, as a one-time method. I don't know if there is a call in Mork to test for the existence of a particular column on a row, but if there was you could look for the existence of To, and if missing then calculate and set it from the existing fields. But you need some sort of plan, short of saying "Please rebuild all of your databases manually"

I think it would be best to discuss the plan rather than to write code first.

In localization, it seems to me that changing the .accesskey entry without changing the key is no different in principle (ie problematic) than changing the text itself. Is that true? We don't allow changing the text without changing the key. This is more a question for others, as I see that those changes have been approved.

NS_IMETHODIMP nsMsgHdr::SetToList(const char *toList)
+{
+        return SetStringColumn(toList, m_mdb->m_toListColumnToken);
+}
+
+NS_IMETHODIMP nsMsgHdr::SetToListArray(const char *names, const char *addresses, uint32_t numAddresses)
+{
+        nsresult ret;
+        nsAutoCString  toRecipients;
+    ret = BuildRecipientsFromArray(names, addresses, numAddresses, toRecipients);
+    if (NS_FAILED(ret))
+        return ret;
+
+        ret = SetToList(toRecipients.get());
+        return ret;
+}
+

nit: This new code should not copy the messed up tabbing of the old code. Just use standard 2 character indentation per level

NS_IMETHODIMP nsMsgHdr::GetMime2DecodedRecipients(nsAString &resultRecipients)
{
-  return m_mdb->RowCellColumnToMime2DecodedString(GetMDBRow(), m_mdb->m_recipientsColumnToken, resultRecipients);
+  nsString ccRecipients, bccRecipients;
+  nsresult rv = GetMime2DecodedTo(resultRecipients);

nit: nsAutoString

+        ret = m_HeaderAddressParser->ParseHeaderAddresses(toList->value,
+                                                        &names, &addresses,
+                                                        &numAddresses);
nit: &names line up with toList (like in if (ccList) below)

+        || sortType == nsMsgViewSortType.byRecipient|| sortType == nsMsgViewSortType.byTo
nit: if you touch this line, add the space before the "||"

Otherwise I did not see any issues with this patch, but you really cannot land it until you fix the issue of updating the existing db entries, so I am reluctantly forced to r- this.

Comment 112

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #111)
> I think this is really close. Thanks for all of the time you have put into
> this. Let's keep plodding to get it finished!

+1 :)

> In localization, it seems to me that changing the .accesskey entry without
> changing the key is no different in principle (ie problematic) than changing
> the text itself. Is that true?

No :)

> We don't allow changing the text without
> changing the key. This is more a question for others, as I see that those
> changes have been approved.

We only require changing the key when we change string values that localizers also need to change in their own languages, and due to an unbelievable design bug in the localization system, that's said to be the only way of notifying localizers about that. But accesskeys are *language-specific* (unique per language), so the fact that we change the accesskey in en-US has no implications whatsoever for other languages, because they have different strings and hence different access keys. E.g. where en-US has "To", German will have "An", so if we change our access key between "T" and "o" is not relevant for German, where the access keys can only be "A" or "n", so it's highly unlikely that they need to solve the same problem of conflicting access keys that we're solving for en-US localization.

Comment 113

[Suyash Agarwal (:sshagarwal)]

(Quoting Kent James (:rkent) from comment #111)
> Looking at old messages on an existing profile, the Recipients and To
> columns seem to be blank. This is because you are pulling from the db
> property 'toList' but it is empty for existing messages. What is the plan
> for fixing that? (That would also be true for the other new db properties)
> If you don't see this, then increment mail.displayname.version for a test
> profile that has messages before this patch.
> 
> So somehow, you need a method that will populate the To values when it is
> missing, as a one-time method. I don't know if there is a call in Mork to
> test for the existence of a particular column on a row, but if there was you
> could look for the existence of To, and if missing then calculate and set it
> from the existing fields. But you need some sort of plan, short of saying
> "Please rebuild all of your databases manually"
> 
> I think it would be best to discuss the plan rather than to write code first.

Any suggestions/opinions? :)

Comment 114

[opera wang]

Previously when we add new db properties, what's the best practice? Just increase the db version and force a rebuild db? If that true, then we might do the same this time.

After all, If we change the definition for Recipients from only To to To + CC + BCC, then the existing value stored in db would be invalid and a rebuild is needed anyway.

And thank Suyash for your great work.

Just my $0.02.

Comment 115

[Andreas Wagner [:TheOne] [use NI]]

It seems the current plan is to keep "nsIMsgHdr.mime2DecodedRecipients", if a future patch won't please add the keyword "addon-compat".

Resetting Target Milestone...

Comment 116

[Charles]

Just stumbled on this bug after answering a question about this on the mozilla-support list...

It looks like it is very close to being fixed... but may have stalled for some reason?

Any chance for inclusion in the next major Thunderbird version (I guess that would be 38)?

Thanks Suyash for working on this one!

Comment 117

[Suyash Agarwal (:sshagarwal)]

(In reply to Charles from comment #116)
> Just stumbled on this bug after answering a question about this on the
> mozilla-support list...
> 
> It looks like it is very close to being fixed... but may have stalled for
> some reason?
> 
> Any chance for inclusion in the next major Thunderbird version (I guess that
> would be 38)?
> 
> Thanks Suyash for working on this one!

Hi,
There's a mime headers' refactoring going on. So, I can work on this only after it has finished and landed.

Comment 118

[Charles]

Perfect, thanks, at least that explains the wait...

Is there a bug to track  the progress of this refactoring?

Thanks again!

Comment 119

[Charles]

Suyash?

(In reply to Charles from comment #118)
> Perfect, thanks, at least that explains the wait...
> 
> Is there a bug to track  the progress of this refactoring?
> 
> Thanks again!

Comment 120

[digitect]

12 year old bug. Is everybody just searching through thousands of messages like Gmail or does anyone still file and visually peruse To, CC, and BCC columns with a subject?