Merge lp:~vanvugt/compiz/fix-1089811 into lp:compiz/0.9.9

[Daniel van Vugt]

Ensure that when a window is fake-minimized by Unity that we don't still
reserve its input region, or else it will behave like a dead zone on the
screen. (LP: #1089811)

Much better than reverting r3513 which triggered the bug (but is not the root cause).

[Daniel van Vugt (vanvugt)]

And yes, we still want priv->region non-empty or else the window becomes transparent during the minimize animation.

[Andrea Azzarone (azzar1)]

Works here.

review: Approve

[Sam Spilsbury (smspillaz)]

Hmm, this irks me somewhat. It will have some interesting side effects, like if the input shape region changes while the window is minimized. It also represents some behavioural coupling that I'd like to avoid.

The one thing I can think of is that I believe the nBounding < 1 and nInput < 1 checks were only put in place in the case of shape support being missing (hence, region and inputRegion would be empty). As such, those checks should really be removed and the assignments should only happen if we actually queried the shape:

So:

bool queriedShape = false;

if (screen->XShape ())
{
    ... Query Shape

    queriedShape = true;
}

r.x = -geom.border ();
...

if (!queriedShape)
{
    boundingShaperects = &r;
    nBounding = 1;
    inputShapeRects = &r;
    nInput = 1;
}

priv->region += ....

if (!queriedShape)
{
    if (!boundingShapeRects)
        XFree ...
}

review: Needs Information

[Sam Spilsbury (smspillaz)]

Yeah, just had a chat with duflu on IRC, lets not merge this until we've discussed it further.

[Daniel van Vugt (vanvugt)]

Yeah something like that sounds even more correct. I'll do it tomorrow if no one else does.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3529
http://jenkins.qa.ubuntu.com/job/compiz-ci/325/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/325/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/325//rebuild/?

review: Approve (continuous-integration)

[Michael Terry (mterry)]

Sounds like this could use a test too, especially since it already regressed once.

[Sam Spilsbury (smspillaz)]

On Thu, Dec 13, 2012 at 10:36 PM, Michael Terry
<email address hidden> wrote:
> Sounds like this could use a test too, especially since it already regressed once.

Well, a test for this wouldn't be all that useful. The problem was
with integration all the way from unity to compiz, getting that under
test would just be insane.

Testing it by itself would just test that the code does what it does.

> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1089811/+merge/139652
> You are reviewing the proposed merge of lp:~vanvugt/compiz/fix-1089811 into lp:compiz.

--
Sam Spilsbury

[Michael Terry (mterry)]

We have integration tests, right? Maybe a unity autopilot test for this case?

[Sam Spilsbury (smspillaz)]

On Thu, Dec 13, 2012 at 10:51 PM, Michael Terry
<email address hidden> wrote:
> We have integration tests, right? Maybe a unity autopilot test for this case?

AP might be appropriate.

> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1089811/+merge/139652
> You are reviewing the proposed merge of lp:~vanvugt/compiz/fix-1089811 into lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

Reviewing Sam's concerns again:
  1. "if the input shape region changes while the window is minimized". Yes this will be a potential (albeit unlikely and rare) problem. And again only a problem when using Unity :(
  2. Code clean up. As usual I would like to avoid cleaning up code as part of a bug fix. They should be separate activities if possible.

OK, well I don't have a good answer to #1 right now. Only two simple solutions come to mind:
  (a) Approve andyrock's revert of r3513. It will cause a regression of LP: #1087193 but that's a minor issue compared to bug 1089811.
  (b) Fix Unity, by deleting the offending code: UnityWindow::[un]minimize[d]()

Until we can do (b), I think I will chose (a).

review: Disapprove

[Sam Spilsbury (smspillaz)]

On Fri, Dec 14, 2012 at 2:08 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Disapprove
>
> Reviewing Sam's concerns again:
> 1. "if the input shape region changes while the window is minimized". Yes this will be a potential (albeit unlikely and rare) problem. And again only a problem when using Unity :(
> 2. Code clean up. As usual I would like to avoid cleaning up code as part of a bug fix. They should be separate activities if possible.
>
> OK, well I don't have a good answer to #1 right now. Only two simple solutions come to mind:
> (a) Approve andyrock's revert of r3513. It will cause a regression of LP: #1087193 but that's a minor issue compared to bug 1089811.
> (b) Fix Unity, by deleting the offending code: UnityWindow::[un]minimize[d]()

OK, if you want to lose minimized previews in the switcher...

What's wrong with changing the check for (if nBounding < 1) ... if (nInput < 1)

>
> Until we can do (b), I think I will chose (a).
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1089811/+merge/139652
> You are reviewing the proposed merge of lp:~vanvugt/compiz/fix-1089811 into lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

Abstain. I might have missed something and will check Sam's approach again.

review: Abstain

[Daniel van Vugt (vanvugt)]

Fixed differently this time.

Talking to Sam, he confirms that Unity sets both regions to empty if a window is fake minimized. However in this case we need to fall back to a simple region for the bounding shape to ensure the minimize animation is visible. Conversely we need to avoid falling back to any input shape so as to avoid the dead zone bug 1089811.

[Sam Spilsbury (smspillaz)]

Yep, for now.

I've got a branch that removes settings ShapeBounding in unity too, so then we can do this properly later.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3530
http://jenkins.qa.ubuntu.com/job/compiz-ci/327/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/327/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/327//rebuild/?

review: Approve (continuous-integration)

=== modified file 'src/window.cpp'
--- src/window.cpp	2012-12-13 08:39:22 +0000
+++ src/window.cpp	2012-12-14 07:56:30 +0000
@@ -905,7 +905,8 @@
 
     priv->region = priv->inputRegion = emptyRegion;
 
-    if (screen->XShape ())
+    bool useXShape = screen->XShape ();
+    if (useXShape)
     {
 	int order;
 
@@ -929,7 +930,7 @@
 	nBounding = 1;
     }
 
-    if (nInput < 1)
+    if (!useXShape)
     {
 	inputShapeRects = &r;
 	nInput = 1;