On Fri, 10 Sep 2010 at 20:51:00 +0400, Alexey I. Froloff wrote:
> Adds WindowTitleMinHeight, WindowTitleMaxHeight, MenuTitleMinHeight and
> MenuTitleMaxHeight options, that provides more control over titlebar
> look.
The user can't change any values, right? It's all default choices.
That does not seem "more control" to me.
What is the difference you personally see with/without the patch?
> 8 files changed, 55 insertions(+), 11 deletions(-)
> + {"WindowTitleMinHeight", "0", NULL,
> + &wPreferences.window_title_min_height, getInt, setClearance,
> NULL, NULL},
Minimum height = 0
> + {"WindowTitleMaxHeight", NUM2STRING(INT_MAX), NULL,
> + &wPreferences.window_title_max_height, getInt, setClearance,
> NULL, NULL},
A quick grep in /usr/include gave me this
define INT_MAX 2147483647
so it appears to be an absurd number in this situation.
> + {"MenuTitleMinHeight", "0", NULL,
> + if (flags & WFF_TITLEBAR) {
> theight = WMFontHeight(*fwin->font) + (*fwin->title_clearance +
> TITLEBAR_EXTEND_SPACE) * 2;
> - else
> + if(theight > *fwin->title_max_height) theight =
> *fwin->title_max_height;
> + if(theight < *fwin->title_min_height) theight =
> *fwin->title_min_height;
> + } else
> theight = 0;
Ignoring the coding style violation :-) I would say the "theight < 0" check
will never be true. Furthermore, I don't think the result of WMFontHeight()
can be greater than INT_MAX, which is a huge number.
Is it really necessary to add all this code to do this never true checks?
> + if(theight > *fwin->title_max_height) theight = *fwin->title_max_height;
> + if(theight < *fwin->title_min_height) theight = *fwin->title_min_height;
Again.
> + if(y*2 + h > *fwin->title_max_height) y =
> (*fwin->title_max_height - h)/2;
> + if(y*2 + h < *fwin->title_min_height) y =
> (*fwin->title_min_height - h)/2;
I have to check whether these conditions can ever be met,
being < 0 or > INT_MAX seems surprising at first.
> + if(h > wPreferences.window_title_max_height) h =
> wPreferences.window_title_max_height;
> + if(h < wPreferences.window_title_min_height) h =
> wPreferences.window_title_min_height;
again
> + if(h > wPreferences.window_title_max_height) h =
> wPreferences.window_title_max_height;
> + if(h < wPreferences.window_title_min_height) h =
> wPreferences.window_title_min_height;
again
> + if(h > wPreferences.window_title_max_height) h =
> wPreferences.window_title_max_height;
> + if(h < wPreferences.window_title_min_height) h =
> wPreferences.window_title_min_height;
and again
Well, unless I am completely missing the point, I won't apply this patch.
I am not convinced about it.
--
To unsubscribe, send mail to [email protected].
