Merge lp:~mc-return/compiz/compiz.merge-fix1070233-resize-info-gradient-color-settings-ignored into lp:compiz/0.9.9

[MC Return]

Resizeinfo Plug-in:
Fixed options to change Gradient Color 2 and Gradient Color 3 having no effect and enabled possibility for a gradient background.
(LP: #1070233)

As requested by Daniel, I am separating the fixes for the resizeinfo problems.
This branch just fixes CCSM resizeinfo plug-in's gradient color settings being ignored.

[Sam Spilsbury (smspillaz)]

Oh, blah, the original author seems to have use GradientN for everything, which makes the code annoying to review:

8 - r = is->optionGetGradient1Red () / (float)0xffff;
9 - g = is->optionGetGradient1Green () / (float)0xffff;
10 - b = is->optionGetGradient1Blue () / (float)0xffff;
11 - a = is->optionGetGradient1Alpha () / (float)0xffff;
12 + r = is->optionGetGradient2Red () / (float)0xffff;
13 + g = is->optionGetGradient2Green () / (float)0xffff;
14 + b = is->optionGetGradient2Blue () / (float)0xffff;
15 + a = is->optionGetGradient2Alpha () / (float)0xffff;

My advice earlier about using static_cast and numeric_limits still applies here.

Do you think you could also change the gradient_n names in the xml file to something more sensible while you're at it too?

[MC Return (mc-return)]

> Oh, blah, the original author seems to have use GradientN for everything,
> which makes the code annoying to review:
>
> 8 - r = is->optionGetGradient1Red () / (float)0xffff;
> 9 - g = is->optionGetGradient1Green () / (float)0xffff;
> 10 - b = is->optionGetGradient1Blue () / (float)0xffff;
> 11 - a = is->optionGetGradient1Alpha () / (float)0xffff;
> 12 + r = is->optionGetGradient2Red () / (float)0xffff;
> 13 + g = is->optionGetGradient2Green () / (float)0xffff;
> 14 + b = is->optionGetGradient2Blue () / (float)0xffff;
> 15 + a = is->optionGetGradient2Alpha () / (float)0xffff;
>
> My advice earlier about using static_cast and numeric_limits still applies
> here.
>

Done.

> Do you think you could also change the gradient_n names in the xml file to
> something more sensible while you're at it too?

Sure, but I would like to do that in a separate branch once this branch and https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1070297-no-possibility-to-change-background-outline-color/+merge/132500 are merged, because otherwise there will be conflicts as I've renamed 'static void gradientChanged' to 'static void backgroundColorChanged' there.

[MC Return (mc-return)]

> Do you think you could also change the gradient_n names in the xml file to
> something more sensible while you're at it too?

I've improved the tooltips in r3449 for now to make it more clear what Gradient Color 1-3 mean.

[Sam Spilsbury (smspillaz)]

> > Oh, blah, the original author seems to have use GradientN for everything,
> > which makes the code annoying to review:
> >
> > 8 - r = is->optionGetGradient1Red () / (float)0xffff;
> > 9 - g = is->optionGetGradient1Green () / (float)0xffff;
> > 10 - b = is->optionGetGradient1Blue () / (float)0xffff;
> > 11 - a = is->optionGetGradient1Alpha () / (float)0xffff;
> > 12 + r = is->optionGetGradient2Red () / (float)0xffff;
> > 13 + g = is->optionGetGradient2Green () / (float)0xffff;
> > 14 + b = is->optionGetGradient2Blue () / (float)0xffff;
> > 15 + a = is->optionGetGradient2Alpha () / (float)0xffff;
> >
> > My advice earlier about using static_cast and numeric_limits still applies
> > here.
> >
>
> Done.
>
> > Do you think you could also change the gradient_n names in the xml file to
> > something more sensible while you're at it too?
>
> Sure, but I would like to do that in a separate branch once this branch and
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1070297-no-
> possibility-to-change-background-outline-color/+merge/132500 are merged,
> because otherwise there will be conflicts as I've renamed 'static void
> gradientChanged' to 'static void backgroundColorChanged' there.

Yeah that's fine.

+1 on the other changes, see the advice on the other MP about the constant too in case you're interested :)

review: Approve

[MC Return (mc-return)]

Sam, thanks again for review and approval.
As commented in the other branch already I would prefer to do this final polishing together with the gradient_1-3 name changes once the resizeinfo branches are merged to prevent conflicts and double-work.

[Daniel van Vugt (vanvugt)]

I'm not sure how it works, if at all, but it probably shouldn't...

static_cast <float> (std::numeric_limits <int>::max ())

that will have a value of 0x7fffffff (32-bit x86) or 0x7fffffffffffffff (64-bit), both of which are wrong. Colours should be relative to 0xffff only, or "65535.0f".

review: Needs Fixing

[MC Return (mc-return)]

> I'm not sure how it works, if at all, but it probably shouldn't...

It was working here...

> static_cast <float> (std::numeric_limits <int>::max ())
>
> that will have a value of 0x7fffffff (32-bit x86) or 0x7fffffffffffffff
> (64-bit), both of which are wrong. Colours should be relative to 0xffff only,
> or "65535.0f".

Going back to (float)0xffff.
As I've stated in the related resizeinfo branch already - if we want to change that style, it would be best to do it in a separate commit and for the whole lp:compiz code and all similar cases.

review: Needs Resubmitting

[Daniel van Vugt (vanvugt)]

OK, looks good now and works.

I would have recommended not updating the strings in the same branch, but it's not worth complaining about.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/57/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=amd64/57/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=i386/57/console

review: Needs Fixing (continuous-integration)

[MC Return (mc-return)]

Seems like some Jenkins failure, from the console log:

bzr: ERROR: Error parsing trunk.recipe:4:25: End of line while looking for the subpath to merge.
Usage: nest-part NAME BRANCH SUBDIR [TARGET-DIR [REVISION]]
ERROR:pbuilderjenkins:Error during build execution
ERROR:pbuilderjenkins:Error while preparing from recipe:
pbuilder returned ERROR.
Build step 'Debian PBuilder NG' marked build as failure

[Martin Mrazik (mrazik)]

Correct. Re-approving.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/59/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=amd64/59/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=i386/59/console

review: Needs Fixing (continuous-integration)

[Martin Mrazik (mrazik)]

Re-approving again (fixed some more jenkins issues).

=== modified file 'plugins/resizeinfo/resizeinfo.xml.in'
--- plugins/resizeinfo/resizeinfo.xml.in	2012-10-15 10:31:51 +0000
+++ plugins/resizeinfo/resizeinfo.xml.in	2012-11-05 10:17:24 +0000
@@ -18,19 +18,19 @@
 	<options>
 	    <option name="fade_time" type="int">
 		<_short>Fade Time</_short>
-		<_long>Fade time (in ms) for popup window</_long>
+		<_long>Fade time (in ms) for the popup window.</_long>
 		<default>500</default>
 		<min>10</min>
 		<max>5000</max>
 	    </option>
 	    <option name="always_show" type="bool">
 		<_short>Show resize info for all windows</_short>
-		<_long>Show resize info for all windows as opposed to just windows with a resize increment of greater than 1.</_long>
+		<_long>Show resize info for all windows as opposed to just for windows with a resize increment of greater than 1.</_long>
 		<default>false</default>
 	    </option>
 	    <option name="text_color" type="color">
-		<_short>Text color</_short>
-		<_long>Color of text on resize popup.</_long>
+		<_short>Text Color</_short>
+		<_long>Color of the resize popup's text.</_long>
 		<default>
 		    <red>0x0000</red>
 		    <blue>0x0000</blue>
@@ -40,7 +40,7 @@
 	    </option>
 	    <option name="gradient_1" type="color">
 		<_short>Gradient Color 1</_short>
-		<_long>Color 1 of the gradient background.</_long>
+		<_long>Color on the left side of the gradient background.</_long>
 		<default>
 		    <red>0xcccc</red>
 		    <green>0xcccc</green>
@@ -50,7 +50,7 @@
 	    </option>
 	    <option name="gradient_2" type="color">
 		<_short>Gradient Color 2</_short>
-		<_long>Color 2 of the gradient background.</_long>
+		<_long>Color in the center of the gradient background.</_long>
 		<default>
 		    <red>0xf332</red>
 		    <green>0xf332</green>
@@ -60,7 +60,7 @@
 	    </option>
 	    <option name="gradient_3" type="color">
 		<_short>Gradient Color 3</_short>
-		<_long>Color 3 of the gradient background.</_long>
+		<_long>Color on the right side of the gradient background.</_long>
 		<default>
 		    <red>0xd998</red>
 		    <green>0xd998</green>

=== modified file 'plugins/resizeinfo/src/resizeinfo.cpp'
--- plugins/resizeinfo/src/resizeinfo.cpp	2012-09-07 23:29:42 +0000
+++ plugins/resizeinfo/src/resizeinfo.cpp	2012-11-05 10:17:24 +0000
@@ -205,16 +205,16 @@
     a = is->optionGetGradient1Alpha () / (float)0xffff;
     cairo_pattern_add_color_stop_rgba (pattern, 0.00f, r, g, b, a);
 
-    r = is->optionGetGradient1Red () / (float)0xffff;
-    g = is->optionGetGradient1Green () / (float)0xffff;
-    b = is->optionGetGradient1Blue () / (float)0xffff;
-    a = is->optionGetGradient1Alpha () / (float)0xffff;
+    r = is->optionGetGradient2Red () / (float)0xffff;
+    g = is->optionGetGradient2Green () / (float)0xffff;
+    b = is->optionGetGradient2Blue () / (float)0xffff;
+    a = is->optionGetGradient2Alpha () / (float)0xffff;
     cairo_pattern_add_color_stop_rgba (pattern, 0.65f, r, g, b, a);
 
-    r = is->optionGetGradient1Red () / (float)0xffff;
-    g = is->optionGetGradient1Green () / (float)0xffff;
-    b = is->optionGetGradient1Blue () / (float)0xffff;
-    a = is->optionGetGradient1Alpha () / (float)0xffff;
+    r = is->optionGetGradient3Red () / (float)0xffff;
+    g = is->optionGetGradient3Green () / (float)0xffff;
+    b = is->optionGetGradient3Blue () / (float)0xffff;
+    a = is->optionGetGradient3Alpha () / (float)0xffff;
     cairo_pattern_add_color_stop_rgba (pattern, 0.85f, r, g, b, a);
     cairo_set_source (cr, pattern);