1005619 - [windows] Using dual monitors, Firefox/Thunderbird opens in wrong monitor

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Saurabh Anand [:sawrubh]]

This is the bug for tracking the Linux specific fix for bug 264030.

Some additional comments from https://bugzilla.mozilla.org/show_bug.cgi?id=264030#c51 :

I have the opposite problem: Firefox always opens on my right-most (primary) monitor, even though I always close it (maximized) on my secondary (left-most) monitor.

Multi-monitor application bugs are one of my pet peeves, partially because Windows has two functions (GetWindowPlacement/SetWindowPlacement) that will do it correctly for you (and handles all the corner cases).  Don't try to reinvent the wheel, you'll most likely screw it up.  If you're not using GetWindowPlacement/SetWindowPlacement (which apparently Firefox isn't), then you've probably (in this case, definitely) overlooked something.

Comment 1

[Saurabh Anand [:sawrubh]]

(In reply to Saurabh Anand [:sawrubh] from comment #0)
> This is the bug for tracking the Linux specific fix for bug 264030.
> 
Sorry I meant 'bug for tracking the Windows specific fix'.
> Some additional comments from
> https://bugzilla.mozilla.org/show_bug.cgi?id=264030#c51 :
> 
> I have the opposite problem: Firefox always opens on my right-most (primary)
> monitor, even though I always close it (maximized) on my secondary
> (left-most) monitor.
> 
> Multi-monitor application bugs are one of my pet peeves, partially because
> Windows has two functions (GetWindowPlacement/SetWindowPlacement) that will
> do it correctly for you (and handles all the corner cases).  Don't try to
> reinvent the wheel, you'll most likely screw it up.  If you're not using
> GetWindowPlacement/SetWindowPlacement (which apparently Firefox isn't), then
> you've probably (in this case, definitely) overlooked something.

Comment 2

[Matt Brubeck (:mbrubeck)]

Note: For anyone working on this bug, the Mac OS X patch in bug 1005620 might be a useful starting point or reference.

Comment 3

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=70fd1771f746

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Add nsIWidget::GetRestoredBounds()

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Use nsIWidget::GetRestoredBounds from appshell

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Implement GetRestoredBounds on Windows

Comment 7

[Masatoshi Kimura [:emk]]

For anyone interested in this bug: a test build is available here.
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fdb7378d1518/try-win32/

Comment 8

[[e-mail]]

Comment on attachment 8420493 [details] [diff] [review]
Part 1: Add nsIWidget::GetRestoredBounds()

>+NS_METHOD nsBaseWidget::GetRestoredBounds(nsIntRect &aRect)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
I'd prefer something like
if (SizeMode() != nsSizeMode_Normal)
  return NS_ERROR_FAILURE;
return GetBounds(aRect);

This would then simplify the calling code.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Add nsIWidget::GetRestoredBounds()

I couldn't shortcut to GetBounds() because subclasses may override GetScreenBounds():
https://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp?rev=6c1c7e45c902#1423

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Use nsIWidget::GetRestoredBounds from appshell

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Implement GetRestoredBounds on Windows

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8420662 [details] [diff] [review]
Part 1: Add nsIWidget::GetRestoredBounds()

Review of attachment 8420662 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIWidget.h
@@ +1088,5 @@
>      NS_IMETHOD GetScreenBounds(nsIntRect &aRect) = 0;
>  
>      /**
> +     * Similar to GetScreenBounds except that this function will always.
> +     * get the normal (restored) widget's dimensions.

This comment isn't clear enough. What exactly does "the normal (restored) widget's dimensions" mean?

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Add nsIWidget::GetRestoredBounds()

Clarified the comment.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8420765 [details] [diff] [review]
Add nsIWidget::GetRestoredBounds()

Review of attachment 8420765 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!

Comment 15

[Neil Lee [:neilio]]

Comment on attachment 8420663 [details] [diff] [review]
Part 2: Use nsIWidget::GetRestoredBounds from appshell

Review of attachment 8420663 [details] [diff] [review]:
-----------------------------------------------------------------

Wrong person. Please make sure you assign to the correct Neil, guys.

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 8420663 [details] [diff] [review]
Part 2: Use nsIWidget::GetRestoredBounds from appshell

Oops.

Comment 17

[[e-mail]]

Comment on attachment 8420663 [details] [diff] [review]
Part 2: Use nsIWidget::GetRestoredBounds from appshell

Thanks, that's looking a lot better. I've only had time for a quick look so far, so I'll just point out that sizeMode could be moved down into the PAD_MISC block and gotRestoredBounds looks a little lonely on its own line ;-) Don't bother updating the patch just yet in case I find something else.

Comment 18

[[e-mail]]

Comment on attachment 8420663 [details] [diff] [review]
Part 2: Use nsIWidget::GetRestoredBounds from appshell

I have a problem. I can't actually reproduce the bug as described. I can for example configure Thunderbird to open the mail window maximised on the primary monitor and the address book maximised on the secondary monitor and it remembers which window should open on which monitor. Could someone provide me some STR?

Comment 19

[Lars Viklund]

The key to reproduce is that the monitor it appears on when starting is the one on which it was last in a non-maximized state on.

The common way to do this is to use the Aero Snap operations on Windows to move a maximized window between monitors (Win+Shift+Left/Right) and then close it.

Comment 20

[Michael]

I dunno about Thunderbird.
But i know it on Firefox:
- Move Firefox to second screen
- Maximise window
- Close Firefox
- Open Firefox again
Now you will find it on primary screen again.

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to [e-mail] from comment #18)
> Comment on attachment 8420663 [details] [diff] [review]
> I have a problem. I can't actually reproduce the bug as described. I can for
> example configure Thunderbird to open the mail window maximised on the
> primary monitor and the address book maximised on the secondary monitor and
> it remembers which window should open on which monitor. Could someone
> provide me some STR?

Hm, certainly the comment #20 issue is not reproducible for me, either. But it looks like this patch still fix the Aero Snap issue (comment #19).

Comment 22

[[e-mail]]

(In reply to Masatoshi Kimura from comment #21)
> (In reply to comment #18)
> > Comment on attachment 8420663 [details] [diff] [review]
> > I have a problem. I can't actually reproduce the bug as described. I can for
> > example configure Thunderbird to open the mail window maximised on the
> > primary monitor and the address book maximised on the secondary monitor and
> > it remembers which window should open on which monitor. Could someone
> > provide me some STR?
> 
> Hm, certainly the comment #20 issue is not reproducible for me, either. But
> it looks like this patch still fix the Aero Snap issue (comment #19).

http://stackoverflow.com/questions/5673242/getwindowplacement-gives-the-incorrect-window-position suggests that using Aero Snap to switch monitors doesn't update the restored window placement.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to [e-mail] from comment #22)
> http://stackoverflow.com/questions/5673242/getwindowplacement-gives-the-
> incorrect-window-position suggests that using Aero Snap to switch monitors
> doesn't update the restored window placement.

The questioner must have misunderstood something. Apps should remember the un-Aero-Snapped dimensions and GetWindowPlacement returns the dimensions correctly. GetWindowsRect returns the Aero-Snapped dimensions. Apps bundled with Windows (such as IE or Notepad) will also remember the un-Aero-Snapped dimensions.

Comment 24

[[e-mail]]

(In reply from comment #22)
> (In reply to Masatoshi Kimura from comment #21)
> > (In reply to comment #18)
> > > I have a problem. I can't actually reproduce the bug as described. I can for
> > > example configure Thunderbird to open the mail window maximised on the
> > > primary monitor and the address book maximised on the secondary monitor and
> > > it remembers which window should open on which monitor. Could someone
> > > provide me some STR?
> > 
> > Hm, certainly the comment #20 issue is not reproducible for me, either. But
> > it looks like this patch still fix the Aero Snap issue (comment #19).
> 
> http://stackoverflow.com/questions/5673242/getwindowplacement-gives-the-
> incorrect-window-position suggests that using Aero Snap to switch monitors
> doesn't update the restored window placement.

Ah, maybe I misunderstood that question. Anyway, I was able to borrow a dual-monitor Windows 7 PC and verified that the Aero Snap bug exists, but I don't have the authority to install an unreleased Firefox on that machine so I'll have to assume the code works by inspection.

Comment 25

[[e-mail]]

Comment on attachment 8420663 [details] [diff] [review]
Part 2: Use nsIWidget::GetRestoredBounds from appshell

(Don't forget to consider my minor nits in comment #17.)

Comment 26

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=1c3fd5d9c9b1

Comment 27

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b52fbd68bf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1cdabfb557
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc60fad06272

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b52fbd68bf3
https://hg.mozilla.org/mozilla-central/rev/8b1cdabfb557
https://hg.mozilla.org/mozilla-central/rev/bc60fad06272

Comment 29

[Loic]

What's the behavior with 2 more monitors (say 3 e.g.)? Does it work as it should be?

Comment 30

[Masatoshi Kimura [:emk]]

Should work with 3 or more monitors.

Comment 31

[Jacek Caban]

I landed a trivial fixup for GCC (mingw) builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/781f4aff3769

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/781f4aff3769

Comment 33

[Mike Kaply [:mkaply]]

Is it possible this caused bug 1250319?