Code review comment for lp:~vanvugt/unity/regionalDamage

> I am keeping shellIsHidden because it is fast and simple. It doesn't re-enter
> compiz and doesn't require any CompRegion logic. It just uses rectangles.

My gripe with shellIsHidden is that it is inaccurate and will run into lots of edge cases. It doesn't make visual sense when you have windows with alpha regions, transparency or transformations preventing the shell from painting if the bounding area occludes the shell. Its pretty much defeats the purpose of having alpha regions, shape regions and transformations in the first place. You can easily run into cases where the shell won't paint even when it should, and I think that's bad. Unless you have a way to handle those cases, or a very strong argument as to why we shouldn't handle those cases, I honestly think its a blocking factor here.

The OpenGL plugin already uses similar looking logic to do its occlusion detection, and there is very little (if any) performance penalty for it. There is hardly any CompRegion allocation at all, because the variables are stored locally. In fact, every time I've callgrinded compiz, paintOutputRegion has not appeared as a hotspot anywhere. The re-entrancy into compiz is also very inexpensive because most plugins will have glPaint disabled if they are not running any animations.

I would suggest at least trying to make it work.

Other bits of review:

121 +std::vector<nux::View*> Controller::Impl::GetPanelViews() const
122 +{
123 + std::vector<nux::View*> views;
124 + views.reserve(windows_.size());
125 + for (auto window: windows_)
126 + views.push_back(ViewForWindow(window));
127 + return views;
128 +}
129 +

I think it probably makes more sense to have the panel controller keep track of the views in its own vector so that you can just return a const ref to the vector. Since we're going to be calling this function every time we call compizDamageNux, the cost of doing the initial tracking for outweighs the cost of the query here.

270 + w->isViewable() &&
271 + !w->inShowDesktopMode() && // Why must this != isViewable?

You probably wanted

w->state () & CompWindowStateHiddenMask

260 + // Loop through windows from back to front
261 + for (CompWindow* w : screen->windows ())
262 + {

If you still want to use this, it would be easier to go front to back, I had to read the code like 8 times to figure out how it worked.

// front to back
for (CompWindow *w : screen->reverseWindows ())
{
 if (w->isMapped() &&
  w->isViewable() &&
  !w->inShowDesktopMode() && // Why must this != isViewable?
  w->geometry().contains(output))
    return true; // a window that covers the whole output discovered at top first
 else
   for (Window id : nuxwins)
     if (w->id () == id && output->intersects (w->geometry ())
       return false; // a nux windows discovered first
}