190298 - already-attached attachments should be able to be renamed

Status: RESOLVED FIXED

(MailNews Core :: Attachments, enhancement)

Description

[Douglas Melniker]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.3b) Gecko/20030122
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.3b) Gecko/20030122

Once an object is attached, you should be able to right-click it and rename it.
For instance: when you drag messages from the folder listing to the attchment
window of a "compose mail" window, the attachment is named
"imap://user@host:imap/dir/stuff" and it appears this way to the recipient too.
However, when you Forward a mail, it becomes an attachment and the name of it is
the mail's subject. 

Reproducible: Always

Steps to Reproduce:
1. select 'compose'
2. drag mail from IMAP folder listing to the attach window of the Compose window
3. observe the attachment's name

Actual Results:  
name is the imap URI

Expected Results:  
name should be the mail's subject OR user should be able to rename the attachment

Comment 1

[Henrik Lynggaard Hansen]

I'm confirming this as I have found no dupes. 


Tweaking Hardware/Os to all

Comment 2

[Doug Wright]

*** Bug 269817 has been marked as a duplicate of this bug. ***

Comment 3

[Rich Gray (:rbgray)]

Bug 220646 will now (1.8.1) cause .eml to be appended to "forward as" e-mail messages.  Some sites are (stupidly) rejecting mail based purely on the presence of the .eml extension.  (They went through fine before I upgraded to SM 1.1a.)

    This message has been rejected because it has
    a potentially executable attachment "Re: ticker news1etter.eml"
    This form of attachment has been used by
    recent viruses or other malware.
    If you meant to send this file then please
    package it up as a zip file and resend it.

This bug offers good work-around to the .eml problem as well being generally useful in many other circumstances of brokenness, privacy, etc. 

Comment 4

[Magnus Melin [:mkmelin]]

Working on it.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Lets the user rename an attachment while composing.

Comment 6

[Karsten Düsterloh]

Comment on attachment 250691 [details] [diff] [review]
proposed fix

Looks good so far, just a couple of nits:
(Since the changes in /mailnews and /mail are almost identical, but more code is added to /mailnews here, I'll add my comments to the /mailnews stuff. You should do the required changes to the respective /mail file as well then.)

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================
>+function MessageGetNumSelectedAttachments()
>+{
>+  var bucketList = document.getElementById("attachmentBucket");
>+
>+  if (bucketList)
>+    return bucketList.selectedItems.length;
>+  else
>+    return 0;

No 'else' needed.

>+function RenameSelectedAttachment()
...
>+  var attachmentName = {value:item.attachment.name};
>+  if(gPromptService.prompt(

Missing spaces after ':' and 'if'.

>+                    {value:0}) && attachmentName.value != "") {

Missing space after ':'.
It'd be better in terms of readability to read the prompt result into a new variable and check that.
Furthermore, the prevalent bracing style in this file is 'aligned curly braces', so the '{' has to go onto the next line.

>Index: mailnews/compose/resources/locale/en-US/composeMsgs.properties
>===================================================================
>+renameAttachmentTitle=Rename attachment

Dialog titles are usually capitalized, eg. "Save Attachment" etc.

>+renameAttachmentMessage=Attachment name:

Better: 'New attachment name:'

>Index: mailnews/compose/resources/locale/en-US/messengercompose.dtd
>===================================================================
>+<!ENTITY renameAttachment.label "Rename">

This menuitem opens a new dialog, thus the label needs a trailing ellipsis ('Rename...').

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Review comments address.

Comment 8

[Karsten Düsterloh]

Comment on attachment 255600 [details] [diff] [review]
proposed fix, v2

>Index: mail/components/compose/content/MsgComposeCommands.js
>===================================================================
>@@ -2734,11 +2739,7 @@
> function MessageGetNumSelectedAttachments()
> {
>   var bucketList = document.getElementById("attachmentBucket");
>-
>-  if (bucketList)
>-    return bucketList.selectedItems.length;
>-  else
>-    return 0;
>+  return (bucketList) ? bucketList.selectedItems.length: 0;

Missing space before ':'. (Same in /mailnews.)

>+function RenameSelectedAttachment()
>+{
>+  var bucket = document.getElementById("attachmentBucket");
>+  if (bucket.selectedItems.length != 1)
>+    return; // not one attachment selected
>+
>+  var item = bucket.getSelectedItem(0);
>+  var attachmentName = {value: item.attachment.name};
>+  if (gPromptService.prompt(
>+                     window,
>+                     sComposeMsgsBundle.getString("renameAttachmentTitle"),
>+                     sComposeMsgsBundle.getString("renameAttachmentMessage"),
>+                     attachmentName,
>+                     null,
>+                     {value: 0}))
>+  {
>+    var modifiedAttachmentName = attachmentName.value;
>+    if(modifiedAttachmentName == "")

Missing space behind 'if'. (Same in /mailnews.)

r=me with those fixed on checkin.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

With old habits fixed;)

Comment 10

[Magnus Melin [:mkmelin]]

Another option is of course a custom dialog, but this gets the job done. 

Scott, ping for the sr?

Comment 11

[Magnus Melin [:mkmelin]]

Scott, have you had time to look at this patch?

Comment 12

[Milan Kupcevic]

(In reply to comment #0)
> 
> Expected Results:  
> name should be the mail's subject OR user should be able to rename the
> attachment
> 

Expected Results: name should be the mail's subject *AND* user should be able to rename the attachment

Comment 13

[Rimas Kudelis [use needinfo to ping]]

I suggest removing the filename hint at all when an empty name is given in rename dialog. Also, I think we need a Cancel button in this dialog (without screenshots, I'm not sure if it is there already).

Comment 14

[Magnus Melin [:mkmelin]]

We'd still have to show something there (so one can access the attachment). And I tried at some point the "no name" thing, but it would require changes elsewhere - now attachments with no name don't get sent. IIRC.

Comment 15

[Milan Kupcevic]

Why just not use the mail's subject sanitized from dots (replace the dots with underscores)?

Comment 16

[Magnus Melin [:mkmelin]]

Because it is an .eml file. Please don't take that discussion here too...

Comment 17

[Rimas Kudelis [use needinfo to ping]]

hmm..(In reply to comment #14)
> We'd still have to show something there (so one can access the attachment). 

Tb could show the subject of the forwarded message there, or anything else. Perhaps those attachments need a unique id that isn't their filename or anything else, but just a (probably) number? In UI, you could display the filename when there is one suggested, or a subject line, for emails without the filename, and so on...

> And I tried at some point the "no name" thing, but it would require changes
> elsewhere - now attachments with no name don't get sent. IIRC.

Hmm, if we can distill those things out, and make the change in all appropriate places, maybe that's OK?

Comment 18

[Magnus Melin [:mkmelin]]

Everything can be done (in theory), but frankly I don't see the point. 

Note also, this bug is for all attachments, not only eml...

Comment 19

[Rimas Kudelis [use needinfo to ping]]

The point is to separate the presentation from what's under the hood. I don't know  when attachments are actually being attached now (i.e. when dragged to the compose window or upon sending), but still, if displayed filename is the only identifier, I suspect it's kind of broken. E.g., how will Tb act if I attach two files with the same name to a message?

BTW, I just edited a message with a nameless attachment as new, and sent it to myself. Here's an excerpt from what I have got:

Content-Type: text/plain;
 name="file:///tmp/nsmail-1.tmp"

I'm not sure that's really OK...

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4 (unbitrotted v3)

Unbitrotted patch for current trunk.

Comment 21

[rsx11m]

(In reply to comment #19)
> BTW, I just edited a message with a nameless attachment as new, and sent it to
> myself. Here's an excerpt from what I have got:
> 
> Content-Type: text/plain;
>  name="file:///tmp/nsmail-1.tmp"

If I read the source correctly, this may be happening in AddAttachment() of MsgComposeCommands.js, which replaces an empty attachment name with parts of the URL of the attachment. Certain sensitive URLs (mailbox-message:, imap-message:, news-message:; and mailbox:, imap:, news:) are replaced with the locale's messageAttachmentSafeName or partAttachmentSafeName entries, to hide internals like profile path or user and host names. Maybe adding "file:" to the list would resolve the issue you mention, causing the attachment to just be named "Attached Message Part" for the en-US locale, as long as it doesn't affect regular file attachments.

Comment 22

[rsx11m]

I have now filed comment #19 as a separate bug 411572 with a patch provided.
Given the progress made already on finalizing the patch here, and that generation of an attachment name is different from editing that name during mail composition, it's probably better to handle this separately rather than to squeeze it into attachment 287684 [details] [diff] [review].

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v5

Unbitrotted patch. Karsten reviewed the seamonkey part already. Phil, please have look at the (more-less identical) tb part.

Comment 24

[Phil Ringnalda (:philor)]

Comment on attachment 312020 [details] [diff] [review]
proposed fix, v5

r=me, thx

Comment 25

[[e-mail]]

Comment on attachment 312020 [details] [diff] [review]
proposed fix, v5

Having Delete above Rename looks strange to me, but Windows Explorer does it too, so what do I know?

It's a pity the attachment list isn't a tree, because then we could have utilised inline editing.

Comment 27

[Magnus Melin [:mkmelin]]

No need for this to block, I'll check it in as soon as I got time (been a bit busy this week).

Comment 28

[Magnus Melin [:mkmelin]]

Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.130; previous revision: 1.129
done
Checking in mail/components/compose/content/messengercompose.xul;
/cvsroot/mozilla/mail/components/compose/content/messengercompose.xul,v  <--  messengercompose.xul
new revision: 1.118; previous revision: 1.117
done
Checking in mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties,v  <--  composeMsgs.properties
new revision: 1.18; previous revision: 1.17
done
Checking in mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd,v  <--  messengercompose.dtd
new revision: 1.11; previous revision: 1.10
done
Checking in mailnews/compose/resources/content/MsgComposeCommands.js;
/cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.414; previous revision: 1.413
done
Checking in mailnews/compose/resources/content/messengercompose.xul;
/cvsroot/mozilla/mailnews/compose/resources/content/messengercompose.xul,v  <--  messengercompose.xul
new revision: 1.299; previous revision: 1.298
done
Checking in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties,v  <--  composeMsgs.properties
new revision: 1.91; previous revision: 1.90
done
Checking in suite/locales/en-US/chrome/mailnews/compose/messengercompose.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/messengercompose.dtd,v  <--  messengercompose.dtd
new revision: 1.92; previous revision: 1.91
done

->FIXED

Comment 29

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v6 (as checked in)

Patch as checked in, adjusted for some bit rot.