Closed
Bug 555536
Opened 14 years ago
Closed 13 years ago
alert on new message does not appear as specified in preferences
Categories
(Thunderbird :: Mail Window Front End, defect)
Tracking
(blocking-thunderbird5.0 beta1+, blocking-thunderbird3.1 -)
RESOLVED
FIXED
Thunderbird 5.0b1
People
(Reporter: mozilla, Assigned: mconley)
References
Details
(Keywords: regression, Whiteboard: [l10n])
Attachments
(2 files, 8 obsolete files)
125.29 KB,
image/png
|
Details | |
50.54 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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
Updated•14 years ago
|
Keywords: regression,
regressionwindow-wanted
Version: unspecified → Trunk
Comment 1•14 years ago
|
||
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 ?
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•14 years ago
|
||
(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 !
Blocks: 478463
Keywords: regressionwindow-wanted
Comment 4•14 years ago
|
||
I need a change like Mac OS X's growl.
Assignee: nobody → m_kato
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
(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•14 years ago
|
||
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•14 years ago
|
||
This sounds like an unintentional regression that we should resolve in some manner before b2 (feature/string freeze) or rc1 at the latest.
blocking-thunderbird3.1: --- → ?
Comment 8•14 years ago
|
||
Makoto, how quickly do you think you could have a patch with a test for this?
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Comment 12•14 years ago
|
||
Makoto, thanks for the WIP patch! What still needs to happen before this patch is review-ready?
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
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>.
Keywords: late-l10n
Comment 15•14 years ago
|
||
Adding thunderbird@localization.bugs for consideration of comment 14.
Comment 16•14 years ago
|
||
Attachment #439509 -
Attachment is obsolete: true
Comment 17•14 years ago
|
||
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?
blocking-thunderbird3.1: ? → rc1+
Keywords: late-l10n
Comment 18•14 years ago
|
||
Clarification: The second * in option 1) depends on the answer to the question about comment 13, I see now.
Comment 19•14 years ago
|
||
4) Sounds too hackish for me ....
Comment 20•14 years ago
|
||
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•14 years ago
|
||
> 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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
Stacked notifications, one per checked server.
Comment 26•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #440673 -
Flags: review?(bugzilla)
Comment 28•14 years ago
|
||
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•14 years ago
|
||
Also, if libnotify isn't supported on platform, ShowAlertNotification of system-alert returns error. So this fix will be included for bug 561427 too.
Status: NEW → ASSIGNED
Comment 30•14 years ago
|
||
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.
blocking-thunderbird3.1: rc1+ → -
Updated•14 years ago
|
Attachment #440673 -
Flags: review?(bugzilla) → review-
Comment 31•14 years ago
|
||
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.
Updated•14 years ago
|
Flags: wanted-thunderbird+
Flags: blocking-thunderbird-next?
Comment 32•14 years ago
|
||
Makoto San are you up for a new patch ?
Updated•14 years ago
|
blocking-thunderbird5.0: --- → ?
Flags: blocking-thunderbird-next?
Updated•13 years ago
|
blocking-thunderbird5.0: ? → needed
Comment 33•13 years ago
|
||
Makoto isn't able to work on this at the moment, I'll see if I can find someone.
Assignee: m_kato → nobody
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mconley
Reporter | ||
Comment 34•13 years ago
|
||
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?
Updated•13 years ago
|
Whiteboard: [shouldn't affect l10n]
Updated•13 years ago
|
blocking-thunderbird5.0: needed → alpha4+
Assignee | ||
Comment 35•13 years ago
|
||
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.
Attachment #440673 -
Attachment is obsolete: true
Attachment #527322 -
Flags: superreview?(bugzilla)
Attachment #527322 -
Flags: review?(dbienvenu)
Comment 36•13 years ago
|
||
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.
Attachment #527322 -
Flags: superreview?(dbienvenu)
Attachment #527322 -
Flags: superreview?(bugzilla)
Attachment #527322 -
Flags: review?(dbienvenu)
Attachment #527322 -
Flags: review?(bugzilla)
Comment 37•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 527322 [details] [diff] [review] patch v2 clearing sr request since we'll want a new patch from the prev comments.
Attachment #527322 -
Flags: superreview?(dbienvenu)
Assignee | ||
Comment 39•13 years ago
|
||
Thanks for the review, David - good catches. I've made the alterations you suggested.
Attachment #527322 -
Attachment is obsolete: true
Attachment #528090 -
Flags: review?(dbienvenu)
Attachment #527322 -
Flags: review?(mbanner)
Comment 40•13 years ago
|
||
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;
Attachment #528090 -
Flags: review?(dbienvenu) → review-
Assignee | ||
Comment 41•13 years ago
|
||
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.
Attachment #528090 -
Attachment is obsolete: true
Attachment #528928 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 42•13 years ago
|
||
Patch v4 wasn't properly based off of trunk.
Attachment #528928 -
Attachment is obsolete: true
Attachment #529469 -
Flags: review?(dbienvenu)
Attachment #528928 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 43•13 years ago
|
||
This patch introduces a new l10n string.
Whiteboard: [shouldn't affect l10n] → [l10n]
Comment 44•13 years ago
|
||
Comment on attachment 529469 [details] [diff] [review] Patch v5 I can't review this per se, but I can sr it...
Attachment #529469 -
Flags: superreview?(dbienvenu)
Attachment #529469 -
Flags: review?(mbanner)
Attachment #529469 -
Flags: review?(dbienvenu)
Comment 45•13 years ago
|
||
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
Attachment #529469 -
Flags: superreview?(dbienvenu) → superreview+
Assignee | ||
Comment 46•13 years ago
|
||
I've integrated bienvenu's suggestions from my +'d superreview.
Attachment #529469 -
Attachment is obsolete: true
Attachment #529746 -
Flags: review?(mbanner)
Attachment #529469 -
Flags: review?(mbanner)
Comment 47•13 years ago
|
||
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 = "new-account@invalid.com"; > + > + 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.
Attachment #529746 -
Flags: review?(mbanner) → review-
Assignee | ||
Comment 48•13 years ago
|
||
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. :)
Attachment #529746 -
Attachment is obsolete: true
Assignee | ||
Comment 49•13 years ago
|
||
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.
Attachment #530785 -
Flags: ui-review?(bwinton)
Attachment #530785 -
Flags: review?(mbanner)
Comment 50•13 years ago
|
||
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.
Attachment #530785 -
Flags: ui-review?(bwinton) → ui-review+
Comment 51•13 years ago
|
||
Note that you should also change the localization key when changing the value.
Updated•13 years ago
|
Whiteboard: [l10n] → [l10n][needs review standard8]
Comment 52•13 years ago
|
||
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.
Attachment #530785 -
Flags: superreview?(dbienvenu)
Comment 53•13 years ago
|
||
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.
Attachment #530785 -
Flags: superreview?(dbienvenu) → superreview+
Assignee | ||
Comment 54•13 years ago
|
||
Fixed Standard8's nits and localization keys.
Attachment #530785 -
Attachment is obsolete: true
Attachment #531261 -
Flags: review?(mbanner)
Attachment #530785 -
Flags: review?(mbanner)
Comment 55•13 years ago
|
||
Comment on attachment 531261 [details] [diff] [review] Patch v8 Review of attachment 531261 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #531261 -
Flags: review?(mbanner) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [l10n][needs review standard8] → [l10n][ready to land]
Comment 56•13 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/ccb90765df69
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [l10n][ready to land] → [l10n]
Target Milestone: --- → Thunderbird 3.3a4
Updated•13 years ago
|
Flags: in-testsuite+
Comment 57•13 years ago
|
||
Just wondering, is it possible to make the notifications support plural forms properly? If so, I could file a bug about this.
Comment 58•13 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•