Closed Bug 708982 Opened 12 years ago Closed 12 years ago

Provide listenable events for attachment operations in the compose window


(Thunderbird :: Message Compose Window, enhancement)

Not set


(Not tracked)

Thunderbird 11.0


(Reporter: squib, Assigned: squib)



(Keywords: dev-doc-needed)


(1 file, 1 obsolete file)

With the "BigFiles" feature coming soon, we'll have two separate things listening for attachments being added or removed (the other is the attachment reminder). Instead of putting more special-purpose calls in the attachment manipulation functions, we should just use the observer service and send out some appropriate notifications. Here's what I'm thinking:

* attachmentAdded: fired when a single attachment is added (passes in the new attachment)
* attachmentRemoved: fired when a single attachment is removed (passes in the
* attachmentBatchAdded: fired when a group of attachments are added (e.g. if you add 3
  attachments from a file picker)
* attachmentBatchRemoved: fired when a group of attachments are deleted (e.g. if you
  delete 3 attachments at once)
* attachmentRenamed: fired when an attachment is renamed

Thus, if you attach 3 files at once from the file picker, it calls attachmentAdded three times, and then attachmentBatchAdded once.

This would be less invasive, so we could put BigFiles and the attachment reminder in separate .js files, and for the attachment reminder, we could make it more efficient (currently it listens for every attachment added, when it could listen for attachmentBatchAdded). It would also open up the possibility for add-ons to do fun and exciting things with this information as well!

What about just "attachmentsAdded" and "attachmentsRemoved", which may pass a list containing a single item?  (This would also remove the unspecified case of when you add a single attachment.  Would the attachmentBatchAdded also fire once or not?)

(Then I'ld want to change it to "attachmentsRenamed", which could conceivably occur if an add-on changed a bunch of names for some reason.)
That's doable, though it's somewhat more complicated, since there are a few instances where we get a bunch of new things to attach and then convert them one at a time to an nsIMsgAttachment. We'd have to change those spots to generate them all at once.

Not sure about "attachmentsRenamed" though, since there's no way in the code as it is now to rename multiple attachments (it requires the user to enter the new name interactively). I guess I could see the argument for future-proofing, but this seems like something that we'd never actually take advantage of.
Here's a patch (sans tests, since I've got a bunch else to do at the moment). The behavior is as follows:

* mail:attachmentsAdded: fired when attachments are added by any means; the
  subject is an nsIArray of nsIMsgAttachments
* mail:attachmentsRemoved: fired when attachments are removed by any means
  (except RemoveAllAttachments, which is only used to reinitialize the compose
  window); the subject is an nsIArray of nsIMsgAttachments
* mail:attachmentRenamed: fired when a single attachment is renamed through the
  UI; the subject is the nsIMsgAttachment
Assignee: nobody → squibblyflabbetydoo
Attachment #581133 - Flags: review?(mconley)
Oh, as a followup, we should convert the attachment reminder to use the new observers (and generally clean it up). I'll do that later.
Hmm, actually we *should* fire a notification when RemoveAllAttachments is called. Otherwise, the notification would stick around.
Attachment #581133 - Attachment is obsolete: true
Attachment #581133 - Flags: review?(mconley)
Attachment #581148 - Flags: review?(mconley)
Comment on attachment 581148 [details] [diff] [review]
Send a notification on RemoveAllAttachments too

Review of attachment 581148 [details] [diff] [review]:

Looks good!  Modulo the few nits I found, r=me.

Thanks for your work,


::: mail/components/compose/content/MsgComposeCommands.js
@@ +2897,4 @@
>        return;
>      let file;
> +    let attachments = [];
> +    for (file in fixIterator(fp.files, Components.interfaces.nsILocalFile))

For readabilities sake, could you please put some newlines before and after this for loop?

@@ -2912,0 +2906,70 @@
> > +
> > +/**
> > + * Convert an nsILocalFile instance into an nsIMsgAttachment.
> > + *
NaN more ...

nit: typo, "were" => "where" twice in this comment

@@ +3635,5 @@
> +            let attachment = Components.classes[";1"]
> +                                       .createInstance(Components.interfaces.nsIMsgAttachment);
> +            attachment.url = rawData;
> +   = prettyName;
> +            if (size !== undefined)

Nit for readability - newlines above and below this if block please.
Attachment #581148 - Flags: review?(mconley) → review+
Checked in:

(I forgot to address the review nits at first...)
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
We should probably document this for add-on developers.
Keywords: dev-doc-needed
Depends on: 731932
You need to log in before you can comment on or make changes to this bug.