Mark Banner (:standard8) Assignee
	Description • 15 years ago

At the moment when a protocol gets an error/warning etc and wants to prompt the user it just throws up a modal dialog attached to a message window e.g.

http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#1731
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#381

We need to start allowing alternatives and therefore I'm proposing that we have a service to centralise where the alerts go through.

Initially it will just deal with the simple alerts e.g. connection failed, but I'm hoping we can later perhaps include passwords into it in some manner. I'll attach a prototype in a while.

	Mark Banner (:standard8) Assignee
	Comment 1 • 15 years ago

This is roughly what I'm thinking of. I've set it up so that if there are no listeners then the service will just default to prompting the user. This will therefore cover not having a listener on start up should we hit that by accident, and also the intermediate times where we haven't got code written that will take advantage of the new interface.

There's probably more instances of alerts I need to hook in, and I want to write a unit test for it, but the basic idea of what I want to do is now there.

	David :Bienvenu
	Comment 2 • 15 years ago

Comment on attachment 365495 [details] [diff] [review]
WIP

makes sense, but I wonder about the name "PromptService" since I don't think what TB is going to do could be called a prompt. If anything, it's more like an Alert.

	neil@parkwaycc.co.uk
	Comment 3 • 15 years ago

I'm assuming that is where the passwords come in ;-)

	David :Bienvenu
	Comment 4 • 15 years ago

(In reply to comment #3)
> I'm assuming that is where the passwords come in ;-)

Heh, now neither prompt nor alert works ;-)

passwords are a special challenge since we're not blocking on the alert/prompt...

	Mark Banner (:standard8) Assignee
	Comment 5 • 15 years ago

How about mnNotificationService?

mn being mailnews (about time we branched out from the old Netscape prefixes on files anyway...).

	David :Bienvenu
	Comment 6 • 15 years ago

or MsgNotificationService...

	neil@parkwaycc.co.uk
	Comment 7 • 15 years ago

(In reply to comment #1)
> I've set it up so that if there are no
> listeners then the service will just default to prompting the user.
An alternative would be to make the listener return a boolean value, indicating that it was interested in the specific notification.

	Mark Banner (:standard8) Assignee
	Comment 8 • 15 years ago

This time I've gone for adding the listener code to nsIMsgMailSession - as its just a small bit of code at the moment, I think it will do fine. Might have to re-evaluate when it comes to implementing something for password prompts, but we'll see what happens.

I've gone for nsIMsgUserFeedbackListener which is a little long, but was about the best generic thing I could think of.

I've also done the modification Neil suggested of seeing if the listeners have handled the alert (i.e. displayed it to the user) or not (this will actually be useful in at least the early stages of hiding these prompts).

	Mark Banner (:standard8) Assignee
	Comment 10 • 15 years ago

Now with unit test.

	neil@parkwaycc.co.uk
	Comment 11 • 15 years ago

Comment on attachment 365860 [details] [diff] [review]
The fix

We'll get started and then think about categorising later :-)

	David :Bienvenu
	Comment 12 • 15 years ago

Comment on attachment 365860 [details] [diff] [review]
The fix

the problem with this patch, if I'm reading it correctly, is that now things like biff and autosync will put up error alerts - previously, that would not happen because we would not put up alerts if the msgWindow is null. I don't think we want to do this, even temporarily.

	David :Bienvenu
	Comment 13 • 15 years ago

I'd say r=me if you don't put up an alert prompt when there's no msg window for now.

	Mark Banner (:standard8) Assignee
	Comment 14 • 15 years ago

(In reply to comment #12)
> (From update of attachment 365860 [details] [diff] [review])
> the problem with this patch, if I'm reading it correctly, is that now things
> like biff and autosync will put up error alerts - previously, that would not
> happen because we would not put up alerts if the msgWindow is null. I don't
> think we want to do this, even temporarily.

David - that would only happen if a call to onAlert function (in a listener) returned true (i.e. it had "handled" the alert in some manner). As we have no listeners at the moment, that won't get called and we'll always put up an alert - although extensions could turn it off if they really wanted to, the effect of which I've tried to explain in the idl.

(In reply to comment #11)
> (From update of attachment 365860 [details] [diff] [review])
> We'll get started and then think about categorising later :-)

My general thought is that the categories will have to come from the back end and be passed through in some manner anyway. The other option would just be to pass error codes, protocol and identity/connection and then have a map in the service where we could translate everything - however we can explore this more in a different bug.

	David :Bienvenu
	Comment 15 • 15 years ago

> 
> David - that would only happen if a call to onAlert function (in a listener)
> returned true (i.e. it had "handled" the alert in some manner). As we have no
> listeners at the moment, that won't get called and we'll always put up an alert
> - although extensions could turn it off if they really wanted to, the effect of
> which I've tried to explain in the idl.

I'm saying we don't want to put up modal alerts when the msg window is null. Putting up non-modal poptart-style alerts when we have those is OK, but for now, I think you want to avoid alerts when the msg window is null.

	Karsten Düsterloh
	Comment 16 • 15 years ago

In SM we have the problem that biff can be triggered by non-message windows (eg on browser startup): nsImapProtocol::GetPassword etc. fail to request the password, because there's no message window to tie the password dialog to.
I'm not sure that requesting *message* windows in this circumstance should be required, it should only be preferred - we don't (ever?) seem to use the *message* aspect anyway...

	Mark Banner (:standard8) Assignee
	Comment 17 • 15 years ago

(In reply to comment #16)
> In SM we have the problem that biff can be triggered by non-message windows (eg
> on browser startup): nsImapProtocol::GetPassword etc. fail to request the
> password, because there's no message window to tie the password dialog to.

Passwords are a special case that I'm not dealing with here, just protocol errors/warnings etc.

> I'm not sure that requesting *message* windows in this circumstance should be
> required, it should only be preferred - we don't (ever?) seem to use the
> *message* aspect anyway...

I've been talking with David on irc about this. Currently if we have a nsIMsgWindow (i.e. a three-pane or standalone window) passed to us, then we prompt, if we don't then we don't prompt but just ignore it. To say it a different way... if we were given an nsIMsgWindow, the user prompted the interaction, it we weren't, then its a background activity, so we shouldn't be sticking a modal dialog in their face.

With the version of the patch that I'm revising, then we will be doing exactly the same - except there's an opportunity for listeners to the new alert function to do a variety of things e.g. log it, put up a non-intrusive alert, set an indicator etc.

So this patch will be pretty much keeping the status quo, but allowing us to experiment with different feedback mechanisms.

	Mark Banner (:standard8) Assignee
	Comment 18 • 15 years ago

Slightly revised prompt only when we have a nsIMsgPrompt (and the listeners return false), updated the idl comments and the unit test. Carrying forward sr, requesting r.

	David :Bienvenu
	Comment 19 • 15 years ago

Comment on attachment 366389 [details] [diff] [review]
The fix v2

looks better, thx. - I haven't tried running this patch, but I ran with the prev patch for a while.

	Mark Banner (:standard8) Assignee
	Comment 20 • 15 years ago

Patch checked in: http://hg.mozilla.org/comm-central/rev/79f7c64d3252

I'll raise separate bugs for extending this and adding more info later (e.g. categories).