Last Comment Bug 190298 - already-attached attachments should be able to be renamed
: already-attached attachments should be able to be renamed
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Attachments (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla1.9
Assigned To: Magnus Melin
:
:
Mentors:
: 269817 427354 441545 446400 (view as bug list)
Depends on: 567987
Blocks: 380354
  Show dependency treegraph
 
Reported: 2003-01-23 07:09 PST by Douglas Melniker
Modified: 2010-05-25 08:13 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (11.75 KB, patch)
2007-01-06 05:07 PST, Magnus Melin
mnyromyr: review-
Details | Diff | Splinter Review
proposed fix, v2 (12.31 KB, patch)
2007-02-18 11:00 PST, Magnus Melin
mnyromyr: review+
Details | Diff | Splinter Review
proposed fix, v3 (12.32 KB, patch)
2007-02-20 11:46 PST, Magnus Melin
no flags Details | Diff | Splinter Review
proposed fix, v4 (unbitrotted v3) (20.95 KB, patch)
2007-11-07 08:05 PST, Magnus Melin
no flags Details | Diff | Splinter Review
proposed fix, v5 (20.96 KB, patch)
2008-03-27 06:18 PDT, Magnus Melin
philringnalda: review+
neil: superreview+
Details | Diff | Splinter Review
proposed fix, v6 (as checked in) (21.05 KB, patch)
2008-04-09 12:38 PDT, Magnus Melin
no flags Details | Diff | Splinter Review

Description Douglas Melniker 2003-01-23 07:09:44 PST
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 2003-05-07 07:33:37 PDT
I'm confirming this as I have found no dupes. 


Tweaking Hardware/Os to all
Comment 2 Doug Wright 2005-03-02 16:00:15 PST
*** Bug 269817 has been marked as a duplicate of this bug. ***
Comment 3 Rich Gray (:rbgray) 2006-08-24 09:52:07 PDT
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 2007-01-02 08:37:21 PST
Working on it.
Comment 5 Magnus Melin 2007-01-06 05:07:41 PST
Created attachment 250691 [details] [diff] [review]
proposed fix

Lets the user rename an attachment while composing.
Comment 6 Karsten Düsterloh 2007-02-15 15:53:00 PST
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 2007-02-18 11:00:04 PST
Created attachment 255600 [details] [diff] [review]
proposed fix, v2

Review comments address.
Comment 8 Karsten Düsterloh 2007-02-19 15:43:57 PST
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 2007-02-20 11:46:29 PST
Created attachment 255808 [details] [diff] [review]
proposed fix, v3

With old habits fixed;)
Comment 10 Magnus Melin 2007-06-01 13:10:53 PDT
Another option is of course a custom dialog, but this gets the job done. 

Scott, ping for the sr?
Comment 11 Magnus Melin 2007-10-11 12:28:20 PDT
Scott, have you had time to look at this patch?
Comment 12 Milan Kupcevic 2007-10-11 19:57:35 PDT
(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 2007-10-13 02:30:39 PDT
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 2007-10-13 08:21:02 PDT
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 2007-10-13 09:05:11 PDT
Why just not use the mail's subject sanitized from dots (replace the dots with underscores)?
Comment 16 Magnus Melin 2007-10-13 10:05:22 PDT
Because it is an .eml file. Please don't take that discussion here too...
Comment 17 Rimas Kudelis 2007-10-13 12:17:56 PDT
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 2007-10-13 13:37:36 PDT
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 2007-10-14 02:44:56 PDT
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 2007-11-07 08:05:16 PST
Created attachment 287684 [details] [diff] [review]
proposed fix, v4 (unbitrotted v3)

Unbitrotted patch for current trunk.
Comment 21 rsx11m 2007-11-07 20:22:16 PST
(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 2008-01-09 14:34:11 PST
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 2008-03-27 06:18:46 PDT
Created attachment 312020 [details] [diff] [review]
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) 2008-03-30 13:31:22 PDT
Comment on attachment 312020 [details] [diff] [review]
proposed fix, v5

r=me, thx
Comment 25 neil@parkwaycc.co.uk 2008-04-02 13:22:14 PDT
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 26 Rod Whiteley 2008-04-06 03:25:58 PDT
*** Bug 427354 has been marked as a duplicate of this bug. ***
Comment 27 Magnus Melin 2008-04-09 08:36:46 PDT
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 2008-04-09 12:37:10 PDT
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 2008-04-09 12:38:42 PDT
Created attachment 314667 [details] [diff] [review]
proposed fix, v6 (as checked in)

Patch as checked in, adjusted for some bit rot.
Comment 30 Jo Hermans 2008-06-24 09:45:59 PDT
*** Bug 441545 has been marked as a duplicate of this bug. ***
Comment 31 Jo Hermans 2008-07-21 16:41:53 PDT
*** Bug 446400 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.