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

RESOLVED FIXED in seamonkey2.18

Status

SeaMonkey
MailNews: Message Display
--
enhancement
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Michael Wolf, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

11.50 KB, image/png
Details
16.76 KB, image/png
Details
21.84 KB, patch
Details | Diff | Splinter Review
17.81 KB, image/png
neil@parkwaycc.co.uk
: ui-review+
Details
42.64 KB, patch
Philip Chee
: review+
Details | Diff | Splinter Review
12.17 KB, image/png
neil@parkwaycc.co.uk
: ui-review+
Details
11.73 KB, image/png
neil@parkwaycc.co.uk
: ui-review+
Details
(Reporter)

Description

10 years ago
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

10 years ago
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: 360488
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

10 years ago
See also bug 200157 and bug 76464.

Updated

9 years ago
Component: MailNews: Notification → MailNews: Message Display
QA Contact: search
Assignee: mail → nobody
QA Contact: search → message-display
(Assignee)

Updated

6 years ago
Duplicate of this bug: 76464
(Assignee)

Updated

6 years ago
Duplicate of this bug: 200157
(Assignee)

Updated

5 years ago
Duplicate of this bug: 536390
(Assignee)

Updated

5 years ago
Blocks: 188754
(Assignee)

Updated

5 years ago
Depends on: 312940, 505056
(Assignee)

Comment 6

5 years ago
Created attachment 688699 [details] [diff] [review]
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.
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
(Assignee)

Comment 7

5 years ago
Created attachment 690074 [details] [diff] [review]
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.
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)
(Assignee)

Comment 8

5 years ago
Created attachment 690195 [details] [diff] [review]
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.
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 9

5 years ago
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 :-(
(Assignee)

Comment 10

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

Comment 11

5 years ago
Created attachment 690424 [details] [diff] [review]
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.
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)
(Assignee)

Updated

5 years ago
Attachment #690424 - Attachment is patch: true

Comment 12

5 years ago
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+
(Assignee)

Comment 13

5 years ago
> 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

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

Comment 15

5 years ago
Created attachment 691370 [details] [diff] [review]
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
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...

Comment 17

5 years ago
I tried it on FreeBSD, but nothing changed.
(Assignee)

Comment 18

5 years ago
> 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)?
(Assignee)

Comment 20

5 years ago
>>+    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+
(Assignee)

Comment 23

5 years ago
Created attachment 698304 [details] [diff] [review]
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.
Attachment #691370 - Attachment is obsolete: true
Attachment #698304 - Flags: review?(neil)
(Assignee)

Comment 24

5 years ago
>>+#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.
(Assignee)

Comment 25

5 years ago
> 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+
(Assignee)

Comment 28

5 years ago
Created attachment 698702 [details]
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 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

5 years ago
Created attachment 701065 [details]
FreeBSD screenshot
Flags: needinfo?(sysadmin)

Comment 31

5 years ago
It seems to partially work, I see the new alert, but not the Subject and the Sender.
(Assignee)

Comment 32

5 years ago
> 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)

Comment 33

5 years ago
true
true
true
Flags: needinfo?(sysadmin)

Comment 34

5 years ago
Created attachment 701704 [details] [diff] [review]
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.
(Assignee)

Comment 35

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

Comment 36

5 years ago
Created attachment 702268 [details]
Screenshot of the "New Mail" alert in Modern.
Attachment #702268 - Flags: ui-review?(neil)
(Assignee)

Comment 37

5 years ago
Created attachment 702272 [details] [diff] [review]
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.
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+
(Assignee)

Comment 39

5 years ago
Created attachment 702873 [details]
Screenshot of the "New Mail" alert in Modern after removing redundant styles.
Attachment #698304 - Attachment is obsolete: true
Attachment #702873 - Flags: ui-review?(neil)
(Assignee)

Comment 40

5 years ago
Created attachment 702878 [details]
Screenshot of the "New Mail" alert in Classic after removing redundant styles.
Attachment #702878 - 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]

Updated

5 years ago
Attachment #702878 - Flags: ui-review?(neil) → ui-review+

Updated

5 years ago
Attachment #702873 - Flags: ui-review?(neil) → ui-review+
(Assignee)

Comment 42

5 years ago
> [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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.18
(Assignee)

Updated

5 years ago
No longer blocks: 360488
(Assignee)

Updated

5 years ago
Blocks: 774377
(Assignee)

Updated

5 years ago
Depends on: 840474
You need to log in before you can comment on or make changes to this bug.