already-attached attachments should be able to be renamed

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
Attachments
--
enhancement
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: Douglas Melniker, Assigned: Magnus Melin)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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
I'm confirming this as I have found no dupes. 


Tweaking Hardware/Os to all
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: SunOS → All
Hardware: Sun → All
Product: MailNews → Core

Comment 2

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

Comment 3

11 years ago
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. 
(Assignee)

Updated

11 years ago
Assignee: mscott → nobody
QA Contact: stephend → attachments
(Assignee)

Comment 4

11 years ago
Working on it.
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: nobody → mkmelin+mozilla
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

11 years ago
Created attachment 250691 [details] [diff] [review]
proposed fix

Lets the user rename an attachment while composing.
Attachment #250691 - Flags: superreview?(mscott)
Attachment #250691 - Flags: review?(mnyromyr)

Comment 6

11 years ago
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...').
Attachment #250691 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 255600 [details] [diff] [review]
proposed fix, v2

Review comments address.
Attachment #250691 - Attachment is obsolete: true
Attachment #255600 - Flags: review?(mnyromyr)
Attachment #250691 - Flags: superreview?(mscott)

Comment 8

11 years ago
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.
Attachment #255600 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 9

11 years ago
Created attachment 255808 [details] [diff] [review]
proposed fix, v3

With old habits fixed;)
Attachment #255600 - Attachment is obsolete: true
Attachment #255808 - Flags: superreview?(mscott)
(Assignee)

Comment 10

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

Scott, ping for the sr?
(Assignee)

Comment 11

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

Comment 12

10 years ago
(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

10 years ago
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).
Blocks: 380354
(Assignee)

Comment 14

10 years ago
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

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

Comment 16

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

Comment 17

10 years ago
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?
(Assignee)

Comment 18

10 years ago
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

10 years ago
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...
(Assignee)

Comment 20

10 years ago
Created attachment 287684 [details] [diff] [review]
proposed fix, v4 (unbitrotted v3)

Unbitrotted patch for current trunk.
Attachment #255808 - Attachment is obsolete: true
Attachment #287684 - Flags: superreview?(mscott)
Attachment #255808 - Flags: superreview?(mscott)

Comment 21

10 years ago
(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

10 years ago
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].
(Assignee)

Comment 23

10 years ago
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.
Attachment #287684 - Attachment is obsolete: true
Attachment #312020 - Flags: review?(philringnalda)
Attachment #287684 - Flags: superreview?(mscott)
Comment on attachment 312020 [details] [diff] [review]
proposed fix, v5

r=me, thx
Attachment #312020 - Flags: review?(philringnalda) → review+
(Assignee)

Updated

10 years ago
Attachment #312020 - Flags: superreview?(neil)

Comment 25

10 years ago
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.
Attachment #312020 - Flags: superreview?(neil) → superreview+

Updated

10 years ago
Duplicate of this bug: 427354

Updated

10 years ago
Flags: blocking-thunderbird3?
(Assignee)

Comment 27

10 years ago
No need for this to block, I'll check it in as soon as I got time (been a bit busy this week).
Flags: blocking-thunderbird3?
(Assignee)

Comment 28

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
(Assignee)

Comment 29

10 years ago
Created attachment 314667 [details] [diff] [review]
proposed fix, v6 (as checked in)

Patch as checked in, adjusted for some bit rot.

Updated

9 years ago
Duplicate of this bug: 441545

Updated

9 years ago
Duplicate of this bug: 446400
Product: Core → MailNews Core

Updated

7 years ago
Depends on: 567987
You need to log in before you can comment on or make changes to this bug.