Open
Bug 699588
Opened 12 years ago
Updated 1 year ago
Message list: Implement missing (optional) columns for CC and BCC recipients (currently only column for TO recipients exists)
Categories
(Thunderbird :: Message Reader UI, enhancement)
Tracking
(Not tracked)
NEW
People
(Reporter: it, Unassigned)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
50.99 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•12 years ago
|
Severity: normal → enhancement
Comment 1•11 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Summary: Feature Request: Option to list CC and BCC recipients in columns in message list window → Message list: Implement missing columns for CC and BCC recipients (currently only column for To recipients exists)
Updated•11 years ago
|
Summary: Message list: Implement missing columns for CC and BCC recipients (currently only column for To recipients exists) → Message list: Implement missing (optional) columns for CC and BCC recipients (currently only column for TO recipients exists)
Updated•11 years ago
|
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Comment 3•11 years ago
|
||
The cc and bcc columns included.
Attachment #778940 -
Flags: ui-review?(bwinton)
Attachment #778940 -
Flags: feedback?(acelists)
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?
Attachment #778940 -
Flags: feedback?(acelists)
Comment 5•11 years ago
|
||
(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•11 years ago
|
||
Patch addressed in comment 5.
Attachment #778940 -
Attachment is obsolete: true
Attachment #778940 -
Flags: ui-review?(bwinton)
Attachment #780922 -
Flags: ui-review?(bwinton)
Attachment #780922 -
Flags: feedback?(acelists)
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.
Attachment #780922 -
Flags: feedback?(acelists) → feedback+
Comment 8•11 years ago
|
||
Made the changes suggested.
Attachment #780922 -
Attachment is obsolete: true
Attachment #780922 -
Flags: ui-review?(bwinton)
Attachment #785342 -
Flags: ui-review?(bwinton)
Attachment #785342 -
Flags: review?(neil)
Attachment #785342 -
Flags: feedback?(acelists)
Comment 9•11 years ago
|
||
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•10 years ago
|
||
Comment on attachment 785342 [details] [diff] [review] Patch v2 (Modified) Cancelling review requests as this patch has no meaning before bug 522886 lands. Thanks.
Attachment #785342 -
Flags: ui-review?(bwinton)
Attachment #785342 -
Flags: review?(neil)
Attachment #785342 -
Flags: feedback?(acelists)
Updated•7 years ago
|
Assignee: syshagarwal → nobody
Status: ASSIGNED → NEW
Comment 11•2 years ago
|
||
Still no CC or BCC column after 10 years?
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•