attachment size should be visible in compose window

RESOLVED FIXED in Thunderbird 3.3a2

Status

enhancement
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: rol3333, Assigned: squib)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 3.3a2
Dependency tree / graph
Bug Flags:
blocking-aviary1.5 -
wanted-thunderbird3 -
blocking-thunderbird2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gs], )

Attachments

(5 attachments, 6 obsolete attachments)

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.
confirming...
Assignee: ducarroz → ssu
Really confirming...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Attachement size should be vissible → Attachement size should be visible in compose window
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...
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.

*** This bug has been marked as a duplicate of 23332 ***
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Product: MailNews → Core
*** Bug 249060 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
*** Bug 298071 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
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.
(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?
(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?
This extension might provide a clue as to fixing this bug:
https://addons.mozilla.org/quicksearch.php?q=attachment+size&section=E
Flags: blocking-aviary2.0?
*** Bug 322916 has been marked as a duplicate of this bug. ***
Summary: Attachement size should be visible in compose window → attachment size should be visible in compose window
Flags: blocking-firefox2?
Flags: blocking-thunderbird2?
Flags: blocking-thunderbird2? → blocking-thunderbird2-
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...
Assignee: ssu0262 → nobody
Status: REOPENED → NEW
QA Contact: esther → composition
*** Bug 364893 has been marked as a duplicate of this bug. ***
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?
Duplicate of this bug: 405447
Hardware: PC → All
Duplicate of this bug: 399965
Product: Core → MailNews Core
Flags: wanted-thunderbird3?
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.
wanted‑thunderbird3-; doesn't mean we wouldn't accept a patch.
Flags: wanted-thunderbird3? → wanted-thunderbird3-
(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.
Probably a good start, yes. But someone still needs to to make a patch.
The extension link doesn't work, and the only extension I can find is for the viewing window, not the compose window.
I believe Worcester meant Attachment Sizes which can be downloaded here:
https://addons.mozilla.org/de/thunderbird/addon/878
(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?
(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.
(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.
"wanted‑thunderbird3-": driving decision not to spend time trying to drive this in for tb3.
Duplicate of this bug: 490212
Blocks: 516787
Duplicate of this bug: 559559
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.
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.
Status: NEW → ASSIGNED
Assignee: nobody → squibblyflabbetydoo
No longer blocks: 516787
Depends on: 516787
Blocks: 559559
(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)
See Also: → 430288
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.
(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.
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.
(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?
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.
Posted patch First cut (obsolete) — Splinter Review
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?).
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.
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.
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.
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
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.
Attachment #454138 - Attachment is obsolete: true
Attachment #479942 - Attachment description: Update patch (depends on patch in bug 559559) → Updated patch (depends on patch in bug 559559)
No longer blocks: 559559
Depends on: 559559
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...
Duplicate of this bug: 602595
Standard8, do you have any pointers for how to test adding attachments in Mozmill?
(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/).
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.
Attachment #479942 - Attachment is obsolete: true
Attachment #486479 - Flags: review?(jonathan.protzenko)
Agh, of course immediately after uploading this, I realize I haven't tested whether forwarding a message with attachments works (it doesn't).
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.
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.
Attachment #486479 - Attachment is obsolete: true
Attachment #486769 - Flags: review?(jonathan.protzenko)
Attachment #486479 - Flags: review?(jonathan.protzenko)
> 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!
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.
Blocks: 304835
I've shipped the review here http://reviews.visophyte.org/r/486769/ along with my comments on your patch.
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.
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.
Posted patch Address comments (obsolete) — Splinter Review
(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.
Attachment #486769 - Attachment is obsolete: true
Attachment #486769 - Flags: review?(jonathan.protzenko)
Attachment #488289 - Flags: review?(jonathan.protzenko)
Posted image 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.
Attachment #488602 - Flags: ui-review?(clarkbw)
(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 on attachment 488289 [details] [diff] [review]
Address comments

Works for me.
Attachment #488289 - Flags: review?(jonathan.protzenko) → review+
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?
Attachment #488289 - Flags: superreview?(bienvenu)
Comment on attachment 488289 [details] [diff] [review]
Address comments

sr=me if you fix this:

+}
\ No newline at end of file
Attachment #488289 - Flags: superreview?(bienvenu) → superreview+
Posted patch Fix missing newline (obsolete) — Splinter Review
This adds the missing newline in test-compose-helpers.js. Pulling r+/sr+ forward.
Attachment #488289 - Attachment is obsolete: true
Attachment #489902 - Flags: superreview+
Attachment #489902 - Flags: review+
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.
Keywords: checkin-needed
Sorry about that, I thought I'd save you the hassle of the last step :).
:clarkbw, ui-review ping?
Comment on attachment 488602 [details]
What it looks like

looks good, you're really putting that file size library to good use :)
Attachment #488602 - Flags: ui-review?(clarkbw) → ui-review+
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.
Keywords: checkin-needed
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...)
(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 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>.
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. :)
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 :)
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?
Attachment #489902 - Attachment is obsolete: true
Duplicate of this bug: 614830
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
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.
As far as I know, this is ok, so it's checkin-needed time!
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/6055a867c7a8
Status: ASSIGNED → RESOLVED
Closed: 15 years ago9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
Depends on: 662087
Depends on: 662088
Depends on: 661993
No longer depends on: 559559
Depends on: 559559
You need to log in before you can comment on or make changes to this bug.