Last Comment Bug 459482 - Dock Icon should show new mail at the same time as the growl alert
: Dock Icon should show new mail at the same time as the growl alert
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: Thunderbird 3.0b1
Assigned To: David Humphrey (:humph)
:
:
Mentors:
Depends on: 308552
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-11 01:08 PDT by Mark Banner (:standard8, afk until Dec)
Modified: 2008-11-11 14:04 PST (History)
8 users (show)
standard8: blocking‑thunderbird3+
standard8: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Attempt to fix notification call ordering (13.36 KB, patch)
2008-10-15 13:06 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review
patch with bienvenu's style corrections (13.81 KB, patch)
2008-10-17 09:20 PDT, David Humphrey (:humph)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review
r/sr+ patch, fixed double-notification regression, nits fixed (15.19 KB, patch)
2008-10-30 14:29 PDT, David Humphrey (:humph)
david.humphrey: review+
david.humphrey: superreview+
Details | Diff | Splinter Review
Updated pref logic (15.20 KB, patch)
2008-11-10 13:36 PST, David Humphrey (:humph)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, afk until Dec) 2008-10-11 01:08:22 PDT
From bug 308552 comment 99:

* Not doing the dock icon badging until after the alert is dismissed is not
what we want to do: not only is it confusing to dismiss a sticky alert and only
then have the icon bounce and get badged, it also opens us up to badging it
with the unexciting but extremely persistent news that you have 0 new emails,
if you have one and read it before the alert goes away.
Comment 1 David Humphrey (:humph) 2008-10-13 20:28:15 PDT
Help me understand what should happen here.  As I see it, there is:

1) growl installed: show growl alert and badge dock icon with count

2) growl not-installed: bounce dock icon and badge dock icon with count

For 2) I need to understand how to apply mail.biff.animate_dock_icon.  I take it to mean "bounce dock" vs. badging the icon (i.e., I assume I should badge the icon no matter the value of the pref).

Make sense?
Comment 2 Mark Banner (:standard8, afk until Dec) 2008-10-14 02:19:06 PDT
I think you've got the right steps there. I think as the growl alerts can go away, and we have the option for the dock icon to show the number, we should maintain that.

(In reply to comment #1)
> For 2) I need to understand how to apply mail.biff.animate_dock_icon.  I take
> it to mean "bounce dock" vs. badging the icon (i.e., I assume I should badge
> the icon no matter the value of the pref).

I think that would be reasonable as a first cut and we can see what feedback we get.
Comment 3 David Humphrey (:humph) 2008-10-15 13:06:41 PDT
Created attachment 343281 [details] [diff] [review]
Attempt to fix notification call ordering

I've split-out badging and bouncing, trying to honour the various alert/icon prefs as made sense based on the above.  After a discussion last night on irc, I'm not clear the right path, given that you can also disable the growl "New Mail" notification from the growl pref panel, and perhaps this is the "right" place to do it.  I'm deathly allergic to bouncing the dock, but I've done it anyway :)

Also, I removed all references to mSuppressBiffIcon, since it is PR_FALSE in all cases.  If I'm missing something important, let me know and I'll add them back.
Comment 4 Mark Banner (:standard8, afk until Dec) 2008-10-15 14:28:00 PDT
Comment on attachment 343281 [details] [diff] [review]
Attempt to fix notification call ordering

Just spoke to David about this on irc, switching reviewers around due to those with time available and who have a mac.
Comment 5 Mark Banner (:standard8, afk until Dec) 2008-10-17 01:36:11 PDT
I've just been testing out the patch and found some issues I think we need to resolve:

- Growl detection doesn't work correctly (you can test this by stopping growl in the system preferences) - we don't get the dock icon bouncing.

When growl isn't installed/running, then we won't get the before-growl-registration notification, see http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/alerts/src/mac/nsAlertsService.mm#145 for more details.

- Preferences UI is now a bit strange.

If we've got growl installed and running, then selecting "Animate the dock icon" does nothing.

So at the moment, I'm thinking either we should honour that pref regardless of if growl is installed or not, or remove/hide it in the preferences UI if growl is installed & running.


Thoughts welcome...
Comment 6 David :Bienvenu 2008-10-17 07:41:52 PDT
Comment on attachment 343281 [details] [diff] [review]
Attempt to fix notification call ordering

there are also some style issues - 

+    } else {
+      mAlertInProgress = PR_TRUE;
+      BounceDock();

} else { should be
}
else
{

and this indentation looks wrong:

+    if (domWindow)
+	{
+      nsCOMPtr<nsIDOMChromeWindow> chromeWindow(do_QueryInterface(domWindow));
+      chromeWindow->GetAttention();
+	}

as does this:

+  if (err != noErr) 
+  {
+	NS_WARNING("FMGetFontFromFontFamilyInstance failed");
+	::EndCGContextForApplicationDockTile(context);
+	return NS_ERROR_FAILURE;
   }

(4 spaces instead of 2?)
Comment 7 David Humphrey (:humph) 2008-10-17 09:20:27 PDT
Created attachment 343560 [details] [diff] [review]
patch with bienvenu's style corrections

Fixed style issues.

Regarding Mark's comments, I've done some more research and testing.  It seems that growl-not-installed and growl-installed-but-disabled are not the same thing from within Mozilla.  In fact, we can't know that the user has disabled (i.e., stopped) growl due to the growl APIs: either the nsIAlertsService is there, or we are thrown NS_ERROR_SERVICE_NOT_AVAILABLE.  Stopping growl won't change this.

This patch works if you remove growl (see http://growl.info/documentation/growl-package-removal.php for instructions).  If you simply stop it, you'll get no growl notification, and also no dock bounce.  I think this is right, and we shouldn't try to circumvent the user disabling growl and fall-back to the icon bounce.

The prefs still might not be what we want, but they work as described above.
Comment 8 David Humphrey (:humph) 2008-10-17 12:13:07 PDT
See also bug 381520.  According to that, I could check for NS_ERROR_NOT_AVAILABLE when calling showAlertNotification, and bounce in that case too.  I still think that's wrong: if the user disabled growl, that's his/her decision and we shouldn't layer bounce on top of that.
Comment 9 Mark Banner (:standard8, afk until Dec) 2008-10-22 02:02:16 PDT
Comment on attachment 343560 [details] [diff] [review]
patch with bienvenu's style corrections

Well, I still don't like the fact that if growl is installed we ignore the "animate the dock icon" preference.

Personally I think we should respect that whatever the state of growl is, although ideally we probably don't want to do both.

However, I'm going to pass the buck and ask our user-experience person (Bryan) for an opinion.

Bryan, if you can't understand the comments in this bug, ping us for a discussion on irc.

One nit:

+  nsCOMPtr<nsIWindowMediator> mediator (do_GetService(NS_WINDOWMEDIATOR_CONTRACTID));

As you're moving the line, please could you drop the space between mediator and (do_GetService.
Comment 10 David :Bienvenu 2008-10-22 07:06:49 PDT
Comment on attachment 343560 [details] [diff] [review]
patch with bienvenu's style corrections

a few more nits:

I know you just moved this code, but there are a bunch of places where the code doesn't need to assign to a temp err var, e.g.,
+  err = ::ATSUCreateStyle(&style);
+  if (err != noErr) 
+  {
+    NS_WARNING("ATSUCreateStyle failed");
+    ::EndCGContextForApplicationDockTile(context);
+    return NS_ERROR_FAILURE;
+  }
+        

can be if (::ATSUCreateStyle(&style) != noErr) ...

I'll leave it up to you if you want to change those or not.

I have *no* opinion about the bouncing dock, other than it's horribly annoying :-)
Comment 11 Bryan Clark (DevTools PM) [:clarkbw] 2008-10-22 14:06:17 PDT
I'm going to wait until this weekend so I can get a demo of this running from davida or someone with the patch ready; I feel like I need see this in action.
Comment 12 David Humphrey (:humph) 2008-10-30 14:29:32 PDT
Created attachment 345567 [details] [diff] [review]
r/sr+ patch, fixed double-notification regression, nits fixed

Thanks Mark, David.  I've addressed your comments.

In fixing these nits, I also noticed I regressed the growl stuff, such that 2 notifications get shown per 1 new mail.  I altered my mAlertInProgress logic in this patch, and it's now fixed.

Clark, I have a fix for bug 460287 I'd like to put up, which builds on this patch.  Any chance you can take a look and let me know if I need to make further changes here before I do?
Comment 13 Bryan Clark (DevTools PM) [:clarkbw] 2008-11-05 13:52:37 PST
Ok, I had a chance to take a look at this yesterday.  Sorry for the slow response time, but I think I have a handle on what's happening here.

If we're going to support growl it would seem that we need to support it with a holistic idea of mail notifications.  With growl and the dock animation we have two systems for notifying a person of new mail and it's a bit overload.  The dock animation is a low-fi way of saying "something happened" while the growl notification is a more hi-fi way of telling the person exactly what happened.  So I don't think we need both happening at the same time.

I am wondering if we should be turning off the dock animation preference if growl is installed, instead of ignoring it.  This way a person could turn it back on themselves.
Comment 14 David Humphrey (:humph) 2008-11-06 11:20:35 PST
(In reply to comment #13)
> I am wondering if we should be turning off the dock animation preference if
> growl is installed, instead of ignoring it.  This way a person could turn it
> back on themselves.

So we're really talking about these two, I think:

mail.biff.animate_dock_icon
mail.biff.show_alert

We don't have any way to know that growl is installed/running except to try and call into it--either it works or it doesn't (i.e., the service is there).  So I'm not clear what to do with "[turn] off the dock animation preference if growl is installed."  Right now my logic is:

OnNewMail {
  if mail.biff.show_alert
    if growl is installed // also includes user disabling/stopping it
      growl()
    else
      if mail.biff.animate_dock_icon
        bounce_dock()
}

So I could set animate_dock_icon = false each time I growl(), but that seems crazy, and would overwrite any true pref the user put in manually.  I could also drop the else and just honour the animate_dock_icon pref, but that gets back to your duplication of effort issue, on which I agree with you.

What to do?  I vote for my way ;)
Comment 15 Bryan Clark (DevTools PM) [:clarkbw] 2008-11-06 11:47:02 PST
Ok, I understand.  Thanks for the explanation!  Your way does seem to make the most sense, lets do that!
Comment 16 Mark Banner (:standard8, afk until Dec) 2008-11-06 11:50:22 PST
I've just noticed that mail.biff.show_alert isn't set in preferences on Mac: http://mxr.mozilla.org/comm-central/source/mail/components/preferences/general.xul#109 

Therefore I don't think we should honour that pref there (and maybe remove it from mailnews.js for mac), especially as its duplication of turning things on/off.
Comment 17 David Humphrey (:humph) 2008-11-06 13:30:27 PST
Thanks Clark.

Mark, I'm thinking about the other mac-biff bugs I'm working on, and thinking that removing this from mailnews.js is going to suck when a user wants to stop growl notifications--unless you mean "remove the mac special case in mailnews.js."

The fact that all we have is "Animate the dock icon" for mac sucks now that growl is a possible alternative.  I don't love this idea, but what about changing the pref panel like so:

When new messages arrive:__________________________________________
[] Animate the dock icon
[] Show an alert (requires Growl)

Presumably we could even dynamically add/remove that second pref option from the dialog, based on whether the service is available?  That's perhaps more clever than it needs to be.
Comment 18 Mark Banner (:standard8, afk until Dec) 2008-11-10 10:39:22 PST
Bryan and I have just been discussing this.

"Animate the dock icon" is off by default. Given that fact, the user experience for new installs will be that if growl is installed, then we'll use that by default.

So the best thing to do here would be to change your OnNewMail function to be:

OnNewMail {
  if mail.biff.show_alert
    if growl is installed // also includes user disabling/stopping it
      growl()
    if mail.biff.animate_dock_icon
      bounce_dock()
}

In that way, we'll use animate dock icon if the user sets it, and we'll use growl if growl is installed. For those users that have been animating the dock icon, then they should already know where the preference is.

Hopefully this will provide a reasonable user experience in all cases.
Comment 19 David Humphrey (:humph) 2008-11-10 11:55:11 PST
OK, I think this makes sense, and I'll update my patch to do what you've outlined above.  Thanks, guys.

Before I do, can you make a decision on comments #16 and #17?  If I need to update the pref UI too, I'd like to do it all at once.
Comment 20 Mark Banner (:standard8, afk until Dec) 2008-11-10 12:24:39 PST
(In reply to comment #19)
> Before I do, can you make a decision on comments #16 and #17?  If I need to
> update the pref UI too, I'd like to do it all at once.

From what we (Bryan and myself) were talking about earlier, we'll just leave the pref UI as it is for now and see how it goes.
Comment 21 David Humphrey (:humph) 2008-11-10 13:36:09 PST
Created attachment 347356 [details] [diff] [review]
Updated pref logic

I've updated the pref logic based on Mark's comments.  I also renamed BounceDock() to BounceDockIcon() in order to maintain naming consistency.
Comment 22 David Humphrey (:humph) 2008-11-10 19:18:56 PST
Comment on attachment 347356 [details] [diff] [review]
Updated pref logic

This is ready for review.  I waited, as I thought I'd broken something, but after more testing realized that the "brokenness" was there before I started.

Specifically, I was concerned that when you get new mail, and a growl notification, subsequent new messages don't cause more notifications.  This happens because in nsMessengerOSXIntegration::OnItemIntPropertyChanged the value of aProperty isn't "BiffState."  So it's doing what it's supposed to, but that doesn't seem right to me.  Am I wrong?

Personally, I'd expect that when I get new mail, I get notified, regardless of what has happened previously.
Comment 23 David :Bienvenu 2008-11-11 13:52:58 PST
(In reply to comment #22)

> 
> Specifically, I was concerned that when you get new mail, and a growl
> notification, subsequent new messages don't cause more notifications.  This
> happens because in nsMessengerOSXIntegration::OnItemIntPropertyChanged the
> value of aProperty isn't "BiffState."  So it's doing what it's supposed to, but
> that doesn't seem right to me.  Am I wrong?

You might be seeing bug 459487 - but in theory, we clear the biff icon/state if you do things like read a message or open a folder, but we don't clear it if you don't do anything.
Comment 24 David :Bienvenu 2008-11-11 13:53:47 PST
Comment on attachment 347356 [details] [diff] [review]
Updated pref logic

thx for doing this!
Comment 25 Mark Banner (:standard8, afk until Dec) 2008-11-11 14:04:36 PST
Patch checked in, thanks.

http://hg.mozilla.org/comm-central/rev/1285f4b5d454

Note You need to log in before you can comment on or make changes to this bug.