699588 - Message list: Implement missing (optional) columns for CC and BCC recipients (currently only column for TO recipients exists)

Status: NEW

(Thunderbird :: Message Reader UI, enhancement)

Description

[it]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1
Build ID: 20111101031108




Expected results:

In Thunderbird 7.0.1 it is possible to change the columns shown in the message list pane.  Right clicking on any of the column headers brings up a list of columns that can be selected for display.   Other mail clients I have used allow 'cc' and / or 'bcc' to be selected as columns so that it is easy to see who the message was sent to using the 'To:' field, and who was copied (or blind copied).  I find this very helpful.

At present there is only a 'Recipient' column and it is not clear from this who was sent the message directly and who was cc'd or bcc'd

Comment 1

[Thomas D. (:thomas8)]

Confirming. Can't see any reason not to offer (optional/separate) CC or BCC column while we do offer To column. Makes a nice set with bug 522885 and bug 522886.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

The cc and bcc columns included.

Comment 4

[:aceman]

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

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

I haven't compiled the patch yet but I have some comments just from reading it.

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +190,5 @@
>  <!ENTITY sortByRecipientsCmd.accesskey "c">
>  <!ENTITY sortByToCmd.label "To">
>  <!ENTITY sortByToCmd.accesskey "o">
> +<!ENTITY sortByCcCmd.label "Cc">
> +<!ENTITY sortByCcCmd.accesskey "k">

This will look awful (as "CC (k)"). But there are not many option left. Any chance you could use "c" key here and choose a different one for Recipients?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +596,4 @@
>  
>  nsresult nsMsgDBView::FetchToRecipients(nsIMsgDBHdr * aHdr, nsAString &aRecipientsString)
>  {
>    return commonFetchRecipients(aHdr, "torecipient_names",

Don't forget about creating the plan on what to do with this "torecipient_names" cache name.

@@ +620,5 @@
>    nsresult rv = FetchToRecipients(aHdr, aRecipientsString);
>    NS_ENSURE_SUCCESS(rv, rv);
>    rv = commonFetchRecipients(aHdr, "ccRecipient_names",
>                               &nsIMsgDBHdr::GetMime2DecodedCC,
>                               aRecipientsString);

OK, so now call FetchCcRecipients and FetchBccRecipients here instead of duplicating them. In the same way you use FetchToRecipients.

@@ +4161,5 @@
>  }
>  
> +nsresult nsMsgDBView::GetRecipientCollationKey(
> +                        nsIMsgDBHdr * msgHdr, nsresult (NS_STDCALL 
> +                                              nsMsgDBView::*fetchRecipients)

Does this need the nsMsgDBView:: prefix?

@@ +4163,5 @@
> +nsresult nsMsgDBView::GetRecipientCollationKey(
> +                        nsIMsgDBHdr * msgHdr, nsresult (NS_STDCALL 
> +                                              nsMsgDBView::*fetchRecipients)
> +                                              (nsIMsgDBHdr*, nsAString&),
> +                        nsString& recipients, nsCOMPtr <nsIMsgDatabase> &dbToUse)

Do you need to get dbToUse passed as argument? Can't it be a local variable?

@@ +4174,5 @@
> +    {
> +      rv = GetDBForHeader(msgHdr, getter_AddRefs(dbToUse));
> +      NS_ENSURE_SUCCESS(rv,rv);
> +    }
> +//    rv = dbToUse->CreateCollationKey(recipients, len, result);

Why is this commented out? If this is not called here then the whole function does not have much sense.

@@ +4214,5 @@
>      case nsMsgViewSortType::byTo:
>        {
>          nsString toRecipients;
> +        nsCOMPtr <nsIMsgDatabase> dbToUse;
> +        rv = GetRecipientCollationKey(msgHdr, &nsMsgDBView::FetchToRecipients,

Does this need the nsMsgDBView:: prefix?

@@ +4226,5 @@
> +    case nsMsgViewSortType::byCc:
> +      {
> +        nsString ccRecipients;
> +        nsCOMPtr <nsIMsgDatabase> dbToUse;
> +        rv = GetRecipientCollationKey(msgHdr, &nsMsgDBView::FetchCcRecipients,

Does this need the nsMsgDBView:: prefix?

@@ +4230,5 @@
> +        rv = GetRecipientCollationKey(msgHdr, &nsMsgDBView::FetchCcRecipients,
> +                                      ccRecipients, dbToUse);
> +        if (NS_SUCCEEDED(rv))
> +        {
> +          rv = dbToUse->CreateCollationKey(ccRecipients, len, result);

Shouldn't this be part of GetRecipientCollationKey ?

@@ +4238,5 @@
> +    case nsMsgViewSortType::byBcc:
> +      {
> +        nsString bccRecipients;
> +        nsCOMPtr <nsIMsgDatabase> dbToUse;
> +        rv = GetRecipientCollationKey(msgHdr, &nsMsgDBView::FetchBccRecipients,

Does this need the nsMsgDBView:: prefix?

@@ +4242,5 @@
> +        rv = GetRecipientCollationKey(msgHdr, &nsMsgDBView::FetchBccRecipients,
> +                                      bccRecipients, dbToUse);
> +        if (NS_SUCCEEDED(rv))
> +        {
> +          rv = dbToUse->CreateCollationKey(bccRecipients, len, result);

Shouldn't this be part of GetRecipientCollationKey ?

::: mailnews/base/src/nsMsgDBView.h
@@ +475,5 @@
>                                   nsresult (NS_STDCALL nsIMsgDBHdr::*mimeDecoder)
>                                              (nsAString&),
>                                   nsAString&);
> +  nsresult GetRecipientCollationKey(nsIMsgDBHdr * msgHdr,
> +                                    nsresult (NS_STDCALL nsMsgDBView::*fetchRecipients)

Does this need the nsMsgDBView:: prefix?

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

Is this correct? Do we really want the same GetRecipients() for all recipient types?

::: suite/locales/en-US/chrome/mailnews/messenger.dtd
@@ +164,5 @@
>  <!ENTITY sortByToCmd.accesskey "o">
> +<!ENTITY sortByCcCmd.label "Cc">
> +<!ENTITY sortByBccCmd.label "Bcc">
> +<!ENTITY sortByCcCmd.accesskey "k">
> +<!ENTITY sortByBccCmd.accesskey "b"> 

Put .accesskey next to .label. And see the "k" issue in the corresponding /mail file.

::: suite/mailnews/threadPane.js
@@ +284,5 @@
>           || sortType == nsMsgViewSortType.bySubject || sortType == nsMsgViewSortType.byTags
>           || sortType == nsMsgViewSortType.byStatus  || sortType == nsMsgViewSortType.byRecipient
>           || sortType == nsMsgViewSortType.byTo || sortType == nsMsgViewSortType.byAccount
> +         || sortType == nsMsgViewSortType.byFlagged || sortType == nsMsgViewSortType.byAttachments
> +         || sortType == nsMsgViewSortType.byCc || sortType == nsMsgViewSortType.byBcc);

Would it not be tidier to add Cc and Bcc after To in this expression and Flagged last?

Comment 5

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #4)
> Comment on attachment 778940 [details] [diff] [review]
> Patch
> ::: mail/locales/en-US/chrome/messenger/messenger.dtd
> @@ +190,5 @@
> >  <!ENTITY sortByRecipientsCmd.accesskey "c">
> >  <!ENTITY sortByToCmd.label "To">
> >  <!ENTITY sortByToCmd.accesskey "o">
> > +<!ENTITY sortByCcCmd.label "Cc">
> > +<!ENTITY sortByCcCmd.accesskey "k">
> 
> This will look awful (as "CC (k)"). But there are not many option left. Any
> chance you could use "c" key here and choose a different one for Recipients?
>
Fixed.
 
> ::: mailnews/base/src/nsMsgDBView.cpp
> @@ +596,4 @@
> >  
> >  nsresult nsMsgDBView::FetchToRecipients(nsIMsgDBHdr * aHdr, nsAString &aRecipientsString)
> >  {
> >    return commonFetchRecipients(aHdr, "torecipient_names",
> 
> Don't forget about creating the plan on what to do with this
> "torecipient_names" cache name.
>
reverted back to the name recipient_names
 
> @@ +620,5 @@
> >    nsresult rv = FetchToRecipients(aHdr, aRecipientsString);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >    rv = commonFetchRecipients(aHdr, "ccRecipient_names",
> >                               &nsIMsgDBHdr::GetMime2DecodedCC,
> >                               aRecipientsString);
> 
> OK, so now call FetchCcRecipients and FetchBccRecipients here instead of
> duplicating them. In the same way you use FetchToRecipients.
>
done. 
> @@ +4161,5 @@
> >  }
> >  
> > +nsresult nsMsgDBView::GetRecipientCollationKey(
> > +                        nsIMsgDBHdr * msgHdr, nsresult (NS_STDCALL 
> > +                                              nsMsgDBView::*fetchRecipients)
> 
> Does this need the nsMsgDBView:: prefix?
> 
I tried to work without the prefixes, but it wasn't working, so put them back.

> @@ +4163,5 @@
> > +nsresult nsMsgDBView::GetRecipientCollationKey(
> > +                        nsIMsgDBHdr * msgHdr, nsresult (NS_STDCALL 
> > +                                              nsMsgDBView::*fetchRecipients)
> > +                                              (nsIMsgDBHdr*, nsAString&),
> > +                        nsString& recipients, nsCOMPtr <nsIMsgDatabase> &dbToUse)
> 
> Do you need to get dbToUse passed as argument? Can't it be a local variable?
> 
> @@ +4174,5 @@
> > +    {
> > +      rv = GetDBForHeader(msgHdr, getter_AddRefs(dbToUse));
> > +      NS_ENSURE_SUCCESS(rv,rv);
> > +    }
> > +//    rv = dbToUse->CreateCollationKey(recipients, len, result);
> 
> Why is this commented out? If this is not called here then the whole
> function does not have much sense.
>
Fixed the GetRecipientCollationKey()

> ::: mailnews/base/src/nsMsgGroupView.cpp
> @@ +181,4 @@
> >      case nsMsgViewSortType::byTo:
> >        (void) msgHdr->GetRecipients(getter_Copies(cStringKey));
> >        CopyASCIItoUTF16(cStringKey, aHashKey);
> >        break;
> 
> Is this correct? Do we really want the same GetRecipients() for all
> recipient types?
>
I am not sure about this, it was the same in bug 522886 and noone objected, so I used it here as well.
 
Fixed the accesskey and ordering issues in suite/ as well.

Thanks.

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Patch addressed in comment 5.

Comment 7

[:aceman]

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

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

Nice, thanks. I will need to compile this now.

::: mail/base/content/mailWindowOverlay.js
@@ +313,5 @@
>        sortType == nsMsgViewSortType.bySubject || sortType == nsMsgViewSortType.byTags ||
>        sortType == nsMsgViewSortType.byRecipient || sortType == nsMsgViewSortType.byAccount ||
>        sortType == nsMsgViewSortType.byStatus || sortType == nsMsgViewSortType.byFlagged ||
> +      sortType == nsMsgViewSortType.byAttachments || sortType == nsMsgViewSortType.byTo ||
> +      sortType == nsMsgViewSortType.byCc || sortType == nsMsgViewSortType.byBcc);

I would have expected byRecipient, byTo, byCc, byBcc to be next to each other here.

@@ +368,5 @@
>                                    sortType == nsMsgViewSortType.byFlagged ||
>                                    sortType == nsMsgViewSortType.byAttachments ||
> +                                  sortType == nsMsgViewSortType.byTo ||
> +                                  sortType == nsMsgViewSortType.byCc ||
> +                                  sortType == nsMsgViewSortType.byBcc);

I would have expected byRecipient, byTo, byCc, byBcc to be next to each other here.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +595,5 @@
>  
>  
>  nsresult nsMsgDBView::FetchToRecipients(nsIMsgDBHdr * aHdr, nsAString &aRecipientsString)
>  {
> +  return commonFetchRecipients(aHdr, "recipient_names",

Ok, so just add a comment here that: 'The correct cache name should have been "torecipient_names" but it is kept as "recipient_names" for legacy reasons to pick up its contents in existing user profiles.'

(We will then think about ways to rename it and regenerate the contents in the other bugs.)

@@ +4169,5 @@
> +    nsCOMPtr <nsIMsgDatabase> dbToUse = m_db;
> +    if (!dbToUse) // probably search view
> +    {
> +      rv = GetDBForHeader(msgHdr, getter_AddRefs(dbToUse));
> +      NS_ENSURE_SUCCESS(rv,rv);

Space after "," .

@@ +4199,1 @@
>        }

It looks like you can now remove the { } on all 4 cases. It was probably to limit the scope of the local variables.

::: suite/mailnews/mailWindowOverlay.js
@@ +235,5 @@
>          || sortType == nsMsgViewSortType.byDate || sortType == nsMsgViewSortType.byReceived || sortType == nsMsgViewSortType.byPriority
>          || sortType == nsMsgViewSortType.bySubject || sortType == nsMsgViewSortType.byTags
>          || sortType == nsMsgViewSortType.byRecipient|| sortType == nsMsgViewSortType.byTo
> +        || sortType == nsMsgViewSortType.byFlagged || sortType == nsMsgViewSortType.byAttachments
> +        || sortType == nsMsgViewSortType.byCc || sortType == nsMsgViewSortType.byBcc);

I would have expected byReciepient, byTo, byCc, byBcc to be next to each other here.

::: suite/mailnews/threadPane.js
@@ +284,5 @@
>           || sortType == nsMsgViewSortType.bySubject || sortType == nsMsgViewSortType.byTags
>           || sortType == nsMsgViewSortType.byStatus  || sortType == nsMsgViewSortType.byRecipient
> +         || sortType == nsMsgViewSortType.byAccount || sortType == nsMsgViewSortType.byFlagged
> +         || sortType == nsMsgViewSortType.byAttachments || sortType == nsMsgViewSortType.byTo
> +         || sortType == nsMsgViewSortType.byCc || sortType == nsMsgViewSortType.byBcc);

I would have expected byReciepient, byTo, byCc, byBcc to be next to each other here.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2 (Modified)

Made the changes suggested.

Comment 9

[Suyash Agarwal (:sshagarwal)]

As the patch for bug 522886 is checked-out, this patch is to be applied atop the patch for bug 522886 for the review (before it gets checked-in again).

Thanks.

Comment 10

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 785342 [details] [diff] [review]
Patch v2 (Modified)

Cancelling review requests as this patch has no meaning before bug 522886 lands.

Thanks.

Comment 11

[digitect]

Still no CC or BCC column after 10 years?