Closed Bug 1152644 Opened 5 years ago Closed 4 years ago

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

Categories

(SeaMonkey :: Preferences, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.42

People

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

References

Details

(Whiteboard: [parity-win32])

Attachments

(4 files, 2 obsolete files)

+++ 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.
Attached image 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.
Attached image 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.
Attached image 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.
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Attachment #8602373 - Flags: ui-review?(neil)
Attachment #8602373 - Flags: review?(iann_bugzilla)
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
I have to remember to also adjust the comments after copy-pasting...  :-[
Attachment #8602373 - Attachment is obsolete: true
Attachment #8602373 - Flags: ui-review?(neil)
Attachment #8602373 - Flags: review?(iann_bugzilla)
Attachment #8602436 - Flags: ui-review?(neil)
Attachment #8602436 - Flags: review?(iann_bugzilla)
Depends on: 1162788
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.
Attachment #8602436 - Flags: review?(iann_bugzilla) → review+
I have double-checked that the Windows side still works as intended...  :-)
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
(In reply to neil@parkwaycc.co.uk 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.
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?
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.
(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 on attachment 8602436 [details] [diff] [review]
Proposed patch (v2)

ui-r=me with those wording tweaks.
Attachment #8602436 - Flags: ui-review?(neil) → ui-review+
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.
Attachment #8602436 - Attachment is obsolete: true
Attachment #8665659 - Flags: ui-review+
Attachment #8665659 - Flags: review?(iann_bugzilla)
Attachment #8665659 - Flags: review?(iann_bugzilla) → review+
Thanks, push for comm-central please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2694ee55d910
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.41
Thanks aceman! I think we are at SM 2.42 on trunk right now, though, thus correcting the milestone.
Target Milestone: seamonkey2.41 → seamonkey2.42
You need to log in before you can comment on or make changes to this bug.