Merge lp:~compiz-team/compiz/compiz.fix_1012956 into lp:compiz/0.9.8

[Sam Spilsbury]

Check if the window would actually paint before painting the shadow, since
it is possible that another plugin could be inhibiting paint of the dock window.

Fixes (LP: #1012956)

Check if the window would actually paint before painting the shadow, since
it is possible that another plugin could be inhibiting paint of the dock window.

Fixes (LP: #1012956)

Unblocks: https://code.launchpad.net/~smspillaz/unity/unity.less-paint-insanity/+merge/110267

[Tim Penhey (thumper)]

I'm going to be pedantic here...

> bool isDock = w->type () & CompWindowTypeDockMask;

This looks fine.

However...

> bool invisible = w->invisible ();
> invisible |= w->destroyed ();
> if (isDock && !invisible)

Seems a bit more weird. Primarily because the local invisible variable reflects the window invisible state, which is fine, but then it becomes "invisible or destroyed", which isn't what the variable reflects.

As an aside, this value is then negated in the if statement.

How about...

  bool drawShadow = !w->invisible ();
  if (w->destroyed ())
    drawShadow = false;

  if (isDock && drawShadow)
  // ...

Having booleans expressed in the positive are simpler to follow.

  bool drawShadow = !(w->invisible () || w-> destroyed ());

Also seems simple enough to follow.

I can't really comment on the internals of the method though.
How expensive is the glPaint call?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Also, I think it's technically wrong to be using an arithmetic OR on a bool:
  invisible |= w->destroyed ();

I'm not sure if that's a new C++ feature, but it shouldn't work normally.

It should probably be:
  bool invisible = w->invisible () || w->destroyed();

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Ack. glPaint is not expensive since plugins will not have their hooks called unless they are running animations on the window and PAINT_WINDOW_NO_CORE_INSTANCE_MASK means that no gl operations occurr

Sent from Samsung Mobile

 Tim Penhey <email address hidden> wrote:

Review: Needs Fixing

I'm going to be pedantic here...

> bool isDock = w->type () & CompWindowTypeDockMask;

This looks fine.

However...

> bool invisible = w->invisible ();
> invisible |= w->destroyed ();
> if (isDock && !invisible)

Seems a bit more weird.  Primarily because the local invisible variable reflects the window invisible state, which is fine, but then it becomes "invisible or destroyed", which isn't what the variable reflects.

As an aside, this value is then negated in the if statement.

How about...

  bool drawShadow = !w->invisible ();
  if (w->destroyed ())
    drawShadow = false;

  if (isDock && drawShadow)
  // ...

Having booleans expressed in the positive are simpler to follow.

  bool drawShadow = !(w->invisible () || w-> destroyed ());

Also seems simple enough to follow.

I can't really comment on the internals of the method though.
How expensive is the glPaint call?
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1012956/+merge/110268
Your team Compiz Maintainers is subscribed to branch lp:compiz.

[Sam Spilsbury (smspillaz)]

Binary or has always worked on booleans.

Proof:

False = 0X0
True = 0X1

a = 0
a |= 1 == 1
a |= 0 == 1

In any case im happy to convert it.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Review: Needs Fixing

Also, I think it's technically wrong to be using an arithmetic OR on a bool:
  invisible |= w->destroyed ();

I'm not sure if that's a new C++ feature, but it shouldn't work normally.

It should probably be:
  bool invisible = w->invisible () || w->destroyed();
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1012956/+merge/110268
Your team Compiz Maintainers is subscribed to branch lp:compiz.

[lp:~compiz-team/compiz/compiz.fix_1012956]

3247. By Sam Spilsbury on 2012-06-18

Express booleans more clearly

3248. By Sam Spilsbury on 2012-06-18

Style

3249. By Sam Spilsbury on 2012-06-19

Indent

Unmerged revisions

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-06-05 09:31:34 +0000
+++ plugins/decor/src/decor.cpp	2012-06-14 09:15:22 +0000
@@ -170,11 +170,30 @@
 	{
 	    foreach (CompWindow *w, dScreen->cScreen->getWindowPaintList ())
 	    {
-		if ((w->type () & CompWindowTypeDockMask) &&
-		    !(w->destroyed () || w->invisible ()))
+		bool isDock = w->type () & CompWindowTypeDockMask;
+		bool invisible = w->invisible ();
+
+		invisible |= w->destroyed ();
+
+		if (isDock && !invisible)
 		{
 		    DecorWindow *d = DecorWindow::get (w);
-		    d->glDecorate (transform, attrib, region, mask);
+
+		    /* Check if the window would draw by
+		     * seeing if glPaint returns true when
+		     * using PAINT_NO_CORE_INSTANCE_MASK
+		     */
+
+		    unsigned int pmask = d->gWindow->lastMask () & ~(PAINT_WINDOW_OCCLUSION_DETECTION_MASK);
+
+		    if (d->gWindow->glPaint (d->gWindow->paintAttrib (),
+					     transform,
+					     region,
+					     pmask | PAINT_WINDOW_NO_CORE_INSTANCE_MASK))
+		    {
+			GLFragment::Attrib fa (d->gWindow->paintAttrib ());
+			d->glDecorate (transform, fa, region, mask);
+		    }
 		}
 	    }
 	}
@@ -185,7 +204,7 @@
 
 void
 DecorWindow::glDecorate (const GLMatrix     &transform,
-		         GLFragment::Attrib &attrib,
+			 GLFragment::Attrib &attrib,
 		         const CompRegion   &region,
 		         unsigned int       mask)
 {