Change attachment events in the composer to be window-local

RESOLVED FIXED in Thunderbird 13.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

11 Branch
Thunderbird 13.0
x86_64
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 2

6 years ago
Created attachment 604588 [details] [diff] [review]
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.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 604696 [details] [diff] [review]
Patch for comm-central (v2)

This should fix everything for the comm-central part.
Attachment #604588 - Attachment is obsolete: true
Attachment #604696 - Flags: review?(mconley)
(Assignee)

Comment 4

5 years ago
Created attachment 604697 [details] [diff] [review]
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 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
Attachment #604696 - Flags: review?(mconley) → review+
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.
(Assignee)

Comment 7

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

Comment 8

5 years ago
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.
Attachment #604696 - Flags: approval-comm-beta?
Attachment #604696 - Flags: approval-comm-aurora?
(Assignee)

Comment 9

5 years ago
Checked into central: http://hg.mozilla.org/comm-central/rev/862863b8547f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(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.
Attachment #604696 - Flags: approval-comm-beta?
Attachment #604696 - Flags: approval-comm-beta-
Attachment #604696 - Flags: approval-comm-aurora?
Attachment #604696 - Flags: approval-comm-aurora-
Target Milestone: --- → Thunderbird 13.0
You need to log in before you can comment on or make changes to this bug.