623346 - Show Total File Size of all attachments

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[dfghjkjhg]

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

Comment 1

[dfghjkjhg]

Attachment: area for total file size of all attachments

Comment 2

[Jim Porter (:squib)]

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

Comment 3

[dfghjkjhg]

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

Comment 4

[Jim Porter (:squib)]

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.

Comment 5

[Jim Porter (:squib)]

Attachment: Add total size to attachment pane in composer

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).

Comment 6

[Jim Porter (:squib)]

Attachment: The patch in action

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

Comment 7

[Jim Porter (:squib)]

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

Comment 8

[Thomas D. (:thomas8)]

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 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 501849 [details]
The patch in action

This looks really good

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

(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/

Comment 11

[Jim Porter (:squib)]

(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.

Comment 12

[Jim Porter (:squib)]

Attachment: Add number of attachments to header

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

Comment 13

[Jim Porter (:squib)]

Attachment: What it looks like now

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 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 503678 [details]
What it looks like now

looks great!  thanks for looking into the l10n issues

Comment 15

[Jim Porter (:squib)]

Attachment: Add some comments

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.

Comment 16

[Ludovic Hirlimann [:Usul]]

Jim how does this behaves on a RTL system ?

Comment 17

[Jim Porter (:squib)]

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...

Comment 18

[Jim Porter (:squib)]

(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.)

Comment 19

[rsx11m]

(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

Comment 20

[Bryan Clark (DevTools PM) [:clarkbw]]

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

Comment 21

[Jim Porter (:squib)]

Attachment: What it look like in RTL

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.)

Comment 22

[Jim Porter (:squib)]

Attachment: Should apply on trunk now

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 23

[Dan Mosedale (:dmosedale, :dmose)]

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 24

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

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.

Comment 25

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 26

[Jim Porter (:squib)]

Attachment: Fix nits

Fix nits, pulling r+ forward.

Comment 27

[Jim Porter (:squib)]

Adding checkin-needed. :)

Comment 28

[Jim Porter (:squib)]

Attachment: Forgot to save a file...

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

Comment 29

[Mark Banner (:standard8)]

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