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?
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.
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
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.
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 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.
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...)
We should probably document this for add-on developers.
6 years ago
6 years ago