Closed Bug 856454 Opened 11 years ago Closed 11 years ago

Add ability to customise new mail alert information

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.20

People

(Reporter: iannbugzilla, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

We should expose the preferences to customise new mail alert information. This should be a fairly straight forward port of the TB work but the help information will also need to be added.
Component: MailNews: Message Display → Preferences
Compared to the Thunderbird implementation which has less space in their options panels and keeps these settings in their General pane, SeaMonkey's Notifications page could accommodate the additional checkboxes easily.

Thus, this may be simpler here given that a separate dialog window isn't needed.
I'm looking into the current implementation of Thunderbird's preferences and was wondering why they seem to be upside-down by placing the "Message Preview Text" option first. Reading the description of the initial bug,

(Quoting Scott MacGregor from bug 360030)
> Based on feedback from mcow and others, we should make it easy for a user to
> choose which pieces of information to show in the new mail alerts. In
> particular, this would allow users to turn off the preview text if they are
> worried about folks seeing message subjects on their machine.

it appears that the checkboxes were sorted in the order of how likely it might be that the user would switch that option off.

Thus, a possible layout for SeaMonkey's notification pane would be

[ ] Show an alert for [__] seconds
    [ ] Show a preview of the message text
    [ ] Show the subject of the message
    [ ] Show the sender of the message

I'm suggesting explicit "Show" labels here as something like "Show an alert for [__] seconds with:" may be a nightmare to localize.
Attached patch Proposed patch (w/o Help) (obsolete) — Splinter Review
This is the patch implementing my comment #2, where I've skipped the repeated "of the message" in the 2nd and 3rd checkbox labels.

A couple of observations:

- This introduces some new vboxes to add thin separators at the end of each
  subgroup, thus to make the dependencies clearer to the user; otherwise, it's
  too tight back to back.

- Consequently, pref-notifications.js now toggles "hidden" of the *Box elements.

- The total display time is specified with one decimal digit, but increments
  are in full integers; should I remove the digit? It's hard to grab to enter
  some fraction of a second, and I'm not sure about the use case.
 
- Hiding the tray icon or dock checkboxes based on the platform is differently
  implemented for the Mac OSX and the Windows versions:

    in bug 322923, a check for not being on a Mac was introduced:

>+  if (!/Mac/.test(navigator.platform))
>+    document.getElementById('newMailNotificationBounce').setAttribute("hidden", true);

    in bug 133971, the following variant was used for Windows:

>+  // show tray icon option currently available for Windows only
>+  var newMailNotificationTrayIconPref = document.getElementById("newMailNotificationTrayIcon");
>+  newMailNotificationTrayIconPref.hidden = !/^Win/.test(navigator.platform);

  since I recall that attribute assignments are more direct than properties,
  I've modified the Windows version to match the Mac OSX version, given that
  both required changes in the target element anyway.

- There were some redundant boxes around the sound preferences which I removed.

This doesn't have a Help update yet, given that I'd like to get ui-review approval first for the overall design before approaching that rewrite.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #732107 - Flags: ui-review?(neil)
Attachment #732107 - Flags: feedback?(iann_bugzilla)
Attached image Screenshot (Windows 7) (obsolete) —
BTW: The preview of the message text part doesn't work for me, with or without the patch (the checkbox toggles the pref correctly). It doesn't show up in the notification regardless of the value of mail.biff.alert.show_preview.
I think I know what's going on. I don't see the preview text with my Gmail IMAP account, which is not synchronized, but with another account which is not Gmail but synchronized, using the same build. Thus, either it's an issue with Gmail or the lack of synchronization for that account (the latter makes intuitively sense as the message content would only be available after downloading the message, thus I may not need to worry too much about that observation).
> since I recall that attribute assignments are more direct than properties,
> I've modified the Windows version to match the Mac OSX version, given that
Please use the .hidden property instead.

> !/^Win/.test(navigator.platform)
In new code and lines touched we should use startsWith/endsWith/contains. E.g.
navigator.platform.startsWith("Win");

> BTW: The preview of the message text part doesn't work for me
What platform are you on?
(In reply to Philip Chee from comment #6)
> Please use the .hidden property instead.

Ok, if that's better, I'll make the Mac version match the Windows version (I won't be able to test that change though if it works on a Mac as I only have Linux and Windows available).

> navigator.platform.startsWith("Win");

I assume that this applies to the Mac version as well? E.g., navigator.platform returns "Linux x86_64" for me, thus the startsWith() would seem to be necessary in this case.

> > BTW: The preview of the message text part doesn't work for me
> What platform are you on?

That was observed on Windows 7, both with a current 2.19a1 trunk build, but also with a 2-week old 2.18a2 aurora build and a January 25 2.18a1 trunk build.
(In reply to rsx11m from comment #7)
> > navigator.platform.startsWith("Win");
> I assume that this applies to the Mac version as well?

https://developer.mozilla.org/en-US/docs/DOM/window.navigator.platform states that "MacPPC" or "MacIntel" are the expected values, thus .startsWith() should work (though the regex translates to .contains() if taken literally).
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
Tested on current Windows and Linux trunk builds, but not on Mac OSX.

(In reply to Philip Chee from comment #6)
> Please use the .hidden property instead.
> navigator.platform.startsWith("Win");

Both items taken care of. I found other examples where .startsWith("Mac") was used, thus I assume that it will work.

(In reply to rsx11m from comment #3)
> - The total display time is specified with one decimal digit, but increments
>   are in full integers; should I remove the digit? It's hard to grab to enter
>   some fraction of a second, and I'm not sure about the use case.

I have removed the decimalplaces="1" line, it simplifies handling of the widget. Users can still change the pref in about:config if they really want 3.142 seconds.

No other changes relative to attachment 732107 [details] [diff] [review].
Attachment #732107 - Attachment is obsolete: true
Attachment #732107 - Flags: ui-review?(neil)
Attachment #732107 - Flags: feedback?(iann_bugzilla)
Attachment #732546 - Flags: ui-review?(neil)
Attachment #732546 - Flags: feedback?(iann_bugzilla)
Comment on attachment 732546 [details] [diff] [review]
Proposed patch (v2)

[I wonder whether we should add mail.biff.show_balloon too]

>+                   onsyncfrompreference="let tot = document.getElementById(this.getAttribute('preference'));
>+                                         return (tot.instantApply ? tot.valueFromPreferences : tot.value) / 1000;"
I'm not sure what the point of this check is; if instantApply is true then value should equal valueFromPreferences (although in pref-proxies.js we test both anyway, probably unnecessarily).

>-        <checkbox id="newMailNotification"
>-                  label="&playSound.label;"
>-                  accesskey="&playSound.accesskey;"
>-                  preference="mail.biff.play_sound"/>
Is this just diff being dumb?

>-      <hbox align="center" class="indent">
>-        <radiogroup id="newMailNotificationType"
So, did you actually change anything useful here? I'm not convinced it's worth the effort just to remove an hbox. (If you are set on it, then I think I saw at least one unused attribute that can be removed.)
Attachment #732546 - Flags: ui-review?(neil) → ui-review+
(In reply to neil@parkwaycc.co.uk from comment #10)
> [I wonder whether we should add mail.biff.show_balloon too]

I was thinking about that, but it opens a bit of a can of worms:

 - it applies only to Windows (or not?);

 - you don't want to have the XUL notification being active at the same time when
   the balloon notification is active, thus they need to be mutually exclusive;

 - it'll also depend on the tray-icon setting, thus would need to be disabled and
   effectively set to false to avoid triggering the icon+balloon when the user
   switches off the tray icon.

Thus, I figured I'm not pursuing that route though it would be nice to have. The interdependencies may be hard to convey in the UI and complicate the JS part.

> I'm not sure what the point of this check is; if instantApply is true then
> value should equal valueFromPreferences (although in pref-proxies.js we test
> both anyway, probably unnecessarily).

I'll have to double-check that. That's a part I've copied verbatim from the patch in bug 856451. Assuming that it only applies to the Linux side, I'd remove it if the pref follows accurately the UI setting and vice versa.

> Is this just diff being dumb?

Probably, it seems to be mixing up old and new <vbox>es here, and white-space changes confuse hg diff frequently as well. Other than rearranging the boxes, I didn't change anything here.

> So, did you actually change anything useful here? I'm not convinced it's
> worth the effort just to remove an hbox.

A couple of patches back you told me to clean up some redundant boxes even in the code I didn't touch directly, thus I had a closer look here as well. I'm basically just pulling the class="indent" attribute from the hbox into the radiogroup itself, which works fine, then getting rid of the hbox around it.

I'm open one way or the other. If we don't want to mess up hg blame too much, I'll revert this to the previous code with surrounding hbox, no problem.

> (If you are set on it, then I think I saw at least one unused attribute that
>  can be removed.)

I don't know what class="iconic" is doing, that might be a candidate?
(In reply to neil@parkwaycc.co.uk from comment #10)
> >-        <checkbox id="newMailNotification"
> >-                  label="&playSound.label;"
> >-                  accesskey="&playSound.accesskey;"
> >-                  preference="mail.biff.play_sound"/>
> Is this just diff being dumb?

I see = this was part of a vbox combining all four checkboxes above the radio buttons, which I've split up into individual vboxes for the subgroups, so this specific part was moved out of the vbox and appears added back further down.
(In reply to rsx11m from comment #11)
> (In reply to neil@parkwaycc.co.uk from comment #10)
> > [I wonder whether we should add mail.biff.show_balloon too]

If we want to go for this, I'd suggest following extensions to XUL and JS parts:

>  - it applies only to Windows (or not?);

I'd think so, and it should be directly tied with the tray-icon checkbox, thus residing in the same vbox (and hence its visibility depending on it):

[ ] Show an alert for [__] seconds
    [ ] Show a preview of the message text
    [ ] Show the subject
    [ ] Show the sender

[ ] Show a tray icon
    [ ] Show a balloon alert for the icon

>  - you don't want to have the XUL notification being active at the same time when
>    the balloon notification is active, thus they need to be mutually exclusive;

Following logic:
 - If you check "Show an alert" to enable it, "Show a balloon" will be unchecked;
 - if you checked "Show a tray icon" and then check "Show a balloon" to enable,
   "Show an alert" will be cleared and all related checkboxes disabled.
   ("Show a balloon" will be disabled with "Show a tray icon" not checked.)

If we accept the user to check both options, the XUL alert will be shown first, and after its fade-out the balloon notification (i.e., not simultaneously).

>  - it'll also depend on the tray-icon setting, thus would need to be disabled and
>    effectively set to false to avoid triggering the icon+balloon when the user
>    switches off the tray icon.

This implies that if you uncheck "Show a tray icon" the "Show a balloon" checkbox is unchecked as well and disabled as the balloon implies the tray icon.

Please let me know what you think.
Flags: needinfo?(neil)
Comment on attachment 732546 [details] [diff] [review]
Proposed patch (v2)

OK, I take it back about the hbox.

>+      <radiogroup id="newMailNotificationType"
>+                  preference="mail.biff.play_sound.type"
>+                  class="indent"
>+                  orient="vertical"
>+                  aria-labelledby="newMailNotification">
I think the orient is unnecessary.

>+        <radio id="system"
>+               class="iconic"
>+               value="0"
>+               label="&systemsound.label;"
>+               accesskey="&systemsound.accesskey;"/>
>+        <radio id="custom"
>+               class="iconic"
>+               value="1"
>+               label="&customsound.label;"
>+               accesskey="&customsound.accesskey;"/>
You're right, the class="iconic" is unnecessary.
Flags: needinfo?(neil)
(In reply to rsx11m from comment #13)
> (In reply to rsx11m from comment #11)
> > (In reply to comment #10)
> > > [I wonder whether we should add mail.biff.show_balloon too]
> >  - it applies only to Windows (or not?);
> [ ] Show a tray icon
>     [ ] Show a balloon alert for the icon
Yes, that makes sense.

> Following logic:
>  - If you check "Show an alert" to enable it, "Show a balloon" will be
> unchecked;
>  - if you checked "Show a tray icon" and then check "Show a balloon" to
> enable,
>    "Show an alert" will be cleared and all related checkboxes disabled.
I'm undecided on this, but maybe IanN has a preference (pun not intended).

> This implies that if you uncheck "Show a tray icon" the "Show a balloon"
> checkbox is unchecked as well and disabled as the balloon implies the tray
> icon.
No, just disabled, in the same way that unchecking "Show an alert" will disable all related fields withut clearing them.
Attached patch Proposed patch (v3) (obsolete) — Splinter Review
(In reply to neil@parkwaycc.co.uk from comment #10)
> >+                                         return (tot.instantApply ? tot.valueFromPreferences : tot.value) / 1000;"
> I'm not sure what the point of this check is; if instantApply is true then
> value should equal valueFromPreferences

Works fine with a simplified version based on "value" alone on both Windows and Linux, thus I've removed the instantApply/valueFromPreferences check and made it a single statement.

(In reply to neil@parkwaycc.co.uk from comment #14)
> I think the orient is unnecessary.

Removed and works.

> You're right, the class="iconic" is unnecessary.

Both instances removed, still works.


(In reply to neil@parkwaycc.co.uk from comment #15)
> > Following logic:
> >  - If you check "Show an alert" to enable it, "Show a balloon" will be unchecked;
> >  - if you checked "Show a tray icon" and then check "Show a balloon" to enable,
> >    "Show an alert" will be cleared and all related checkboxes disabled.
> I'm undecided on this, but maybe IanN has a preference (pun not intended).

That's what this patch implements, thus to allow you plating with it. There is a trade-off either way:

(1) The user looks at the Notifications pane and wants to try the balloon notification, thus checks the "Show balloon" box.

(2a) If we don't uncheck the "Show alert" box, first the regular alert appears for 10 seconds, then fades out, and after that the balloon appears.

(2a1) The user may be confused about this behavior and the duplication of the alert.

(2a2a) He or she gets the idea and unchecks the "Show alert" box.

(2a2b) He or she thinks it's a bug and unchecks the "Show balloon" box again.

(2b) If we uncheck the "Show alert" box as proposed, only the balloon notification appears on new mail without any further action by the user.

(2b1) The user may be confused here as well about the interdependencies between the different checkboxes.

(2b2) The user tries to check "Show alert" again, at which point the "Show balloon" box is cleared and only the regular alert is shown.

(2b2a) The user figures that they are mutually exclusive by checking either one or the other.

(2b2b) The user remains to be confused and leaves it at the "Show alert" setting.

(2b2c) The user indeed wants to have both alerts shown on purpose, in which case he or she needs to be pointed to about:config for a custom configuration.

Looking at both scenarios, I think that making the boxes mutually exclusive is better for the feature to be properly discovered and used.

Either way, this will need a Note in the Help content.

> > This implies that if you uncheck "Show a tray icon" the "Show a balloon"
> > checkbox is unchecked as well and disabled as the balloon implies the tray
> > icon.
> No, just disabled, in the same way that unchecking "Show an alert" will
> disable all related fields withut clearing them.

This won't work, unfortunately. I have to actively reset the preference (and thus the checkbox) if "Show tray icon" is cleared. That's a backend issue, if you have mail.biff.show_balloon set it will show an icon regardless of the mail.biff.show_tray_icon value. I don't know which trade-offs may have been necessary to implement the feature, but as of now the patch needs to uncheck the box as is.

I can look into the logic behind that, but this would be a separate bug for MailNews Core. Thus, I'd like to keep the unchecking part here for the time being.

I'm re-requesting ui-review? given that there were quite a few UX-relevant changes.
Attachment #732546 - Attachment is obsolete: true
Attachment #732546 - Flags: feedback?(iann_bugzilla)
Attachment #732837 - Flags: ui-review?(neil)
Attachment #732837 - Flags: feedback?(iann_bugzilla)
This illustrates the logic:

 - 1st image: Default on Windows
   - click on "Show a balloon alert" -> go to 2nd image

 - 2nd image: "Show an alert" unchecked
   - click on "Show an alert" -> back to 1st image
   - uncheck "Show a tray icon" -> go to 3rd image

 - 3rd image: "Show a balloon alert" automatically unchecked too
   - check "Show an alert" again to get close to 1st image
     - check "Show a tray icon" again to match 1st image
   - check "Show a tray icon" again to get close to 2nd image
     - check "Show a balloon alert" again to match 2nd image
Attachment #732109 - Attachment is obsolete: true
(In reply to rsx11m from comment #16)
> That's what this patch implements, thus to allow you plating with it.

Oops, s/plating/playing/
Depends on: 857647
Comment on attachment 732837 [details] [diff] [review]
Proposed patch (v3)

>+function EnableBalloon(aEnable)
>+{
Nit: Should be EnableTrayIcon for consistency with EnableAlert

>+  // the balloon alert implies the icon, thus we need them both cleared
>+  if (!aEnable) 
>+    document.getElementById("mail.biff.show_balloon").value = false;
So bug 857647 will make this redundant, depending on which lands first.
Attachment #732837 - Flags: ui-review?(neil) → ui-review+
(In reply to neil@parkwaycc.co.uk from comment #19)
> Nit: Should be EnableTrayIcon for consistency with EnableAlert

Ok, will do.

> So bug 857647 will make this redundant, depending on which lands first.

Yes, that's the plan.
Attached patch Proposed patch (v4) (obsolete) — Splinter Review
Neil's comments addressed, I'll be working on updating the Help content now.
Attachment #732837 - Attachment is obsolete: true
Attachment #732837 - Flags: feedback?(iann_bugzilla)
Attachment #733104 - Flags: ui-review+
Attachment #733104 - Flags: feedback?(iann_bugzilla)
I'm struggling to verify if this statement in the current Help is still accurate:

"Show an alert: ... The sliding alert only appears once when new messages arrive, and won't appear again until you bring the Mail & Newsgroups window to the front."

It won't appear for me when first opening the Mail & News window and new mail is collected (that may be on purpose), but the balloon does. When closed, the alerts only show for the first message(s) received but not for any subsequent batches. For the balloon, the determining factor is whether or not the icon is still shown. Thus, focusing the Mail & News window alone won't remove it, only when a new message is opened.

If the same applies to the desktop alert, the last sentence should read "until you read a new message." instead to clear the icon?
The bugmail for the previous post answered this question, it is "yes" - for the desktop alert to appear, the tray icon needs to be cleared as well. Even more, after sending myself two message without seeing the alert, then reading one to clear the icon, sending another one, I saw an alert with "1 new message" but then three entries (one for the actually new message and two more for the ones which arrived with the previous batch).
Attached patch Proposed patch (v5) (obsolete) — Splinter Review
Same as attachment 733104 [details] [diff] [review] but now with Help content added:

 - updated desktop alert screenshot, mail_newmail_alert.png
 - created a new balloon screenshot, mail_newmail_balloon.png

 - updates to "Show an alert" description, removing "sliding"
 - added description of the alert's customization options

 - added note on stupid Windows 7 default to hide the icon by default
 - added balloon alert description, along with a note per comment #16

Remarks:
 - using 'x' for the close button is on purpose, UTF-8 '×' looks too small
 - the &nbsp; in "Balloon alert" is on purpose too, to ensure that it wraps
   as a whole when shrinking the width of the Help window

This is now ready for IanN's review, carrying forward Neil's ui-r+.
Attachment #733104 - Attachment is obsolete: true
Attachment #733104 - Flags: feedback?(iann_bugzilla)
Attachment #733343 - Flags: ui-review+
Attachment #733343 - Flags: review?(iann_bugzilla)
Attached patch Proposed patch (v6) (obsolete) — Splinter Review
Updated to tip with the workaround for bug 857647 removed per comment #19. Unchecking "Show an icon" will only disable but not clear "Show a balloon" now, which will however still be cleared if "Show an alert" is checked (and vice versa) to avoid that both alerts are active at the same time.

This also fixes three typos in the Help content and modifies the statements when alert and icon are shown again. Note that "Get Msgs" will clear the icon if no messages are available, but won't show a desktop alert if there are new messages to grab (in contrast to the balloon alert which also shows when checking for new mail manually). I've omitted that difference as it may be more confusing than helpful to list too many special cases when and when not the alerts are shown.
Attachment #733343 - Attachment is obsolete: true
Attachment #733343 - Flags: review?(iann_bugzilla)
Attachment #734236 - Flags: ui-review+
Attachment #734236 - Flags: review?(iann_bugzilla)
For reference: Not showing the alert when pending despite newer mail arriving is bug 210148; the task-bar icon hiding by default on Windows 7 is bug 634656.
Comment on attachment 734236 [details] [diff] [review]
Proposed patch (v6)


>+++ b/suite/locales/en-US/chrome/common/help/mailnews_preferences.xhtml

> <ul>
>+  <li><strong>Show an alert for [__] seconds</strong>: Select this if you want 
>+    &brandShortName; Mail &amp; Newsgroups to display an alert on your desktop
>+    when new messages arrive. This alert is usually located above your system
>+    tray in the lower right corner of your screen. The alert only appears once
>+    when new messages arrive, stays for the specified amount of time, and
>+    won&apos;t appear again until you visited one of your folders with new
Missing "have", "...until you have visited one of your folders..."
>+    mail, read one of the new messages, or checked for new messages manually.

>     <p>The new message alert will continue to work even after you close the
>-      Mail window (as long as another &brandShortName; application is running).
>+      Mail window (as long as another &brandShortName; window is open).
If you are going to change this, then you need to change the one in the "Play a sound" section further down too.

>   <li class="win"><strong>Show a tray icon</strong>: Select this if you want
>     &brandShortName; Mail &amp; Newsgroups to display an icon in your system
>     tray (which is usually found in the lower right corner of your screen) when
>     new messages arrive. This icon will stay in the system tray until you have
>+    visited one of your folders with new mail, read one of the new messages,
>+    or checked for new messages manually.
>     <p style="text-indent: 20px"><img src="images/mail_newmail_trayicon.png"
>       alt=""/>&nbsp;&nbsp;<strong>New mail tray icon</strong></p>
>     <p>When the icon appears, double-clicking it will open the &brandShortName;
>       Mail &amp; Newsgroups main window.</p>
>+    <p><strong>Note</strong>: On Windows 7 and above, the Notification Area
>+      Icons settings for &brandShortName; must read <q>Show icon and
>+      notifications</q> for the icon to stay visible. Otherwise, it may be
>+      hidden after a few seconds.</p>
Not sure it is "a few seconds", I read somewhere that it was 45 seconds, perhaps we should say "a short period of time"?

r=me with those points addressed.
Attachment #734236 - Flags: review?(iann_bugzilla) → review+
Attached patch Final patch (v7)Splinter Review
Thanks Ian, all comments addressed.

(In reply to Ian Neal from comment #27)
> Missing "have", "...until you have visited one of your folders..."

Done.

> >+      Mail window (as long as another &brandShortName; window is open).
> If you are going to change this, then you need to change the one in the
> "Play a sound" section further down too.

Yes, I felt that "application" is wrong here as its the same program, but missed the other occurrence. So done.

> Not sure it is "a few seconds", I read somewhere that it was 45 seconds,
> perhaps we should say "a short period of time"?

It's a system setting. I've modified it somehow, thus I don't know the default. Given that it may change in the future, being neutral here should be better. Done.
Attachment #734236 - Attachment is obsolete: true
Attachment #737200 - Flags: ui-review+
Attachment #737200 - Flags: review+
Push for trunk, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
https://hg.mozilla.org/comm-central/rev/feb408f0b984
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.20
Blocks: 861911
(In reply to rsx11m from comment #4)
> BTW: The preview of the message text part doesn't work for me, with or
> without the patch (the checkbox toggles the pref correctly). It doesn't show
> up in the notification regardless of the value of mail.biff.alert.show_preview.

I've spun this issue off as bug 861911.
Depends on: 866503
Depends on: 872133
Depends on: 874899
You need to log in before you can comment on or make changes to this bug.