Re: [Gnustep-cvs] r34174 - in /libs/gui/trunk: ChangeLog Headers/Additions/GNUstepGUI/GSTheme.h Source/GSTheme.m Source/GSThemeDrawing.m Source/NSMenuItemCell.m

[Fred Kiefer]

On 16.11.2011 11:32, Quentin Math wrote:
> Author: qmathe
> Date: Wed Nov 16 11:32:15 2011
> New Revision: 34174
>
> URL: http://svn.gna.org/viewcvs/gnustep?rev=34174&view=rev
> Log:
> Improved the menu theming to support some common menu look variations.
>
> Fixed bug #34792 too.
>
> Modified:
>      libs/gui/trunk/ChangeLog
>      libs/gui/trunk/Headers/Additions/GNUstepGUI/GSTheme.h
>      libs/gui/trunk/Source/GSTheme.m
>      libs/gui/trunk/Source/GSThemeDrawing.m
>      libs/gui/trunk/Source/NSMenuItemCell.m

Hi Quentin,

I like the changes you made for menu theming. And would like to use 
these as a starting point for a bit of a general discussion on theming.

In this change you introduced a few new colour methods on GSTheme and 
commented on another place in NSMenuItemCell.m
// TODO: Make the color lookup simpler.

Now the way we make the colour lookup in GSTheme was decided by Richard 
some time ago. He wanted to have basic lookup methods for all the 
different theme operations to be similar and came up with the current 
interface for colour lookup. It is not that I like this interface that 
much, but we should try to stay consistent. For this reason I think we 
should use this as the official interface for GSTheme instead of 
introducing new methods as we go. I know I did so myself for example for 
toolbar colours.

I like your new themeControlState method. We should use this in all the 
other places where it is useful, for example in the method 
backgroundColor. This is actually the only place where you use one of 
your new colour methods outside of the GSTheme class, maybe we should 
just change the lookup key here to menuItemBackgroundColor and adjust 
all the themes?

Then there is this call in your code:
NSInterfaceStyleForKey(@"NSMenuInterfaceStyle", nil);
We have plenty of similar calls all over the place, still I think this 
is wrong. When ever possible we should pass in a responder as the second 
argument of this function call. Every view is allowed to have its 
specific interface style and at least the standard drawing code should 
respect this. Themes may handle this differently or just ignore that value.

And one final point. You us the method call [path setLineWidth: 0.0]; 
This isn't as well defined as it may seem. Most people think that this 
will select the smallest possible line width. At least with our current 
code this may not be true for the cairo backend. There is a long and 
ongoing discussion about zero line widths on the cairo mailing list.

Cheers
Fred