655536 - MsgHdrToMimeMessage could understand detached attachments and feed enclosures

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[alta88]

Attachment: debug log

1. a part type Unknown is returned for a)detached attachments, b)feed enclosures.  thus allAttachments will not reflect what a user will see in the attachments pane when opening the actual message.

2. not enough info is returned in the part to distinguish Unknown (but valid) parts between detached/feed types and those from among non real attachments.  for example, X-Mozilla-Altered would be highly useful (bug 633837).  i mean, if it's got an X-Moz header then we can pretty much be sure it's a valid attachment..  oddly, deleted attachments are returned in a usefully constructed part.

3. allAttachments and allUserAttachments are quite unclear.  see the attached file with 4 test examples with some prettyPrints.

in general, is it expected that the caller needs to do further parsing with this function anyway?  the important thing is that the function needs to return exactly what the user sees on opening the message, otherwise confusion will reign.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Thank you for the feedback about the API!  The API is a result of the needs of existing consumers and enhancements they have made and is not intended to have equivalent behavior to the message reader.  The nominal consumers are gloda, multi-message/thread summaries, and Thunderbird Conversations.  I believe enigmail may also use the API.

I've re-titled the subject to match points 1 and 2 which would seem like a fix for either would be a fix for both.

Point 3 is not currently actionable as phrased and it sounds like it wants its own bug.  It's probably best to think of allAttachments as what gloda search wants to know (all of the attachments on a message, even if they are living on sub-messages that have been attached/forwarded), and what Thunderbird Conversations wants to know (all of the attachments on the root message, but none of the attachments on attached/forwarded messages).  Patches to improve the documentation are always welcome!

Comment 2

[alta88]

the direct quote from the doc is:

"MimeMessage instances now feature a brand new allUserAttachments property which accurately represents the attachments that will be displayed in the message reader."

toward this end, i restate that this api is Unusable.

i've attached a log that shows the api results, and i'm chosing my words carefully, as all over the map, and undecipherable (by me).  yet your suggestion is a doc patch?

your response is surreal.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Thanks for pasting the documentation snippet; I had not recently perused it (and did not write it).  Indeed, it sounds like that comment is writing checks the implementation can't cash.  I thought you were saying the comments were confusing rather than misleading.  Unfortunately, both detached attachments and RSS feeds are among the least tested bits of Thunderbird, so it's not surprising we get it wrong and didn't notice.

Anywho, the API is what it is, patches to make it work better (with tests) or to fix the comments to be less misleading (or less confusing) are greatly appreciated.

Comment 4

[Jonathan Protzenko [:protz]]

Hi,

I'm the one who wrote the comment. And yes, I don't exercise every feature in Thunderbird, I never use detached attachments, and I don't know what a feed enclosure is. I've only been working on Thunderbird for two years, sorry about that.

Now to give you an explanation, Gloda used to treat attached messages (.eml attachments) not as attachments, but as regular body parts, which was confusing. If message A has two pictures attached, and message B has message A attached,
- allAttachments for message B will show the two pictures and
- allUserAttachments for message B will show message A only.

This is what the goal of the API was, and I'm constantly trying to improve the API. If you have an alternative suggestion on how to reword that comment, I'll gladly include it. As to detached attachments, I'll look into it. Any constructive comments / patches / suggestions are welcome, as I'd love to see people use that API to examine messages (actually, that's the only reasonable way you can examine a message).

Comment 5

[Jonathan Protzenko [:protz]]

Attachment: WIP patch

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: Proper patch

You should first apply the patch from bug 564423, then from bug 527927, then this one. This patch is relatively simple, and works surprisingly well. There's a test in it too.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 550551 [details] [diff] [review]
Proper patch

Looks good, please spin off a bug on feed enclosures if one does not already exist and cite it in the bug.  Then retitle this bug to indicate the feed enclosure feature request has not been addressed.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

oh, do please remove the commented out dump statement though.

Comment 9

[Jonathan Protzenko [:protz]]

Just to make things clear, detached feed enclosures appear exactly the same way in the message source, with the same headers, so libmime notifies us in the same way. Since our test infrastructure is pretty nonexistent for RSS feeds, let's just say that the test I wrote for detached attachments in emails also covers the case of feed enclosures.

I'll add a comment for that in the test file.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #9)
> Just to make things clear, detached feed enclosures appear exactly the same
> way in the message source, with the same headers, so libmime notifies us in
> the same way. Since our test infrastructure is pretty nonexistent for RSS
> feeds, let's just say that the test I wrote for detached attachments in
> emails also covers the case of feed enclosures.
> 
> I'll add a comment for that in the test file.

agreed

Comment 11

[Jonathan Protzenko [:protz]]

So concerning the size information not being available for RSS feed enclosures, this is a different issue that warrants a separate bug, namely "make libmime understand the size= parameter of feed enclosures, and re-emit the size information later on".

Comment 12

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/b1d3493ea466

Please CC me on any followup bugs.