User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b9pre) Gecko/20110104 Firefox/4.0b9pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110105 Thunderbird/3.3a2pre Would be nice if TB would show the total file size of all attachments, e. g. obove the attachment box right next to the word "ATTACHMENTS:" --> see attached screenshot --> yellow area could display the total file size of all attachments. Then the user could see how big the message will become in total and if the e-mail's size will exceed his e-mail-provider's message-size-restrictions. Reproducible: Always

This seems reasonable, and would tie in with bug 282068 (see attachment 498899 [details] for an example of what it will look like there).

Wow, looks great ! :-) Then obviously my bug report here will be completely covered by bug 282068.

Well, I'm not going to dupe to bug 282068, but if that bug gets fixed, I'll do the same with this one. Anyway, confirming.

Here's the patch to do this. Tests included. Not flagging this for review at the moment since it won't apply cleanly to trunk (it's dependent on another patch in my queue, and I don't want to mess around with the order of my patches unless someone knows a good way to do this).

Here's what the patch looks like in action. :clarkbw, opinions?

Ok, this technically depends on my changes in bug 229224, so marking it as such for the time being.

attachment 501849 [details] looks gorgeous to me, thanks a lot Jim! Could we include the number of attachments in compose window, too, as you did for received messages? So the headline of the list of attachments in compose window would look like this: 4 attachments /12MB/ I assume information about the number of attachments would be just as useful (if not more) for composing as it is for reading messages. Using negligible space, we would provide an intuitive, non-intrusive, yet valuable feedback which allows the user to double-check at a glance if the intended number of attachments has actually already been attached to the current composition. clarkbw, jim, opinions?

Comment on attachment 501849 [details] The patch in action This looks really good

(In reply to comment #8) > attachment 501849 [details] looks gorgeous to me, thanks a lot Jim! > > Could we include the number of attachments in compose window, too, as you did > for received messages? So the headline of the list of attachments in compose > window would look like this: > > 4 attachments /12MB/ Yeah, that works. For l10n the number would have to be in parens and probably afterward. Something like this: Attachments (#) /##MB/

(In reply to comment #10) > Yeah, that works. For l10n the number would have to be in parens and probably > afterward. Something like this: > > Attachments (#) /##MB/ Since I already have to update the "Attachments" string (to get rid of the colon), I could pretty easily put in a pluralizable string there, so it would read "1 Attachment /##MB/", "2 Attachments /##MB/", and so on.

This adds the number of attachments to the header as well.

This is what it looks like now. L10N should be ok as long as all the plural forms of "attachment" have at least one character in common (for the Alt-C accelerator), which I think is probably reasonable.

Comment on attachment 503678 [details] What it looks like now looks great! thanks for looking into the l10n issues

This is probably ready for review now. Some notes: * The color for the total file size of the attachments is hardcoded, since I copied it from the message header. Maybe GrayText is a better choice. * I made the composition string bundle a global in MsgComposeCommands.js, since it was used all over the place. A bunch of the changes (by line count) are related to that.

Jim how does this behaves on a RTL system ?

Comment on attachment 503951 [details] [diff] [review] Add some comments Clearing review, since I forgot that this depends on bug 229224, and that bug will probably take a bit more to get finished up...

(In reply to comment #16) > Jim how does this behaves on a RTL system ? Is there a way I can test RTL in Thunderbird? (I vaguely recall such a thing, but I have no idea how.)

(Quoting Simon Montagu, bug 594369 comment #27) > You can simulate an RTL build by setting intl.uidirection.en-US (or any other > locale) to "rtl" in about:config

there's also a Force RTL extension: https://addons.mozilla.org/en-US/thunderbird/addon/7438

Here's what it looks like in RTL. Note that the contents of the listbox will probably be updated in a separate patch (I used the text in parens because it was simpler and I'd need to update some XBL bindings to make it look better.)

Ok, I reordered my patch queue so this should apply now. :dmose, do you mind taking a look at this? See comment 15 for some notes about the patch.

Comment on attachment 504835 [details] [diff] [review] Should apply on trunk now I'm not having much time for reviews at the moment, so I'm hoping Blake can pick this one up...

Comment on attachment 504835 [details] [diff] [review] Should apply on trunk now >+++ b/mail/components/compose/content/MsgComposeCommands.js >@@ -100,27 +100,29 @@ var gMsgSubjectElement; >@@ -138,24 +140,26 @@ function InitializeGlobalVariables() >+ gComposeBundle = document.getElementById("bundle_composeMsgs"); I think you missed a couple of replacements at line 3070 and 3616, or was there a reason you couldn’t use this global there? >+++ b/mail/components/compose/content/messengercompose.xul >@@ -712,18 +712,21 @@ > <vbox id="attachments-box" collapsed="true"> >- <label id="attachmentBucketText" value="&attachments.label;" crop="right" >- accesskey="&attachments.accesskey;" control="attachmentBucket"/> >+ <hbox> >+ <label id="attachmentBucketCount" crop="right" >+ accesskey="&attachments.accesskey;" control="attachmentBucket"/> >+ <label id="attachmentBucketSize"/> >+ </hbox> If you’re already splitting this into two lines, there’s no real reason to make it go over 80 characters. I recommend going with: <label id="attachmentBucketCount" control="attachmentBucket" crop="right" accesskey="&attachments.accesskey;"/> >+++ b/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties >@@ -293,16 +293,19 @@ addressInvalid=%1$S is not a valid e-mai >+## Pluralizable string; #1 is the number of attachments >+attachmentCount=#1 attachment;#1 attachments I think this should be more like the attachmentReminderKeywordsMsgs block: # LOCALIZATION NOTE (attachmentCount): Semi-colon list of plural forms. # See: http://developer.mozilla.org/en/Localization_and_Plurals # #1 number of attachments attachmentCount=#1 attachment;#1 attachments >+++ b/mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd >@@ -204,17 +204,18 @@ >-<!ENTITY attachments.label "Attachments:"> >+<!-- this access key should correspond to the strings in attachmentCount in >+ composeMsgs.properties --> > <!ENTITY attachments.accesskey "c"> I think we should make this a localization note, too: <!--LOCALIZATION NOTE attachments.accesskey This access key should correspond to the strings in attachmentCount in composeMsgs.properties --> <!ENTITY attachments.accesskey "c"> >+++ b/mail/test/mozmill/composition/test-attachment.js >@@ -127,62 +131,113 @@ function check_attachment_size(controlle >+function check_total_attachment_size(controller, count) { >+ let bucket = controller.e('attachmentBucket'); >+ let nodes = bucket.getElementsByTagName('listitem'); >+ let sizeNode = controller.e('attachmentBucketSize'); We seem to use "s instead of 's in other places in this file, so should probably stick with that. So, I’ve mentioned a couple of things, but none of them are particularly serious or hard to do, so r=me with those nits fixed. Later, Blake.

Just wanted to make a note after my discussion w/ bwinton about this bug. I think the colors we have chosen for this patch will work and we should have a separate bug about using opacity levels to replace hard coded colors in at least the qute/gnomestripe themes. In that separate but we'll choose some opacity levels that work for various situations (some standards) and then make a sweet of certain colors values that need to be replaced with opacity. All of that seems best in a separate bug than here because it's going to be a little involved.

Fix nits, pulling r+ forward.

Adding checkin-needed. :)

Whoops, I forgot to save one of the files before uploading the patch. Fixed.

Checked in: http://hg.mozilla.org/comm-central/rev/c327ca4f9453