Closed Bug 389423 Opened 13 years ago Closed 12 years ago

Clicking on Dock icon doesn't restore last minimized window in Minefield

Categories

(Core :: XUL, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: marcia, Assigned: smichaud)

Details

Attachments

(1 file, 3 obsolete files)

Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200707240404 Minefield/3.0a7pre.

STR:
1. Minimize an open window.
2. Click on the Minefield dock icon.

Expected: The open window will launch.
Reality: Clicking does nothing, the open page does not maximize.
My guess is this is some kind of Apple Event silliness, which it might
be possible to deal with in toolkit/xre/MacApplicationDelegate.mm.

I'll take a look when I have the chance (probably later this week or
next week).
Turns out this bug is caused by problems in both
MacApplicationDelegate.mm and nsNativeAppSupport.cpp.

Here's a patch that changes only MacApplicationDelegate.mm (it
replaces the methods from nsNativeAppSupport.cpp that need to be
changed).

This works just fine, but it violates the nsINativeAppSupport
paradigm.

So (hopefully later today) I'll also post a second, alternative patch
that changes both MacApplicationDelegate.mm and nsNativeAppSupport.cpp.

The applicationShouldHandleReopen: delegate method (in
MacApplicationDelegate) is called whenever the user clicks the Dock
icon.  The current code correctly handles the case where the browser
has no open windows (not even minimized ones) -- it calls
nsNativeAppSupportMac::ReOpen(), which opens a new browser window.

But (as Marcia reported in comment #0) it doesn't correctly handle the
browser has open windows but all of them are minimized.  There are two
reasons for this:

1) The current applicationShouldHandleReopen: method does nothing if
   hasVisibleWindows is TRUE.  But on OS X a minimized window counts
   as a visible window.

2) The code in nsNativeAppSupportMac::ReOpen() that handles minimized
   windows only works on Carbon apps -- not on Cocoa apps.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Assignee: smichaud → jag
Status: ASSIGNED → NEW
Component: OS Integration → XP Toolkit/Widgets
Product: Firefox → Core
QA Contact: os.integration → xptoolkit.widgets
Hardware: PC → Macintosh
Assignee: jag → smichaud
Here's my alternate patch, which I promised earlier, that uses
nsINativeAppSupport.  I made a few changes to
nsNativeAppSupportMac.cpp (to make it support Cocoa apps instead of
Carbon ones) and changed its name to nsNativeAppSupportCocoa.mm.

Because support for Mac widgets has been dropped (in favor of Cocoa
widgets), we no longer need a nsNativeAppSupportMac.cpp on the trunk.

Warning:  This is not a proper patch, and won't apply to the trunk.
In order to make it more readable (and to show how few changes I made
to nsNativeAppSupportMac.cpp), I changed the "original"
nsNativeAppSupportMac.cpp file's name to nsNativeAppSupportCocoa.mm
before generating the patch.

Side note:  For good measure I also changed the code in
nsNativeAppSupportCocoa::Start() to check for at least OS X 10.4.  I
haven't yet had a chance to test this.  Hopefully I'll be able to do
that sometime this afternoon.
Benjamin, which of these two kinds of patches do you prefer?

Whichever one you pick, I'll ask someone to review it.
Flags: blocking1.9?
Comment on attachment 274633 [details] [diff] [review]
Fix that doesn't use/change nsNativeAppSupportMac.cpp

I talked to Benjamin and Michael Wu on #developers, and the
nsINativeAppSupport patch won by a nose.

But if someone has strong preferences the other way, let me know.
Attachment #274633 - Attachment is obsolete: true
Attachment #274661 - Flags: review?(joshmoz)
> Side note:  For good measure I also changed the code in
> nsNativeAppSupportCocoa::Start() to check for at least OS X 10.4.  I
> haven't yet had a chance to test this.  Hopefully I'll be able to do
> that sometime this afternoon.

I just tested this in OS X 10.3 ... and it sort-of worked.

No alert message was displayed, but the browser did quit immediately.
Today's Minefield nightly also quits immediately, but only after
displaying an ld error about an undefined reference to
_GetAliasSizeFromPtr.  So my build's behavior is an improvement.

Better still would be to display an alert box and then die.

I don't know the ancient Mac techniques used in the code for
nsNativeAppSupportCocoa::Start() that I borrowed from
nsNativeAppSupportMac.cpp.  But I could manage to display some kind of
Cocoa alert dialog ... which should be fine for our purposes.

I'll work on that (probably tomorrow).
On second thought maybe I shouldn't display an alert box.

It would be a cop-out to display it always in English ... but
localizing it would be a _lot_ of work (most of which would have to be
done by other people).

Maybe it's best to change my patch to explicitly make the browser die
silently on OS X versions < 10.4.

If someone wants to make nsNativeAppSupportCocoa::Start() display an
alert box on OS X versions less than 10.4, that can be the subject of
another bug.
Steven, 10.3 is no longer supported on the trunk. You probably don't need to worry about it for this patch.
Here's a revision of my nsINativeAppSupport patch that gets rid of the
ancient (and no longer workable) dialog cruft from
nsNativeAppSupportCocoa::Start() and replaces it with code that logs
to the console (in English) and quits on OS X versions less than 10.4.

This doesn't fully resolve the problem of how to let the user know
that the browser won't run on these OS X versions ... but that should
be saved for another bug.

My current code is a decent interim solution ... and is definitely an
improvement on displaying a linker error :-)
Attachment #274661 - Attachment is obsolete: true
Attachment #274661 - Flags: review?(joshmoz)
Attachment #274781 - Flags: review?(joshmoz)
Comment on attachment 274781 [details] [diff] [review]
Revised nsINativeAppSupport patch (quit with console error on less than 10.4)

Please search for and replace all the tabs in nsNativeAppSupportCocoa.mm with two spaces. Looks like there are more in that file. This can be done on checkin.

+            *a_nativeWindow = (NSWindow *)mruWidget->GetNativeData(NS_NATIVE_WINDOW);

Please no space between the NSWindow and its pointer '*'.

NS_ADDREF( *aResult );

Please remove the spaces around the *aResult.
Attachment #274781 - Flags: review?(joshmoz) → review+
Attachment #274781 - Attachment is obsolete: true
Attachment #276642 - Flags: superreview?(benjamin)
Attachment #276642 - Flags: superreview?(benjamin) → superreview+
Attachment #276642 - Flags: approval1.9?
Comment on attachment 276642 [details] [diff] [review]
nsINativeAppSupport patch rev2 (follow Josh's suggestions)

Approval is not required in toolkit/
Attachment #276642 - Flags: approval1.9?
OK, thanks.

I'll land this tomorrow.
Landed on trunk.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Minefield icon unresponsive when selected from the Dock → Clicking on Dock icon doesn't restore last minimized window in Minefield
Will Thunderbird need a similar fix? 
> Will Thunderbird need a similar fix?

I don't know.

Does Thunderbird currently use Cocoa widgets and XRE_main() (in
toolkit/xre/nsAppRunner.cpp)?  If so, current trunk versions should
already have this fix.

(I assume you're talking about Thunderbird on the trunk.  Branch
versions will almost certainly not need this fix.)
Yes, I'm talking about the trunk. I've cc'd mscott. 
yeah that patch would be used by Thunderbird too Asa. Did the fix not work for you?
Sorry, I haven't gotten today's build yet for trunk Tbird. I'll assume for now that all is cool here. Just wanting to make sure though. Thanks guys. 
You need to log in before you can comment on or make changes to this bug.