708982 - Provide listenable events for attachment operations in the compose window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Jim Porter (:squib)]

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
  ex-attachment)
* 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!

Thoughts?

Comment 1

[Blake Winton (:bwinton) (:☕️)]

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

[Jim Porter (:squib)]

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

[Jim Porter (:squib)]

Attachment: 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

[Jim Porter (:squib)]

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

[Jim Porter (:squib)]

Attachment: 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

[Mike Conley (: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,

-Mike

::: 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["@mozilla.org/messengercompose/attachment;1"]
> +                                       .createInstance(Components.interfaces.nsIMsgAttachment);
> +            attachment.url = rawData;
> +            attachment.name = prettyName;
> +            if (size !== undefined)

Nit for readability - newlines above and below this if block please.

Comment 7

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/51098d68a6f3
            http://hg.mozilla.org/comm-central/rev/8461a3a0d970

(I forgot to address the review nits at first...)

Comment 8

[Jim Porter (:squib)]

We should probably document this for add-on developers.