1152644 - Add UI in Notifications preference pane whether or not to use libnotify for new-mail alerts on Linux

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1144719 +++

[...] 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.

This bug aims for adding a checkbox or radiogroup for this preference setting.

Comment 1

[rsx11m]

Attachment: Screenshot WIP #1

Checkbox version, with a thin separator to visualize that this setting is a bit different than the ones on the alert content before.

Easiest to implement, though it's not quite conveying the "either/or" paradigm seen in the radiogroup selector for the sound. The Windows balloon and Mac OSX bouncing-icon options are checkboxes given that they are mechanisms different from the alert itself.

Comment 2

[rsx11m]

Attachment: Screenshot WIP #2

Radiogroup version as a separate vbox. Looks more consistent with the sound options underneath, I had to change an existing accesskey though to accommodate the built-in notification option.

Interestingly, it makes a difference if class="indent" is specified for the vbox or the individual checkboxes it contains (which result in an indentation of a few pixels less). Even more, that doesn't apply to the radiogroup which has a consistent indentation level. So, this looks a bit odd.

Comment 3

[rsx11m]

Attachment: Screenshot WIP #3

Moving all checkboxes and the radiogroup into a common vbox with class="indent" applied results in a consistent indentation level of all elements.

This version was implemented.

Comment 4

[rsx11m]

Attachment: Proposed patch

Fairly straight-forward. As a drive-by fix, I'm disabling the logic for the interaction between the balloon and the regular alert when not on Windows as is was creating a mail.biff.show_balloon preference on Linux which has no effect.

The inner useSystemAlertBox vbox was added for consistency with the other platforms and to allow hidding both the radiogroup and the separator with a single id.

Help content was updated accordingly and should be visible on Linux only.

Comment 5

[rsx11m]

Attachment: Proposed patch (v2)

I have to remember to also adjust the comments after copy-pasting...  :-[

Comment 6

[Ian Neal]

Comment on attachment 8602436 [details] [diff] [review]
Proposed patch (v2)

Seems to do the right thing for linux, hopefully Neil can confirm it's good for Windows too.

Comment 7

[rsx11m]

I have double-checked that the Windows side still works as intended...  :-)

Comment 8

[[e-mail]]

Comment on attachment 8602436 [details] [diff] [review]
Proposed patch (v2)

Well, I got here at last.

If we're using system alerts, should we disable the preview options? Also I'm not 100% convinced about the wording. Maybe something like this?

[X] Show an alert for [10] seconds
    [X] Use XUL notifications
    [X] Show a preview of the message text
    [X] Show the subject
    [X] Show the sender

Comment 9

[rsx11m]

(In reply to [e-mail] from comment #8)
> If we're using system alerts, should we disable the preview options?

My thinking is that the options still (should) apply, but it's a bit broken right now anyway as the subject and sender items aren't included in the alert even if their boxes are checked (bug 1152773).

>     [X] Use XUL notifications

That label is definitely too "geeky" and could be grasped only by someone familiar with Gecko and XUL. Also, as explained in comment #2, I've used the radio buttons on purpose to be able to (a) have two sufficiently explicit labels for each open, and (b) to separate visually the "how" from the "what" options. I'm flexible with the wording if you have a better idea, but would prefer the chosen layout.

Comment 10

[rsx11m]

Assuming from your suggestion that the aim is to be more explicit with the labels, how about this:

  ( ) Use the operating system's desktop notifications
  (o) Use SeaMonkey's own notification windows

and refer to XUL (we have a short glossary entry for that already) and libnotify in the Help text?

Comment 11

[[e-mail]]

Sorry for taking so long to get back to this.

(In reply to comment #8)
> If we're using system alerts, should we disable the preview options?

Not only that, but I think the alert time doesn't apply to system alerts either, so that's going to fall flat anyway.

Comment 12

[[e-mail]]

(In reply to rsx11m from comment #10)
> Assuming from your suggestion that the aim is to be more explicit with the
> labels, how about this:
> 
>   ( ) Use the operating system's desktop notifications
>   (o) Use SeaMonkey's own notification windows

This is good, but I think Help should mention that some of the customisation options might not apply to desktop notifications.

Comment 13

[[e-mail]]

Comment on attachment 8602436 [details] [diff] [review]
Proposed patch (v2)

ui-r=me with those wording tweaks.

Comment 14

[rsx11m]

Attachment: Proposed patch (v3)

Neil's comments addressed in this patch:
  - labels modified following comment #10
    - requires adding brand.dtd to XUL code
    - allowed to retain accesskey for showAlertSender;
  - Help content updated per comments #12 and #10
    - rather than tweaking the logic, per comment #11.

IanN, re-requesting review from you, primarily for the Help-content additions.

Comment 15

[rsx11m]

Thanks, push for comm-central please.

Comment 16

[:aceman]

https://hg.mozilla.org/comm-central/rev/2694ee55d910

Comment 17

[rsx11m]

Thanks aceman! I think we are at SM 2.42 on trunk right now, though, thus correcting the milestone.