Closed Bug 404580 Opened 12 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.18

People

(Reporter: milupo, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 6 obsolete files)

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
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...
Blocks: TB2SM
Status: UNCONFIRMED → NEW
Ever confirmed: true
See also bug 200157 and bug 76464.
Component: MailNews: Notification → MailNews: Message Display
QA Contact: search
Assignee: mail → nobody
QA Contact: search → message-display
Duplicate of this bug: 76464
Duplicate of this bug: 200157
Duplicate of this bug: 536390
Blocks: 188754
Depends on: 312940, 505056
Attached patch Patch v0.1 WIP. (obsolete) — Splinter Review
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #688699 - Flags: feedback?(neil)
Attachment #688699 - Flags: feedback?(mnyromyr)
Attachment #688699 - Flags: feedback?(iann_bugzilla)
Summary: Additional feature for SM 2.0: Information like preview text, subject and sender in mail notification window - as in Thunderbird → Provide information like preview text, subject and sender in mail notification window - as in Thunderbird
Version: unspecified → Trunk
Attached patch Patch v1.0 for review. (obsolete) — Splinter 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.
Attachment #688699 - Attachment is obsolete: true
Attachment #688699 - Flags: feedback?(neil)
Attachment #688699 - Flags: feedback?(mnyromyr)
Attachment #688699 - Flags: feedback?(iann_bugzilla)
Attachment #690074 - Flags: review?(neil)
Attachment #690074 - Flags: feedback?(mnyromyr)
Attachment #690074 - Flags: feedback?(iann_bugzilla)
Attached patch Patch v1.1 for review (obsolete) — Splinter 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.
Attachment #690074 - Attachment is obsolete: true
Attachment #690074 - Flags: review?(neil)
Attachment #690074 - Flags: feedback?(mnyromyr)
Attachment #690074 - Flags: feedback?(iann_bugzilla)
Attachment #690195 - Flags: review?(neil)
Attachment #690195 - Flags: feedback?(mnyromyr)
Attachment #690195 - Flags: feedback?(iann_bugzilla)
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 :-(
> This won't link with external linkage :-(
I was afraid of this. How do I work around this?
>>-#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.
Attachment #690195 - Attachment is obsolete: true
Attachment #690195 - Flags: review?(neil)
Attachment #690195 - Flags: feedback?(mnyromyr)
Attachment #690195 - Flags: feedback?(iann_bugzilla)
Attachment #690424 - Flags: review?(neil)
Attachment #690424 - Flags: feedback?(mnyromyr)
Attachment #690424 - Flags: feedback?(iann_bugzilla)
Attachment #690424 - Attachment is patch: true
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. ;-)
Attachment #690424 - Flags: feedback?(mnyromyr) → feedback+
> 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.
I can build and test a patch on FreeBSD, if/when provided. I've been waiting for this feature for years :-)
Attached patch Patch v1.3 (obsolete) — Splinter Review
> 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
Attachment #690424 - Attachment is obsolete: true
Attachment #690424 - Flags: review?(neil)
Attachment #690424 - Flags: feedback?(iann_bugzilla)
Attachment #691370 - Flags: review?(neil)
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...
I tried it on FreeBSD, but nothing changed.
> 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 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)?
>>+    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 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 on attachment 691370 [details] [diff] [review]
Patch v1.3

Need proper hg history on all the "new" files please.
Attachment #691370 - Flags: review?(neil) → feedback+
>>+  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.
Attachment #691370 - Attachment is obsolete: true
Attachment #698304 - Flags: review?(neil)
>>+#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.
> 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.
Flags: needinfo?(sysadmin)
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 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.)
Attachment #698304 - Flags: review?(neil) → review+
> and I'd appreciate some screenshots on the offchance that I'd like some CSS
> tweaks.
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 ;-)
Attached image FreeBSD screenshot
Flags: needinfo?(sysadmin)
It seems to partially work, I see the new alert, but not the Subject and the Sender.
> 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
Flags: needinfo?(sysadmin)
true
true
true
Flags: needinfo?(sysadmin)
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.
> Unix patch against SM 2.15
Ah, this patch is not guaranteed to work on anything except trunk ;)
Attachment #702268 - Flags: ui-review?(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.
Attachment #702272 - Flags: review+
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.
Attachment #702268 - Flags: ui-review?(neil) → ui-review+
Attachment #698304 - Attachment is obsolete: true
Attachment #702873 - Flags: ui-review?(neil)
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]
Attachment #702878 - Flags: ui-review?(neil) → ui-review+
Attachment #702873 - Flags: ui-review?(neil) → ui-review+
> [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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.18
No longer blocks: TB2SM
Blocks: 774377
Depends on: 840474
You need to log in before you can comment on or make changes to this bug.