731932 - Change attachment events in the composer to be window-local

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jim Porter (:squib)]

The attachment events added in bug 708982 are too global and get sent to all compose windows, which causes strange behavior. We should probably use DOM events instead. There are a couple of options:

1) Create the events in MsgComposeCommands.js like they are now
2) Create the events in the XBL binding

3) Fire one event for each attachment added/removed
4) Fire one event for all attachments added/removed at once

I think I prefer (1) and (4) respectively. (1) because add/remove events don't make much sense in the reader, and (4) just because that's how it is now, and it reduces the number of events.

Another question is what node the events get fired on. I'm thinking the attachmentBucket for add/remove, and the individual <attachmentitem> for rename.

I'm open to suggestions for any of these.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I concur, regarding not mucking around in the XBL binding.  Let's create them in MsgComposeCommands.js.

I also concur on 4, since we might as well stick as close to what we're doing as possible.

In fact, I agree with everything you're saying - including where to fire the events.  It all sounds reasonable.

So, thumbs up for me.

Comment 2

[Jim Porter (:squib)]

Attachment: Patch for comm-central

Here's a patch for comm-central to fix this. Not asking for review yet, since it's totally untested (I don't have a comm-central build at the moment, since my VM doesn't have space for two full builds).

Try build incoming, though: <http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=b421305d3fb5>. Assuming that passes, this can be reviewd.

Comment 3

[Jim Porter (:squib)]

Attachment: Patch for comm-central (v2)

This should fix everything for the comm-central part.

Comment 4

[Jim Porter (:squib)]

Attachment: Patch for big-files

Also, here's a patch for the big-files repo, which updates all the interfaces to use the new way.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 604696 [details] [diff] [review]
Patch for comm-central (v2)

Review of attachment 604696 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - just one nit, and one note.

-Mike

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3806,5 @@
>  
>      gContentChanged = true;
> +
> +    let event = document.createEvent("CustomEvent");
> +    event.initCustomEvent("attachment-renamed", true, true, originalName);

Hm.  At first glance, it seems that we don't pass back the renamed attachment to event handlers.  However, clearly, we do - since the attachment-renamed tests are still passing.

I guess what I'm saying is that we'll certainly want dev-doc for this, since it's not immediately clear how to get the original attachment by just looking at the code.

::: mail/test/mozmill/attachment/test-attachment-events.js
@@ +304,4 @@
>  }
>  
>  /**
> + * Test that we don't see the attachments_removed event if no attachments

Nit - attachments-removed instead of attachments_removed

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 604697 [details] [diff] [review]
Patch for big-files

Review of attachment 604697 [details] [diff] [review]:
-----------------------------------------------------------------

I know you haven't asked for review on this yet, but I just noticed this, and thought I'd point it out.

Cheers,

-Mike

::: mail/components/compose/content/bigFileObserver.js
@@ +75,5 @@
>      if (this.hidden)
>        return;
>  
> +    const callbacks = {
> +      "attachments-added": this.attachmentsAdded,

this.attachmentsAdded, this.attachmentsRemoved and this.attachmentsConverted do not exist.

Instead, it's this.attachmentAdded, this.attachmentRemoved, this.attachmentConverted.

Comment 7

[Jim Porter (:squib)]

(In reply to Mike Conley (:mconley) from comment #5)
> Comment on attachment 604696 [details] [diff] [review]
> Patch for comm-central (v2)
> 
> Review of attachment 604696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good - just one nit, and one note.
> 
> -Mike
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +3806,5 @@
> >  
> >      gContentChanged = true;
> > +
> > +    let event = document.createEvent("CustomEvent");
> > +    event.initCustomEvent("attachment-renamed", true, true, originalName);
> 
> Hm.  At first glance, it seems that we don't pass back the renamed
> attachment to event handlers.  However, clearly, we do - since the
> attachment-renamed tests are still passing.

That's because of the next line, which dispatches the event to the specific attachment item being renamed:

> +    item.dispatchEvent(event);

> I guess what I'm saying is that we'll certainly want dev-doc for this, since
> it's not immediately clear how to get the original attachment by just
> looking at the code.

It would probably useful to add some documentation about all of these events so that extension authors can use these hooks.

> > + * Test that we don't see the attachments_removed event if no attachments
> 
> Nit - attachments-removed instead of attachments_removed

Fixed.

Comment 8

[Jim Porter (:squib)]

Comment on attachment 604696 [details] [diff] [review]
Patch for comm-central (v2)

Should we take this on aurora/beta? The non-test changes are tiny, no one uses these events (yet), and I'd prefer to get out quickly so that extension authors can start using them, instead of having to wait until 13 for the new API in this patch.

Comment 9

[Jim Porter (:squib)]

Checked into central: http://hg.mozilla.org/comm-central/rev/862863b8547f

Comment 10

[Mark Banner (:standard8)]

(In reply to Jim Porter (:squib) from comment #8)
> Comment on attachment 604696 [details] [diff] [review]
> Patch for comm-central (v2)
> 
> Should we take this on aurora/beta? The non-test changes are tiny, no one
> uses these events (yet), and I'd prefer to get out quickly so that extension
> authors can start using them, instead of having to wait until 13 for the new
> API in this patch.

Except you're also removing APIs that extensions may already rely on. So we should give them the maximum amount of time to adopt.