404580 - Provide information like preview text, subject and sender in mail notification window - as in Thunderbird

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Michael Wolf]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; hsb-DE; rv:1.8.1.9) Gecko/20071030 SeaMonkey/1.1.6
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; hsb-DE; rv:1.8.1.9) Gecko/20071030 SeaMonkey/1.1.6

It would be good if SM would have - similar as Thunderbird - more information in the mail notification window - preview text, subject and sender of the incoming mail. And it should be configurable in the preferences dialog of SM (Edit-->Preferences-->Mail & Newsgroups -->Notifications) and/or by about:config

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Actual Results:  
Feature not available.

Expected Results:  
Feature should be available in Seamonkey

Comment 1

[Karsten Düsterloh]

Relevant TB base bug: bug 312940 - but anyone porting that should look at the code, since lots of other bugs added their share in tuning that...

Comment 2

[zug_treno]

See also bug 200157 and bug 76464.

Comment 6

[Philip Chee]

Attachment: Patch v0.1 WIP.

Putting this up for discussion.

TODO:

1. Remove a few more #ifdef MOZ_THUNDERBIRD
2. Re-write and simplify newmailalert.{js|xul|css}
Currently this is a direct copy of Thunderbird's XUL newmailalert which was based on a older implementation of the tookit alert.xul. The fade-in/fade-out animation should be simplified using css3 animations as in the current toolkit alert.xul.

Comment 7

[Philip Chee]

Attachment: Patch v1.0 for review.

Changes since the last patch:

1. Remove a few more #ifdef MOZ_THUNDERBIRD
2. Re-write and simplify newmailalert.{js|xul|css}
2.1 Rebase the code based on the latest toolkit alert.
2.2 Use CSS3 animation to simplify the code.
2.3 Position the close button using the new stack right and top attributes.

Comment 8

[Philip Chee]

Attachment: Patch v1.1 for review

> 1. Remove a few more #ifdef MOZ_THUNDERBIRD
> 2. Re-write and simplify newmailalert.{js|xul|css}
> 2.1 Rebase the code based on the latest toolkit alert.
> 2.2 Use CSS3 animation to simplify the code.
> 2.3 Position the close button using the new stack right and top attributes.

Changes since the last patch:

3. Move all animation related CSS into content newmailalert.css.
(Perhaps I should get rid of the content css and put everything in classic and modern?)
4. Replace let with var.
5. [Fixed] Clicking on the Biff icon in the system tray to re-open the new mail alert will bring up a blank window with opacity 0.

Comment 9

[[e-mail]]

Comment on attachment 690195 [details] [diff] [review]
Patch v1.1 for review

>-#ifdef MOZ_THUNDERBIRD
> #include "mozilla/LookAndFeel.h"
>-#endif
This won't link with external linkage :-(

Comment 10

[Philip Chee]

> This won't link with external linkage :-(
I was afraid of this. How do I work around this?

Comment 11

[Philip Chee]

Attachment: Patch v1.2 Reimplement LookAndFeel::GetInt().

>>-#ifdef MOZ_THUNDERBIRD
>> #include "mozilla/LookAndFeel.h"
>>-#endif
> This won't link with external linkage :-(

So since we can't use mozilla/LookAndFeel.h with external linkage, We have to implement LookAndFeel::GetInt() ourselves. Fortunately it's relatively self contained.

Comment 12

[Karsten Düsterloh]

Comment on attachment 690424 [details] [diff] [review]
Patch v1.2 Reimplement LookAndFeel::GetInt().

I do build on Linux (with external linkage!) and Mac, but you only touched nsMessengerWinIntegration, hence I don't see much effect. ;-)
Maybe you can do a (rough?) #ifdef sortout for nsMessengerUnixIntegration as well? (nsMessengerOSXIntegration/nsMessengerOS2Integration don't seem to use the newmailalert stuff at the moment anyway.)

>+#define SHOW_NEW_ALERT_PREF "mail.biff.show_new_alert"

I'm not convinced yet that we need that. The new alert would clearly be an improvement compared to the old one — either you do like such alerts or you don't (and turn them off completely).


I have to trust you Windows guys at this stage that it is working there, hence just some minor nits to follow:

>+function prefillAlertInfo()
…
>+  var rootFolder;
>+  if (foldersWithNewMail && foldersWithNewMail.Count() > 0)
>+     rootFolder = foldersWithNewMail.GetElementAt(0)
>+                                    .QueryInterface(Components.interfaces.nsIWeakReference)
>+                                    .QueryReferent(Components.interfaces.nsIMsgFolder);
>+  else
>+   return;

How about 
  if (!foldersWithNewMail || foldersWithNewMail.Count() < 1)
    return;
  var rootFolder = foldersWithNewMail.GetElementAt(0) … etc.

>+urlListener.prototype = {

Please adhere to the brace-on-its-own-line policy (especially below in showAlert()).

>+function showAlert()
…
>+  var alertContainer = document.getElementById("alertContainer");
>+   // Don't fade in if the user opened the alert or the pref is true.

Misindentation.

>+  if (document.getElementById('folderSummaryInfo').hasMessages)
>+  {
…
>+  }
>+  else
>+    closeAlert(); // no mail, so don't bother showing the alert...

If one branch has braces, the other one should have as well.


So, from a mere feedback pov, it's a + at this stage. ;-)

Comment 13

[Philip Chee]

> Maybe you can do a (rough?) #ifdef sortout for nsMessengerUnixIntegration as well?
Looking through blame I think that what happens in Thunderbird is:
1. Use libnotify if available.
2. If not available, fallback to XUL notifications (newmailalert.xul).

What happens in SeaMonkey is:
1. Use libnotify if available.
2. If not available, give up.

So I think it's safe to remove all #ifdef MOZ_THUNDERBIRD and any SeaMonkey only code (in #else blocks). But someone on *nix needs to build and test this out.

Comment 14

[Alex Dupre]

I can build and test a patch on FreeBSD, if/when provided. I've been waiting for this feature for years :-)

Comment 15

[Philip Chee]

Attachment: Patch v1.3

> Patch v1.2 Reimplement LookAndFeel::GetInt().

Fixes since the last patch:
1. Fix nits.
2. Modern fixes:
2a. Fix typo in jar.mn s/classic/modern/
2b. Use #FFFFFF for background colour of the alert.
2c. Use #696969 /* DimGrey */ for .folderSummary-previewText.
2d. Point #closeButton list-style-image to something that actually exists:
chrome://global/skin/icons/closebox.gif

Comment 16

[[e-mail]]

Well, the good news is that I got around to trying this out, trying being the operative word, since I was operating via two layers of remote control which made it quite hard to actually capture the alert...

Comment 17

[Alex Dupre]

I tried it on FreeBSD, but nothing changed.

Comment 18

[Philip Chee]

> I tried it on FreeBSD, but nothing changed.
That's because I haven't done anything in nsMessengerUnixIntegration yet. Once the Windows implementation lands in the tree, I'll attach a patch for *nix for you to test on.

Comment 19

[[e-mail]]

Comment on attachment 691370 [details] [diff] [review]
Patch v1.3

>+    origin = LookAndFeel::GetInt(LookAndFeel::eIntID_AlertNotificationOrigin);
[This actually reads a pref whose name I forget offhand so you can test the notification origin without moving the taskbar.]

>+  nsresult rv;
>+  bool showNewAlert = false;
>+  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  prefBranch->GetBoolPref(SHOW_NEW_ALERT_PREF, &showNewAlert);
Two options:
1. NS_ENSURE_SUCCESS_VOID(rv)
2. if (prefBranch)

>+  if (showNewAlert)
>+    ShowNewAlertNotification(false, accountName, animatedAlertText);
>+  else
>     ShowAlertMessage(accountName, animatedAlertText, EmptyCString());
> #else
>     ShowNewAlertNotification(false, accountName, animatedAlertText);
> #endif
[Could write this as:]
  if (!showNewAlert)
    ShowAlertMessage(accountName, animatedAlertText, EmptyCString());
  else
#else
    ShowNewAlertNotification(false, accountName, animatedAlertText);
#endif

>+    content/messenger/newmailalert.css
>+    content/messenger/newmailalert.js
>+    content/messenger/newmailalert.xul
Are these copies of the Thunderbird files? If not, what are the differences?

>+++ b/suite/themes/classic/messenger/newmailalert.css
Is this a copy of a Thunderbird file, or heavily based on one (which)?

Comment 20

[Philip Chee]

>>+    content/messenger/newmailalert.css
>>+    content/messenger/newmailalert.js
>>+    content/messenger/newmailalert.xul
> Are these copies of the Thunderbird files? If not, what are the differences?

Back then in ancient Rome Thunderbird/mscott started off with a fork of the toolkit alert.{js|xul} and then modified to fit requirements. Back then they were using JS to do the fade in animation. In this current patch, I started off with a fork of the current alert.{js|xul|css} files which uses CSS animations to do the fade effect and then retrofitted the bits of the Thunderbird files that were relevant. See Comment 7:

> Changes since the last patch:
> 
> 1. Remove a few more #ifdef MOZ_THUNDERBIRD
> 2. Re-write and simplify newmailalert.{js|xul|css}
> 2.1 Rebase the code based on the latest toolkit alert.
> 2.2 Use CSS3 animation to simplify the code.
> 2.3 Position the close button using the new stack right and top attributes.

>>+++ b/suite/themes/classic/messenger/newmailalert.css
> Is this a copy of a Thunderbird file, or heavily based on one (which)?

This is heavily based on:
mail/themes/qute/mail/newmailalert.css but simplified a bit.

http://mxr.mozilla.org/comm-central/find?text=&string=newmailalert

Comment 21

[[e-mail]]

Comment on attachment 691370 [details] [diff] [review]
Patch v1.3

>diff --git a/suite/themes/*/messenger/newmailalert.css b/suite/themes/*/messenger/newmailalert.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/themes/*/messenger/newmailalert.css
If these are based on mail/themes/qute/mail/newnmailalert.css then these should be an hg copy (-A) so that you can see what's what.

>+#newMailAlertNotification {
>+  background-color: rgba(255, 255, 255, 0);
What does this achieve?

>+  border: ridge SteelBlue 4px;
[Classic: Hmm, old colour was hardcoded too...]

>+#alertTextBox {
>+  padding: 4px;
>+  -moz-padding-end: 16px;
>+}
>+
>+#alertTitle {
>+  font-weight: bold;
>+}
[Swap these back to the order qute has them.]

>+  color: #696969; /* DimGrey */
Modern: I don't see a previous use of #696969, maybe #8C99AB might be better.

Comment 22

[[e-mail]]

Comment on attachment 691370 [details] [diff] [review]
Patch v1.3

Need proper hg history on all the "new" files please.

Comment 23

[Philip Chee]

Attachment: Patch v1.4 Addressed review issues.

>>+  prefBranch->GetBoolPref(SHOW_NEW_ALERT_PREF, &showNewAlert);
> Two options:
> 1. NS_ENSURE_SUCCESS_VOID(rv)
> 2. if (prefBranch)
I chose (1)

>>+  if (showNewAlert)
>>+    ShowNewAlertNotification(false, accountName, animatedAlertText);
>>+  else
>>     ShowAlertMessage(accountName, animatedAlertText, EmptyCString());
>> #else
>>     ShowNewAlertNotification(false, accountName, animatedAlertText);
>> #endif
> [Could write this as:]
>   if (!showNewAlert)
>     ShowAlertMessage(accountName, animatedAlertText, EmptyCString());
>   else
> #else
>     ShowNewAlertNotification(false, accountName, animatedAlertText);
> #endif
Fixed.

>>+#alertTextBox {
>>+  padding: 4px;
>>+  -moz-padding-end: 16px;
>>+}
>>+
>>+#alertTitle {
>>+  font-weight: bold;
>>+}
> [Swap these back to the order qute has them.]
Fixed.

>>+  color: #696969; /* DimGrey */
> Modern: I don't see a previous use of #696969, maybe #8C99AB might be better.
Fixed.

> Need proper hg history on all the "new" files please.
Done! Except for this file which is copied from toolkit and then adapted:

> diff --git a/suite/mailnews/newmailalert.css b/suite/mailnews/newmailalert.css
> new file mode 100644
> --- /dev/null

> +    alertContainer.addEventListener("animationend", function hideAlert(event) {
> +      if (event.animationName == "alert-animation") {
> +        alertContainer.removeEventListener("animationend", hideAlert, false);
> +        let remaining = Math.min(Math.round(gOpenTime - event.elapsedTime * 1000), 0);
> +        setTimeout(closeAlert, remaining);
It occured to me that I could be more accurate if I take into account the time taken for the opening animation.

Comment 24

[Philip Chee]

>>+#newMailAlertNotification {
>>+  background-color: rgba(255, 255, 255, 0);
> What does this achieve?
Nothing :( I was trying to get the background transparent so that you could see the underlying bit of screen when the alert faded in but all that happens is that a white rectangle appears first then the alert fades in. Without this the colour of the rectangle is the default dialog background colour.

Comment 25

[Philip Chee]

> I tried it on FreeBSD, but nothing changed.
I've now included changes to nsMessengerUnixIntegration.cpp/nsMessengerUnixIntegration.h
Obviously untested. Also you can only see the XUL new mail alert if libnotify isn't installed.

Comment 26

[[e-mail]]

Comment on attachment 698304 [details] [diff] [review]
Patch v1.4 Addressed review issues.

[Could do with some screenshots to double-check the CSS.]

>+    int32_t origin = 0;
[Would be nice if this read the pref...]

>+#ifndef MOZ_THUNDERBIRD
>+// from LookAndFeel.h
>+#define NS_ALERT_HORIZONTAL 1
>+#define NS_ALERT_LEFT       2
>+#define NS_ALERT_TOP        4
>+#endif
This belongs in the .cpp rather than the .h file.

>+# LOCALIZATION NOTE(newMailNotification_message): %1$S is the name of the account %2$S is the number of new messages  
>+newMailNotification_message=%1$S received %2$S new message
>+
>+# LOCALIZATION NOTE(newMailNotification_messages): %1$S is the name of the account %2$S is the number of new messages  
>+newMailNotification_messages=%1$S received %2$S new messages
This should really use PluralForm.

Comment 27

[[e-mail]]

Comment on attachment 698304 [details] [diff] [review]
Patch v1.4 Addressed review issues.

I just realised that those strings are copied from Thunderbird too, so they can go in like that if you want to land this before the uplift and then you can merge the two sets of code and convert to PluralForm at a later date. (But please still move the #defines and I'd appreciate some screenshots on the offchance that I'd like some CSS tweaks.)

Comment 28

[Philip Chee]

Attachment: Screenshot of the "New Mail" alert in Classic.

> and I'd appreciate some screenshots on the offchance that I'd like some CSS
> tweaks.

Comment 29

[[e-mail]]

Comment on attachment 698702 [details]
Screenshot of the "New Mail" alert in Classic.

As I thought, that border doesn't look very "Classic". But, it does blend in quite well with our logo ;-)

Comment 30

[Alex Dupre]

Attachment: FreeBSD screenshot

Comment 31

[Alex Dupre]

It seems to partially work, I see the new alert, but not the Subject and the Sender.

Comment 32

[Philip Chee]

> It seems to partially work, I see the new alert, but not the Subject and the Sender.
What are your about:config settings for:

mail.biff.alert.show_preview
mail.biff.alert.show_sender
mail.biff.alert.show_subject

Comment 33

[Alex Dupre]

true
true
true

Comment 34

[Alex Dupre]

Attachment: Unix patch against SM 2.15

This is the patch I applied to SM 2.15. It should be equivalent to yours, but I'm attaching it here to be sure I didn't miss anything.

Comment 35

[Philip Chee]

> Unix patch against SM 2.15
Ah, this patch is not guaranteed to work on anything except trunk ;)

Comment 36

[Philip Chee]

Attachment: Screenshot of the "New Mail" alert in Modern.

Comment 37

[Philip Chee]

Attachment: Patch v1.5 fix remaining nits. Carrying forward r=Neil

Changes since the last patch:

1. Move LookAndFeel defines to the .cpp file.
2. Try to read the origin from ui.alertNotificationOrigin before querying the OS.
3. Fix braino: Math.min should be Math.max.

Comment 38

[[e-mail]]

Comment on attachment 702268 [details]
Screenshot of the "New Mail" alert in Modern.

I'm not convinced that the white background is right. If the regular Modern background makes the text too hard to read then perhaps we should use the tooltip colours instead.

Comment 39

[Philip Chee]

Attachment: Screenshot of the "New Mail" alert in Modern after removing redundant styles.

Comment 40

[Philip Chee]

Attachment: Screenshot of the "New Mail" alert in Classic after removing redundant styles.

Comment 41

[[e-mail]]

Comment on attachment 702272 [details] [diff] [review]
Patch v1.5 fix remaining nits. Carrying forward r=Neil

> .folderSummaryMessage:hover > .folderSummary-message-row {
>   cursor: pointer;
>-  color: blue;
>+  color: #0000FF;
> }
[Not sure whether .text-link is the right style to copy but strangely it is in fact blue rather than #0000FF in Modern's global.css]

Comment 42

[Philip Chee]

> [Not sure whether .text-link is the right style to copy but strangely it is in
> fact blue rather than #0000FF in Modern's global.css]

I traced it back to the old XPFE communicator/formatting.css. In Bug 106127 it
was changed from #424F63 to "blue". :S

>  #newMailAlertNotification {
>    min-height: 60px;
> -  border: ridge #5486DA 4px;
> -  background-color: -moz-Dialog;
> -  color: -moz-DialogText;
> +  border: ridge SteelBlue 4px;
>  }

I just realized that global.css is pulled in so the global styles for <window>
already apply so no need to be repetitive.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3b6458d65543