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

Categories

(MailNews Core :: Backend, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed, seamonkey2.34 wontfix, seamonkey2.35 fixed, seamonkey2.36 fixed, seamonkey2.37 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
seamonkey2.34 --- wontfix
seamonkey2.35 --- fixed
seamonkey2.36 --- fixed
seamonkey2.37 --- fixed

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

(Whiteboard: [parity-win32])

Attachments

(2 files, 1 obsolete file)

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
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch Proposed patchSplinter Review
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.
Attachment #8581140 - Attachment is obsolete: true
Attachment #8581187 - Flags: review?(neil)
Attachment #8581187 - Flags: review?(Pidgeot18)
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.
Attachment #8581187 - Flags: review?(neil) → review+
Attachment #8581187 - Flags: review?(Pidgeot18)
Since Neil approved it and Joshua didn't voice any opinion, I assume that this is ready to land.
Push for comm-central, please.
Keywords: checkin-needed
Whiteboard: [parity-win32] → [parity-win32][c-n: #8581187 for comm-central]
Blocks: 1152644
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.
Attachment #8581187 - Flags: approval-comm-release?
Attachment #8581187 - Flags: approval-comm-beta?
Attachment #8581187 - Flags: approval-comm-aurora?
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.
Attachment #8581187 - Flags: approval-comm-beta?
Attachment #8581187 - Flags: approval-comm-beta+
Attachment #8581187 - Flags: approval-comm-aurora?
Attachment #8581187 - Flags: approval-comm-aurora+
(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?
Flags: needinfo?(iann_bugzilla)
Blocks: 1144693
No longer depends on: 1144693
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.
Attachment #8581187 - Flags: approval-comm-release?
Flags: needinfo?(bugspam.Callek)
You need to log in before you can comment on or make changes to this bug.