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

Proposed by Sam Spilsbury on 2012-06-19
Status: Merged
Merged at revision: 3258
Proposed branch: lp:~compiz-team/compiz/compiz.fix_1012956
Merge into: lp:compiz/0.9.8
Diff against target: 39 lines (+24/-3)
1 file modified
plugins/decor/src/decor.cpp (+24/-3)
To merge this branch: bzr merge lp:~compiz-team/compiz/compiz.fix_1012956
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-06-19 Approve on 2012-06-22
Tim Penhey 2012-06-19 Pending
Review via email: mp+110954@code.launchpad.net

This proposal supersedes a proposal from 2012-06-18.

Description of the change

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

To post a comment you must log in.
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Please clean up the comment and line wrapping. Your comments are wrapped prematurely while the code next to them is not wrapped at all. I recommend wrapping everything at column 80, but even if you disagree with that then please ensure at least the comments are not wrapped before column 78.

And please remove the irrelevant indentation change from DecorWindow::glDecorate.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

And please remove the irrelevant indentation change from DecorWindow::glDecorate. :)

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

Looks good and seems to work no problem.

review: Approve
Daniel van Vugt (vanvugt) wrote :

I think bug 1012956 is a duplicate of bug 1011120... ?

Daniel van Vugt (vanvugt) wrote :

Ignore that comment. It seems I've already asked that question before.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/decor/src/decor.cpp'
2--- plugins/decor/src/decor.cpp 2012-06-05 09:31:34 +0000
3+++ plugins/decor/src/decor.cpp 2012-06-19 06:09:19 +0000
4@@ -170,11 +170,32 @@
5 {
6 foreach (CompWindow *w, dScreen->cScreen->getWindowPaintList ())
7 {
8- if ((w->type () & CompWindowTypeDockMask) &&
9- !(w->destroyed () || w->invisible ()))
10+ bool isDock = w->type () & CompWindowTypeDockMask;
11+ bool drawShadow = !(w->invisible () || w->destroyed ());
12+
13+ if (isDock && drawShadow)
14 {
15 DecorWindow *d = DecorWindow::get (w);
16- d->glDecorate (transform, attrib, region, mask);
17+
18+ /* If the last mask was an occlusion pass, glPaint
19+ * return value will mean something different, so
20+ * remove it */
21+ unsigned int pmask = d->gWindow->lastMask () &
22+ ~(PAINT_WINDOW_OCCLUSION_DETECTION_MASK);
23+
24+ /* Check if the window would draw by seeing if glPaint
25+ * returns true when using PAINT_NO_CORE_INSTANCE_MASK
26+ */
27+ pmask |= PAINT_WINDOW_NO_CORE_INSTANCE_MASK;
28+
29+ if (d->gWindow->glPaint (d->gWindow->paintAttrib (),
30+ transform,
31+ region,
32+ pmask))
33+ {
34+ GLFragment::Attrib fa (d->gWindow->paintAttrib ());
35+ d->glDecorate (transform, fa, region, mask);
36+ }
37 }
38 }
39 }

Subscribers

People subscribed via source and target branches