Add event about closing compose window

RESOLVED FIXED in Thunderbird 48.0

Status

MailNews Core
Composition
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Thunderbird 48.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.89 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8738278 [details] [diff] [review]
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).
Attachment #8738278 - Flags: review?(mkmelin+mozilla)
Attachment #8738278 - Flags: review?(iann_bugzilla)

Comment 2

2 years ago
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
Attachment #8738278 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 3

2 years ago
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

2 years ago
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()"
(Assignee)

Comment 5

2 years ago
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

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

Comment 7

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

Comment 8

2 years ago
(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?
(Assignee)

Comment 9

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

Comment 10

2 years ago
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

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

Comment 12

2 years ago
Created attachment 8740520 [details] [diff] [review]
patch v2

OK.
Attachment #8738278 - Attachment is obsolete: true
Attachment #8738278 - Flags: review?(iann_bugzilla)
Attachment #8740520 - Flags: review?(iann_bugzilla)

Comment 13

2 years ago
(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.

Updated

2 years ago
Attachment #8740520 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 14

2 years ago
Thanks.
Keywords: checkin-needed
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/comm-central/rev/dfbfdf30f1ca8e96d18b075938bd7ce27bab0481
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.