Merge lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options into lp:compiz/0.9.10

[MC Return]

*Screenshot, xml:

Better plugin summary.
Added option to disable the drawing of the screenshot
selection indicator.
Added options to change color and opacity of the outline
and the inside of the rectangle indicator, default colors
have not been changed.
Other minor improvements.

*Screenshot, code:

Introduced the new variable bool selectionSizeChanged,
which is true if the size of the selection has changed.

We just want to draw the screenshot selection box, if
we are grabbed, the size has changed and the CCSM
option to draw it is enabled.
Do not render anything if indicator is disabled.

Indicator colors are now configurable.
Increased indicator box outline line width to 2.0.

Only damage the full screen once, if the grab gets
terminated, during grab just damage the region painted,
which is the screenshot selection box (LP: #1172234).

*SCreenshot, cleanup:

Minor cleanup in the rendering code.
Removed redundant #ifndef USE_GLES compiler option,
GLES can cope with enabling and disabling GL_BLEND.
Use prefix- instead of postfix increments.
Declaration and assignment of local variables in one line.
Reduced the scope of the variables x1, y1, x2 and y2.
Replaced magic number 65535.0f with const float MaxUShortFloat =
std::numeric_limits <unsigned short>::max ();.
Indentation fixes.
C++ style for comments, added comments.

(LP: #1169353, LP: #1172234)

[Sam Spilsbury (smspillaz)]

Looks mostly good, just a few tips:

115 +#ifndef USE_GLES
116 + glEnable (GL_BLEND);
117 +#endif
147 -#ifndef USE_GLES
148 - glEnable (GL_BLEND);
149 -#endif

Its not necessary to move that as it only needs to be enabled when draw commands are sent to the GPU. So it can stay in the same place.

119 + float alpha = ((float) optionGetFillColorAlpha () / 65535.0f);

This can be replaced with

float alpha = optionGetFillColorAlpha () / static_cast <float> (std::numeric_limits <unsigned short>::max ());

Three things to note with that:
 1. In order to get floating point division, you only need to have the denominator be a float and not the numerator.
 2. std::numeric_limits <unsigned short::max () is the preferable way of saying 65536
 3. static_cast should be preferred over c-style casts (eg (Type));

also:

125 + colorData[3] = alpha * 65535.0f;
becomes

colorData[3] = alpha * std::numeric_limits <unsigned short>::max ();

Finally, there are a few formatting changes scattered around here (though not as much as before). Try to split those two up as much as possible, as the areas where formatting changes were made were not really related to what this change is doing. They're fine to keep though, as they're correct.

[MC Return (mc-return)]

> Looks mostly good, just a few tips:
>
> 115 +#ifndef USE_GLES
> 116 + glEnable (GL_BLEND);
> 117 +#endif
> 147 -#ifndef USE_GLES
> 148 - glEnable (GL_BLEND);
> 149 -#endif
>
> Its not necessary to move that as it only needs to be enabled when draw
> commands are sent to the GPU. So it can stay in the same place.
>

Question: Do we really need those #ifndefs ?
GLES should have no problem with glEnable (GL_BLEND); and glDisable (GL_BLEND);, no ?

> 119 + float alpha = ((float) optionGetFillColorAlpha () /
> 65535.0f);
>
> This can be replaced with
>
> float alpha = optionGetFillColorAlpha () / static_cast <float>
> (std::numeric_limits <unsigned short>::max ());
>
I removed the redundant (float) casts in r3659.

> Three things to note with that:
> 1. In order to get floating point division, you only need to have the
> denominator be a float and not the numerator.
> 2. std::numeric_limits <unsigned short::max () is the preferable way of
> saying 65536
> 3. static_cast should be preferred over c-style casts (eg (Type));
>

2. AFAIK an unsigned short can have a different size, depending on the
hardware.
I am not convinced this is the best way to say 65535...

Maybe we can find another way ? (see the Ezoom MP also)

>
> Finally, there are a few formatting changes scattered around here (though not
> as much as before). Try to split those two up as much as possible, as the
> areas where formatting changes were made were not really related to what this
> change is doing. They're fine to keep though, as they're correct.

Yeah, sorry about that (again) - but it will get better each time we touch a
specific file.
Once most stuff has been cleaned up, it won't get cleaned up again ;)

[MC Return (mc-return)]

review: Needs Information

[Sam Spilsbury (smspillaz)]

The ifndefs are not necessary for enabling the blend state. Its a side effect of when Travis had turned on blend full-time, but we decided to revert that change because while it was certainly convenient, it had some performance implications and there are also some situations where blending is not desired.

short unsigned int basically going to be 16 bits on every architecture we care about. If we start caring about really exotic hardware then maybe that will need to change, but I don't imagine its the case now.

65535 is a lot easier to typo if you have it everywhere.

[MC Return (mc-return)]

Grmpf, during testing I found out that damage handling is also broken, I filed bug #1172234 about it.
This should be pretty easy to fix, so I'll do it here...

I still have a minor blending problem to fix, if you have a tip how to fix the
the-screenshot-selection-overlay-is-visible-on-screenshots-if-fbos-are-enabled
please shout out loud in the meantime... ;)

review: Needs Information

[MC Return (mc-return)]

Okay, all fixed now, except the
the-screenshot-selection-overlay-is-visible-on-screenshots-if-fbos-are-enabled issue...

The rest works perfectly now - had to fix some additional issues, please see
http://bazaar.launchpad.net/~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options/revision/3661
for details.

The preview was overdrawing all the time, if no cursor movement was done. <- Fixed now.
The whole screen was damaged, instead of just the selection rectangle. <- Fixed now as well.

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

On Wed, Apr 24, 2013 at 4:03 AM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
> Grmpf, during testing I found out that damage handling is also broken, I filed bug #1172234 about it.
> This should be pretty easy to fix, so I'll do it here...
>
Please do it separately. If there are comments I have about the damage
handling fix they'll also hold up this merge request too.

> I still have a minor blending problem to fix, if you have a tip how to fix the
> the-screenshot-selection-overlay-is-visible-on-screenshots-if-fbos-are-enabled
> please shout out loud in the meantime... ;)

You need to set the read buffer to GL_BACK I think. I'm a bit hesitant
about giving that advice though until we have a proper abstraction
for tracking framebuffer attachment points inside of compiz - querying
OpenGL is sloowwwww.

> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options/+merge/159038
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

> Please do it separately. If there are comments I have about the damage
> handling fix they'll also hold up this merge request too.
>
It is all already done and all fixed, I will explain in detail what I changed
to fix the damage handling, it was just a minor issue:

267 -
268 - cScreen->damageScreen ();
269 }
270 +
271 + cScreen->damageScreen ();

It was damaging the whole screen during a size change instead of just damaging
the screen once the operation was finished (the mouse button is released).

The other thing I fixed in r3661 was a permanent redraw problem, because the
overlay got overdrawn when there was no movement. This is now fixed also.
This was the blending problem I discovered and have written about earlier here:

> > I still have a minor blending problem to fix,

This was fixed by introducing the new variable bool selectionSizeChanged,
which is true if the size of the selection has changed, please see r3661
for details on this:

http://bazaar.launchpad.net/~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options/revision/3661

>
> You need to set the read buffer to GL_BACK I think. I'm a bit hesitant
> about giving that advice though until we have a proper abstraction
> for tracking framebuffer attachment points inside of compiz - querying
> OpenGL is sloowwwww.
>

I will do some more experiments and try to fix this last issue in another MP.
Thanks a lot for the tip. :)

[MC Return (mc-return)]

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

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

[MC Return (mc-return)]

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

> 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.

You are right -> the fix is not entirely correct. The cScreen->damageScreen ();
call should be removed completely as we do never need to damage more than the
actual selection...

I'll fix that immediately.

[MC Return (mc-return)]

And the whole damage handling could be improved here, but without
the cScreen->damageScreen (); call it is already acting much more
sane, but it has some room for optimization still (no need to damage
the region twice for example)...

[lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options]

3662. By MC Return on 2013-04-24

We really never need to damage more than the selected region, removed
call to cScreen->damageScreen (); completely

[MC Return (mc-return)]

I will create a short video showing how the damaging for screenshot and ezoom now work if you like ?

Show Repaint is *awesome* for debugging this stuff !!!

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

Looks good as it is now.

Don't change the current damageRegion code as it stands, its actually correct in its current form. Where you have a rectangle that varies in size, you should always mark the old rectangle as damaged as well as the new one, because you need to redraw the area underneath where the old rectangle was. This handles the case where the overlay-rectangle is getting smaller.

review: Approve

[MC Return (mc-return)]

> Looks good as it is now.
>

+1

> Don't change the current damageRegion code as it stands, its actually correct
> in its current form. Where you have a rectangle that varies in size, you
> should always mark the old rectangle as damaged as well as the new one,
> because you need to redraw the area underneath where the old rectangle was.
> This handles the case where the overlay-rectangle is getting smaller.

Never stop learning ;)

Now lets move to the freaking final screenshot issue... :)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/screenshot/screenshot.xml.in'
--- plugins/screenshot/screenshot.xml.in	2012-10-15 10:31:51 +0000
+++ plugins/screenshot/screenshot.xml.in	2013-04-24 15:45:35 +0000
@@ -2,7 +2,7 @@
 <compiz>
     <plugin name="screenshot" useBcop="true">
 	<_short>Screenshot</_short>
-	<_long>Screenshot plugin</_long>
+	<_long>Easily capture parts of your screen and save them as .png image</_long>
 	<category>Extras</category>
 	<deps>
 	    <requirement>
@@ -17,18 +17,43 @@
 	<options>
 	    <option name="initiate_button" type="button">
 		<_short>Initiate</_short>
-		<_long>Initiate rectangle screenshot</_long>
+		<_long>Initiate the capture, release the button to take the shot.</_long>
 		<default>&lt;Super&gt;Button1</default>
 	    </option>
+		<option name="draw_selection_indicator" type="bool">
+			<_short>Draw Screenshot Selection Indicator</_short>
+			<_long>Draw a rectangle outline indicating the screenshot selection.</_long>
+			<default>true</default>
+		</option>
+		<option name="selection_outline_color" type="color">
+			<_short>Selection Indicator Outline Color</_short>
+			<_long>Color and opacity of the screenshot selection rectangle outline.</_long>
+			<default>
+			    <red>0x2fff</red>
+			    <green>0x2fff</green>
+			    <blue>0x4fff</blue>
+			    <alpha>0x9f9f</alpha>
+			</default>
+		</option>
+		<option name="selection_fill_color" type="color">
+			<_short>Selection Indicator Fill Color</_short>
+			<_long>Fill color and opacity of the screenshot selection rectangle.</_long>
+			<default>
+			    <red>0x2fff</red>
+			    <green>0x2fff</green>
+			    <blue>0x4fff</blue>
+			    <alpha>0x4fff</alpha>
+			</default>
+		</option>
 	    <option name="directory" type="string">
-		<_short>Directory</_short>
-		<_long>Put screenshot images in this directory. If empty, the desktop directory will be used.</_long>
+		<_short>Save Directory</_short>
+		<_long>Save screenshot images in this directory. If empty, the desktop directory will be used.</_long>
 		<hints>directory;</hints>
 		<default></default>
 	    </option>
 	    <option name="launch_app" type="string">
 		<_short>Launch Application</_short>
-		<_long>Automatically open screenshot in this application</_long>
+		<_long>Automatically open the screenshot in this application.</_long>
 		<default></default>
 	    </option>
 	</options>

=== modified file 'plugins/screenshot/src/screenshot.cpp'
--- plugins/screenshot/src/screenshot.cpp	2013-04-13 22:29:01 +0000
+++ plugins/screenshot/src/screenshot.cpp	2013-04-24 15:45:35 +0000
@@ -34,8 +34,6 @@
   #define scandir(a,b,c,d) scandir((a), (b), (c), (int(*)(const void*,const void*))(d));
 #endif
 
-
-
 COMPIZ_PLUGIN_20090315 (screenshot, ShotPluginVTable)
 
 bool
@@ -58,7 +56,7 @@
     if (state & CompAction::StateInitButton)
 	action->setState (action->state () | CompAction::StateTermButton);
 
-    /* start selection screenshot rectangle */
+    /* Start selection screenshot rectangle */
 
     mX1 = mX2 = pointerX;
     mY1 = mY2 = pointerY;
@@ -82,7 +80,7 @@
 
     if (mGrabIndex)
     {
-	// Enable screen capture
+	/* Enable screen capture */
 	cScreen->paintSetEnabled (this, true);
 	::screen->removeGrab (mGrabIndex, 0);
 	mGrabIndex = 0;
@@ -121,9 +119,9 @@
 	int nDigits = 0;
 
 	for (; number > 0; number /= 10)
-	    nDigits++;
+	    ++nDigits;
 
-	// Make sure there are no trailing characters in the name
+	/* Make sure there are no trailing characters in the name */
 	if ((int) strlen (d->d_name) == 14 + nDigits)
 	    return 1;
     }
@@ -154,15 +152,13 @@
 
     if (mGrab)
     {
-	int x1, x2, y1, y2;
-
-	x1 = MIN (mX1, mX2);
-	y1 = MIN (mY1, mY2);
-	x2 = MAX (mX1, mX2);
-	y2 = MAX (mY1, mY2);
-
 	if (!mGrabIndex)
 	{
+	    int x1 = MIN (mX1, mX2);
+	    int y1 = MIN (mY1, mY2);
+	    int x2 = MAX (mX1, mX2);
+	    int y2 = MAX (mY1, mY2);
+
 	    int w = x2 - x1;
 	    int h = y2 - y1;
 
@@ -176,6 +172,7 @@
 		    dir = getXDGUserDir (XDGUserDirDesktop);
 
 		buffer = (GLubyte *)malloc (sizeof (GLubyte) * w * h * 4);
+
 		if (buffer)
 		{
 		    struct dirent **namelist;
@@ -196,7 +193,7 @@
 				    "screenshot%d.png",
 				    &number);
 
-			number++;
+			++number;
 
 			if (n)
 			    free (namelist);
@@ -238,18 +235,36 @@
     GLMatrix        transform (matrix);
     GLfloat         vertexData[12];
     GLushort        colorData[4];
+    GLushort        *color;
 
     bool status = gScreen->glPaintOutput (attrib, matrix, region, output, mask);
 
     if (status && mGrab)
     {
-	int x1 = MIN (mX1, mX2);
-	int y1 = MIN (mY1, mY2);
-	int x2 = MAX (mX1, mX2);
-	int y2 = MAX (mY1, mY2);
+	/* We just want to draw the screenshot selection box if
+	 * we are grabbed, the size has changed and the CCSM
+	 * option to draw it is enabled. */
 
-	if (mGrabIndex)
+	if (mGrabIndex &&
+	    selectionSizeChanged &&
+	    optionGetDrawSelectionIndicator ())
 	{
+	    int x1 = MIN (mX1, mX2);
+	    int y1 = MIN (mY1, mY2);
+	    int x2 = MAX (mX1, mX2);
+	    int y2 = MAX (mY1, mY2);
+
+	    const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
+
+	    /* draw filled rectangle */
+	    float alpha = optionGetSelectionFillColorAlpha () / MaxUShortFloat;
+	    color = optionGetSelectionFillColor ();
+
+	    colorData[0] = alpha * color[0];
+	    colorData[1] = alpha * color[1];
+	    colorData[2] = alpha * color[2];
+	    colorData[3] = alpha * MaxUShortFloat;
+
 	    vertexData[0]  = x1;
 	    vertexData[1]  = y1;
 	    vertexData[2]  = 0.0f;
@@ -263,12 +278,6 @@
 	    vertexData[10] = y2;
 	    vertexData[11] = 0.0f;
 
-	    colorData[0] = 0x2fff;
-	    colorData[1] = 0x2fff;
-	    colorData[2] = 0x4fff;
-	    colorData[3] = 0x4fff;
-
-
 	    transform.translate (-0.5f, -0.5f, -DEFAULT_Z_CAMERA);
 	    transform.scale (1.0f / output->width (),
 			     -1.0f / output->height (),
@@ -277,9 +286,8 @@
 				 -output->region ()->extents.y2,
 				 0.0f);
 
-#ifndef USE_GLES
 	    glEnable (GL_BLEND);
-#endif
+
 	    streamingBuffer->begin (GL_TRIANGLE_STRIP);
 
 	    streamingBuffer->addColors (1, colorData);
@@ -288,22 +296,35 @@
 	    streamingBuffer->end ();
 	    streamingBuffer->render (transform);
 
-	    streamingBuffer->begin (GL_LINE_LOOP);
+	    /* draw outline */
+	    alpha = optionGetSelectionOutlineColorAlpha () / MaxUShortFloat;
+	    color = optionGetSelectionOutlineColor ();
+
+	    colorData[0] = alpha * color[0];
+	    colorData[1] = alpha * color[1];
+	    colorData[2] = alpha * color[2];
+	    colorData[3] = alpha * MaxUShortFloat;
 
 	    vertexData[6]  = x2;
 	    vertexData[7]  = y2;
 	    vertexData[9]  = x2;
 	    vertexData[10] = y1;
-	    colorData [3]  = 0x9fff;
+
+	    glLineWidth (2.0);
+
+	    streamingBuffer->begin (GL_LINE_LOOP);
 
 	    streamingBuffer->addColors (1, colorData);
 	    streamingBuffer->addVertices (4, vertexData);
 
 	    streamingBuffer->end ();
-#ifndef USE_GLES
+	    streamingBuffer->render (transform);
+
 	    glDisable (GL_BLEND);
-#endif
-	    streamingBuffer->render (transform);
+
+	    /* we finished painting the selection box,
+	     * reset selectionSizeChanged now */
+	    selectionSizeChanged = false;
 	}
     }
 
@@ -315,8 +336,12 @@
 			       int yRoot)
 {
     /* update screenshot rectangle size */
-    if (mGrabIndex)
+    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;
@@ -333,8 +358,6 @@
 	y2 = MAX (mY1, mY2) + 1;
 
 	cScreen->damageRegion (CompRegion (x1, y1, x2 - x1, y2 - y1));
-
-	cScreen->damageScreen ();
     }
 }
 
@@ -366,12 +389,13 @@
     cScreen (CompositeScreen::get (screen)),
     gScreen (GLScreen::get (screen)),
     mGrabIndex (0),
-    mGrab (false)
+    mGrab (false),
+    selectionSizeChanged (false)
 {
     optionSetInitiateButtonInitiate (boost::bind (&ShotScreen::initiate, this,
-    						  _1, _2, _3));
+						  _1, _2, _3));
     optionSetInitiateButtonTerminate (boost::bind (&ShotScreen::terminate, this,
-    						   _1, _2, _3));
+						   _1, _2, _3));
 
     ScreenInterface::setHandler (screen, false);
     CompositeScreenInterface::setHandler (cScreen, false);

=== modified file 'plugins/screenshot/src/screenshot.h'
--- plugins/screenshot/src/screenshot.h	2013-04-13 22:29:01 +0000
+++ plugins/screenshot/src/screenshot.h	2013-04-24 15:45:35 +0000
@@ -66,6 +66,7 @@
 
 	CompScreen::GrabHandle mGrabIndex;
 	bool                   mGrab;
+	bool                   selectionSizeChanged;
 
 	int  mX1, mY1, mX2, mY2;
 };