Last Comment Bug 708982 - Provide listenable events for attachment operations in the compose window
: Provide listenable events for attachment operations in the compose window
: dev-doc-needed
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 11.0
Assigned To: Jim Porter (:squib)
Depends on: 731932
Blocks: 715490 715961
  Show dependency treegraph
Reported: 2011-12-09 00:48 PST by Jim Porter (:squib)
Modified: 2012-02-29 23:25 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Add the appropriate notifications (13.60 KB, patch)
2011-12-12 18:05 PST, Jim Porter (:squib)
no flags Details | Diff | Splinter Review
Send a notification on RemoveAllAttachments too (14.64 KB, patch)
2011-12-12 19:32 PST, Jim Porter (:squib)
mconley: review+
Details | Diff | Splinter Review

Description User image Jim Porter (:squib) 2011-12-09 00:48:39 PST
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!

Comment 1 User image Blake Winton (:bwinton) (:☕️) 2011-12-09 07:13:48 PST
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.)
Comment 2 User image Jim Porter (:squib) 2011-12-09 09:54:10 PST
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.
Comment 3 User image Jim Porter (:squib) 2011-12-12 18:05:24 PST
Created attachment 581133 [details] [diff] [review]
Add the appropriate notifications

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
Comment 4 User image Jim Porter (:squib) 2011-12-12 18:55:49 PST
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.
Comment 5 User image Jim Porter (:squib) 2011-12-12 19:32:50 PST
Created attachment 581148 [details] [diff] [review]
Send a notification on RemoveAllAttachments too

Hmm, actually we *should* fire a notification when RemoveAllAttachments is called. Otherwise, the notification would stick around.
Comment 6 User image Mike Conley (:mconley) 2011-12-13 07:59:55 PST
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.
Comment 7 User image Jim Porter (:squib) 2011-12-14 21:53:49 PST
Checked in:

(I forgot to address the review nits at first...)
Comment 8 User image Jim Porter (:squib) 2011-12-26 18:03:20 PST
We should probably document this for add-on developers.

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