Closed Bug 331387 Opened 17 years ago Closed 14 years ago

The AttachFile() should be two function so extensions easier could add an attachment

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: hesslow, Assigned: gkw)

References

Details

Attachments

(1 file, 2 obsolete files)

The AttachFile-function always prompt the user for picking a file. If you make an extension that automaticly add attachments it would be better if the function was divided into two function. One for picking file and one for making the file to an attachment that you use for when calling AddAttachment.

Patch is coming.
Attached patch Patch #1 (obsolete) — Splinter Review
I couldn't come up with any better name of the function than makeAttachmentOfFile so I be happy to change it.
Assignee: mscott → hesslow
Status: NEW → ASSIGNED
QA Contact: message-compose
Emil, could you please come up with an updated patch for the latest Shredder Trunk builds?
irrc Emil no longer users TB.
assigning => nobody, but would be pleased if Emil comes back :)
Assignee: hesslow → nobody
Keywords: helpwanted
Whiteboard: [patchlove] has patch needs owner
Attached patch unbitrotted patch v1 (obsolete) — Splinter Review
Since Emil is no longer working on this bug, here's the unbitrotted version, if it's still relevant in current environments.
Assignee: nobody → nth10sd
Attachment #215929 - Attachment is obsolete: true
Attachment #347724 - Flags: review?(mkmelin+mozilla)
Keywords: helpwanted
Whiteboard: [patchlove] has patch needs owner → [patchlove]
Comment on attachment 347724 [details] [diff] [review]
unbitrotted patch v1

The old |var attachments;| should be removed also.

Maybe name it |function AttachFiles(files)| ?

Also please remove the braces for the if clause 

if (fp.show() == nsIFilePicker.returnOK) {

(We prefer not to have any for one line ifs.)
Attached patch second passSplinter Review
second take with mkmelin's comments incorporated.
Attachment #347724 - Attachment is obsolete: true
Attachment #347796 - Flags: review?(mkmelin+mozilla)
Attachment #347724 - Flags: review?(mkmelin+mozilla)
Comment on attachment 347796 [details] [diff] [review]
second pass

Thx, looks good!
Attachment #347796 - Flags: review?(mkmelin+mozilla) → review+
changeset:   1081:666827c06d9d
http://hg.mozilla.org/comm-central/rev/666827c06d9d

->FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
Blocks: 464651
IMHO this particular division is not very useful for two reasons:

1. The parameter to AttachFiles is an nsISimpleEnumerator which isn't yet a
   common pattern for JS callers (although jminta's module helps).
2. AttachFiles sets the last attached directory, which is probably wrong for
   use by extensions.

I suggest creating AddFileAttachment and/or AddUrlAttachment methods.
Filed bug 468335 to fix that.
Whiteboard: [patchlove]
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.