209629 - Drag & drop of messages into composition's attachment pane uses same mime filename="Attached Message" for *all* forwarded messages, instead of "unique subject of forwarded msg.eml" (as we do for non-drag forwarding) [mime content-type:message/rfc822]

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Henrik Gemal]

if you drag a mail into attachment part of compose windows it should show the subject not just "attached message"

if you forward an mail it will show the subject, so should dragging.

20030613

Comment 1

[Mike Cowperthwaite]

xref bug 168926 -- maybe a dupe, maybe not.

Comment 2

[Mike Cowperthwaite]

See also bug 109288.

Comment 3

[Mike Cowperthwaite]

Also note: the ".eml" extension is not included, causing the same problems as 
seen in bug 220646.

Comment 4

[Thomas D. (:thomas8)]

Surprisingly, this bug is still around in TB 1.5. Perhaps "drag&drop mail msg into composer => forward msg as attachment" and "forward msg as attachment" should share the same code immediately after the drop? Something like a function add_msg_as_attachment(msg_path)? Any reason for the different handling after tb knows the path of the msg to be attached? In BOTH cases, a link to the attached msg is included, of the pattern "mailbox-message://profileid@profilefoldername/mailfoldername/msgid". From a programming point of view, I don't understand why there seems to be redundant/different code for the same action... but maybe I'm wrong?

Comment 6

[Seungbeom Kim]

This bug was reported 5 years ago, and hasn't been fixed yet. Too bad.
I guess it would not be very hard to fix it, for I could even do the simple but tedious job of saving the message as a draft, closing Thunderbird, and editing the Drafts file manually. I wish I could fix it myself.

Comment 8

[Thomas D. (:thomas8)]

OMG Dragging messages into attachment box is the only way of attaching messages, and the UX is very poor:
User will not have any indication whatsoever which messages he really attached.
When attaching 2 or more messages, this gets very confusing.

An answer to comment 4 would also be appreciated, because we have code for the same action that can do better, but it looks we're branching the code at the wrong points.

Comment 9

[Seungbeom Kim]

Another issue is that the forward message doesn't get the "Forwarded" flag when dragged and dropped into attachment.

(You cannot mark it as Forwarded even manually in Thunderbird; you need help from another client like mutt.)

Comment 10

[Thomas D. (:thomas8)]

This isn't minor.

- Drag multiple messages into composition's attachment pane:
-> they will all have "Attached Message" (without .eml) as filename
-> no way of veryfying from primary UI which msgs have been attached, can't tell those clones apart!
- worse when receiving such msg with multiple msgs attached:
- "save all" will attempt to save *all* attached msgs into *one* file, because they all share the same filename ("Attached message"). Perfectly useless, and, well, pretty poor UX.

Given that we have correct and working code when attaching multiple messages as attachment with a *non-drag* method (subject1.eml, subject2.eml, subject3.eml): Perhaps we're branching or duplicating code wrongly, and perhaps this isn't all that hard to fix for someone familiar with the code? Even if it means adding the subjects to the drag flavors, how hard would it be to add the real subject instead of the placeholder subject?

Comment 11

[Emre]

I'm using Thunderbird in corporate environment and this is really annoying not to see attached messages with their subjects as their name, rather than just seeing "attachedmessage.eml" or "forwardedmessage.eml". 
And indeed, much worse when you have multiple of them attached. 

This is very common practice in the business world.

Comment 12

[Thomas D. (:thomas8)]

Emre, do you know any developer from your business or private connections who would be willing to fix this for you? Any such contributions of coding manpower are most welcome. Given that TB is now maintained by the community, that's your best bet to get this fixed (and probably the only one).

As a workaround:
Ctrl+Click or Shift+click to select messages that you want to forward, then right-click, forward as attachments. which will preserve email subjects.
If messages for forwarding are not near each other, use searches first, or copy messages in a temporary folder before forwarding them.

Comment 13

[Thomas D. (:thomas8)]

Per comment 10, this is actually quite bad, in spite of the workaround, and the workaround will be way too clumsy for cross-folder cases. -> major.

ux-trust: this has an impact on user trust of TB product, because users will come to expect that TB can't handle dragging.

ux-consistency: As explained in comment 10, there's no reason why dragging mails into attachments pane should yield different result over "forward as attachment".

ux-error-prevention: As explained in comment 10, when receiving a msg with 10 attachments all named "attached message" (due to this bug), these messages will mutually overwrite themselves when trying to save them as .eml files.

Comment 14

[Emre]

Thomas, unfortunately i don't think my company would do anything for this. We can hopefully find a community developer.
The workaround are not realy usable as it's cumbersome and generally these messages are attached when replying to another thread with some history.

Comment 15

[Seungbeom Kim]

What you can do when you want an attached message in reply to another is: open the composition window for the reply as usual, open another for the attachment as described in comment 12, and drag the attachment from the latter window and drop it to the former. I see that the correct filename is preserved then.

Yes, it's cumbersome, but this is what I'll do when I really care about the filename. (The only alternative I know is to save the message in "Unsent Messages" via "Send Later", quit Thunderbird, edit the message file with an editor, reopen Thunderbird, and do "Send Unsent Messages"... which is much more trouble.)

Comment 16

[Thomas D. (:thomas8)]

Sorry to repeat myself, but I just ran into this again and the UX of this is really quite ghastly (see my comment 10, and comment 13)!!! Worse, this is the only method to add existing messages *after* having started your composition. I.e. when forwarding messages, if you forget some, you'll have a hard time adding more because they all look alike (you can have n-times "Attached Message"), and get sent like that. Not even .eml extension. Ugly. True dogfood.

I think finding the offending code starts from here where we're assigning the name:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4221-4225
    // For security reasons, don't allow *-message:// uris to leak out.
    // We don't want to reveal the .slt path (for mailbox://), or the username
    // or hostname.
    if (/^mailbox-message:|^imap-message:|^news-message:/i.test(attachment.name))
      attachment.name = getComposeBundle().getString("messageAttachmentSafeName");

I conclude from that that the message URL gets passed as attachment.name, which should never happen in the first place.
So we're getting it wrong before going into this function, AddAttachments(aAttachments, aCallback).
Next step is to find the drag and drop code which ends up here, i.e. callers for AddAttachments(...).

Comment 17

[Thomas D. (:thomas8)]

Trying to trace the problem going backwards:

This is the offending caller:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4951
>        AddAttachments(attachments);

And this is the drop handler containing this call:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4851-4985
> // we can drag and drop addresses, files, messages and urls into the compose envelope
> var envelopeDragObserver = {
> ...
> }

Attachment name in drop code is generated here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4926
>            attachment.name = prettyName;

Apparently, prettyName is only set for "else" case, i.e. for
item.flavour.contentType == "text/x-moz-url"
>           else
>               ...
>               prettyName = pieces[1];

But we don't set prettyName here:
>          else if (item.flavour.contentType == "text/x-moz-message")
>          {
>            size = gMessenger.messageServiceFromURI(rawData)
>                             .messageURIToMsgHdr(rawData).messageSize;
          }
Assuming that dropped messages have contentType "text/x-moz-message", there's no pretty name defined (but maybe dropped messages could have contentType == "text/x-moz-url"?)
If that's correct, than we'd just have to get the pretty name in a similar way we get size in the code just above this.
Maybe where messageSize is available, messageSubject could also be available?

Comment 18

[Thomas D. (:thomas8)]

Taking.

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks. Yes "Attached Message" is really suboptimal. Let us know if you need help. I'm new to this bug, so I don't have any insights. I'd check how messageAttachmentSafeName=Attached Message is used.

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

Fixing this was a one-liner, and quite simple to track per comment 16, comment 17. So 15 years after this bug was filed... tadaaaa!

>          else if (item.flavour.contentType == "text/x-moz-message")
>          {
>            size = gMessenger.messageServiceFromURI(rawData)
>                             .messageURIToMsgHdr(rawData).messageSize;
prettyName = gMessenger.messageServiceFromURI(rawData)
                              .messageURIToMsgHdr(rawData).mime2DecodedSubject + ".eml";

>          }

That's all which is needed for minimal fix.

I figured this would be too simple for Jörg's review ;)
For a bit of brain jogging, I couldn't resist cleaning up the needlessly nested code in this corner, and sort out variable declarations to match reality and coding style.

There's an existing bug not caused by this patch that certain high-level UTF8 characters in subject (or body, didn't check) will cause an error:
772 URIError: malformed URI sequence 1 msgMail3PaneWindow.js:1457:38
	ThreadPaneOnDragStart chrome://messenger/content/msgMail3PaneWindow.js:1457:38
	ondragstart chrome://messenger/content/messenger.xul:1:1

That error also occurs with "Attached Message" as attachment name, because it's in ondragstart.

Otherwise, from my tests this seems to work flawlessly as expected.

I did not test URLs.

Comment 21

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #20)
> There's an existing bug not caused by this patch that certain high-level
> UTF8 characters in subject (or body, didn't check) will cause an error:
> 772 URIError: malformed URI sequence 1 msgMail3PaneWindow.js:1457:38
> 	ThreadPaneOnDragStart
> chrome://messenger/content/msgMail3PaneWindow.js:1457:38
> 	ondragstart chrome://messenger/content/messenger.xul:1:1
> 
> That error also occurs with "Attached Message" as attachment name, because
> it's in ondragstart.

That existing error cause any subsequent selected messages NOT to be attached when dropping, but surprisingly the offending message itself seems to be attached correctly.

Comment 22

[Thomas D. (:thomas8)]

To play safe in case anything unknown gets dropped on us (you never know these days with that guy from North Korea...), I'll add this in next iteration:

default: {
isValidAttachment = false;
}

...which prevents (attempts of) creating attachments from unknown flavors, while saving us some lines to positively assign isValidAttachment = true for all known flavors. Also makes it explicit what will happen to such flavors, namely nothing.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8911218 [details] [diff] [review]
Patch V.1: Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #20)
> I did not test URLs.
Why not? Do you expect the reviewer to do this? I did, it still works.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #21)
> That existing error cause any subsequent selected messages NOT to be
> attached when dropping, but surprisingly the offending message itself seems
> to be attached correctly.
Looks like you tested this, so can you please attach such a message or at least tell me how to construct one. Is there an existing bug?

As for the patch: It looks very good, the only suggestion I would make is to reverse the logic of 'isValidAttachment'. I'd start with false and set it for the known good ones.

Comment 24

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: (revert isValidAttachment logic) Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

Thanks for rapid feedback.
This patch reverts isValidAttachment logic as requested by reviewer. A bit cleaner and more readable indeed.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8911316 [details] [diff] [review]
Patch V.1.1: (revert isValidAttachment logic) Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

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

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #24)
> This patch reverts isValidAttachment logic as requested by reviewer. A bit
> cleaner and more readable indeed.
Yes, but you need to follow through with it to get the full benefit.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4894,4 @@
>            }
> +
> +          case "text/x-moz-url": {
> +            isValidAttachment = true;

Lose this.

@@ +4910,2 @@
>                if (scheme == "mailto")
> +                isValidAttachment = false;

Revert this.

@@ +4912,3 @@
>              }
>              catch (ex) {
> +              isValidAttachment = false;

Lose this.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Patch V.1.2: (4-line polish of Thomas' patch V.1.1) Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

I made the adjustments I wanted. I need a try for this since this may affect tests. Anecdote: I corrected an alert message the other day and after landing, I found out that the wrong message appeared in a test :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a7324f894669050537aff69a61e238358a32c79b

Comment 27

[Jorg K (CEST = GMT+2)]

Good news, no test failures, so ready to go.

Comment 28

[Jorg K (CEST = GMT+2)]

BTW, I tried saving a draft and editing it again and also editing a message as new and the attachment name persisted. Nice.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d2b98c955e08
Use %subject%.eml as name for messages attached via drag & drop; code cleanup for onDrop(). r=jorgk

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8911390 [details] [diff] [review]
Patch V.1.2: (4-line polish of Thomas' patch V.1.1) Use subject as name for messages attached via drag & drop, complete code cleanup for onDrop() handler

Nice improvement that we can fast-track to a beta.

Comment 31

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/131fb9972ec0836a730d2aca0320c25d5d2bbaa3

Comment 32

[Hendrik Maryns]

Woohoo!  Thanks!  (Thought some positivity in this place wouldn’t harm:-)

Comment 33

[Jorg K (CEST = GMT+2)]

I personally like the feature very much, that's why I helped the volunteer contributor as much as I could and I have fast-tracked it to the next upcoming beta release.