Code review comment for lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Hrm, I don't think the way you've fixed this is really correct:

void
ShotScreen::handleMotionEvent (int xRoot,
          int yRoot)
{
    /* update screenshot rectangle size */
    if (mGrabIndex &&
 (mX2 != xRoot || mY2 != yRoot))
    {
 /* the size has changed now */
 selectionSizeChanged = true;

 int x1 = MIN (mX1, mX2) - 1;
 int y1 = MIN (mY1, mY2) - 1;
 int x2 = MAX (mX1, mX2) + 1;
 int y2 = MAX (mY1, mY2) + 1;

 cScreen->damageRegion (CompRegion (x1, y1, x2 - x1, y2 - y1));

 mX2 = xRoot;
 mY2 = yRoot;

 x1 = MIN (mX1, mX2) - 1;
 y1 = MIN (mY1, mY2) - 1;
 x2 = MAX (mX1, mX2) + 1;
 y2 = MAX (mY1, mY2) + 1;

 cScreen->damageRegion (CompRegion (x1, y1, x2 - x1, y2 - y1));
    }

    cScreen->damageScreen ();
}

The whole idea of those two damageRegion () calls is to mark the *old* selection rectangle region as damaged and then mark the new selection region as damaged, so that the damage area covers the selection area.

This change does that *and* damages the whole screen every time there is a motion event, regardless of whether or not screenshot even has an active grab or the mouse cursor position actually changed.

If the damage code isn't working, then you should look into why the current region-based damage isn't working rather than damaging the whole screen every time there is a motion event.

review: Needs Fixing

« Back to merge proposal