Show Total File Size of all attachments

RESOLVED FIXED in Thunderbird 3.3a3

Status

enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dfghjkjhg, Assigned: squib)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 3.3a3
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
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).
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Posted image The patch in action (obsolete) —
Here's what the patch looks like in action. :clarkbw, opinions?
Attachment #501849 - Flags: ui-review?(clarkbw)
Ok, this technically depends on my changes in bug 229224, so marking it as such for the time being.
Status: ASSIGNED → NEW
Depends on: 229224
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
Attachment #501849 - Flags: ui-review?(clarkbw) → ui-review+
(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.
Attachment #501848 - Attachment is obsolete: true
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.
Attachment #501849 - Attachment is obsolete: true
Attachment #503678 - Flags: ui-review?(clarkbw)
Comment on attachment 503678 [details]
What it looks like now

looks great!  thanks for looking into the l10n issues
Attachment #503678 - Flags: ui-review?(clarkbw) → ui-review+
Posted patch Add some comments (obsolete) — Splinter Review
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.
Attachment #503675 - Attachment is obsolete: true
Attachment #503951 - Flags: review?(dmose)
Status: NEW → ASSIGNED
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...
Attachment #503951 - Flags: review?(dmose)
(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
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.)
Posted patch Should apply on trunk now (obsolete) — Splinter Review
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.
Attachment #503951 - Attachment is obsolete: true
Attachment #504835 - Flags: review?(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...
Attachment #504835 - Flags: review?(dmose) → review?(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.
Attachment #504835 - Flags: review?(bwinton) → review+
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.
Posted patch Fix nits (obsolete) — Splinter Review
Fix nits, pulling r+ forward.
Attachment #504835 - Attachment is obsolete: true
Attachment #508584 - Flags: review+
Adding checkin-needed. :)
Keywords: checkin-needed
Whoops, I forgot to save one of the files before uploading the patch. Fixed.
Attachment #508584 - Attachment is obsolete: true
Attachment #508591 - Flags: review+
Checked in: http://hg.mozilla.org/comm-central/rev/c327ca4f9453
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Depends on: 663695
You need to log in before you can comment on or make changes to this bug.