Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Provide listenable events for attachment operations in the compose window

RESOLVED FIXED in Thunderbird 11.0

Status

Thunderbird
Message Compose Window
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

({dev-doc-needed})

Trunk
Thunderbird 11.0
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.)
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #581133 - Flags: review?(mconley)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
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,

-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.
Attachment #581148 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

6 years ago
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...)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
(Assignee)

Comment 8

6 years ago
We should probably document this for add-on developers.
Keywords: dev-doc-needed
Blocks: 715490
Blocks: 715961
(Assignee)

Updated

6 years ago
Depends on: 731932
You need to log in before you can comment on or make changes to this bug.