Bug 1690093 Comment 33 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

>You mean the Windows/Linux/Mac notifications, not popup windows within Thunderbird, right? That's fine, because they are intended for background notifications, i.e. actions that the application initiated and not the end user. I agree, that's an appropriate place to show such errors.

>As far as I remember, AlertUserEventUsingName() is not the same as these OS notifications, but IIRC can get converted back into an error for the caller. That may or may not do what we want here. You'd need to go back the call stack, for all possible callers - there are several scenarios. If you can show what this does in all the different cases, and that is the right thing in every case, I'd be happy to revert my r- into a r+ for that patch.

Ok, sorry I misunderstood this comment when I first read it about 2 weeks ago. I think you are saying you "almost approve" of my 2nd diff patch above except you are not sure about the call to AlertUserEventUsingName(). 
I was not able to get the notification to appear as an OS pop-up. It only appeared in the Activity Manager. I did some deeper debugging. The message string only goes to activity manager  because the associated URL has no msgWindow.

What happens is here
https://searchfox.org/comm-central/rev/e85e0354ecc1a5343a501e8c7a61eb935c1b8e81/mailnews/base/src/nsMsgMailSession.cpp#183
OnAlert() is called as defined here:
https://searchfox.org/comm-central/rev/8b2121df11b0c25fcd4d594247c411e324e81072/mail/components/activity/modules/alertHook.jsm#45
It looks like OnAlert() sends the warning message to the activity manager and, since the msgWindow is null for the URL, OnAlert() is exited without actually triggering the alert. 
Where OnAlert() is called from the .cpp code in AlertUser() at the first searchfox link, bool listenerNotified is set true so AlertUser() exits resulting in just the message appearing in activity mgr. But even if listenerNotified wasn't true, again nothing more would happen in AlertUser() because msgWindow is null. (FWIW, I changed to code in AlertUser() to ignore msgWindow and allowed to it use the "default" dialog and a modal window appeared with my message in it with an OK button.)

This allows tb to trigger the OS notification by ignoring the null msgWindow in OnAlert():
```
diff --git a/mail/components/activity/modules/alertHook.jsm b/mail/components/activity/modules/alertHook.jsm
--- a/mail/components/activity/modules/alertHook.jsm
+++ b/mail/components/activity/modules/alertHook.jsm
@@ -55,17 +55,18 @@ var alertHook = {
       warning.groupingStyle = Ci.nsIActivity.GROUPING_STYLE_STANDALONE;
     }
 
     this.activityMgr.addActivity(warning);
 
     // If we have a message window in the url, then show a warning prompt,
     // just like the modal code used to. Otherwise, don't.
     try {
-      if (!aUrl || !aUrl.msgWindow) {
+      if (!aUrl) {
         return true;
       }
     } catch (ex) {
       // nsIMsgMailNewsUrl.msgWindow will throw on a null pointer, so that's
       // what we're handling here.
       if (
         ex instanceof Ci.nsIException &&
         ex.result == Cr.NS_ERROR_INVALID_POINTER
```
I don't know if this causes other problems but with this change my new message now appears as an OS notification instead of only in Activity Mgr.
>You mean the Windows/Linux/Mac notifications, not popup windows within Thunderbird, right? That's fine, because they are intended for background notifications, i.e. actions that the application initiated and not the end user. I agree, that's an appropriate place to show such errors.

>As far as I remember, AlertUserEventUsingName() is not the same as these OS notifications, but IIRC can get converted back into an error for the caller. That may or may not do what we want here. You'd need to go back the call stack, for all possible callers - there are several scenarios. If you can show what this does in all the different cases, and that is the right thing in every case, I'd be happy to revert my r- into a r+ for that patch.

Ok, sorry I misunderstood this comment when I first read it about 2 weeks ago. I think you are saying you "almost approve" of my 2nd diff patch above except you are not sure about the call to AlertUserEventUsingName(). 
I was not able to get the notification to appear as an OS pop-up. It only appeared in the Activity Manager. I did some deeper debugging. The message string only goes to activity manager  because the associated URL has no msgWindow.

What happens is here
https://searchfox.org/comm-central/rev/e85e0354ecc1a5343a501e8c7a61eb935c1b8e81/mailnews/base/src/nsMsgMailSession.cpp#183
OnAlert() is called as defined here:
https://searchfox.org/comm-central/rev/8b2121df11b0c25fcd4d594247c411e324e81072/mail/components/activity/modules/alertHook.jsm#45
It looks like OnAlert() sends the warning message to the activity manager and, since the msgWindow is null for the URL, OnAlert() is exited without actually triggering the alert. 
Where OnAlert() is called from the .cpp code in AlertUser() at the first searchfox link, bool listenerNotified is set true so AlertUser() exits resulting in just the message appearing in activity mgr. But even if listenerNotified wasn't true, again nothing more would happen in AlertUser() because msgWindow is null. (FWIW, I temporarily changed to code in AlertUser() to ignore msgWindow and allowed to it use the "default" dialog and a modal window appeared with my message in it with an OK button.)

This allows tb to trigger the OS notification by ignoring the null msgWindow in OnAlert():
```
diff --git a/mail/components/activity/modules/alertHook.jsm b/mail/components/activity/modules/alertHook.jsm
--- a/mail/components/activity/modules/alertHook.jsm
+++ b/mail/components/activity/modules/alertHook.jsm
@@ -55,17 +55,18 @@ var alertHook = {
       warning.groupingStyle = Ci.nsIActivity.GROUPING_STYLE_STANDALONE;
     }
 
     this.activityMgr.addActivity(warning);
 
     // If we have a message window in the url, then show a warning prompt,
     // just like the modal code used to. Otherwise, don't.
     try {
-      if (!aUrl || !aUrl.msgWindow) {
+      if (!aUrl) {
         return true;
       }
     } catch (ex) {
       // nsIMsgMailNewsUrl.msgWindow will throw on a null pointer, so that's
       // what we're handling here.
       if (
         ex instanceof Ci.nsIException &&
         ex.result == Cr.NS_ERROR_INVALID_POINTER
```
I don't know if this one line change causes other problems but with this change my new message now appears as an OS notification instead of only in Activity Mgr.

Back to Bug 1690093 Comment 33