459485 - Clicking on Growl alert doesn't always bring Thunderbird (and maybe SeaMonkey) in front of a native app.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

From bug 308552 comment 99:

* I'm not really clear (having been too lazy to actually look) on why we have
separate actions for Tb and SM when you click the alert, but the Tb one doesn't
seem to do what it wants to (which is probably Widget:Cocoa's fault): with Tb
behind Fx, clicking the alert will generally get us to the top, without
changing the selected folder the way the SM version does, but if we're behind a
native app, we only come up to the top of the stack behind it.

Comment 1

[David Humphrey (:humph)]

I've spent some time on this, and I'm not sure what's up.  I've traced it down through nsChildView::SetFocus(), where sendFocusEvent:NS_GOTFOCUS gets called: 

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#1090

I'm wondering if the growl notification is hanging onto focus or otherwise causing some sort of timing bug?  I tried asking in #growl, but they didn't seem to know/care.

CC'ing Shawn for other ideas.

Also, I agree that the SM/TB code here can be merged, once this is sorted out.  I'll do that.

Comment 2

[Shawn Wilsher :sdwilsh]

I had issues with this with my addon as well.  Is the code just calling .focus on the window?  I had to do some cocoa call to bring the application front and ceter as well.

Comment 3

[David Humphrey (:humph)]

Yeah, I just .Focus() from an nsIDOMWindowInternal.  Where is your ext and I'll try what you did?

Comment 4

[Shawn Wilsher :sdwilsh]

I don't have access to the source code anymore, and it's native code :/

Comment 5

[Shawn Wilsher :sdwilsh]

Turns out I had a local svn checkout still from before their repo died.  The code looked like this:
NS_IMETHODIMP
grNotifications::MakeAppFocused()
{
  OSErr err;

  ProcessSerialNumber psn;
  err = ::GetCurrentProcess(&psn);
  if (err != 0) return NS_ERROR_FAILURE;

  err = ::SetFrontProcess(&psn);
  if (err != 0) return NS_ERROR_FAILURE;

  return NS_OK;
}

Before you do anything like it, you should check with josh first of course.

Comment 6

[David Humphrey (:humph)]

Attachment: Fix using sdwilsh's native magic (aka, all blame to Shawn!)

Fixed with native focus code, which isn't that much of a problem here, given that we do native stuff to badge the dock icon as well.  However, Josh, can you please look at this?

I've also merged the TB code with the SM code, just using the SM code.  Not sure if this needs a SM review too, since they now use this native focus call too.

Untested on SM.

Comment 7

[David Humphrey (:humph)]

Josh, thanks for your review.  Question: is there a reason we don't do it like this in Focus/SetFocus?  Should I file a bug to fix it there?

Comment 8

[Josh Aas]

Please file a bug on that so we can at least have a bug to sort out the issue. cc me and smichaud and mstange. Thanks!

Comment 9

[David Humphrey (:humph)]

> Please file a bug on that so we can at least have a bug to sort out the issue.
> cc me and smichaud and mstange. Thanks!

Filed bug 465446

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 348254 [details] [diff] [review]
Fix using sdwilsh's native magic (aka, all blame to Shawn!)

>+// HACK: Gets around limitations of nsIDOMWindowInternal::Focus() on Mac

Please reference bug 465446 here, so that we can come back and fix it later.

>@@ -88,19 +102,17 @@ static void openMailWindow(const nsACStr
>     if (!aFolderUri.IsEmpty())
>     {
>       nsCOMPtr<nsIMsgWindowCommands> windowCommands;
>       topMostMsgWindow->GetWindowCommands(getter_AddRefs(windowCommands));
>       if (windowCommands)
>         windowCommands->SelectFolder(aFolderUri);
>     }
>     
>-    nsCOMPtr<nsIDOMWindowInternal> domWindow;
>-    topMostMsgWindow->GetDomWindow(getter_AddRefs(domWindow));
>-    domWindow->Focus();
>+    FocusAppNative();

Not quite right. You still need the domWindow->Focus() because if the user has multiple windows open and the mail window is behind another window then your version will bring the app to the front, but not the mail window. So keep both sets in please. I don't think it matters which way round, I tested it with the FocusAppNative() first though.

r/sr=me with that fixed.

Comment 11

[David Humphrey (:humph)]

Attachment: version 2 with bug ref comment, added back domWindow->Focus()

Thanks for the review.  I've made your corrections.  This should be ready for check-in.

Comment 12

[Mark Banner (:standard8)]

Thanks David, checked in: http://hg.mozilla.org/comm-central/rev/832c859036ef