195702 - attachment size should be visible in compose window

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Rolandas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210

It would be nice to see the size of attached attachments in mail composition
window. Sometimes the size of attachments is restricted - for example - not
bigger than 1 MB or smth. When attaching multiple files it is difficult to
remember the size of attached files. 

Similar feature is implemented in Outlook Express. The size of attached files is
displayed together with the file name. 

This is probably the only thing that prevents me from using Mozilla Mail. 

I think that it would be nice to see TOTAL size of all attached files in the
mail composition window too.

Thank you!


Reproducible: Always

Steps to Reproduce:
1.Launch Mozilla mail
2.Click on compose new mail message
3.Click on attach and start attaching files. 

See the same in Outlook Express.

Comment 1

[Rolandas]

Attachment: no file atachement sizes

Comment 2

[Rolandas]

Attachment: atachement size visible in Outlook

Comment 3

[Jean-Francois Ducarroz]

confirming...

Comment 4

[Mike Cowperthwaite]

Really confirming...

Comment 5

[Roman J. Mashirov]

Isn't it duplicate of 189231, 26517?

Though, it would be very useful feature to limit the size of message to send in
prefs, and generate user-friendly message, if the message too large. It's one of
the main things preventing me from setting up mozilla for people in the office...

Comment 6

[Mike Cowperthwaite]

Not quite a dupe of either:  bug 189231 requests a display of the overall size 
of the mail being composed, including mail body; bug 26517 requests display of 
attachment size for received messages.

Comment 7

[Asa Dotzler [:asa]]

*** This bug has been marked as a duplicate of 23332 ***

Comment 8

[Mike Cowperthwaite]

I disagree about the dupe, Asa.  An attachment Properties dialog would be 
useful, particularly if things like Content-Type, charset, and 
Content-Disposition could be changed via the dialog (bug 192262 and related); 
but having the attachment size displayed _in situ_ (including a total) is what 
this bug is about.

However, like so many lesser Mozilloids, I cower in fear before your power and 
so won't reopen this bug; I hope you will.

Comment 9

[Jo Hermans]

*** Bug 249060 has been marked as a duplicate of this bug. ***

Comment 10

[Mike Cowperthwaite]

*** Bug 298071 has been marked as a duplicate of this bug. ***

Comment 11

[Lucas Nussbaum]

I don't think some of you realize how important this bug is. Teaching good
practices to email users is very important - stuff like : "prefer text email
messages", "answer below", "don't send 10MB emails with lots of pictures", etc.
It's very difficult to tell people not to send such mails if they have no way to
check whether their attachments are small enough or not.

Comment 12

[Worcester12345]

(In reply to comment #7)
> 
> *** This bug has been marked as a duplicate of 23332 ***

Shouldn't this one be resolved since it is a duplicate?

Comment 13

[Mike Cowperthwaite]

(In reply to comment #12)
> (In reply to comment #7)
> > 
> > *** This bug has been marked as a duplicate of 23332 ***
> 
> Shouldn't this one be resolved since it is a duplicate?

How about if you learn how to use the "view bug activity" link, above, and stop 
commenting where it isn't appropriate?

Comment 14

[Worcester12345]

This extension might provide a clue as to fixing this bug:
https://addons.mozilla.org/quicksearch.php?q=attachment+size&section=E

Comment 15

[Mike Cowperthwaite]

*** Bug 322916 has been marked as a duplicate of this bug. ***

Comment 16

[Andreas Stricker]

I'm another user who like to see this feature.

I think the size per file is not as important as the total size of all attachments.

Then there's the question wheter to show the original or the base64 encoded size. The latter may confuse users but the first may confuse users too, if the 10MB Mail don't get through the 10MB limit on server...

Comment 17

[Mike Cowperthwaite]

*** Bug 364893 has been marked as a duplicate of this bug. ***

Comment 18

[Majken Connor [:Kensie]]

The default windows picker view doesn't list file size either.  This would definitely be handy.

Was this denied blocking because it's not a priority or is it wanted but it was too late in the cycle?

Comment 21

[Vincent (caméléon)]

I put a flag "wanted‑thunderbird3"
I think this is an important enhancement that doesn't cost too much resources that we should have in TB3.

Comment 22

[Magnus Melin [:mkmelin]]

wanted‑thunderbird3-; doesn't mean we wouldn't accept a patch.

Comment 23

[Worcester12345]

(In reply to comment #14)
> This extension might provide a clue as to fixing this bug:
> https://addons.mozilla.org/quicksearch.php?q=attachment+size&section=E

(In reply to comment #22)
> wanted‑thunderbird3-; doesn't mean we wouldn't accept a patch.

Can't the code from the extension be used as the patch? It is pretty simple, and it works.

Comment 24

[Magnus Melin [:mkmelin]]

Probably a good start, yes. But someone still needs to to make a patch.

Comment 25

[Jon B]

The extension link doesn't work, and the only extension I can find is for the viewing window, not the compose window.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I believe Worcester meant Attachment Sizes which can be downloaded here:
https://addons.mozilla.org/de/thunderbird/addon/878

Comment 27

[Jon B]

(In reply to comment #26)
> I believe Worcester meant Attachment Sizes which can be downloaded here:
> https://addons.mozilla.org/de/thunderbird/addon/878

Oh, I see.  It's for the compose window and the viewing window.  Why can't this be built into Thunderbird to fix Bug 195702 and Bug 26517 at once?

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #27)
> Oh, I see.  It's for the compose window and the viewing window.  Why can't this
> be built into Thunderbird to fix Bug 195702 and Bug 26517 at once?

It needs someone to volunteer on it. At least the developer of that add-on should be contacted and point him to this bug. Hopefully the code can be reused to create a good first patch.

Comment 29

[Worcester12345]

(In reply to comment #22)
> wanted‑thunderbird3-; doesn't mean we wouldn't accept a patch.

It is not WANTED??? Don't you mean not blocking? I don't understand this nomenclature.

Comment 30

[Magnus Melin [:mkmelin]]

"wanted‑thunderbird3-": driving decision not to spend time trying to drive this in for tb3.

Comment 33

[u344514]

Hello everyone,

Thank you very much to all of the developers who put in such great work on Thunderbird, the latest v3 is lovely to use.  There is just one missing core feature that I am continually asked about by users, which is to provide the size of attachments in the compose window.  As more and more email servers implement email size constraints this feature becomes increasingly more important, I do hope it can be given some attention by one of the talented developers.

I originally posted bug 490212 in relation to this issue with screenshots, however I am very happy someone posted this feature request before I did.

Thank you in advance.

Comment 34

[Thomas D. (:thomas8)]

It's surprising that an excellent email client like TB can resist such a basic and obviously useful feature for such a long time... :)

...while pointing users to an addon that's currently not updated to work with TB 3.0 (yet has 94.000 downloads so far).

http://www.getsatisfaction.com/mozilla_messaging/topics/how_i_see_attachment_file_size
http://www.getsatisfaction.com/mozilla_messaging/topics/display_attachment_size_in_thunderbird
http://www.getsatisfaction.com/mozilla_messaging/topics/attachment_size_display_needed_in_tb
http://www.getsatisfaction.com/mozilla_messaging/topics/see_file_size_when_attach_it

http://forums.mozillazine.org/viewtopic.php?f=28&t=526576
http://groups.google.com/group/mozilla.support.thunderbird/browse_thread/thread/9c1624df5c733a53?fwc=2
http://redcarton.com/view.php?id=thunderbird_wishlist#9

Comment 35

[Jim Porter (:squib)]

Taking this for now. It can't be that hard, can it? (Famous last words.)

I'll probably go for something like attachment 374659 [details] unless someone posts a better idea.

Comment 36

[Thomas D. (:thomas8)]

(In reply to comment #35)
> Taking this for now. It can't be that hard, can it? (Famous last words.)

Jim, that's great!

> I'll probably go for something like attachment 374659 [details] unless 
> someone posts a better idea.

Maybe we can start out from looking at the real-life ui of "Attachment Sizes" Addon!

Original version on AMO, incl. user reviews and download statistics:
https://addons.mozilla.org/en-US/thunderbird/addon/878/

Current version (Version: 0.0.9 – works with:Thunderbird3.0 – 3.1.1pre)
http://fux.zuhage.de/attachment_sizes/

Jim, you could probably benefit from looking at/making use of the UI and source code of the addon.

So this could be attachment pane UI while composing:
+-----------------------------+
|Attachments:                 |
+-----------------------------+
|[#] Attached file1 (123 kb)  |
|[#] Attached file2 (25 MB)   |
|[#] Attached file2 (5 GB)    |
|                             |
+-----------------------------+

Attachment Sizes Addon has this nice option dialogue where you can choose between
a) use same unit for all attachments (show all sizes in bytes OR kbytes OR mbytes OR gbytes...)
b) use automatic units for different attachments (bytes, kbytes, mbytes, or gbytes as appropriate for each file's size).

-> Can someone post screenshots of a) and b) in action, with some files of different sizes as in the ASCII draft above?
-> We should then get UI-review from bryan (clarkbw)

For added motivation, a quote from one of AMO's user reviews:
> This feature is so basic and important that I didn't even remember that it is > not part of the core until updating to TB 3.0.

xref Bug 430288 - For composing, show additional tooltip information on mouseover of attachment filename (modify timestamp, file size)(In reply to comment #35)

Comment 37

[mad.engineer]

Don't mean to pile on here, but even Alpine which is a text based mail client, and considered by may as old school compared to TB and other clients, has this feature since day 1. See the attached screen shot.

Comment 38

[mad.engineer]

Attachment: Attachment size visible in Alpine

Comment 39

[Vincent (caméléon)]

(In reply to comment #36)
> (...)
> Attachment Sizes Addon has this nice option dialogue where you can choose
> between
> a) use same unit for all attachments (show all sizes in bytes OR kbytes OR
> mbytes OR gbytes...)
> b) use automatic units for different attachments (bytes, kbytes, mbytes, or
> gbytes as appropriate for each file's size).
In my opinion, this is not something that the average user wants to set.. Eventually, it could be easy to do with user pref or an add-on for advanced users.

Comment 40

[Susurrus]

I second having a sane default than relying on a hidden pref or add-on to change this. I think an auto-selecting scaling is appropriate for almost everyone.

Is it feature creep to give a total attachment size within the window in this bug? This is really where I see attachment size being most important: in determining the total size of the message and whether you can send it and the receiver can receive it.

Comment 41

[Thomas D. (:thomas8)]

(In reply to comment #40)
> I second having a sane default than relying on a hidden pref or add-on to
> change this.

Do you mean: I agree with having a sane default AND having a hidden pref or add-on to change it? (just clarifying the grammar)

> I think an auto-selecting scaling is appropriate for almost
> everyone.

Probably not: Different people, different needs. We'll definitely have people whining (for good reason) *whichever* default we chose:

if we choose a) as default - same unit for all sizes - we'll have people complaining about large numbers like 123,000,000 KB which they find hard to read and taking up too much space

if b) - dynamic units for different sizes - we'll have people complaining that they have no sane way of adding up or comparing the sizes because small numbers can represent large data (4 GB), and vice versa (1234 kB), which is visually confusing.

There's too many stories in BMO where customizability has been neglected and in the long run, it never works out and people end up discussing forever in fair defense of their personal needs. I think the time is ripe to foresee such problems and avoid them by providing customizable solutions right away.

Having said which, the better should not be the enemy of the good: If it turns out too much work for now, having sizes at all is surely better than not. I really couldn't tell if I'd prefer a) or b). For myself, maybe a). For the general public, probably b).

> Is it feature creep to give a total attachment size within the window in this
> bug? This is really where I see attachment size being most important: in
> determining the total size of the message and whether you can send it and the
> receiver can receive it.

It's an excellent idea (and much needed). If we can easily agree on an UI for this - why not. If we can't, let's have a new bug for this and get the basics done before we add tricky details.

UI proposal for showing total size of all attachments:

A1) total size on top-left,
no label
+-----------------------------+
|Attachments (5 GB):          |
+-----------------------------+
|[#] Attached file1 (123 kb)  |
|[#] Attached file2 (25 MB)   |
|[#] Attached file2 (5 GB)    |
+-----------------------------+

A2) top-left, with label
+------------------------------+
|Attachments (Total size: 5 GB)|
+------------------------------+
|[#] Attached file1 (123 kb)   |
|[#] Attached file2 (25 MB)    |
|[#] Attached file2 (5 GB)     |
|                              |
+------------------------------+

B1) bottom-left, no line (and italics?)
+------------------------------+
|Attachments:                  |
+------------------------------+
|[#] Attached file1 (123 kb)   |
|[#] Attached file2 (25 MB)    |
|[#] Attached file2 (5 GB)     |
|/Total size: 5 GB/            |
+------------------------------+

B1) bottom-left, with line (is it technically possible?)
+------------------------------+
|Attachments:                  |
+------------------------------+
|[#] Attached file1 (123 kb)   |
|[#] Attached file2 (25 MB)    |
|[#] Attached file2 (5 GB)     |
|---------------------------   |
| Total size: 5 GB             |
+------------------------------+

B2) Bottom-*right*, with line

|[#] Attached file2 (5 GB)     |
|---------------------------   |
|            Total size: 5 GB  |
+------------------------------+

C) "total size on demand"
User needs to select all attachments (ctrl+a) before seeing total size
C1) tooltip information
C2) status bar information
C3) context menu entry showing total size(?)
C4) implement properties dialogue, bug 23332
C5) ?

D) total size always visible, but not in attachment panel
D1) status bar
D2) ?

All of these on the assumption that we'll not have a tabular grid yet (more complicated), but that we are adding the size to the caption of each file name, so sizes will usually not be vertically aligned.

-> Could someone prepare some mockup screenshots of these?

Comment 42

[Susurrus]

My comments in #40 suggest a reasonable default and then also a hidden pref to change this to the other option. I would argue for the auto-formatting of the filesize because it's easier to see an individual files file size. If you want to know how they all compare you could sort then if a gridview (or whatever the proper name for it is) is used or you can look at the total size of all the attachments.

As for displaying the total file size I would vote for putting it up along with "Attachments:" text, with A1 being the preferred text: less to translate and allows for smaller windows. I would suggest against putting the file size at the bottom of the list of attachments as then the user has to scroll to find this information. I think if it's displayed as an additional line within the attachments view it should be at the top so it's initially viewable at least.

Comment 43

[Jim Porter (:squib)]

Attachment: First cut

A simple version of the patch, with a kinda lame UI: "attachment.txt (123 KB)". This is bad because if the attachment name is long, the size is off-screen.

So far I've tested this and it works with regular files and messages of all sorts. It doesn't work if you drag an attachment from another message onto it (I have no idea how to get the size from that), and it doesn't work if you attach a webpage (does that even have a size?).

Comment 44

[u344514]

Hello, I hope you don't mind me chiming in.  I would suggest having a look at ``ls'' the unix command it has a ``-h'' argument which seems to auto pick the right units for the file in question.

Comment 45

[Jim Porter (:squib)]

The file size units question is already answered for me, since I plan on using the funky fresh file size formatting function I wrote for the message list, and that automatically picks the right units. :)

If the only reason people have for wanting to use the same unit for all files is that they want to be able to mentally add the sizes, then we should be adding a total size instead.

Comment 46

[Vincent (caméléon)]

I would have put all the file size in Mo, because most of the ISP got a limit between 2-3Mo and 20MB. I don't care if my file is under 0,1MB, and email will never send attachment bigger than 50MB in a nearly future.

Comment 47

[mad.engineer]

Curious if this bug have fall off the radar for TB 3.2?. This looks stalled as there don't seem to be any progress to this bug lately. Thanks

Comment 48

[Jim Porter (:squib)]

Attachment: Updated patch (depends on patch in bug 559559)

Here's an updated version that reports sizes for attachments that were dragged-and-dropped from other messages. It also handles things in a slightly different (and in my opinion, saner) way. This patch expects attachment 479941 [details] [diff] [review] from bug 559559 to be already applied.

Comment 49

[Jim Porter (:squib)]

Oh also, does anyone actually know what is supposed to happen when you attach a web page from Thunderbird? Gmail's SMTP server doesn't seem to allow that, so I can't easily see what the resulting message looks like...

Comment 51

[Jim Porter (:squib)]

Standard8, do you have any pointers for how to test adding attachments in Mozmill?

Comment 52

[Mark Banner (:standard8)]

(In reply to comment #51)
> Standard8, do you have any pointers for how to test adding attachments in
> Mozmill?

The problem is the file dialog which tends to be native and so MozMill can't interact with it.

It looks like there's may be a couple of ways around this:

1) Write a shared-modules function that calls AddUrlAttachment or AddFileAttachment direct.
2) Create an email in a folder with an attachment and forward that email (although this would be slower than the first one).
3) Fake a drag and drop routine - I'm not sure if this is possible, the Firefox guys may have done it if we haven't (http://hg.mozilla.org/qa/mozmill-tests/).

Comment 53

[Jim Porter (:squib)]

Attachment: Now with tests (and some other changes)

Here's a version complete with some tests. It doesn't look like I can use Mozmill to simulate a drag-and-drop, so I only test adding files/web pages.

Jonathan, I hope you don't mind reviewing this, since you reviewed bug 559559, and this just builds on that a bit more.

Comment 54

[Jim Porter (:squib)]

Agh, of course immediately after uploading this, I realize I haven't tested whether forwarding a message with attachments works (it doesn't).

Comment 55

[Jim Porter (:squib)]

Even worse, it turns out that making forwarding work requires delving deep into the warrens of libmime. It'll probably take me a day or two to figure out where the attachment size isn't getting passed along through the call stack (I've fixed a few spots already), but I'm going to leave this old patch up for review, since the JS code shouldn't change at all.

Comment 56

[Jim Porter (:squib)]

Attachment: Make sure this works when forwarding messages with attachments

Here we go! I think this works in all cases now, except for forwarding emails as attachments, but I'm pretty sure that's bug 606699 again. One other issue is that somewhere, libmime decides to emit a bonus newline, so sizes aren't exactly the same, but that's also a known issue, since bug 561851 at least.

Anyway, Mozmill now tests attached files, attached web pages, and forwarded emails with raw- and b64-encoded attachments.

Comment 57

[Jonathan Protzenko [:protz]]

> Jonathan, I hope you don't mind reviewing this, since you reviewed bug 559559,
> and this just builds on that a bit more.

Absolutely not, I'm just impressed by your patch throughput! I'll probably review this in a couple days. Thanks for the hard work!

Comment 58

[Jim Porter (:squib)]

Pathological case I just discovered: on Linux (Ubuntu anyway), clicking "attach" and then *dragging* a file to the attachment area in the compose window doesn't pick up the attachment size. It looks like doing this causes a text/x-moz-url object to get dropped instead of an application/x-moz-file. I'm curious if this happens on other OSes as well.

Comment 59

[Jonathan Protzenko [:protz]]

I've shipped the review here http://reviews.visophyte.org/r/486769/ along with my comments on your patch.

Comment 60

[Jonathan Protzenko [:protz]]

Oh, and I forgot, r+ for me assuming you fix the various nits I pointed out. Do you mind if I create a followup bug for the missing tests (namely, checking the size when sending an email with an attached email, and when viewing an email with an attached email) that depends on bug 606699, and assign to you? That bug would actually fill the missing tests from bug 559559 as well as this one.

Comment 61

[Jonathan Protzenko [:protz]]

For posterity, here's a text/plain version of the review copy/pasted below from the reviewboard.

A very good patch, as usual. It's a shame there still are some edge-cases that
cannot handled properly (like, the never-ending issue of .eml attachments).
I'd like very much to see them tested even though the test is bound to fail,
just to make sure that I'm motivated to fix the oranges, but I'm afraid that's
not acceptable...

I'm hoping to fix the issue with libmime sometime this week, if all goes well.


on file: mail/base/content/msgHdrViewOverlay.js line 2199
>                                info + "\n" + attachment.displayName + "\n" + attachment.size);

While I trust you this is perfectly ok, maybe a small extra comment indicating
why it's legal to store for a moz-url something that's actually more than a
moz-url would be nice. (I mean, the flavor is x-moz-url but the data is
clearly more than a x-moz-url).


on file: mail/components/compose/content/MsgComposeCommands.js line 71
> var gMessenger = Components.classes["@mozilla.org/messenger;1"]

Good, thanks :)


on file: mail/components/compose/content/MsgComposeCommands.js line 2892
>   var nameAndSize = attachment.name

So this means that the value that's attached to the listbox will be null from
there on. Are you positive that there's no code that relies on this value
being a String? (I can imagine some worst case scenario where some code just
calls .length on the value, or whatever).


on file: mail/components/compose/content/MsgComposeCommands.js line 3092
>       var msgHdr = gMessenger.messageServiceFromURI(attachmentUrl).messageURIToMsgHdr(attachmentUrl);

Thanks for replacing this one with gMessenger.


on file: mail/components/compose/content/MsgComposeCommands.js line 3531
>               if (size != undefined)

This is question of style, but why this and not if (size)? If you want to
document the fact that it may be null, then remember that null == undefined,
so you might wanna use !==. Otherwise, I think if (size) is fine.


on file: mail/test/mozmill/composition/test-attachment.js line 180
> // XXX: Test attached emails and files pulled from other emails (this probably

What do you think is required for enabling this?


on file: mailnews/mime/src/mimedrft.cpp line 2020
>     ret = MimeDecoderWrite(mdd->decoder_data, buf, size, &outsize);

Good! I'm glad you were able to hook up onto the "count attachment sizes"
feature.

Comment 62

[Jim Porter (:squib)]

Attachment: Address comments

(In reply to comment #61)
> on file: mail/base/content/msgHdrViewOverlay.js line 2199
> >                                info + "\n" + attachment.displayName + "\n" + attachment.size);
> 
> While I trust you this is perfectly ok, maybe a small extra comment indicating
> why it's legal to store for a moz-url something that's actually more than a
> moz-url would be nice. (I mean, the flavor is x-moz-url but the data is
> clearly more than a x-moz-url).

I'm actually not sure why it's legal, but I suppose adding the size is no more illegal than adding the display name, which was there already. :)
 
> on file: mail/components/compose/content/MsgComposeCommands.js line 2892
> >   var nameAndSize = attachment.name
> 
> So this means that the value that's attached to the listbox will be null from
> there on. Are you positive that there's no code that relies on this value
> being a String? (I can imagine some worst case scenario where some code just
> calls .length on the value, or whatever).

Added the empty-string arg back. (It seems to work how it was, but I really just forgot about that when I changed it.)

> on file: mail/components/compose/content/MsgComposeCommands.js line 3092
> >       var msgHdr = gMessenger.messageServiceFromURI(attachmentUrl).messageURIToMsgHdr(attachmentUrl);
> 
> Thanks for replacing this one with gMessenger.
> 
> 
> on file: mail/components/compose/content/MsgComposeCommands.js line 3531
> >               if (size != undefined)
> 
> This is question of style, but why this and not if (size)? If you want to
> document the fact that it may be null, then remember that null == undefined,
> so you might wanna use !==. Otherwise, I think if (size) is fine.

I wanted to allow size to be 0 here. Using !== is probably a good idea, though.

> on file: mail/test/mozmill/composition/test-attachment.js line 180
> > // XXX: Test attached emails and files pulled from other emails (this probably
> 
> What do you think is required for enabling this?

I think Mozmill needs to be patched, since the documentation says that drag-and-drop simulation doesn't work for chrome, only for content. Of course, it's possible the documentation is out of date, but my (very) limited attempts in getting it to work didn't go well.

Comment 63

[Jim Porter (:squib)]

Attachment: What it looks like

I guess I should get UI review for this. :) Like with bug 559559, I don't think this is the ideal UI, but I think it's a step in the right direction.

Comment 64

[Jonathan Protzenko [:protz]]

(In reply to comment #62)
> I'm actually not sure why it's legal, but I suppose adding the size is no more
> illegal than adding the display name, which was there already. :)
Not sure either.
> 
> Added the empty-string arg back. (It seems to work how it was, but I really
> just forgot about that when I changed it.)
I have no doubt it works, but we never know. I've seen tricky bugs happen because someone would use .trim() on something that happens to be null, for instance.
> 
> I wanted to allow size to be 0 here. Using !== is probably a good idea, though.
> 
> 
> I think Mozmill needs to be patched, since the documentation says that
> drag-and-drop simulation doesn't work for chrome, only for content. Of course,
> it's possible the documentation is out of date, but my (very) limited attempts
> in getting it to work didn't go well.

Is there any bug for that? Have you pinged asuth (I think he's the mozmill guy) to see how complicated that would be?

Comment 65

[Jonathan Protzenko [:protz]]

Comment on attachment 488289 [details] [diff] [review]
Address comments

Works for me.

Comment 66

[Jim Porter (:squib)]

Comment on attachment 488289 [details] [diff] [review]
Address comments

:Bienvenu, since you sr'ed the corresponding patch for the message reader (bug 559559), do you think you could take a look at this too?

Comment 67

[David :Bienvenu]

Comment on attachment 488289 [details] [diff] [review]
Address comments

sr=me if you fix this:

+}
\ No newline at end of file

Comment 68

[Jim Porter (:squib)]

Attachment: Fix missing newline

This adds the missing newline in test-compose-helpers.js. Pulling r+/sr+ forward.

Comment 69

[Jim Porter (:squib)]

Removing checkin-needed for now, since I forgot to get ui-review earlier, and I'm waiting for clarkbw to take a look at it.

Comment 70

[Jonathan Protzenko [:protz]]

Sorry about that, I thought I'd save you the hassle of the last step :).

Comment 71

[Jim Porter (:squib)]

:clarkbw, ui-review ping?

Comment 72

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

Comment on attachment 488602 [details]
What it looks like

looks good, you're really putting that file size library to good use :)

Comment 73

[Mark Banner (:standard8)]

I landed this, but had to back it out again due to test bustage on Windows (Mac was fine):

http://hg.mozilla.org/comm-central/rev/212f044b3d86
http://hg.mozilla.org/comm-central/rev/c785aae4b1d1

TEST-UNEXPECTED-FAIL | e:\buildbot\comm-central-trunk-win32-opt-unittest-mozmill\build\mozmill\composition\test-attachment.js | test_file_attachment
  EXCEPTION: no message!

Typically this kind of message happens if there's some weird exception in the test function (test_file_attachment) that mozmill doesn't handle properly.

Comment 74

[Jim Porter (:squib)]

Argh, Windows. What's the best way to test stuff on Windows? Tryserver? (I have no idea how that works or if it's even the right thing...)

Comment 75

[Mark Banner (:standard8)]

(In reply to comment #74)
> Argh, Windows. What's the best way to test stuff on Windows? Tryserver? (I have
> no idea how that works or if it's even the right thing...)

sid0 may be able to help.

Comment 76

[Rain [:rain1] (inactive)]

Comment on attachment 489902 [details] [diff] [review]
Fix missing newline

>+function test_file_attachment() {
>+  let cwc = open_compose_new_mail();
>+  let attachment = Cc["@mozilla.org/messengercompose/attachment;1"]
>+                     .createInstance(Ci.nsIMsgAttachment);
>+  attachment.url = "file:///some/file/here.txt";
>+
>+  attachment.size = 1234;
>+  add_attachment(cwc, attachment);

I haven't run with this patch yet, but my guess would be that that non-Windows URL wouldn't make the moz-icon resolution code too happy on Windows, around <http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2910>.

Comment 77

[Jim Porter (:squib)]

Hm, that could be an issue. If you could try changing that to whatever URL format Windows likes (or if someone could point me to some docs on how to send a patch to the try server, or whatever it's called), that would be great. :)

Comment 78

[Jonathan Protzenko [:protz]]

I think you can use a runtime detection technique as described there https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests. Windows uses something along the lines of file:///C:/Documents/duh.txt

Concerning access to the try server, you can send me a patch and I'll happily push it for you and send you a pointer to the logs. Otherwise, you should probably discuss this with :Standard8 on IRC. http://www.mozilla.org/hacking/commit-access-policy/ might be worth a read too :)

Comment 79

[Jim Porter (:squib)]

Attachment: Fix Windows failures (maybe)

This might fix Windows issues. I'm not sure if Windows URIs should be "file:///C:/foo" or "file:///C|/foo", but the former is apparently the canonical version, so let's go with that for now.

Jonathan, do you mind pushing this to the test servers to see if things are ok now?

Comment 81

[Jonathan Protzenko [:protz]]

Jim, I've just pushed your patch to try. I've based your patch on the Thunderbird 3.3 a1 release tag, which reduces the risk of seeing unrelated failures (well, I hope).

Results should be available here in a couple hours.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry

Comment 82

[Jonathan Protzenko [:protz]]

Sorry for the delay, I managed to push this properly to the tryservers this time.

Windows: 100% green.
Linux: unrelated failures, see http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1291029251.1291030083.28016.gz
OSX: unrelated XPCShell failure, green mozmill.

I think it's alright now. I'll let you mark this as checkin-needed if you feel it's ok on your side.

Comment 83

[Jim Porter (:squib)]

As far as I know, this is ok, so it's checkin-needed time!

Comment 84

[Mark Banner (:standard8)]

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

Comment 85

[Worcester12345]

For reference, the new links:

https://addons.mozilla.org/en-US/thunderbird/addon/attachment-sizes/

also:

https://addons.mozilla.org/en-US/thunderbird/addon/mail-size-report/