654702 - The attachments pane should not show 0 length

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[alta88]

Feed enclosures may or may not have a length published.  If they do, the fix in bug 646105 will expose that.  If not, 0 length should not be shown, as what we really mean is that we don't know it, not that it's 0.

Comment 1

[Jim Porter (:squib)]

A length of 0 means the file is 0 bytes. "Unknown" is represented as null in the reader, or -1 in the composer. This is more an issue with the fix in bug 646105 than with the attachment pane.

Comment 2

[alta88]

right now, the attachment pane in the reader shows 0 when the length is unknown (unpublished), which is incorrect, and clearly the responsibility of the attachment UI.

if an attachment does not have a 'size' attribute, the literal '0 bytes' should not be shown.

Comment 3

[Jim Porter (:squib)]

That's currently what happens. The attachment UI *already* handles the unknown size case for deleted attachments (and detached attachments where the external file has gone missing). The RSS case shouldn't be setting the size to zero if the size is actually unknown.

Comment 4

[Jim Porter (:squib)]

Looking at the feed example in bug 646105, I don't see a size at all for the attachment (this is before the patch). I do see a total size of 0 bytes (i.e. not for the individual attachment), but I don't really see that as a problem.

Comment 5

[Jim Porter (:squib)]

:bwinton, what's your opinion on showing "0 bytes" for the total size of all attachments when the size is unknown? I'm guessing that's what this bug is about (maybe?), but it's not very clear.

Corollary question: what to do if there are several attachments with known size and one with unknown?

Comment 6

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

My first instinct is that I would prefer "size unknown" for the total of a bunch of unknown sizes, and "At least 50k" for some known and some unknown attachments.

Comment 7

[alta88]

(In reply to comment #3)
> That's currently what happens. The attachment UI *already* handles the unknown
> size case for deleted attachments (and detached attachments where the external
> file has gone missing). The RSS case shouldn't be setting the size to zero if
> the size is actually unknown.

feed processing doesn't ever set 'size=0'.  if there is no 'length' published with the enclosure, there is never a 'size' added in the attachment mimepart.  the feed patch above will *now* add a 'size' iff 'length' is not null or 0 (see http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/content/feed-parser.js#200).

if the UI is always receiving 'size=0', it's likely in mime processing, which then is making the incorrect assumption.

Comment 8

[Jim Porter (:squib)]

Attachment: Handle cases where we don't know all the attachments' sizes

Here's a patch that does what comment 6 suggests. For reference, I made the strings all lower-case, since that's the standard for the message header and attachment bar.

I'm still not totally sure if this is what comment 0 is referring to, though...

Comment 9

[Jim Porter (:squib)]

Attachment: Handle cases where we don't know all the attachments' sizes, redux

Oops, I initialized a variable in the wrong place. Fixed.

Comment 10

[alta88]

not sure how to be more clear, but the patch is doing what it should.  if 'size=null' (for 1 or each 1 of many), don't show literal '0 bytes'.

personally, i don't think we should show any string at all if size is unknown, it adds to UI clutter.  what is important is not to show '0 bytes' incorrectly.

thanks.

Comment 11

[Jim Porter (:squib)]

But there are two places where size is shown: the total size in the header, and the size for each attachment in the list.

Comment 12

[alta88]

my suggestion is both places should have no string if 'size=null'.  for more than 1 attachment with at least 1 size != null then the "At least .." string in the header is most appropriate to convey the situation.  if 1 or more and all have a non null size, then the current method.

although feed parser will never pass a 'size=0' i suppose it could be legit for mail, but in that case it would correctly reflect an actual size of 0.

Comment 13

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

Comment on attachment 530249 [details] [diff] [review]
Handle cases where we don't know all the attachments' sizes, redux

Review of attachment 530249 [details] [diff] [review]:

This doesn't apply on trunk for me, so I'm going to clear the review flag until that's fixed.

Thanks,
Blake.

Comment 14

[Jim Porter (:squib)]

Sorry. The patch here is dependent on the second patch in bug 646032. I could swap the order if you like, or we can work on getting bug 646032 resolved.

Comment 15

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

This patch seems to be closer to landing, so let's swap the order.

Thanks,
Blake.

Comment 16

[Jim Porter (:squib)]

Attachment: Make this apply to trunk

Ok, here's a version that applies cleanly to trunk. I feel like I should test the total size when it's "unknown size" or "at least 123 KB", but I'm not sure how to make that work in a locale-neutral way in Mozmill.

Comment 17

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

Comment on attachment 530490 [details] [diff] [review]
Make this apply to trunk

Review of attachment 530490 [details] [diff] [review]:
-----------------------------------------------------------------

> I feel like I should test the total size when it's "unknown size"
> or "at least 123 KB", but I'm not sure how to make that work in a
> locale-neutral way in Mozmill.

I think we can assume en-US for the mozmill tests, at least for now.  Otherwise, perhaps you could check whether the string contains any digits or not?

Other than that, it looks good, so r=me.

Thanks,
Blake.

Comment 18

[Jim Porter (:squib)]

Attachment: Add tests

I realized that there's a really obvious way to test those cases without worrying about locales: just get the stringbundle and grab the strings in Mozmill. Pulling r+ forward; here's the patch for checkin.

Comment 19

[Jim Porter (:squib)]

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

Comment 20

[Jim Porter (:squib)]

Hmmm... the more I think about it, the less sure I am that "at least 123 KB" is the right thing to do for deleted attachments. Should we just say "123 KB" in those cases? (Luckily, the fix for this is really easy.)

Comment 21

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

Does "deleted attachments" also include detached attachments where the external file has gone missing?

And is there any way to get back a deleted attachment?

Thanks,
Blake.

Comment 22

[Jim Porter (:squib)]

By "deleted attachments", I don't mean detached attachments with a missing external file, just ones that were explicitly deleted via Thunderbird. As far as I know, it's impossible to "undelete" an attachment, but there is a bug report about adding the original size to the deleted attachment metadata: bug 419625. That might affect what we do with this when/if that bug is fixed.

Comment 23

[Jim Porter (:squib)]

Er, to clarify: we definitely *should* show "at least 123 KB" (or "unknown size") when there's a detached attachment with a missing external file.

Comment 24

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

Okay, if the attachment is actually deleted, then saying "123 KB" seems like the right thing.  (Also, that should be the only way we can ever see "0 bytes", if I understand correctly…  A test for that might be nice. ;)

Comment 25

[Jim Porter (:squib)]

Attachment: Treat deleted attachments as having a size of 0 instead of unknown

Here we go. I rearranged the tests a bit to be more explicit about what's being tested, but the non-test change is about as simple as it gets.

Comment 26

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

Comment on attachment 530890 [details] [diff] [review]
Treat deleted attachments as having a size of 0 instead of unknown

Review of attachment 530890 [details] [diff] [review]:
-----------------------------------------------------------------

I think I like this patch.  r=me with the comment below addressed.

::: mail/test/mozmill/attachment/test-attachment-size.js
@@ -326,1 +338,1 @@
> >        size += currSize;

So, this test will only check whether currSize is 0 or not, right?  If so, then we can remove the whole if test, and just add 0 to the size.  (Perhaps not, if nodes[i].getAttribute("attachmentSize") could return -1 or

Comment 27

[Jim Porter (:squib)]

It's really checking if currSize is NaN. When the attachment size is unknown, attachmentSize is the empty string, which when passed into parseInt, returns NaN. I could check it with isNaN(), but I had reverted that change (the first patch added isNaN), since it wasn't necessary anymore. I'll put it back for the sake of clarity, though.

Comment 28

[Jim Porter (:squib)]

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