Hi Fred, Sorry for the late reply.
On Thu, Nov 17, 2011 at 10:12 AM, Fred Kiefer <fredkie...@gmx.de> wrote: > 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<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. > Sounds like a good idea. > 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 must admit I don't know how to edit the color lists easily. Thematic only knows about some precise colors but cannot edit arbitrary color lists. For example, how can I make +[NSColor controlBackgroundColor] uses +[NSColor greenColor] ? Can I do that with the built-in color picker ? We also seem to have no tools to convert binary encoded NSColorList into the ASCII format and vice-versa. About the current API… The GSTheme color lookup API includes a method -colorNamed:state:, but the state is almost never used based on a quick grep over the Gui source code (only once in NSMenuItemCell.m). So I would remove the last argument and encode the state within the color name. NSColor API names the color like that, this approach would avoid to introduce two color naming schemes (GTheme vs NSColor) for the themable colors. The state arguments makes sense with the tile lookup methods, but for the themable colors I'm not convinced. Having color methods declared in GSTheme (such as the toolbar one) as syntactic sugar over -colorNamed: doesn't go against the current color lookup. It might even be a good way to clearly document the colors that can be customized. > 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. Yes, it could even be used in other classes such as -[NSButtonCell drawBezelWithFrame:inView:]. > 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? > For -[NSMenuItemCell backgroundColor], I would suggest to remove: color = [[GSTheme theme] colorNamed: @"NSMenuItem" state: state]; if (color == nil) and use either: if ((state == GSThemeHighlightedState) || (state == GSThemeSelectedState)) { color = [NSColor selectedMenuItemColor]; } else { color = [[GSTheme theme] menuItemBackgroundColor]; } or if ((state == GSThemeHighlightedState) || (state == GSThemeSelectedState)) { color = [NSColor selectedMenuItemColor]; } else { color = [[GSTheme theme] colorNamed: @"menuItemBackgroundColor"]; } Should I use this last solution? 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. > I'll remove this call in -menuSeparatorColor, it isn't needed now that the Windows theme uses native menus as Gregory pointed it out in the bug report #34792. I agree about passing a responder every time to the GSTheme methods where it makes sense, and limiting the number of places where we use NSInterfaceStyleForKey(). Most uses of NSInterfaceStyleForKey() should be in GSTheme subclasses I think. > 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. > ok, I wasn't aware of this issue. Which width should I use to get the thinnest possible line or something close? 0.5 or 0.1 for example? Cheers, Quentin.
_______________________________________________ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
