Closed Bug 324574 Opened 14 years ago Closed 14 years ago

No event/notify when Compose windows is ready

Categories

(Thunderbird :: Message Compose Window, defect)

All
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hesslow, Assigned: hesslow)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

From what I can see there is no way to know when the compose window is ready. eComposeFieldsReady is fired when the compose fields are ready but that is before the body of the mail is set up. Or that is at least what I can see here: http://lxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#1369

This means that it is very hard to make any kind of extension that inserts text to the body automatically without replacing the init-function or polling the editor. Polling the editor is possible but you have to check for a specific number of changes of the editor and it is different if it is a reply, you have a signature or not. And changing the init-function means that you probably will break something. I'm the developer of Quicktext (http://www.hesslow.se/quicktext) and there is many feature I would like to add if I could. Like text automatically got inserted when you reply a mail and stuff like that.

I would be very greatful if this was added and I'm sure everyone that uses my extension also will be.
Attached patch patch #1 (obsolete) — Splinter Review
This patch both adds an event so extensions know when gMsgCompose is initialized so they can attach an statelistener. It also adds the ComposeBodyReady notify which is sent when the body of the mail is ready so extension could insert text to the body. It also changes TStateListenerNotification to an interface.

I know that the name of variablename/functionname probably could change. But I didn't come up which anything better.
Assignee: mscott → hesslow
Status: NEW → ASSIGNED
Attachment #216065 - Flags: review?(bienvenu)
I thought we added an event very similar to this for the smime folks several years ago.
Scott: If you are right it would be great because then I could use it in my extension today. But I have not been able to find anything like this and no one I've asked has been able to tell me how this could be done.
Comment on attachment 216065 [details] [diff] [review]
patch #1

This looks pretty good. Here's one thing to keep in mind, however - I assume you want this to go into 2.0, and for 2.0, we're trying really hard not to change any interfaces, so changing
notifyStateListeners is not great for 2.0

also, this seems to be a bit strange:

+interface nsIMsgComposeNotificationType : nsISupports


should it just be

+interface nsIMsgComposeNotificationType

? Or does that mean you can't pass it into notifyStateListeners? I don't know what declaring it as an nsISupports does when it gets passed in. You could also just define the consts in nsIMsgCompose instead of having a separate type...

thoughts? For 2.0, the less interface changes the better.
Scott, this would require adding a method to nsIMsgComposeStateListener to get called when the compose window is ready for the user, e.g., the reply quoting is done and the sig has been inserted. Do we think that's OK for 2.0? Or is there some other way of knowing this, e.g., some UI element gets enabled?
Attached patch patch #2Splinter Review
This just removes nsISupports that David taked about. If you don't want me to change the enum to its own interface for 2.0 because it also changes nsIMsgCompose interface just tell me and I fix a patch without that change.
Attachment #216065 - Attachment is obsolete: true
Attachment #219413 - Flags: review?(bienvenu)
Attachment #216065 - Flags: review?(bienvenu)
Comment on attachment 219413 [details] [diff] [review]
patch #2

looks good for the trunk.
Attachment #219413 - Flags: review?(bienvenu) → review+
Attachment #219413 - Flags: superreview?(mscott)
When you have time to review Scott it would be nice if you could tell me if you want this for 2.0 (I want :)) and if so if you want the change for the nsIMsgCompose-interface or I should remove that for 2.0.
(In reply to comment #3)
> Scott: If you are right it would be great because then I could use it in my
> extension today. But I have not been able to find anything like this and no one
> I've asked has been able to tell me how this could be done.
> 

I must have been thinking of window-compose-reopen which doesn't do what you want.
Comment on attachment 219413 [details] [diff] [review]
patch #2

I think the event name should be compose-window-init to be consistent with the other events we fire from this window:

compose-window-close
compose-window-reopen
compose-window-sendmessage
etc.

In lines like:

+      compose->NotifyStateListeners(nsIMsgComposeNotificationType::ComposeProcessDone,aStatus);
add a space after the comma even though the earlier code didn't have that space, it should have.

Let's put together a separate patch for the branch only which doesn't have the interface change. But we can do that after you make my minor two nits and we land this on the trunk. 

Thanks for working on this and for your patience.
Attachment #219413 - Flags: superreview?(mscott) → superreview+
Attached patch patch #3Splinter Review
This fixes the small comments from Scott. So if someone could check this in that would be good.
fix checked into the trunk, marking fixed. Thx, Emil!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This is the same patch as the last one except that I removed the change in nsIMsgCompose.
Attachment #223360 - Flags: review?(bienvenu)
Attachment #223360 - Flags: approval-branch-1.8.1?(bienvenu)
Comment on attachment 223360 [details] [diff] [review]
patch #1 for TB 2.0

I'll apply and run with this change in my 1.8.1 branch build before checking in.
Attachment #223360 - Flags: review?(bienvenu)
Attachment #223360 - Flags: review+
Attachment #223360 - Flags: approval-branch-1.8.1?(bienvenu)
Attachment #223360 - Flags: approval-branch-1.8.1+
Hey David, I can land this on the branch if you think it's ready.
landed on the 1.8.1 branch - thx for the reminder, Emil.
Keywords: fixed1.8.1
Blocks: TB2SM
No longer blocks: TB2SM
Depends on: 341312
You need to log in before you can comment on or make changes to this bug.