1262262 - Add event about closing compose window

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #777732 comment 62 +++

I just want to provide a symmetric event when the compose window is going away. I don't say it is required now, just that somebody could use it. You said you didn't look at all those addons. Maybe some of them was observing "compose-window-close" just because there was nothing better available (even if unreliable). And window.onunload does not get the job done as the variables are teared down.

> Look, it's a five minute job to code:
>     document.getElementById("msgcomposeWindow").dispatchEvent(
>       new Event("compose-window-about-to-be-destroyed", { bubbles: false ,
> cancelable: false }));

Yes, that is all that is needed (at the top of ComposeUnload).

Comment 1

[:aceman]

Attachment: patch

This event is different to the just removed compose-window-close, that is why it has a different name. The old one was fired when the window was just hidden for later recycling. Thus, code run on this event cleaned up widgets/fields to prepare them for the next composition.

The new event is run when the compose window is being destroyed. So the code/addons do not need to clear any fields. It is intended more to allow them to e.g. store their state (in a backend outside of compose window). This should be noted in the documentation (if we have a TB52 for developers).

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8738278 [details] [diff] [review]
patch

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

LGTM. Not sure if anyone really need it, but it doesn't hurt. r=mkmelin

Comment 3

[:aceman]

Thanks. It is the inverse event to the existing compose-window-init which runs after compose window is up and has variables initialized.

Comment 4

[Jorg K (CEST = GMT+2)]

Thinking about this some more, wouldn't "compose-window-unload" be a better name?

It corresponds with the 'unload' event of the window, only that this doesn't fire in the order we'd like, see bug 777732 comment #61.

It also matches the other nomenclature in messengercompose.xul: onunload="ComposeUnload()"

Comment 5

[:aceman]

Yes, I thought about "-unload" but it looked to me like it may imply only some partial closing. Like when we used -close even when the window was not really closed, just hidden "for recycling". So I wanted to have strong association that that window is really going away and no traces will be left.
Yes, I understand your reasoning for consistency with other already existing events.

So whichever way Magnus decides :)

Comment 6

[Philip Chee]

> > +    new Event("compose-window-destroy", { bubbles: false, cancelable: false }));
But "compose-window-init" is { bubbles: false , cancelable: true }
Vy?

Comment 7

[:aceman]

Do we want it to be cancelable? Maybe we can wait with that until somebody actually uses it and asks for cancelability?

Comment 8

[Philip Chee]

(In reply to :aceman from comment #7)
> Do we want it to be cancelable? Maybe we can wait with that until somebody
> actually uses it and asks for cancelability?
What did the old recycle event use?

Comment 9

[:aceman]

-    document.getElementById("msgcomposeWindow").dispatchEvent(
-      new Event("compose-window-close", { bubbles: false , cancelable: true }));

Comment 10

[Philip Chee]

Hmm if the window is really closing we don't want it to be cancellable I think.

Maybe should open another bug to convert compose-window-* to observer notifications?

Comment 11

[Magnus Melin [:mkmelin]]

Yes, in the current patch that's how it is (cancellable false). 
Re the name, I don't have a strong opinion. "compose-window-unload" is fine by me

Comment 12

[:aceman]

Attachment: patch v2

OK.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Philip Chee from comment #10)
> Maybe should open another bug to convert compose-window-* to observer
> notifications?
IMHO, we don't want that since this would be a global observer. We want something that's attached to the window or something in it.

Comment 14

[:aceman]

Thanks.

Comment 15

[:aceman]

https://hg.mozilla.org/comm-central/rev/dfbfdf30f1ca8e96d18b075938bd7ce27bab0481