Merge lp:~sjakthol/compiz/fix-986051 into lp:compiz/0.9.10

[Sami Jaktholm]

Decor: Use maximized border extents only if window is fully maximized.

The decorator draws a normal border around semi-maximized windows. When
maximized border extents were used for semi-maximized windows, compiz didn't
reserve any space for the border in its geometry calculations.

At least following problems are a result of this behavior:
- Semi-maximized windows have 1px borders drawn on adjacent workspaces
  (LP: #986051).
- Grid placed window overlaps the adjacent viewport (LP: #898870).

Some observations
- this seems fix the issues with gtk-window-decorator (both metacity and cairo mode)
- haven't been able to test kde-window-decorator
- Emerald seems to still be broken with this but that's because Emerald gives compiz invalid frame extents for restored windows.

[Sam Spilsbury (smspillaz)]

Hmm, I though the convention was to use the maximized border extents whenever a window was semi-maximized. I can't remember now what the design was supposed to be though. I think it might have been to use the normal borders.

If that's the case, then you'll need to update the acceptance tests for this. They're found in decor/tests/acceptance/xorg-gtest/compiz_decor_acceptance_tests. There should be a few tests in there that vertically maximize and horizontally maximize windows and expect the window to be placed with respect to its maximized extents. Those expectations need to be replaced with ones that use the normal extents instead.

[Sam Spilsbury (smspillaz)]

All that being said, "not reserving any space for the border in geometry calculations" very much sounds like an issue that could be resolved without changing the extents used for maximized windows. Since quite recently the X and Width co-ordinates for the window border should be preserved through a maximize, so:

restored.x - restoredBorder.left == maximized.x - maximizedBorder.left.

There are even tests to verify that behaviour: PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSameYHeight
PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSameXWidth

All in all, if this were still a problem, I would think it would be that the window was already one-pixel offscreen by its border before being maximized. The grid plugin moves the window before maximizing it, so that could be a cause.

[Sami Jaktholm (sjakthol)]

> Hmm, I though the convention was to use the maximized border extents whenever
> a window was semi-maximized. I can't remember now what the design was supposed
> to be though. I think it might have been to use the normal borders.

I don't know about the design but currently with gtk-window-decorator I can see that:
- normal windows have normal border
- semi-maximized windows have normal border
- maximized windows have the maximized border (which is no border).

> All that being said, "not reserving any space for the border in geometry
> calculations" very much sounds like an issue that could be resolved without
> changing the extents used for maximized windows.

With that I meant that decor plugin sets border extents to (0, 0, 28, 0) while the window really had a border with extents of (1, 1, 28, 1). Geometry calculations would have reserved space for the border had they known there was one.

> Since quite recently the X and Width co-ordinates for the window border
> should be preserved through a maximize, so:
>
> restored.x - restoredBorder.left == maximized.x - maximizedBorder.left.
>
> There are even tests to verify that behaviour:
> PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSameYHeight
> PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSameXWidth

I don't think that applies to this case. In case of horizontally maximized windows, those tests ensure that Y and height don't change. However, the problem here is that when a window is horizontally maximized X and Width are wrong. The X for window content is 0 and Width is screen->width (). Now if there's a border around the contents, the border doesn't fit to the screen.

> All in all, if this were still a problem, I would think it would be that the
> window was already one-pixel offscreen by its border before being maximized.
> The grid plugin moves the window before maximizing it, so that could be a
> cause.

Grid might very be buggy. But this also happens with core semi-maximized windows. If you horizontally semi-maximize a window by right clicking the maximize button, the left border is shown in the viewport on the left and right border in the one on the right. There's no moving involved there. Just a change to the window state and a call to addWindowSizeChanges which takes care of the maximization.

But I'll fix those tests...

[Sami Jaktholm (sjakthol)]

I've investigated the tests and been able to fix the following cases:
- PixmapDecoratedWindowAcceptance.MaximizeBorderExtentsOnMaximize
- PixmapDecoratedWindowAcceptance.MaximizedBorderExtentsOnVertMaximize (renamed to RestoredBorderExtents...)
- PixmapDecoratedWindowAcceptance.MaximizedBorderExtentsOnHorzMaximize (renamed to RestoredBorderExtents...)

I'll push a fix for those in a moment.

However, there's still few tests broken:

- PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
- PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeEqOutputXWidth

I'm not sure whether these above are relevant anymore. As semi-maximized windows have a restored border they will also have a restored frame. So these would basically become tests for the correct frame window size of restored windows.

- PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeSameXWidth
- PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeSameYHeight

For these two, I'm not really sure what's actually going on. It seems that currently in trunk the frame window size stays constant altough the frame extents change on semi-maximize. But when the extents stay constant, the frame window size changes. As I'm not very familiar with decor, window frames or extents I need to take a closer look at the code to put some sense into this.

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

3766. By Sami Jaktholm on 2013-07-19

Merge trunk

3767. By Sami Jaktholm on 2013-07-19

Test fixes

[Sam Spilsbury (smspillaz)]

>
> With that I meant that decor plugin sets border extents to (0, 0, 28, 0) while
> the window really had a border with extents of (1, 1, 28, 1). Geometry
> calculations would have reserved space for the border had they known there was
> one.

Ah okay that makes sense. I think that would have been gtk-window-decorator using the actual restored decoration while sending the maximized extents, so we needed to fix this in decor too. Thanks.

>
> > Since quite recently the X and Width co-ordinates for the window border
> > should be preserved through a maximize, so:
> >
> > restored.x - restoredBorder.left == maximized.x - maximizedBorder.left.
> >
> > There are even tests to verify that behaviour:
> > PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSameYHeight
> > PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSameXWidth
>
> I don't think that applies to this case. In case of horizontally maximized
> windows, those tests ensure that Y and height don't change. However, the
> problem here is that when a window is horizontally maximized X and Width are
> wrong. The X for window content is 0 and Width is screen->width (). Now if
> there's a border around the contents, the border doesn't fit to the screen.
...
> - PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeEqOutputYHeight
> - PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeEqOutputXWidth
>
> I'm not sure whether these above are relevant anymore. As semi-maximized
> windows have a restored border they will also have a restored frame. So these
> would basically become tests for the correct frame window size of restored
> windows.

The border geometry should exactly fit within the maximize area (eg, x == 0, w == screen->width ()) and the window content should fit exactly within the border (eg, x = border.left, w = screen->width () - (border.left + border.right)).

I think the tests check for the frame window geometry, but perhaps they don't check the border geometry?

>
> - PixmapDecoratedWindowAcceptance.VertMaximizeFrameWindowSizeSameXWidth
> - PixmapDecoratedWindowAcceptance.HorzMaximizeFrameWindowSizeSameYHeight
>
> For these two, I'm not really sure what's actually going on. It seems that
> currently in trunk the frame window size stays constant altough the frame
> extents change on semi-maximize. But when the extents stay constant, the frame
> window size changes. As I'm not very familiar with decor, window frames or
> extents I need to take a closer look at the code to put some sense into this.

I should probably clarify:

Frame Geometry == Geometry of the topmost parent window (which includes the invisible resize area, AKA "input area", the border, and the window). It might often be the case that this window would be larger than the output size when maximized.
Border Geometry == Geometry of the client including any decorations. It is never the case that this geometry would be larger than the output size when maximized.
Client Geometry == Geometry of the client. It might often be the case that this window would be smaller than the output size when maximized, but it will never be larger.

The decor plugin is somewhat complicated due to the large numb...

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

3768. By Sami Jaktholm on 2013-07-19

More text fixes.

[Sami Jaktholm (sjakthol)]

The rest of the tests are now fixed and passing. I just hope they are testing what they are supposed to test.

[Sam Spilsbury (smspillaz)]

Yep, that looks fine to me. For semi-maximized windows with restored decorations, they might still have an extended input area, so that needs to be reflected in the width and height.

I'll approve this for now, but it might make the release after 0.9.10.

review: Approve

[MC Return (mc-return)]

Wow, Sami -> you are on fire ! +1
Will test ASAP.

[MC Return (mc-return)]

Sami, did you test Emerald with disabled Emerald setting "Use Decoration Cropping" ?

review: Needs Information

[Sami Jaktholm (sjakthol)]

I only tested emerald with default settings, and it didn't work with or without this branch.

[Sam Spilsbury (smspillaz)]

Emerald is not supported we should not block branches if it is giving incorrect extents information in its protocol usage.

review: Approve

[MC Return (mc-return)]

> Emerald is not supported we should not block branches if it is giving
> incorrect extents information in its protocol usage.

I did not want to block anything. It was just a question.
I really do not understand the hate regarding Emerald and I do not understand why the most advanced of the decorators is the only one we choose to not support...
It was always working perfectly until the global menu came along...

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2013-07-15 04:21:45 +0000
+++ plugins/decor/src/decor.cpp	2013-07-19 19:21:27 +0000
@@ -1608,7 +1608,7 @@
 	/* Set extents based on maximize/unmaximize state
 	 * FIXME: With the new type system, this should be
 	 * removed */
-	if (decorMaximizeState)
+	if (decorMaximizeState == MAXIMIZE_STATE)
 	    window->setWindowFrameExtents (&decoration->maxBorder,
 					   &decoration->maxInput);
 	else if (!window->hasUnmapReference ())
@@ -1779,7 +1779,7 @@
 	parent = window->frame ();
 
     /* Determine frame extents */
-    if ((window->state () & MAXIMIZE_STATE))
+    if ((window->state () & MAXIMIZE_STATE) == MAXIMIZE_STATE)
     {
 	border = wd->decor->maxBorder;
 	input = wd->decor->maxInput;
@@ -1935,7 +1935,7 @@
     CompWindowExtents	 input;
 
     /* Determine frame extents */
-    if ((window->state () & MAXIMIZE_STATE))
+    if ((window->state () & MAXIMIZE_STATE) == MAXIMIZE_STATE)
 	input = wd->decor->maxInput;
     else
 	input = wd->decor->input;

=== modified file 'plugins/decor/tests/acceptance/xorg-gtest/compiz_decor_acceptance_tests.cpp'
--- plugins/decor/tests/acceptance/xorg-gtest/compiz_decor_acceptance_tests.cpp	2013-07-15 04:21:45 +0000
+++ plugins/decor/tests/acceptance/xorg-gtest/compiz_decor_acceptance_tests.cpp	2013-07-19 19:21:27 +0000
@@ -2159,6 +2159,8 @@
 			 AddState,
 			 "_NET_WM_STATE_MAXIMIZED_VERT",
 			 mTestWindow);
+			 
+    WaitForPropertyNotify (Display (), mTestWindow, "_NET_FRAME_EXTENTS");
 
     ChangeStateOfWindow (Display (),
 			 AddState,
@@ -2180,7 +2182,7 @@
     EXPECT_THAT (frameExtents, IsExtents (MaxEx, MaxEx, MaxEx, MaxEx));
 }
 
-TEST_F (PixmapDecoratedWindowAcceptance, MaximizeBorderExtentsOnVertMaximize)
+TEST_F (PixmapDecoratedWindowAcceptance, RestoredBorderExtentsOnVertMaximize)
 {
     ChangeStateOfWindow (Display (),
 			 AddState,
@@ -2197,12 +2199,12 @@
     unsigned long *frameExtents =
 	reinterpret_cast <unsigned long *> (data.get ());
 
-    unsigned int MaxEx = MaximizedBorderExtent;
+    unsigned int ActEx = RealDecorationActiveBorderExtent;
 
-    EXPECT_THAT (frameExtents, IsExtents (MaxEx, MaxEx, MaxEx, MaxEx));
+    EXPECT_THAT (frameExtents, IsExtents (ActEx, ActEx, ActEx, ActEx));
 }
 
-TEST_F (PixmapDecoratedWindowAcceptance, MaximizeBorderExtentsOnHorzMaximize)
+TEST_F (PixmapDecoratedWindowAcceptance, RestoredBorderExtentsOnHorzMaximize)
 {
     ChangeStateOfWindow (Display (),
 			 AddState,
@@ -2219,9 +2221,9 @@
     unsigned long *frameExtents =
 	reinterpret_cast <unsigned long *> (data.get ());
 
-    unsigned int MaxEx = MaximizedBorderExtent;
-
-    EXPECT_THAT (frameExtents, IsExtents (MaxEx, MaxEx, MaxEx, MaxEx));
+    unsigned int ActEx = RealDecorationActiveBorderExtent;
+    
+    EXPECT_THAT (frameExtents, IsExtents (ActEx, ActEx, ActEx, ActEx));
 }
 
 TEST_F (PixmapDecoratedWindowAcceptance, MaximizeFrameWindowSizeEqOutputSize)
@@ -2258,7 +2260,7 @@
 								  matcher)));
 }
 
-TEST_F (PixmapDecoratedWindowAcceptance, VertMaximizeFrameWindowSizeEqOutputYHeight)
+TEST_F (PixmapDecoratedWindowAcceptance, VertMaximizeFrameWindowYHeight)
 {
     XWindowAttributes attrib;
     XGetWindowAttributes (Display (), DefaultRootWindow (Display ()), &attrib);
@@ -2268,12 +2270,13 @@
 			 "_NET_WM_STATE_MAXIMIZED_VERT",
 			 mTestWindow);
 
+    int offset = RealDecorationActiveBorderExtent - ActiveInputExtent;
     ct::ConfigureNotifyXEventMatcher matcher (None,
 					      0,
 					      0,
-					      0,
-					      0,
-					      attrib.height,
+					      offset,
+					      0,
+					      attrib.height - 2 * offset,
 					      CWY | CWHeight);
 
     EXPECT_TRUE (Advance (Display (),
@@ -2285,7 +2288,7 @@
 								  matcher)));
 }
 
-TEST_F (PixmapDecoratedWindowAcceptance, HorzMaximizeFrameWindowSizeEqOutputXWidth)
+TEST_F (PixmapDecoratedWindowAcceptance, HorzMaximizeFrameWindowXWidth)
 {
     XWindowAttributes attrib;
     XGetWindowAttributes (Display (), DefaultRootWindow (Display ()), &attrib);
@@ -2295,11 +2298,12 @@
 			 "_NET_WM_STATE_MAXIMIZED_HORZ",
 			 mTestWindow);
 
+    int offset = RealDecorationActiveBorderExtent - ActiveInputExtent;
     ct::ConfigureNotifyXEventMatcher matcher (None,
 					      0,
-					      0,
-					      0,
-					      attrib.width,
+					      offset,
+					      0,
+					      attrib.width - 2 * offset,
 					      0,
 					      CWX | CWWidth);
 
@@ -2312,34 +2316,11 @@
 								  matcher)));
 }
 
-namespace
-{
-void WindowBorderPositionAttributes (Display *dpy,
-				     Window  w,
-				     XWindowAttributes &attrib,
-				     unsigned int ActiveBorderExtent,
-				     unsigned int ActiveInputExtent)
-{
-    XGetWindowAttributes (dpy, w, &attrib);
-
-    /* Remove input - border offset */
-    attrib.x += (ActiveInputExtent - ActiveBorderExtent);
-    attrib.y += (ActiveInputExtent - ActiveBorderExtent);
-    attrib.width -= (ActiveInputExtent - ActiveBorderExtent) * 2;
-    attrib.height -= (ActiveInputExtent - ActiveBorderExtent) * 2;
-}
-}
-
 TEST_F (PixmapDecoratedWindowAcceptance, VertMaximizeFrameWindowSizeSameXWidth)
 {
     XWindowAttributes rootAttrib, attrib;
     XGetWindowAttributes (Display (), DefaultRootWindow (Display ()), &rootAttrib);
-
-    WindowBorderPositionAttributes (Display (),
-				    mTestWindowParent,
-				    attrib,
-				    RealDecorationActiveBorderExtent,
-				    ActiveInputExtent);
+    XGetWindowAttributes (Display (), mTestWindowParent, &attrib);
 
     ChangeStateOfWindow (Display (),
 			 AddState,
@@ -2347,12 +2328,13 @@
 			 mTestWindow);
 
     /* Wait for the window to be maximized first */
+    int offset = RealDecorationActiveBorderExtent - ActiveInputExtent;
     WaitForConfigureOn (Display (),
 			mTestWindowParent,
 			0,
-			0,
-			0,
-			rootAttrib.height,
+			offset,
+			0,
+			rootAttrib.height - 2 * offset,
 			CWY | CWHeight);
 
     /* Query the window geometry and ensure that the width and
@@ -2372,12 +2354,7 @@
 {
     XWindowAttributes rootAttrib, attrib;
     XGetWindowAttributes (Display (), DefaultRootWindow (Display ()), &rootAttrib);
-
-    WindowBorderPositionAttributes (Display (),
-				    mTestWindowParent,
-				    attrib,
-				    RealDecorationActiveBorderExtent,
-				    ActiveInputExtent);
+    XGetWindowAttributes (Display (), mTestWindowParent, &attrib);
 
     ChangeStateOfWindow (Display (),
 			 AddState,
@@ -2385,11 +2362,12 @@
 			 mTestWindow);
 
     /* Wait for the window to be maximized first */
+    int offset = RealDecorationActiveBorderExtent - ActiveInputExtent;
     WaitForConfigureOn (Display (),
 			mTestWindowParent,
-			0,
-			0,
-			rootAttrib.width,
+			offset,
+			0,
+			rootAttrib.width - 2 * offset,
 			0,
 			CWX | CWWidth);