Closed Bug 1144719 Opened 5 years ago Closed 5 years ago
Allow the user to decide whether or not to use libnotify for new-mail alerts on Linux
Bug 858919 allows usage of libnotify on Linux again, if available. The underlying code for nsMessengerUnixIntegration::ShowAlertMessage() in mailnews/base/src/nsMessengerUnixIntegration.cpp#318 attempts to submit the message using the system alert service first; if that fails, it falls back to using ShowNewAlertNotification() instead. Bug 1144693 issues aside, there is no reason why to force the user to depend on libnotify for new-mail (and possibly other) alerts. On Windows, there is a dedicated preference ("mail.biff.show_balloon") to use the system's mechanism of icon ballons for new-mail notifications. Similarly, a preference on Mac OSX ("mail.biff.animate_dock_icon") enables or disables that desktop effect. There may be reasons for a user to revert to the built-in XUL notification instead: Be it configurability for the time it is shown (thus being able to read it before it disappears again), or simply to have a dedicated style for such alerts raising better attention to it. Thus, suggestion to provide a preference on Linux ("mail.biff.use_system_alert" defaulting to "true") which would determine in nsMessengerUnixIntegration::ShowAlertMessage() whether or not the if() statement for using the system alert service and returning if it was successful is entered. With that pref set to "false" the fallback ShowNewAlertNotification() would be used instead.
> configurability for the time it is shown there are dozens of providers for the notification API. users can always choose one that can be configured how they want it. you’re still right: why *not* addd this option?
(In reply to flying sheep from comment #1) > there are dozens of providers for the notification API. users can always > choose one that can be configured how they want it. Thanks, guess I just didn't find that option yet in the KDE preferences for the notifications... Anyway, I still rather like the distinct XUL notification for new mail, thus I'll go ahead and work out a patch for adding the preference (proof of concept works per bug 1144693 comment #2).
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Minimal patch, with room for improvement. The comments in the prefs should probably be converted into real #ifdef clauses, thus we don't offer those prefs for platforms which aren't using them. Also, if useSystemAlert is false, no need to obtain alertsService in the first place.
This patch resolves a typo in the pref definition of the WIP version and won't check for the libnotify service being available if the pref is false anyway. Also "#ifdef"s the pref (and some other platform-specific prefs as a drive-by fix). As said, without this pref, users won't be able to get notified for new mail by the built-in alert at all and have to rely on libnotify to do the job.
This patch is only needed for testing and goes on top of attachment 8581187 [details] [diff] [review]. Since the libnotify alerts may not be working right now, this patch is adding some crude stderr debug output to see when the notification was triggered. Also, it works around bug 1144693 comment #3 by explicitly calling AlertFinished() upon success of alertsService->ShowAlertNotification(). If the pref is introduced as proposed before bug 1144693 is fixed, the pref could be initially set to "false" rather than "true" and thus preventing the case that the user doesn't see any notification whatsoever until that issue is fixed.
Since Neil approved it and Joshua didn't voice any opinion, I assume that this is ready to land. Push for comm-central, please.
Whiteboard: [parity-win32] → [parity-win32][c-n: #8581187 for comm-central]
Comment on attachment 8581187 [details] [diff] [review] Proposed patch [Approval Request Comment] Regression caused by (bug #): Bug 858919 User impact if declined: no or uncertain new-mail notifications on Linux Testing completed (on c-c, etc.): works fine on trunk Risk to taking this patch (and alternatives if risky): low, default behavior unchanged pending bug 1144693 attachment 8590044 [details] [diff] [review] Requesting approval-comm-release for possible SeaMonkey 2.33.2 and/or 2.34.
Whiteboard: [parity-win32][c-n: #8581187 for comm-central] → [parity-win32]
Comment on attachment 8581187 [details] [diff] [review] Proposed patch https://hg.mozilla.org/releases/comm-aurora/rev/415721b0657e https://hg.mozilla.org/releases/comm-beta/rev/de98ea1bf621 comm-release is SeaMonkey only, so I did not do the approval or uplift for that.
(In reply to Kent James (:rkent) from comment #9) > comm-release is SeaMonkey only, so I did not do the approval or uplift for that. Thanks, Ian?
Depends on whether there is another 2.33.x
Flags: needinfo?(iann_bugzilla) → needinfo?(ewong)
(In reply to Ian Neal from comment #11) > Depends on whether there is another 2.33.x The likelihood is no. Right now 2.35 is release and I'm wondering if it's even likely we do a 2.35 beta; but I suppose that's Callek's decision.
Flags: needinfo?(ewong) → needinfo?(bugspam.Callek)
If we can do a 2.35 in a reasonable time frame, that's certainly preferable. I've left the requests (here and for the dependent patch in bug 1144693) up in case we can't.
Comment on attachment 8581187 [details] [diff] [review] Proposed patch SM 2.33.2 is off the table, and 2.35 has the fix, thus cancelling request.
You need to log in before you can comment on or make changes to this bug.