The attachments pane should not show 0 length

RESOLVED FIXED in Thunderbird 5.0b1

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: alta88, Assigned: squib)

Tracking

unspecified
Thunderbird 5.0b1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
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.
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.
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.
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.
: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?
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.
(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.
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...
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #530225 - Flags: review?(bwinton)
Oops, I initialized a variable in the wrong place. Fixed.
Attachment #530225 - Attachment is obsolete: true
Attachment #530225 - Flags: review?(bwinton)
Attachment #530249 - Flags: review?(bwinton)
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.
But there are two places where size is shown: the total size in the header, and the size for each attachment in the list.
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 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.
Attachment #530249 - Flags: review?(bwinton)
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.
This patch seems to be closer to landing, so let's swap the order.

Thanks,
Blake.
Posted patch Make this apply to trunk (obsolete) — Splinter Review
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.
Attachment #530249 - Attachment is obsolete: true
Attachment #530490 - Flags: review?(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.
Attachment #530490 - Flags: review?(bwinton) → review+
Posted patch Add testsSplinter Review
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.
Attachment #530490 - Attachment is obsolete: true
Attachment #530882 - Flags: review+
Checked in: http://hg.mozilla.org/comm-central/rev/a7f971d73a3b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
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.)
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.
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.
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.
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. ;)
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.
Attachment #530890 - Flags: review?(bwinton)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #530890 - Flags: review?(bwinton) → review+
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.
Checked in: http://hg.mozilla.org/comm-central/rev/4965cf0026ca
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.