Closed Bug 561273 Opened 14 years ago Closed 13 years ago

[OS/2] Window Position Bugs

Categories

(Core Graveyard :: Widget: OS/2, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dragtext, Assigned: dragtext)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch Window Position patch for v1.9.3 (obsolete) — Splinter Review
These patches fix 3 bugs revealed by this scenario:
- open multiple browser windows, then minimize one or more of them
- shutdown the app, selecting the option to save open tabs
- restart the app

1 - windows that had been minimized open in a restored state but the top portions of the windows are offscreen.  This only occurs in v1.9.3 and is caused by an error I made when rewriting os2FrameWindow::Resize().

2 - windows that had been minimized will not display any menus;  they have to be minimized again then restored before menus work.  The reason is that mozilla thinks they're currently minimized, so it suppresses popups.   This bug is present in all versions of 1.9.x.

3 - since mozilla expects these windows to be minimized, the fact that they aren't is a bug.  The existing code doesn't support this because it was written years ago, long before there was any need to minimize or maximize a window programmatically.

The attached patches for v1.9.1, 1.9.2, and 1.9.3 fix these bugs.

Note:  Bug 559288 reports the same menu problem described in item 2 above, but for maximized windows.  I believe that problem is also due to a size-mode mismatch.  I'm marking that bug as a duplicate of this one and assuming that these patches will fix it as well.
Attachment #440943 - Flags: review?(wuno)
Attached patch Window Position patch for v1.9.2 (obsolete) — Splinter Review
Attachment #440944 - Flags: review?(wuno)
Attached patch Window Position patch for v1.9.1 (obsolete) — Splinter Review
Attachment #440945 - Flags: review?(wuno)
Comment on attachment 440943 [details] [diff] [review]
Window Position patch for v1.9.3

I tested your scenario with your patch. 3 Windows, 1 with a lot of open tabs, 1 minimized, 2 maximized, closed the application and saved the tabs. Then after a restart, 2 windows were maximized the 3rd minimized and all tabs were restored, cool.
Attachment #440943 - Flags: review?(wuno) → review+
Mixed results with SeaMonkey built against 1.9.3.
Previously Window #1 and #2 would restore to Window #1's position with #3 where #2 is expected. I think this behaviour has been around for quite a while but I usually only have one window open.
With the patch and killing SeaMonkey to invoke the session manager, sometimes it comes up without the session manager, just restores everything. In this case the behaviour is the same as previous.
When the session manager comes up it usually works as expected, minimized windows flash on the desktop then minimize. Occasionally the minimized window restores non-minimized and even more occasionally the non-minimized window is at the same co-ordinates as window #1. This is quite possibly bugs with the SeaMonkey session manager.
One consistent bug is that SeaMonkey's window menu does not restore the other windows, whether minimized or just behind window #1. The Warpcentre menu does restore them then the window menu works to change focus between the windows.
(In reply to comment #5)
> One consistent bug is that SeaMonkey's window menu does not restore the other
> windows, whether minimized or just behind window #1. The Warpcentre menu does
> restore them then the window menu works to change focus between the windows.

Was this the behavior before the patch?  IIUC, windows that get minimized during startup can't be restored by the window menu.  What about windows minimized during the current session (via the Min button)?  Does the menu work for them?

Also, for windows that should be minimized at startup but aren't, do their menus work?  If not, then they should be minimized but that isn't happening.  OTOH, if the menus work, then SM has lost track of the fact that they were minimized when the last session ended.
(In reply to comment #6)
> (In reply to comment #5)
> > One consistent bug is that SeaMonkey's window menu does not restore the other
> > windows, whether minimized or just behind window #1. The Warpcentre menu does
> > restore them then the window menu works to change focus between the windows.
> 
> Was this the behavior before the patch?  IIUC, windows that get minimized
> during startup can't be restored by the window menu.  What about windows
> minimized during the current session (via the Min button)?  Does the menu work
> for them?

Both before and after the patch, windows minimized during the current session do not respond to the window menu.
Before the patch the window menu could bring the previously minimized (saved by the session manager and restored behind window #1) to the front, menus do not work on this previously minimized window.
After the patch the window menu does not work (saved by session manager minimized)

> 
> Also, for windows that should be minimized at startup but aren't, do their
> menus work?  If not, then they should be minimized but that isn't happening.

Before patch, previously minimized windows at startup do not have working menus, need to maximize or minimize them before they work.
After patch and using the Warpcentre window list to restore the minimized window, menus do work.
 
> OTOH, if the menus work, then SM has lost track of the fact that they were
> minimized when the last session ended.
Comment on attachment 440944 [details] [diff] [review]
Window Position patch for v1.9.2

>diff --git a/widget/src/os2/nsFrameWindow.cpp b/widget/src/os2/nsFrameWindow.cpp
>--- a/widget/src/os2/nsFrameWindow.cpp
>+++ b/widget/src/os2/nsFrameWindow.cpp
>@@ -382,8 +382,9 @@
>       {
>          PSWP pSwp = (PSWP) mp1;
> 
>-         // Note that client windows never get 'move' messages (well, they won't here anyway)
>-         if( pSwp->fl & SWP_MOVE && !(pSwp->fl & SWP_MINIMIZE))
>+         // Don't save the new position or size of a minimized or
>+         // maximized window, or else it won't be restored correctly.
>+         if (pSwp->fl & SWP_MOVE && !(pSwp->fl & (SWP_MINIMIZE | SWP_MAXIMIZE))) {
>          {
Here's an extra brace
>             // These commented-out `-1's cancel each other out.
>             POINTL ptl = { pSwp->x, pSwp->y + pSwp->cy /* - 1 */ };
>@@ -394,7 +395,7 @@
>          }
> 
>          // When the frame is sized, do stuff to recalculate client size.
>-         if( pSwp->fl & SWP_SIZE && !(pSwp->fl & SWP_MINIMIZE))
>+         if (pSwp->fl & SWP_SIZE && !(pSwp->fl & (SWP_MINIMIZE | SWP_MAXIMIZE))) {
>          {
again, this brace has to be removed
>             mresult = (*fnwpDefFrame)( mFrameWnd, msg, mp1, mp2);
>             bDone = TRUE;

Those braces have also to be removed from the 191-branch patch

However, the main problem of this patch is an immediate crash after maximizing the window (minimized and "normal" windows are ok).
Attachment #440944 - Flags: review?(wuno) → review-
(In reply to comment #0)
> Created an attachment (id=440943) [details]
> Window Position patch for v1.9.3
> 
When I'm being asked before quitting Minefield if I want to save the tabs content the upcoming dialog is moved a bit to the left, when the main window is maximized. In non-maximized windows it is centered. After backing out this patch it's centered again also in maximized windows (as it is in firefox-3.5.9 for example).
(In reply to comment #9)
> (In reply to comment #0)
> > Created an attachment (id=440943) [details]
> > Window Position patch for v1.9.3
> > 
> When I'm being asked before quitting Minefield if I want to save the tabs
> content the upcoming dialog is moved a bit to the left, when the main window is
> maximized. In non-maximized windows it is centered. After backing out this
> patch it's centered again also in maximized windows (as it is in firefox-3.5.9
> for example).

to specify it a bit more, the modal saving dialog seems to remember the last position of a normalized window. When you go from maximized to normal window size and move the normalized window e.g. to the left (such that parts of it are out of the desktop edge), the saving dialog will be centered relative to the normalized window. Then, if you press "cancel" and maximize the window again and close afterwards the saving dialog will have the same position as it was for the normal window (i.e. it does not recognize that the window was maximized in the meantime)
Attachment #510132 - Flags: review?(wuno)
Attachment #440943 - Attachment is obsolete: true
Attachment #440944 - Attachment is obsolete: true
Attachment #440945 - Attachment is obsolete: true
Attachment #510134 - Flags: review?(wuno)
Attachment #440945 - Flags: review?(wuno)
(In reply to comment #10)
> if you [...] maximize the window again and close afterwards the saving
> dialog will have the same position as it was for the normal window
> (i.e. it does not recognize that the window was maximized in the meantime)

The patches for v1.9.x prevented a window's size and position from being updated when it was maximized.  This avoided problems when the app restarted and tried to reopen and reposition the previous session's windows.  The mispositioning of the popup was an unfortunate side-effect of the fix - it was using the window's normal coordinates rather than the maximized values.

In Gecko v2.0, the original bug has been fixed, so we can go back to updating a maximized window's position - this fixes the mispositioned popup.  For v1.9.3, the popup issue is relatively minor compared to the bug being fixed, so I've left that version as-is (the updated patch just adds a "From" and a check-in message).

BTW... I "obsoleted" the patches for v1.9.1 and v1.9.2.
Attachment #510132 - Flags: review?(wuno) → review+
Comment on attachment 510134 [details] [diff] [review]
Window Position patch for v1.9.3 - v2

This one appears to be obsolete 1.9.3 -> 2.0
Attachment #510134 - Flags: review?(wuno)
Keywords: checkin-needed
Whiteboard: attachment 510132, a=NPOTB OS/2 file only
Pushed to m-c, leaving open for 1.9.3:

http://hg.mozilla.org/mozilla-central/rev/4d146de846b3
There is no 1.9.3 branch any more, and the patch in attachment 510132 [details] [diff] [review] doesn't apply cleanly any more.
Keywords: checkin-needed
Whiteboard: attachment 510132, a=NPOTB OS/2 file only
(In reply to comment #16)
> There is no 1.9.3 branch any more, and the patch in attachment 510132 [details] [diff] [review] doesn't
> apply cleanly any more.
Sorry, misunderstanding, there is indeed no 1.9.3 tree anymore and attachment 510134 [details] [diff] [review] would have been the right patch. I couldn't mark it obsolete that's why I added #c14.
I am resolving the bug fixed now, thanks to all helpers.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: