Merge lp://qastaging/~mc-return/compiz/compiz.merge-fix1166195-fix1166196-fix116245-resizeinfo-fixes into lp://qastaging/compiz/0.9.10

Hi,

Please see my previous comments on the usage of the boy-scout rule.

These were the only changes that were actually substantial:

77 -const unsigned short RESIZE_POPUP_WIDTH = 85;
78 -const unsigned short RESIZE_POPUP_HEIGHT = 50;
79 +const unsigned short RESIZE_POPUP_WIDTH = 100;
80 +const unsigned short RESIZE_POPUP_HEIGHT = 33;

149 pango_font_description_set_family (font,"Sans");
150 - pango_font_description_set_absolute_size (font, 12 * PANGO_SCALE);
151 + pango_font_description_set_absolute_size (font,
152 + is->optionGetResizeinfoFontSize () *
153 + PANGO_SCALE);
154 pango_font_description_set_style (font, PANGO_STYLE_NORMAL);
155 - pango_font_description_set_weight (font, PANGO_WEIGHT_BOLD);
156 -
157 +
158 + if (is->optionGetResizeinfoFontBold ())
159 + pango_font_description_set_weight (font, PANGO_WEIGHT_BOLD);
160 + else
161 + pango_font_description_set_weight (font, PANGO_WEIGHT_NORMAL);

As for those changes, they seem fine to me. I haven't got time to look at the formatting things just yet though.
162 +

>
> These were the only changes that were actually substantial:
>
> 77 -const unsigned short RESIZE_POPUP_WIDTH = 85;
> 78 -const unsigned short RESIZE_POPUP_HEIGHT = 50;
> 79 +const unsigned short RESIZE_POPUP_WIDTH = 100;
> 80 +const unsigned short RESIZE_POPUP_HEIGHT = 33;
>
> 149 pango_font_description_set_family (font,"Sans");
> 150 - pango_font_description_set_absolute_size (font, 12 * PANGO_SCALE);
> 151 + pango_font_description_set_absolute_size (font,
> 152 + is->optionGetResizeinfoFontSize () *
> 153 + PANGO_SCALE);
> 154 pango_font_description_set_style (font, PANGO_STYLE_NORMAL);
> 155 - pango_font_description_set_weight (font, PANGO_WEIGHT_BOLD);
> 156 -
> 157 +
> 158 + if (is->optionGetResizeinfoFontBold ())
> 159 + pango_font_description_set_weight (font, PANGO_WEIGHT_BOLD);
> 160 + else
> 161 + pango_font_description_set_weight (font, PANGO_WEIGHT_NORMAL);
>
> As for those changes, they seem fine to me. I haven't got time to look at the
> formatting things just yet though.
> 162 +

TBH, the most substantial change in this MP was not listed by you ;)
It is this:

241 - (x + RESIZE_POPUP_WIDTH + 5),
242 - (y + RESIZE_POPUP_HEIGHT + 5));
243 + (RESIZE_POPUP_WIDTH + 5),
244 + (RESIZE_POPUP_HEIGHT + 5));

It reduces the damage rectangle's size by the window size...

This does not mean anything for someone reading the tooltip:

<_long>Show resize info for all windows as opposed to just for windows with a resize increment of greater than 1.</_long>

so I changed it to:

<_long>Show resize info for all windows as opposed to just for text based windows like terminals.</_long>

The other changes in the tooltips are:
If the user adjusts the color, he can also adjust the opacity.

In the code I did some minor indentation fixes, I am sorry I can't let them untouched if I notice those...
Other than that and the things already mentioned by yourself, I just changed assignment and declaration of
local variables to be done in the same line, if possible...

> This does not mean anything for someone reading the tooltip:
>
> <_long>Show resize info for all windows as opposed to just for windows with a
> resize increment of greater than 1.</_long>
>
> so I changed it to:
>
> <_long>Show resize info for all windows as opposed to just for text based
> windows like terminals.</_long>
>
> The other changes in the tooltips are:
> If the user adjusts the color, he can also adjust the opacity.
>
> In the code I did some minor indentation fixes, I am sorry I can't let them
> untouched if I notice those...
> Other than that and the things already mentioned by yourself, I just changed
> assignment and declaration of
> local variables to be done in the same line, if possible...

Indentation fixes and code cleanups are useful and appreciated. However, they tend to just confuse things when mixed them with substantive changes. Especially when the reviewer isn't immediately familiar with the section of code that's being edited.

I think this guideline should be followed when making such changes:
 1. If making a substantive change (eg, a change in behaviour, fixing the logic or a bug) keep the formatting, stylistic and other minor changes restricted to the code section in which the application logic is being changed. Do not change formatting, indentation, variable declarations or perform any other refactoring in unrelated code sections in the same, or other files.
 2. Merge proposals which change only indentation, formatting, variable declarations and perform other refactoring are 100% acceptable and encouraged (so long as they improve the quality of the code), however, they must be marked as separate so that reviewers know what to look out for.

Doing changes in this manner means that reviewers can get through both sets faster, because the type of cognitive load is different for each. In addition, it means that substantive changes aren't blocked on formatting changes which other reviewers might have suggestions on.

>
> TBH, the most substantial change in this MP was not listed by you ;)
> It is this:
>
> 241 - (x + RESIZE_POPUP_WIDTH + 5),
> 242 - (y + RESIZE_POPUP_HEIGHT + 5));
> 243 + (RESIZE_POPUP_WIDTH + 5),
> 244 + (RESIZE_POPUP_HEIGHT + 5));
>
> It reduces the damage rectangle's size by the window size...

Right, this is exactly the problem I'm referring to. Mixing behavioural and formatting changes can cause important things like this to be missed.

179 - int height = RESIZE_POPUP_HEIGHT;
180 - int width = RESIZE_POPUP_WIDTH;
181 float r, g, b, a;
182
183 INFO_SCREEN (screen);
184 @@ -197,7 +199,9 @@
185 cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
186
187 /* Setup Gradient */
188 - pattern = cairo_pattern_create_linear (0, 0, width, height);
189 + pattern = cairo_pattern_create_linear (0, 0,
190 + RESIZE_POPUP_WIDTH,
191 + RESIZE_POPUP_HEIGHT);
192
193 r = is->optionGetGradient1Red () / (float)0xffff;
194 g = is->optionGetGradient1Green () / (float)0xffff;
195 @@ -220,10 +224,14 @@
196
197 /* Rounded Rectangle! */
198 cairo_arc (cr, border, border, border, PI, 1.5f * PI);
199 - cairo_arc (cr, border + width - 2 * border, border, border,
200 + cairo_arc (cr, border + RESIZE_POPUP_WIDTH - 2 * border,
201 + border, border,
202 1.5f * PI, 2.0 * PI);
203 - cairo_arc (cr, width - border, height - border, border, 0, PI / 2.0f);
204 - cairo_arc (cr, border, height - border, border, PI / 2.0f, PI);
205 + cairo_arc (cr, RESIZE_POPUP_WIDTH - border,
206 + RESIZE_POPUP_HEIGHT - border,
207 + border, 0, PI / 2.0f);
208 + cairo_arc (cr, border, RESIZE_POPUP_HEIGHT - border,
209 + border, PI / 2.0f, PI);

Instead of removing the declarations of width and height, it might be better to change their purpose. We seem to be doing a lot of this:

RESIZE_POPUP_WIDTH - border ... RESIZE_POPUP_HEIGHT - border etc

Why not change the declaration to

unsigned int widthWithoutBorder = RESIZE_POPUP_WIDTH - border;
unsigned int heightWithoutBorder = RESIZE_POPUP_HEIGHT - border;

Then you can just do:

cairo_arc (cr, widthWithoutBorder,
206 + heightWithoutBorder,
207 + border, 0, PI / 2.0f);

etc

>
> Indentation fixes and code cleanups are useful and appreciated. However, they
> tend to just confuse things when mixed them with substantive changes.
> Especially when the reviewer isn't immediately familiar with the section of
> code that's being edited.
>
Yes, you are right - I apologize once again.

> I think this guideline should be followed when making such changes:
> 1. If making a substantive change (eg, a change in behaviour, fixing the
> logic or a bug) keep the formatting, stylistic and other minor changes
> restricted to the code section in which the application logic is being
> changed. Do not change formatting, indentation, variable declarations or
> perform any other refactoring in unrelated code sections in the same, or other
> files.
> 2. Merge proposals which change only indentation, formatting, variable
> declarations and perform other refactoring are 100% acceptable and encouraged
> (so long as they improve the quality of the code), however, they must be
> marked as separate so that reviewers know what to look out for.
>
Very good suggestion. I will do it that way from now on.

First I'll do a merge proposal with just substantial changes, then a follow-up
MP cleaning up that file's indentation and formatting problems only...

> Doing changes in this manner means that reviewers can get through both sets
> faster, because the type of cognitive load is different for each. In addition,
> it means that substantive changes aren't blocked on formatting changes which
> other reviewers might have suggestions on.

Agreed. I will try my best ;)

> 179 - int height = RESIZE_POPUP_HEIGHT;
> 180 - int width = RESIZE_POPUP_WIDTH;
> 181 float r, g, b, a;
> 182
> 183 INFO_SCREEN (screen);
> 184 @@ -197,7 +199,9 @@
> 185 cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
> 186
> 187 /* Setup Gradient */
> 188 - pattern = cairo_pattern_create_linear (0, 0, width, height);
> 189 + pattern = cairo_pattern_create_linear (0, 0,
> 190 + RESIZE_POPUP_WIDTH,
> 191 + RESIZE_POPUP_HEIGHT);
> 192
> 193 r = is->optionGetGradient1Red () / (float)0xffff;
> 194 g = is->optionGetGradient1Green () / (float)0xffff;
> 195 @@ -220,10 +224,14 @@
> 196
> 197 /* Rounded Rectangle! */
> 198 cairo_arc (cr, border, border, border, PI, 1.5f * PI);
> 199 - cairo_arc (cr, border + width - 2 * border, border, border,
> 200 + cairo_arc (cr, border + RESIZE_POPUP_WIDTH - 2 * border,
> 201 + border, border,
> 202 1.5f * PI, 2.0 * PI);
> 203 - cairo_arc (cr, width - border, height - border, border, 0, PI /
> 2.0f);
> 204 - cairo_arc (cr, border, height - border, border, PI / 2.0f, PI);
> 205 + cairo_arc (cr, RESIZE_POPUP_WIDTH - border,
> 206 + RESIZE_POPUP_HEIGHT - border,
> 207 + border, 0, PI / 2.0f);
> 208 + cairo_arc (cr, border, RESIZE_POPUP_HEIGHT - border,
> 209 + border, PI / 2.0f, PI);
>
> Instead of removing the declarations of width and height, it might be better
> to change their purpose. We seem to be doing a lot of this:
>
> RESIZE_POPUP_WIDTH - border ... RESIZE_POPUP_HEIGHT - border etc
>
> Why not change the declaration to
>
> unsigned int widthWithoutBorder = RESIZE_POPUP_WIDTH - border;
> unsigned int heightWithoutBorder = RESIZE_POPUP_HEIGHT - border;
>
> Then you can just do:
>
> cairo_arc (cr, widthWithoutBorder,
> 206 + heightWithoutBorder,
> 207 + border, 0, PI / 2.0f);
>
> etc

Yes, you are right. That would be a possibility.
But
"RESIZE_POPUP_WIDTH - border" is just used once and
"RESIZE_POPUP_HEIGHT - border" is just used twice in the code

So does this really justify creating 2 new variables ?
I do not think I agree here...

On Tue, Apr 9, 2013 at 5:24 PM, MC Return <email address hidden> wrote:
>>
>> Indentation fixes and code cleanups are useful and appreciated. However, they
>> tend to just confuse things when mixed them with substantive changes.
>> Especially when the reviewer isn't immediately familiar with the section of
>> code that's being edited.
>>
> Yes, you are right - I apologize once again.

No need to apologize, this was merely an observation.

>
>> I think this guideline should be followed when making such changes:
>> 1. If making a substantive change (eg, a change in behaviour, fixing the
>> logic or a bug) keep the formatting, stylistic and other minor changes
>> restricted to the code section in which the application logic is being
>> changed. Do not change formatting, indentation, variable declarations or
>> perform any other refactoring in unrelated code sections in the same, or other
>> files.
>> 2. Merge proposals which change only indentation, formatting, variable
>> declarations and perform other refactoring are 100% acceptable and encouraged
>> (so long as they improve the quality of the code), however, they must be
>> marked as separate so that reviewers know what to look out for.
>>
> Very good suggestion. I will do it that way from now on.
>
> First I'll do a merge proposal with just substantial changes, then a follow-up
> MP cleaning up that file's indentation and formatting problems only..

That makes sense, thanks :)
.
>
>> Doing changes in this manner means that reviewers can get through both sets
>> faster, because the type of cognitive load is different for each. In addition,
>> it means that substantive changes aren't blocked on formatting changes which
>> other reviewers might have suggestions on.
>
> Agreed. I will try my best ;)

> "RESIZE_POPUP_WIDTH - border" is just used once and
> "RESIZE_POPUP_HEIGHT - border" is just used twice in the code
> So does this really justify creating 2 new variables ?

My rule of thumb tends to be more extreme than the prevailing view,
which is that if you have some value that is being used as a value
passed to a function, then its a good idea to keep it in a constant if
the calculation is done more than once, as opposed to doing the
calculation multiple times as nested expressions within its
parameters. Its just a minor thing though, so its not something that
bothers me a whole lot.

> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1166195-fix1166196-fix116245-resizeinfo-fixes/+merge/157670
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix1166195-fix1166196-fix116245-resizeinfo-fixes into lp:compiz.

--
Sam Spilsbury