555536 - alert on new message does not appear as specified in preferences

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Segaja]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3pre) Gecko/20100326 Namoroka/3.6.3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.2pre) Gecko/20100327 Lanikai/3.1b2pre

The preferences are, that the alert for new messages should contain all three elements (Message Preview Text, Subject, Sender).

What I really see is a alert popup that contains the Account name where the new message arrived and the text "1 new message."

Reproducible: Always

Steps to Reproduce:
Let Thunderbird run and the bug occurs, when you get a new message.
Actual Results:  
I see the faulty alert message

Expected Results:  
An alert message, where the message preview text, the subject and the sender is displayed (as it is in the 3.0 nightlies).

I use the 3.1 nightly builds from the mozilla ftp on an ubuntu 9.10

Comment 1

[Ludovic Hirlimann [:Usul]]

Segaja, nathan, can someone hunt the regression window using http://www.rumblingedge.com/2009/02/24/howto-find-regression-windows-through-manual-binary-search/ as a tutorial ?

Comment 2

[Segaja]

Okay, I did that for the linux builds.

The alert message works right up to the thundebird 3.1b2pre build 2010-03-07-03.
The build of thunderbird 3.1b2pre from 2010-03-08-03 does not work.

Comment 3

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)
> Okay, I did that for the linux builds.
> 
> The alert message works right up to the thundebird 3.1b2pre build
> 2010-03-07-03.
> The build of thunderbird 3.1b2pre from 2010-03-08-03 does not work.

Thank you !

Comment 4

[Makoto Kato [:m_kato]]

I need a change like Mac OS X's growl.

Comment 5

[Segaja]

(In reply to comment #4)
> I need a change like Mac OS X's growl.

I'm sorry, I don't understand what you mean.

Comment 6

[Makoto Kato [:m_kato]]

From 3.1, I changed it from XUL-based notification window to system-alert that uses libnotify on many desktop env.  I simply changed to common code with SeaMonkey.

As desktop integration, we should use libnotify-base alert box on GTK+/UNIX.

Also, on 3.0
Windows -> XUL base (same as old style of GTK+/UNIX)
Mac OS X -> system-alert (Growl)

So, about notification messages on new mail we should use same system-alert base system (Mac OS X) 's message on GTK+/libnotify.

Comment 7

[Mark Banner (:standard8)]

This sounds like an unintentional regression that we should resolve in some manner before b2 (feature/string freeze) or rc1 at the latest.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Makoto, how quickly do you think you could have a patch with a test for this?

Comment 9

[Makoto Kato [:m_kato]]

On Mac implements, there is no subject and preview in notification messages.
I have to get summary like newmailalert.xul...

Also, I will be able to create a patch into next week.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

At this point, I'm not convinced that this bug meets our current blocking criteria of:

a) make the upgrade experience from TB2 very painful for a large number of users

or 

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

Even if it would appear as a regression for Tb2 users (which I suspect is the case, but haven't verified), it just doesn't seem _that_ severe.  Do other people disagree?

That said, I'd really love for us to get this fixed.  Makoto, to make it easier for you to schedule your own time in relation to this, our current plan is to close the tree for b2 and all strings at 23:59 next Tuesday.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: WIP v1

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Makoto, thanks for the WIP patch!  What still needs to happen before this patch is review-ready?

Comment 13

[Makoto Kato [:m_kato]]

(In reply to comment #12)
> Makoto, thanks for the WIP patch!  What still needs to happen before this patch
> is review-ready?

I may need consider fallback code.  Some desktops/distributions configuration doesn't support notification system even if it has libnotify.  So I need add XUL notification.  After that, I set review today or tomorrow.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

If we take this for 3.1(b2), it has missed string freeze, which was last night.
The process for handling such patches is described <https://developer.mozilla.org/en/Thunderbird_Localization#Breaking_the_string_freeze>.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Adding [e-mail] for consideration of comment 14.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: patch v1

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Before I make the blocking call, I want to provide some context, and ask some questions.

This seems to me like it might be fairly painful to some people.  That said, it's interesting data that the bug didn't get filed until weaks after the original change landed, and there are still a small number of people CCed on the bug.

Roland, Wayne, Ludo, have you guys seen much feedback about this in any other bugs or fora?

Makoto, is this patch ui-review/review-ready?  If not, what else does it need?  If so, when would you be able to have unit tests ready?

Comment 13 seems to suggest that some Linux systems do not current get biffed at all because they don't have libnotify.  Looking at toolkit/components/alerts/src/nsAlertsService.cpp makes me think that even systems that don't have libnotify using a built in XUL notifier.  mkato, am I misunderstanding comment 13?

FWIW, I think we have to get all our strings in _today_ if we're going to keep our current dates, which I believe we should do.

Our options, as I'm currently aware of them look like this

1) live with the current state functionality regression, which is this:
* message subject, sender, and body text that Tb2 and Tb3 offered is no longer displayed in the biff notification at all
* some systems will no longer get any biff notifications at all, because they don't have libnotif
* We have a bunch of prefs that claim to control subject/sender/body text that now don't do anything on Linux.  We could hide the (customize) button on Linux to make this be less bad.

mkato, others, does the above description of the current state seem correct?

2) take this patch
* this is presumed to fix the problem (I don't have a Linux build for easy testing at the moment), which is great
* it forces us to take another late-l10n string, and we've already had 6 late-l10n bugs, which is too many
* we don't have unit-tests, tests, or review yet

3) write a different patch that switches back to the old notification XUL window for biffing (i.e. effectively back out the libnotify functionality change).
* wouldn't need any new strings!
* no functionality regression at all 
* no libnotify (not sure what the user-perceived difference here would be.  can someone elaborate?)
* maybe other pros & cons?

4) switch to using the Mac string & hide the customize button
* no new strings
* gets us a bit more info than we have now (the email address)
* still loses functionality compared to Tb2 and Tb3

Thoughts?

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Clarification: The second * in option 1) depends on the answer to the question about comment 13, I see now.

Comment 19

[Ludovic Hirlimann [:Usul]]

4) Sounds too hackish for me ....

Comment 20

[David :Bienvenu]

The only advantage of 3 over 2 is that it doesn't require a new string, but it requires a new patch, and has the same disadvantage as 2 (no tests or review) and a bunch of other disadvantages. So I'd throw out 3, personally.

Comment 21

[Wayne Mery (:wsmwk)]

> it's interesting data that the bug didn't get filed until weaks after the
> original change landed, and there are still a small number of people CCed on
> the bug. ... have you guys seen much feedback about this in any other
> bugs or fora?

only a month since landing 2010-03-07 so doesn't surprise me it's recently surfaced. What I can offer, as one who doesn't use it:
- we know the makeup of our branch users doesn't mirror release users (eg I stopped using pop up notify long ago) so no shrieks from beta users shouldn't be our benchmark
- people do seem to get upset when they don't know they have new mail (rants like - I didn't know I got a message from my boss)
- I tend not to pay attention to these issues in the forums, bugzilla postings, etc so I'm otherwise not a good judge as to the potential %/number of users affected

perhaps the main point to remember is the - majority of our users are windows, and they won't welcome a regression that lasts the window between v3.1 and v3.2

Comment 22

[Merike Sell (:merike)]

When considering option 3:
There's one more issue with using libnotify. If it is installed then notifications implemented in bug 123440 appear empty. There's no text in them, just the icon. Without libnotify these appear with text. That's surely another bug, possibly filed (couldn't find though), but together with this one it degrades usefulness of using libnotify noticeably.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

bienvenu: part of the idea behind option 3) was that it we're willing to bend
the feature freeze rule for it, we could take it as part of RC1, which would
give extra time for tests & review.

wsmwk: this is a Linux-only bug.  You're quite right that we have entirely
insufficient data here, and Ludo also pointed out that Linux users often do
their bug reporting to the distros.

Merike: you mean that even with libnotify installed, the notification is showing a window with an icon in it and nothing else?  Might you be able to post a screenshot?

Comment 24

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

(In reply to comment #17)
> Before I make the blocking call, I want to provide some context, and ask some
> questions.
> 
> This seems to me like it might be fairly painful to some people.  That said,
> it's interesting data that the bug didn't get filed until weaks after the
> original change landed, and there are still a small number of people CCed on
> the bug.
> 
> Roland, Wayne, Ludo, have you guys seen much feedback about this in any other
> bugs or fora?

i did a search in google and in Get Satisfaction to search Get Satisfaction for 3.1beta2, 3.1 beta2 and 3.1 beta and I can't find any mention of this problem which isn't surprising since most of the folks on Get Satisfaction don' t use betas i.e. they use official releases only

Comment 25

[Merike Sell (:merike)]

Attachment: Startup without network connection

Stacked notifications, one per checked server.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Thanks, Merike!  That's serious badness.  Would you be willing to find or file a bug on that and nominate it to block 3.1 also?  I suspect that it's going to make sense to base our decisions on as much data as we can get about both that bug and this one.  In particular, since that's not already on the radar, I wonder if that's because it only comes up in certain situations...

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

The other bug has been filed as bug 561427, and turns to not to be biff-related.

I'm leaving this bug as blocking RC1+ on the assumption that we'll go with solution 3 or 4.  If someone with the bandwidth picks this up and drives reviews & ui-reviews (and ideally, but not necessarily) tests today, I'd consider approving this for b2, however.

Comment 28

[Makoto Kato [:m_kato]]

Comment on attachment 440673 [details] [diff] [review]
patch v1

I forget flag.

If paltform doesn't libnotify such as Ubuntu (Ubuntu doesn't support click-able notification), use old XUL biff.

Comment 29

[Makoto Kato [:m_kato]]

Also, if libnotify isn't supported on platform, ShowAlertNotification of system-alert returns error.  So this fix will be included for bug 561427 too.

Comment 30

[Mark Banner (:standard8)]

I've just backed out bug 478463 from comm-1.9.2 (only, not trunk), this bug remains to fix the issue on trunk builds which will go into 3.1.next. However, now doesn't need to block 3.1, so minusing the blocking flag.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 440673 [details] [diff] [review]
patch v1

Sorry for the delay in getting back to this. I've not tested it yet but the general idea looks reasonable. If you can address the comments below, I'll do the testing next time around, thanks.

>+  // enumerate over the folders under this root folder till we find one with new mail....
>+  nsCOMPtr<nsISupportsArray> allFolders;
>+  NS_NewISupportsArray(getter_AddRefs(allFolders));
>+  aFolder->ListDescendents(allFolders);
>+  if (allFolders)
>+  {
>+    nsCOMPtr<nsIEnumerator> enumerator;
>+    allFolders->Enumerate(getter_AddRefs(enumerator));
>+    if (enumerator)
>+    {

I don't like the multi-nested if structure here. I suggest you do:

if (!allFolders)
  return PR_FALSE;

...

if (!enumerator)
  return PR_FALSE;

or maybe use an rv check e.g. NS_ENSURE_SUCCESS(rv, PR_FALSE);

Also rather than using allFolders->Enumerate, please use a for loop, using do_QueryElementAt to get at the elements.

>+          nsCOMPtr<nsIMsgFolder> msgFolder;
>+          msgFolder = do_QueryInterface(supports);
>+          if (msgFolder)
>+          {

I think you should if (!msgFolder) continue here.

>+            PRInt32 numNewMessages = 0;
>+            msgFolder->GetNumNewMessages(PR_FALSE, &numNewMessages);
>+            if (numNewMessages > 0) {

likewise: use continue to just go onto the next folder.

>+              // found new meesage in this folder.  Get or generate preview text

nit: spelling of message.

>+                  for (PRUint32 keyIndex = 0; keyIndex < numNewKeys; keyIndex++)
>+                  {
>+                    // there is too many preview items
>+                    if (notificationCount >= MAX_MSG_HDRS_IN_POPUP)
>+                      break;

That seems like an and condition that you could just work into the for statement.

>+                      for (PRUint32 i = 0; i < num; i++)
>+                      {
>+                        PR_FREEIF(emails[i]);
>+                        PR_FREEIF(names[i]);
>+                        PR_FREEIF(fullnames[i]);
>+                      }
>+                      PR_FREEIF(emails);
>+                      PR_FREEIF(names);
>+                      PR_FREEIF(fullnames);
>+                    }

Use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY on each array - it does the right things for you.

>+                    notificationCount++;
>+                  }
>+                  NS_Free(newMessageKeys);

Are we always freeing newMessageKeys correctly? Even when we continue?


>+  PRBool showAlert = PR_TRUE;
>+  prefBranch->GetBoolPref(SHOW_ALERT_PREF, &showAlert);
>+
>+  if (showAlert)
>+  {

Again, its probably just clearer to abort and return early.

>+    if (BuildNotificationTitle(folder, bundle, alertTitle))
>+      if (BuildNotificationBody(folder, bundle, alertBody))
>+        ShowAlertMessage(alertTitle, alertBody, EmptyCString());

These should be one if statement.

Comment 32

[Ludovic Hirlimann [:Usul]]

Makoto San are you up for a new patch ?

Comment 33

[Mark Banner (:standard8)]

Makoto isn't able to work on this at the moment, I'll see if I can find someone.

Comment 34

[Segaja]

I'm confused... I reported this a year ago and it was fixed some time after that in the TB3.1pre nightlies. Now I have switched to the 3.3 nightlies and the bug is back. Why is that?

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: patch v2

So, my approach is quite a bit different from the original patch author:

1)  We now store a hashtable of timestamps, keyed on server rootFolder URI's.  This way, we know the last time a particular account notified, independent of other accounts.  This is for the scenario when we have several accounts, receive notification on 1 while the other is syncing, and then want to receive notifications on the 2nd even though the messages are older.

2)  The system notifications, when they work, are not clickable.  This is due to the design of Ubuntu's notify-osd.

3)  If the system notification service is not found, we revert to newmailalert.xul.

4)  I've added a series of Mozmill tests, and had to slightly modify some of our test message generation tools.

5)  I've asked for a superreview because this code touches SeaMonkey, and I want to make sure I'm not screwing them up somehow.

All feedback welcome.

Comment 36

[David :Bienvenu]

Comment on attachment 527322 [details] [diff] [review]
patch v2

sadly, I can't review this since linux is the one platform I can't run. But I will do the sr, and look over the code best I can.

Comment 37

[David :Bienvenu]

some general comments:

You should try to use Services.jsm and mailServices.jsm, in the js code i.e.,

Components.utils.import("resource:///modules/mailServices.js");
Components.utils.import("resource://gre/modules/Services.jsm");

and replace gPrefSvc with Services.prefs and use MailServices.accounts for the account manager.

copyright should be 2011

+ * Portions created by the Initial Developer are Copyright (C) 2009

+    // We set MsgDatabase to nsnull in order to close the database,
+    // thereby preventing a memory leak.
+    folderWithNewMail->SetMsgDatabase(nsnull);

You want to do this *after* you're done with the DB, not before. This looks a bit tricky because of all the early returns in that function, but it's not doing much good where it is, because later code in that function will cause the folder's db to get cached all over again.

Comment 38

[David :Bienvenu]

Comment on attachment 527322 [details] [diff] [review]
patch v2

clearing sr request since we'll want a new patch from the prev comments.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: patch v3

Thanks for the review, David - good catches.  I've made the alterations you suggested.

Comment 40

[David :Bienvenu]

Comment on attachment 528090 [details] [diff] [review]
patch v3

+    // We're done with the MsgDatabase, so we set to nsnull
+    // in order to close it, thereby preventing a memory leak.
+    folderWithNewMail->SetMsgDatabase(nsnull);

You're still not done. FetchMsgPreviewText is going to open the db again. Also, you're not preventing a leak, just bloat. And you don't want to do this for things like the Inbox, since we keep inbox db's open since we're likely to need them open a lot. I'm inclined to say you should just remove this code, but if you want to keep it, you should put it right before these two lines:

+
+    // Show the notification
+    ShowAlertMessage(alertTitle, alertBody, EmptyCString());

nit - you don't need the braces here:

+    if (folderWithNewMail) {
+      break;
+    }

in fact, you could just put the check into the for condition, i.e.,

i < count && !folderWithNewMail;

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Thanks again for the review, David.

> You're still not done. FetchMsgPreviewText is going to open the db 
> again. Also, you're not preventing a leak, just bloat. And you
> don't want to do this for things like the Inbox, since we keep inbox
> db's open since we're likely to need them open a lot. I'm inclined
> to say you should just remove this code

Ah, my mistake.  I've removed all instances of folderWithNewMail->SetMsgDatabase(nsnull);.

I've also nixed the break conditional, and added the condition to the for loop declaration as you suggested.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5

Patch v4 wasn't properly based off of trunk.

Comment 43

[Mike Conley (:mconley) (:⚙️)]

This patch introduces a new l10n string.

Comment 44

[David :Bienvenu]

Comment on attachment 529469 [details] [diff] [review]
Patch v5

I can't review this per se, but I can sr it...

Comment 45

[David :Bienvenu]

Comment on attachment 529469 [details] [diff] [review]
Patch v5

Review of attachment 529469 [details] [diff] [review]:

sr=me

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ +218,5 @@
+{
+  nsresult rv;
+
+  PRUint32 aElement1Timestamp;
+  rv = aElement1->GetDateInSeconds(&aElement1Timestamp);

can move decl of nsresult to here, where you first use it

@@ +504,5 @@
+
+    // Create the notification title
+    nsString alertTitle;
+    nsresult rv;
+    rv = BuildNotificationTitle(folder, bundle, alertTitle);

combine decl of rv with first use

@@ +784,5 @@
+#ifdef MOZ_THUNDERBIRD
+  if (NS_SUCCEEDED(aExitCode))
+  {
+    // preview fetch is done.
+    FillToolTipInfo();

don't need braces here

@@ +796,5 @@
+                                                     PRUint32 *aLastMRUTime)
+{
+  nsresult rv;
+  nsCOMPtr<nsIMsgFolder> rootFolder = nsnull;
+  rv = aFolder->GetRootFolder(getter_AddRefs(rootFolder));

move decl of rv here

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6

I've integrated bienvenu's suggestions from my +'d superreview.

Comment 47

[Mark Banner (:standard8)]

Comment on attachment 529746 [details] [diff] [review]
Patch v6

Review of attachment 529746 [details] [diff] [review]:
-----------------------------------------------------------------

You've not actually added the test to mozmilltests.list so it won't run by default.

In trying this out, I've noticed that you don't get a new notification if there are already emails marked as "new" in the folder. I think that's wrong - even if I hadn't looked at the folder, and I got more emails in, I'd expect to get a notification (for example, if I'd seen the first notification and decided to ignore it, then got a new email that I wanted to look at at the next check, I won't currently get the notification).

I also don't get the message preview if the only thing that is selected in the preferences is the message preview.

Not sure if we also need to get Blake's ui-review for the bits that have changed slightly.

::: mail/test/mozmill/notification/test-notification.js
@@ +42,5 @@
> +
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +let Cu = Components.utils;
> +let Cr = Components.results;

Aren't these already defined?

@@ +50,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// Our global folder variables...
> +let gFolder = null;
> +let gFolder2 = null;

var at the global level please (multiple places)

@@ +58,5 @@
> +
> +// We'll use this mock alerts service to capture notification events
> +let gMockAlertsService = {
> +
> +  _doFail: false,

nit: no blank line necessary before this.

@@ +127,5 @@
> +  wh.installInto(module);
> +
> +  // Register the mock alerts service
> +  Components.manager.QueryInterface(Ci.nsIComponentRegistrar)
> +      .registerFactory(Components.ID("{1bda6c33-b089-43df-a8fd-111907d6385a}"),

nit: either 2-space indent the dot, or preferably align with the first dot in the line above.

@@ +144,5 @@
> +  identity2.email = "[e-mail]";
> +
> +  var server = MailServices.accounts
> +               .createIncomingServer("nobody",
> +                                     "Test Local Folders", "pop3");

nit: align dots (or 2-space indent if too wide).

@@ +163,5 @@
> +  gMockAlertsService._doFail = false;
> +  gFolder.biffState = Ci.nsIMsgFolder.nsMsgBiffState_NoMail;
> +  gFolder2.biffState = Ci.nsIMsgFolder.nsMsgBiffState_NoMail;
> +
> +  Services.prefs.setBoolPref("mail.biff.alert.show_subject", true);

You should reset the prefs after running the test to avoid getting in the way of others.

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ +85,5 @@
>  
>  #define ALERT_CHROME_URL "chrome://messenger/content/newmailalert.xul"
>  #define NEW_MAIL_ALERT_ICON "chrome://messenger/skin/icons/new-mail-alert.png"
>  #define SHOW_ALERT_PREF "mail.biff.show_alert"
> +#define SHOW_ALERT_PREVIEW_LENGTH "mail.biff.alert.preview_length"

This should probably be added to all-thunderbird.js.

@@ +137,5 @@
>    NS_NewISupportsArray(getter_AddRefs(mFoldersWithNewMail));
>  }
>  
> +NS_IMPL_ISUPPORTS4(nsMessengerUnixIntegration, nsIFolderListener, nsIObserver, nsIMessengerOSIntegration,
> +                   nsIUrlListener)

nit: looks like nsIMessengerOSIntegration wants to start the new line.

@@ +230,5 @@
> +}
> +
> +
> +PRBool
> +nsMessengerUnixIntegration::BuildNotificationBody(nsIMsgDBHdr *aHdr, nsIStringBundle *aBundle, nsString &aBody)

nit: this could do with wrapping.

@@ +287,5 @@
> +    author.Assign(names[0] ? names[0] : emails[0]);
> +
> +    for (PRUint32 i = 0; i < num; i++)
> +    {
> +      PR_FREEIF(emails[i]);

NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY does this for you...

@@ +397,5 @@
> +  prefBranch->GetBoolPref(SHOW_ALERT_PREF, &showAlert);
> +
> +  if (showAlert)
> +  {
> +    nsCOMPtr<nsISupportsArray> argsArray;

Please change this to an nsIMutableArray. nsISupportsArray is going away, and OpenWindow can cope with the nsIMutableArray version.

@@ +432,5 @@
> +
> +    mAlertInProgress = PR_TRUE;
> +  }
> +
> +  // if the user has turned off the mail alert, or  openWindow generated an error,

nit: double space between or and openWindow.

@@ +503,5 @@
> +    // Create the notification title
> +    nsString alertTitle;
> +    nsresult rv = BuildNotificationTitle(folder, bundle, alertTitle);
> +
> +    if (NS_FAILED(rv))

nit: no blankspace needed before this.

@@ +577,5 @@
> +    nsString alertBody;
> +
> +    // Build the body text of the notification.
> +    rv = BuildNotificationBody(newMsgHdrs[0], bundle, alertBody);
> +    if (NS_FAILED(rv))

I think for these you could probably just do if (NS_FAILED(...)) and skip the rv assignment.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v7

Thanks for the review Standard8.

I've made the fixes you suggested.  I've also made it so that we now notify on every new mail received.  I had to insert a new SetMRUTime in nsMsgDBFolder::SetBiffState to do this - I hope that's OK.  The comment in SetHasNewMessages notes that we should probably update the MRUTime for every new message received, but that we don't for performance reasons.  That comment was, according to blame, made by Bienvenu back in 2006 (see line 375 here:  http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/util/nsMsgDBFolder.cpp&rev=1.353).  Not sure if the concern still applies or not.

I also added a Mozmill test for the every-mail notifications.

I've also fixed the bug where the preview wasn't showing up if the sender/subject were not displayed.  I've added a regression test for that, as well as similar tests for sender and subject.

Finally, I changed some wording in messenger.properties.  I did this because we now try to notify every time new mail is received (as opposed to just when the biff-state on a folder is set to new mail received) - but the count that is displayed in the popup said "Folder has X new message(s)", which is not necessarily the case, since X is the number of *newly received* messages.  So that's my rationale there.  I'm open to argument.  :)

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 530785 [details] [diff] [review]
Patch v7

I'm requesting a UI review on the small language change, as well as the fact that we're notifying not just on biff-state changes, but every time new mail is received in a folder.

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 530785 [details] [diff] [review]
Patch v7

>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
>@@ -383,8 +383,12 @@
>-newBiffNotification_message=%1$S has %2$S new message
>-newBiffNotification_messages=%1$S has %2$S new messages
>+newBiffNotification_message=%1$S received %2$S new message
>+newBiffNotification_messages=%1$S received %2$S new messages

So, a number of languages have different forms for plurality, so the 1 vs. more than 1 split won't work for them. https://developer.mozilla.org/en/Localization_and_Plurals suggests that we should be using PluralForm.jsm instead, but I see from https://bugzilla.mozilla.org/show_bug.cgi?id=477831 that it's not possible, so this is okay. I think I might prefer "%1$S received a new message", but that's a fairly minor preference, and it might mess up the translation code (which may be expecting two parameters).

And for the different notification style, I guess I expected the new message notification to notify me when I got new messages, so having it do that seems sensible.  :)

So, given what I said above, ui-r=me.

Thanks,
Blake.

Comment 51

[Magnus Melin [:mkmelin]]

Note that you should also change the localization key when changing the value.

Comment 52

[Mark Banner (:standard8)]

Comment on attachment 530785 [details] [diff] [review]
Patch v7

Review of attachment 530785 [details] [diff] [review]:
-----------------------------------------------------------------

Some fly-by comments, re-requesting sr from David because of the MRUTime additions. I'm going to be pushing this to try and testing it tomorrow.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +383,3 @@
>  biffNotification_messages=has %1$S new messages
>  
>  # LOCALIZATION NOTE(biffNotification): %1$S is the name of the account %2$S is the number of new messages  

This should start:

# LOCALIZATION NOTE(newBiffNotification_message):

Please also duplicate for ewBiffNotification_messages.

@@ +383,4 @@
>  biffNotification_messages=has %1$S new messages
>  
>  # LOCALIZATION NOTE(biffNotification): %1$S is the name of the account %2$S is the number of new messages  
> +newBiffNotification_message=%1$S received %2$S new message

As Magnus said, we need to change the id for this and newBiffNotification_messages.

@@ +385,5 @@
>  # LOCALIZATION NOTE(biffNotification): %1$S is the name of the account %2$S is the number of new messages  
> +newBiffNotification_message=%1$S received %2$S new message
> +newBiffNotification_messages=%1$S received %2$S new messages
> +
> +# LOCALIZATION NOTE(biffNotification): %1$S is subject of new message and %2$S is sender of new message.

This should start:

# LOCALIZATION NOTE(newBiffNotification_messagetitle):

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ +213,5 @@
> +/* This comparator lets us sort an nsCOMArray of nsIMsgDBHdr's by
> + * their dateInSeconds attributes in ascending order.
> + */
> +static int
> +nsMsgDbHdrTimestampComparator (nsIMsgDBHdr *aElement1,

nit: no space after the function name (and please realign the two lines below).

@@ +798,5 @@
> +}
> +
> +#ifdef MOZ_THUNDERBIRD
> +nsresult
> +nsMessengerUnixIntegration::PutMRUTimestampForFolder(nsIMsgFolder *aFolder,

Ok, I want to get David's review on the MRU parts because he'll probably knows it better to comment on whether comment 48 still applies or not.

Comment 53

[David :Bienvenu]

Comment on attachment 530785 [details] [diff] [review]
Patch v7

since you're only setting the mrutime on a folder when the state goes from not new to new, like the other code you referenced, then I think it's OK.

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v8

Fixed Standard8's nits and localization keys.

Comment 55

[Mark Banner (:standard8)]

Comment on attachment 531261 [details] [diff] [review]
Patch v8

Review of attachment 531261 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 56

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/ccb90765df69

Comment 57

[Rimas Kudelis [use needinfo to ping]]

Just wondering, is it possible to make the notifications support plural forms properly? If so, I could file a bug about this.

Comment 58

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #57)
> Just wondering, is it possible to make the notifications support plural
> forms properly? If so, I could file a bug about this.

nevermind. Comment #50 answers this.