Code review comment for lp:~compiz-team/compiz/compiz.fix_1012956

Revision history for this message
Tim Penhey (thumper) wrote :

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

« Back to merge proposal