----- Douglas Torrance <dtorra...@monmouthcollege.edu> a écrit :
> On 11/05/2013 05:58 PM, Christophe wrote:
> > Although I do like the idea brought by the patch, there are a few things in
> > this implementation that I would suggest to do differently:
>
> Absolutely! I'm a mathematician, not a programmer, so I appreciate all
> the input I can get!
>
> >> +WMTheme* W_GetTheme(WMScreen *scr);
> > I think this should appear in the visible API of the toolkit.
> > A theme is something internal to the toolkit, the application using it
> > should not see it
> >
> I'm a little confused here. Are you suggesting that themes should be
> visible or not? Also, what determines this? Is it WINGs.h vs.
> WINGsP.h? I couldn't find any documentation on what goes where, so I
> just tried to follow the example of what was already there.
It not really documented yet (although I have some patches ideas to make it
more clear in the future), but the info you need is:
- WINGs.h is the public API of the toolkit, which should be touched with care
for compatibility and consistency;
- WINGsP.h is an internal header to WINGs (P=Private)
On the topic of what should be private or public, my point of view is:
- the theme structure itself should be public, because applications often need
to create custom widgets so they need theses colors for consistency (that is
currently done through the not so great white/black/gray/... that you pointed
out;
- the theme loading and handling should be private, because it is the
responsibility of the toolkit to take care of this, not of the application (and
it also reduces the risk of breaking compatibility with old apps)
> >> +typedef struct WMTheme {
> >> + WMColor *background;
> >> + WMColor *foreground;
> >> + WMColor *shadow;
> >> + WMColor *highlight;
> >> + WMColor *unselectedTabBackground;
> >> + WMColor *unselectedTabHighlight;
> >> +} WMTheme;
> > I would suggest to rethink the names and organisation of this structure,
> > because there will need more, for example: background is for what? the
> > standard bg of the window I suppose, but text fields are likely to have a
> > different background, aren't they? and the same happens for the foreground,
> > no?
> >
>
> Absolutely. As it stands now, the patch is mostly just a quick hack to
> replace what WINGs historically has called "white", "black", "gray", and
> "darkGray". I chose names that represented what these colors (most of
> the time) are used for. There are definitely major issues with these
> choices. (White is a big problem. It is used for button highlights,
> selected text, and textfield backgrounds, all of which are very different.)
>
> The ultimate solution I think would be to gut the hardcoded names
> entirely and create more descriptive names. I'm beginning to work on
> this, but it will take some time. :)
A possible solution to help find the list of used colours would be to add the
attribute deprecated to the variables in the current structure, that would
point you all current usage, and then with a little bit of work you'd get a
good base for choosing names
> >> +{
> >> + WMTheme *theme;
> >> + WMUserDefaults *defaults;
> >> +
> >> + char *background;
> >> + char *foreground;
> >> + char *shadow;
> >> + char *highlight;
> >> + char *unselectedTabBackground;
> >> + char *unselectedTabHighlight;
> >> +
> >> + defaults = WMGetStandardUserDefaults();
> >> +
> >> + if (defaults) {
> >> + background = WMGetUDStringForKey(defaults, "Background");
> >> [...]
> >> +
> >> + theme->background = WMCreateNamedColor(scr,background,True);
> >> + if (!theme->background)
> >> + theme->background = WMCreateRGBColor(scr, 0xaeba, 0xaaaa,
> >> 0xaeba, True);
> > I'm not sure this behaves as good as you expect.
> >
> What do you mean?
Maybe I should not be so cryptic... There are 2 things in this code:
- if 'defaults' happens to be NULL, then 'background' will be uninitialised,
which WMCreateNamedColor may not like that much; yet a look at
WMGetStandardUserDefaults shows that it cannot be NULL so the code should be
simplified to avoid a potential compiler warning on uninitialized variable
- if the key "Background" does not exist (yet), then background will be NULL,
which is also not so great to call WMCreateNamedColor with
There are also a few more details a lot less important:
- today I have not tried hard to understand where the settings were actually
saved, but that a point I care about, because I always feel picky about keeping
data properly organised
- some colours should not have a hard-coded default but use a formula, for
example the light+dark greys used make that 3D like effect: if you specify an
orange colour for the background (wouldn't that be lovely?) you'd expect the 3D
effect to use automatically light-orange and dark-orange, it's just a matter of
choosing the proper coefficients to get the calculated colours to fall back to
the current defaults when called with the default bg as input (but I guess
you'll be better than me on formulas ;-) )
>
> Thanks again. I really appreciate it!
> Doug
--
To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.
