Closed Bug 389423 Opened 13 years ago Closed 12 years ago
Clicking on Dock icon doesn't restore last minimized window in Minefield
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
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.
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
> 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 #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 #276642 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 276642 [details] [diff] [review] nsINativeAppSupport patch rev2 (follow Josh's suggestions) Approval is not required in toolkit/
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.