Merge lp:~sjakthol/compiz/fix-878516 into lp:compiz/0.9.9

[Sami Jaktholm]

Grid: Correctly restore semi-maximized windows.

Changes include:
1) Remove the special status of fully maximized windows - it just adds
   complexity to the restoration process. Core can handle the
   restoration of maximized windows perfectly well.
2) Catch un-maximation of a gridded semi-maximized window in
   stateChangeNotify and call restoreWindow to revert the changes.
3) Refactor restoreWindow to work with semi-maximized windows.

This fixes LP: #878516 and also LP: #925867 as a side effect.

NOTE: These fixes cause bug 1102024 to also manifest itself with semi-maximized terminal windows. Previously isGridMaximized wasn't unset once a semi-maximized window was restored as the restoration code was never called. Consequently validateResizeRequest blocked the size from being changed by the client. With this branch gnome-terminal fails to restore its geometry but other applications get their old geometries back.

I'm not sure whether it's worth fixing an annoying bug 878516 affecting every application by introducing another annoying bug that only affects one application. But anyways I'm putting my work up here if someone wants to take a shot at fixing the regression because I'm out of ideas...

[MC Return (mc-return)]

Top job !
Works like expected and announced.
Makes those nasty mouse-resized grid windows much more usable.
Strange thing with the terminal windows though, but still a huge improvement of grid.

review: Approve

[Sam Spilsbury (smspillaz)]

Most of this makes a lot of sense, I only have a few comments which might
be worth considering (diff inline).

sampo555 has proposed merging lp:~sampo555/compiz/fix-878516 into lp:compiz.

Commit message:
Grid: Correctly restore semi-maximized windows.

Changes include:
1) Remove the special status of fully maximized windows - it just adds
   complexity to the restoration process. Core can handle the
   restoration of maximized windows perfectly well.
2) Catch un-maximation of a gridded semi-maximized window in
   stateChangeNotify and call restoreWindow to revert the changes.
3) Refactor restoreWindow to work with semi-maximized windows
=== modified file 'plugins/grid/src/grid.cpp'
--- plugins/grid/src/grid.cpp 2013-01-18 18:10:07 +0000
+++ plugins/grid/src/grid.cpp 2013-01-20 12:47:23 +0000
@@ -198,8 +198,8 @@
                cw->configureXWindow (CWX | CWY, &xwc);
            }
            cw->maximize (MAXIMIZE_STATE);
- gw->isGridResized = true;
- gw->isGridMaximized = true;
+ gw->isGridResized = false;
+ gw->isGridMaximized = false;

Once I had read the whole diff, this change made sense, though it was a bit
confusing in isolation, especially because we maximize the window and then
set isGridMaximized to false.

Some things that might be worth doing about this:
1) Rename isGridMaximed to isGridSemiMaximized
2) Stick a comment there about how we're letting core handle the
fully-maximized case.

                for (unsigned int i = 0; i < animations.size (); i++)
                        animations.at (i).fadingOut = true;
            return true;
@@ -926,7 +926,11 @@
 {
     if (lastState & MAXIMIZE_STATE &&
        !(window->state () & MAXIMIZE_STATE))
+ {
        lastTarget = GridUnknown;
+ if (isGridMaximized)
+ gScreen->restoreWindow(0, 0, gScreen->o);
+ }

I don't think this will be a problem, although I think it should be clear
that the condition should only ever be entered if the window went from
being semi-maximized to not-maximized at all (and guarded against further
change which would enter this condition if the window went from
semi-maximized to maximized)

     else if (!(lastState & MAXIMIZE_STATE) &&
             window->state () & MAXIMIZE_STATE)
     {
@@ -952,6 +956,7 @@
                           CompOption::Vector &option)
 {
     XWindowChanges xwc;
+ int xwcm = 0;
     CompWindow *cw = screen->findWindow (screen->activeWindow ());

     if (!cw)
@@ -959,35 +964,59 @@

     GRID_WINDOW (cw);

- if (!gw->isGridResized)
- return false;
-
- if (gw->isGridMaximized & !(cw->state () & MAXIMIZE_STATE))
- {
- gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
- gw->isGridMaximized = false;
- }
- else
- {
- if (cw == mGrabWindow)
- {
- xwc.x = pointerX - (gw->originalSize.width () >> 1);
- xwc.y = pointerY + (cw->border ().top >> 1);
- }
- else
- {
- xwc.x = gw->originalSize.x ();
- xwc.y = gw->originalSize.y ();
- }
- xwc.width = gw->originalSize.width ();
- xwc.height = gw->originalSize.height ();
- cw->maximize (0);
- ...

[lp:~sjakthol/compiz/fix-878516]

3571. By Sami Jaktholm on 2013-01-21

isGridMaximized changed to isGridSemiMaximized and comment added about core handling fully maximized windows. Added assert to the else clause in restoreWindow, changed shifts to divide by 2 and added check for xwcm before calling sendSyncRequest. Also added an extra check so that we only call restoreWindow from stateChangeNotify if lastState was CompWindowStateMaximizedVertMask (e.g. not a fully maximized window incorrectly marked as isGridSemiMaximized).

[Sami Jaktholm (sjakthol)]

Changed the things you pointed out. About the extra check in stateChangeNotify, I hope you meant to check that we call restoreWindow only if the window was only vertically maximized. The other case (semi-maximized->maximized) should'n be possible as the previous if checks that the window is not maximized at all.

[Sam Spilsbury (smspillaz)]

Ah okay.

I'm that case you want to check for CompWindowStateMaximizeVertMask.
On 21/01/2013 11:27 PM, "sampo555" <email address hidden> wrote:

> Changed the things you pointed out. About the extra check in
> stateChangeNotify, I hope you meant to check that we call restoreWindow
> only if the window was only vertically maximized. The other case
> (semi-maximized->maximized) should'n be possible as the previous if checks
> that the window is not maximized at all.
> --
> https://code.launchpad.net/~sampo555/compiz/fix-878516/+merge/144020
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~sampo555/compiz/fix-878516 into lp:compiz.
>

[MC Return (mc-return)]

Just a note:
The latest version fails to restore horizontal size of left/right mousegridded == semi-maximized windows...
Hope this gets corrected again as the first version was doing that correctly ?

[Daniel van Vugt (vanvugt)]

Testing both bugs, neither appear fixed. I can still reproduce both with this branch.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

All -

Please remember to test proposals manually and not just read the code.

[lp:~sjakthol/compiz/fix-878516]

3572. By Sami Jaktholm on 2013-01-22

Filter non-maximation related states from lastState before comparing it to VertMask.

[Sami Jaktholm (sjakthol)]

Of course I forgot that lastState in stateChangeNotify contains everything about window state - not just the bits related to maximization. This should now be fixed.

[MC Return (mc-return)]

Great !!! \o/

With r3572 in place everything seems to work as announced.
I cannot even reproduce the striking gnome-terminal issue anymore :)
This is another very important Grid fix and a important step in the right direction.
Well done, sampo555 !

review: Approve

[Daniel van Vugt (vanvugt)]

Nice. At first the bugs appear completely fixed, which is excellent. But then I noticed a slight regression similar to bug 878516 but with the height. If I repeatedly place a small terminal window to the side with grid and unsnap it, it will eventually stop restoring the original window height and will stay tall. This does not happen to the height with trunk (but it does happen with the width on trunk, obviously).

Also minor cleanup: In C++, <assert.h> should be <cassert>

And concerning hack:
    gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
I was really hoping we could remove the sizeHints hacking. It's devious and evil. But I guess if you don't need to change it to resolve the bugs here then it's a job for another day.

review: Needs Fixing

[lp:~sjakthol/compiz/fix-878516]

3573. By Sami Jaktholm on 2013-01-23

Change assert.h to cassert

[Sami Jaktholm (sjakthol)]

About the terminal height problem, that's what I was trying to say in the merge message. It has really nothing to do with grid as it also happens with the maximize/unmaximize keybindings. Prior to this branch terminals would keep their heights only because grid was not clearing isGridMaximized flags when the windows were restored (as the restoration code was not called at all). And thus validateResizeRequest was blocking these changes to the terminal windows. I was hoping someone could take a look at it (bug 1102024) before this goes in...

The sizeHints are there to resolve a bug where an empty space is left around left and right gridded terminal windows. To fix that without sizeHints modification core would have to allow plugins to force the size of the window without taking sizeHints into consideration. But that's a job for another day...

[Daniel van Vugt (vanvugt)]

Ok.

Yeah I know removing sizeHints would create space. And I would argue that's more correct, technically. But it's too contentious to argue in this little text box right now.

review: Approve

=== modified file 'plugins/grid/src/grid.cpp'
--- plugins/grid/src/grid.cpp	2013-01-18 18:10:07 +0000
+++ plugins/grid/src/grid.cpp	2013-01-23 06:20:25 +0000
@@ -24,6 +24,7 @@
 
 #include <boost/bind.hpp>
 #include <cmath>
+#include <cassert>
 #include "grid.h"
 #include "grabhandler.h"
 
@@ -198,8 +199,12 @@
 		cw->configureXWindow (CWX | CWY, &xwc);
 	    }
 	    cw->maximize (MAXIMIZE_STATE);
-	    gw->isGridResized = true;
-	    gw->isGridMaximized = true;
+	    /* Core can handle fully maximized windows so we don't
+	     * have to worry about them. Don't mark the window as a
+	     * gridded one.
+	     */
+	    gw->isGridResized = false;
+	    gw->isGridSemiMaximized = false;
 		for (unsigned int i = 0; i < animations.size (); i++)
 			animations.at (i).fadingOut = true;
 	    return true;
@@ -368,7 +373,7 @@
 
 		cw->configureXWindow (CWX | CWY | CWWidth | CWHeight, &rwc);
 
-		gw->isGridMaximized = true;
+		gw->isGridSemiMaximized = true;
 		gw->isGridResized = false;
 
 		/* Maximize the window */
@@ -384,7 +389,7 @@
 	    else
 	    {
 	        gw->isGridResized = true;
-	        gw->isGridMaximized = false;
+	        gw->isGridSemiMaximized = false;
 	    }
 
 	    int dw = (lastBorder.left + lastBorder.right) - 
@@ -839,7 +844,7 @@
     /* Don't allow non-pagers to change
      * the size of the window, the user
      * specified this size, thank-you */
-    if (isGridMaximized)
+    if (isGridSemiMaximized)
 	if (source != ClientTypePager)
 	    xwcm = 0;
 }
@@ -863,7 +868,7 @@
 	pointerBufDx = pointerBufDy = 0;
 	grabMask = mask;
 
-	if (!isGridResized && !isGridMaximized && gScreen->optionGetSnapbackWindows ())
+	if (!isGridResized && !isGridSemiMaximized && gScreen->optionGetSnapbackWindows ())
 	    /* Store size not including borders when grabbing with cursor */
 	    originalSize = gScreen->slotToRect(window,
 						    window->serverBorderRect ());
@@ -904,7 +909,7 @@
 {
     window->moveNotify (dx, dy, immediate);
 
-    if (isGridResized && !isGridMaximized && !GridScreen::get (screen)->mSwitchingVp)
+    if (isGridResized && !isGridSemiMaximized && !GridScreen::get (screen)->mSwitchingVp)
     {
 	if (window->grabbed () && (grabMask & CompWindowGrabMoveMask))
 	{
@@ -926,7 +931,11 @@
 {
     if (lastState & MAXIMIZE_STATE &&
 	!(window->state () & MAXIMIZE_STATE))
+    {
 	lastTarget = GridUnknown;
+	if (isGridSemiMaximized && (lastState & MAXIMIZE_STATE) == CompWindowStateMaximizedVertMask)
+	    gScreen->restoreWindow(0, 0, gScreen->o);
+    }
     else if (!(lastState & MAXIMIZE_STATE) &&
 	     window->state () & MAXIMIZE_STATE)
     {
@@ -952,6 +961,7 @@
 			   CompOption::Vector &option)
 {
     XWindowChanges xwc;
+    int xwcm = 0;
     CompWindow *cw = screen->findWindow (screen->activeWindow ());
 
     if (!cw)
@@ -959,35 +969,59 @@
 
     GRID_WINDOW (cw);
 
-    if (!gw->isGridResized)
+    if (!gw->isGridResized && !gw->isGridSemiMaximized)
+    {
+	/* Grid hasn't touched this window or has maximized it. If it's
+	 * maximized, unmaximize it and get out. */
+	if (cw->state () & MAXIMIZE_STATE)
+	    cw->maximize(0);
+	return true;
+    }
+
+    else if (!gw->isGridResized && gw->isGridSemiMaximized)
+    {
+	/* Window has been vertically maximized by grid. We only need
+	 * to restore the X and width - core handles Y and height. */
+	if (gw->sizeHintsFlags)
+	    gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
+	xwcm |= CWX | CWWidth;
+    }
+
+    else if (gw->isGridResized && !gw->isGridSemiMaximized)
+	/* Window is just gridded (top, bottom, center, corners). We
+	 * need to handle everything. */
+	xwcm |= CWX | CWY | CWWidth | CWHeight;
+    else
+    {
+	/* This should never happen. But if it does, just bail out
+	 * gracefully. */
+	assert (gw->isGridResized && gw->isGridSemiMaximized);
 	return false;
+    }
 
-    if (gw->isGridMaximized & !(cw->state () & MAXIMIZE_STATE))
+    if (cw == mGrabWindow)
     {
-	gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
-	gw->isGridMaximized = false;
+	xwc.x = pointerX - (gw->originalSize.width () / 2);
+	xwc.y = pointerY + (cw->border ().top / 2);
     }
     else
     {
-        if (cw == mGrabWindow)
-	{
-	    xwc.x = pointerX - (gw->originalSize.width () >> 1);
-	    xwc.y = pointerY + (cw->border ().top >> 1);
-	}
-	else
-	{
-	    xwc.x = gw->originalSize.x ();
-	    xwc.y = gw->originalSize.y ();
-	}
-	xwc.width  = gw->originalSize.width ();
-	xwc.height = gw->originalSize.height ();
-	cw->maximize (0);
-	gw->currentSize = CompRect ();
-	cw->configureXWindow (CWX | CWY | CWWidth | CWHeight, &xwc);
-	gw->pointerBufDx = 0;
-	gw->pointerBufDy = 0;
+	xwc.x = gw->originalSize.x ();
+	xwc.y = gw->originalSize.y ();
     }
+    xwc.width  = gw->originalSize.width ();
+    xwc.height = gw->originalSize.height ();
+
+    if (cw->mapNum() && xwcm)
+	cw->sendSyncRequest();
+    cw->configureXWindow (xwcm, &xwc);
+    gw->currentSize = CompRect ();
+    gw->pointerBufDx = 0;
+    gw->pointerBufDy = 0;
+    gw->isGridSemiMaximized = false;
     gw->isGridResized = false;
+    if (cw->state () & MAXIMIZE_STATE)
+	cw->maximize(0);
     gw->resizeCount = 0;
     gw->lastTarget = GridUnknown;
 
@@ -1001,7 +1035,7 @@
     GRID_WINDOW (screen->findWindow
 		    (CompOption::getIntOptionNamed (o, "window")));
     gw->isGridResized = false;
-    gw->isGridMaximized = false;
+    gw->isGridSemiMaximized = false;
     gw->resizeCount = 0;
 }
 
@@ -1164,7 +1198,7 @@
     gWindow (GLWindow::get(window)),
     gScreen (GridScreen::get (screen)),
     isGridResized (false),
-    isGridMaximized (false),
+    isGridSemiMaximized (false),
     grabMask (0),
     pointerBufDx (0),
     pointerBufDy (0),

=== modified file 'plugins/grid/src/grid.h'
--- plugins/grid/src/grid.h	2013-01-18 18:10:07 +0000
+++ plugins/grid/src/grid.h	2013-01-23 06:20:25 +0000
@@ -182,7 +182,7 @@
 	GridScreen *gScreen;
 
 	bool isGridResized;
-	bool isGridMaximized;
+	bool isGridSemiMaximized;
 	unsigned int grabMask;
 	int pointerBufDx;
 	int pointerBufDy;