Hi Fred and Quentin,
On 2011-11-22, at 8:17 AM, Quentin Mathé wrote:
>
> 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.
Yes, agreed.
> 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?
I added a fix to the cairo backend a few months ago which will interpret 0 as a
thin line (I think previously I broke a game which was relying on that
behaviour.)
However, I recommend drawing a filled rectangle instead of using lines. In
general, anywhere you have lines in a user interface, they should probably be
filled rectangles. Filled rectangles scale up in a predictable way and are easy
to pixel-align (just make the origin and size integers.)
Eric_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@gnu.org
https://lists.gnu.org/mailman/listinfo/gnustep-dev
