Re: GNUstep themes [Was: Obscure timing issue]

[Richard Frith-Macdonald]

On 12 Dec 2008, at 00:20, Fred Kiefer wrote:
> What I am suggesting is to separate these two concepts also in two  
> classes.
> What I don't see is how this would duplicate code, as these classes  
> will
> inherit from another and that way share most of the code.

Ah ... sounds like your design is changing ... now instead of two  
classes we have three ... a common theme class and two subclasses ...  
more on that later.


> As you write, we don't have much methods in that class that already
> support full theming. Lets look at one of those:
> - (void) drawButton: (NSRect)frame
>                 in: (NSCell*)cell
>               view: (NSView*)view
>              style: (int)style
>              state: (GSThemeControlState)state
> {
>  GSDrawTiles    *tiles = nil;
>  NSColor        *color = nil;
>
>  if (state == GSThemeNormalState)
>    {
>      tiles = [self tilesNamed: @"NSButtonNormal" cache: YES];
>      color = [NSColor controlBackgroundColor];
>    }
>  else if (state == GSThemeHighlightedState)
>    {
>      tiles = [self tilesNamed: @"NSButtonHighlighted" cache: YES];
>      color = [NSColor selectedControlColor];
>    }
>  else if (state == GSThemeSelectedState)
>    {
>      tiles = [self tilesNamed: @"NSButtonPushed" cache: YES];
>      color = [NSColor selectedControlColor];
>    }
>
>  if (tiles == nil)
>    {
>      switch (style)
>        {
>           case NSRoundRectBezelStyle:
>           case NSTexturedRoundBezelStyle:
>           case NSRoundedBezelStyle:
>             [self drawRoundBezel: frame withColor: color];
>             break;
>           case NSTexturedSquareBezelStyle:
>             frame = NSInsetRect(frame, 0, 1);
>           case NSSmallSquareBezelStyle:
>           case NSRegularSquareBezelStyle:
>           case NSShadowlessSquareBezelStyle:
>             [color set];
>             NSRectFill(frame);
>             [[NSColor controlShadowColor] set];
>             NSFrameRectWithWidth(frame, 1);
>             break;
>           case NSThickSquareBezelStyle:
>             [color set];
>             NSRectFill(frame);
>             [[NSColor controlShadowColor] set];
>             NSFrameRectWithWidth(frame, 1.5);
>             break;
>           case NSThickerSquareBezelStyle:
>             [color set];
>             NSRectFill(frame);
>             [[NSColor controlShadowColor] set];
>             NSFrameRectWithWidth(frame, 2);
>             break;
>           case NSCircularBezelStyle:
>             frame = NSInsetRect(frame, 3, 3);
>           case NSHelpButtonBezelStyle:
>             [self drawCircularBezel: frame withColor: color];
>             break;
>           case NSDisclosureBezelStyle:
>           case NSRoundedDisclosureBezelStyle:
>           case NSRecessedBezelStyle:
>             // FIXME
>             break;
>           default:
>             [color set];
>             NSRectFill(frame);
>
> 	    if (state == GSThemeNormalState || state ==  
> GSThemeHighlightedState)
>               {
>                 [self drawButton: frame withClip: NSZeroRect];
>               }
>             else if (state == GSThemeSelectedState)
>               {
>                 [self drawGrayBezel: frame withClip: NSZeroRect];
>               }
>         }
>    }
>  else
>    {
>      /* Use tiles to draw button border with central part filled  
> with color
>       */
>      [self fillRect: frame
>            withTiles: tiles
>           background: color
>            fillStyle: GSThemeFillStyleNone];
>    }
> }
>
> Half the code does the old drawing, the other half is about tiles.  
> They
> have nothing in common and the method would surely be easier to read,
> when we separated them out. Which is exactly what I propose.

I don't see separating into different classes as making things  
necessarily easier to read.  Essentially there's clear separation  
already, as the pseudo code is:

draw_button
  if (we want to draw buttons via tiling)
     then use tiling
  else
     use original drawing code


Now, you could have a hierarchy with one class doing the original  
drawing code, in which case the subclass in the hierarchy would  
contain the tiling support and your pseudo code would look like:

draw_button
  if (we want to draw buttons via tiling)
     then use tiling
  else
     [super draw_button]

That sort of organisation seems ok to me (it does lose the advantage  
of having all the code close together, but the subclass/superclass  
relationship is familiar, so it's easy to find the superclass code  
when you want to look at it) I see no particular advantage to it, but  
no particular harm either.  However you have said that you should have  
two separate classes (now inheriting from a common superclass) with  
the different drawing code in each:

Class1
draw_button
  use original drawing code

Class2
draw_button
  use tiling

The problem with this is that if someone is working on producing a new  
theme, and they inherit from Class1, then they can't using tiling  
anywhere in their theme unless they duplicate the particular bit of  
tiling code they want.  If they inherit from Class2 then they can't  
use the original drawing code anywhere to maintain a usable theme  
while doing development, unless they duplicate code.


Going back to your desire to separate the two paths (perhaps you  
simply think the method code is too long ... I've noticed that I  
prefer to keep code all in one place more than most people), you could  
of course have two private methods:

draw_button
  if (we want to draw buttons via tiling)
     _draw_button_tiled
  else
     _draw_button_original

That way you could have a file structure with the tiled methods in a  
category in one file, and the untiled methods in a category in another  
file, and the common code in a third file.

> > > As soon as you switch your theme setting to TiledTheme.theme (or
> > > whatever we call it) you have similar code as now, that will just  
> > > fall
> > > back to the non-tiled super implementation, when no tile is found.
> >
> > That really does sound like you are advocating code duplication ...  
> > with
> > a theme which can't be used for anything but drawing the traditional
> > gnustep look, and another theme which follows the current design of
> > drawing the traditional look but allowing people to use image  
> > tiling and
> > other mechanisms to create new looks without (or with) any  
> > programming.
> > As far as I can see that has three problems ... extra work to do it,
> > code duplication, and a tendency to work against development of a  
> > real
> > theme engine since a lot of people would just use the traditional
> > look/feel and thus not test the theme engine at all.
>
> As these implementation would inherit, the tile theme could just fall
> back to the old drawing code, when there are no tiles.

Here you are contradicting yourself ...
Earlier in this email you said that the two theme classes would  
inherit from another, and in this and the previous email you said that  
one theme class would use the old drawing code and one use tiling.  If  
that's the case then (unless you have the old drawing code both in  
your non-tiling theme and in the common superclass), your tiling code  
can't fall back to the drawing code.  Alternatively, if the common  
superclass contains the old drawing code then your non-tiling theme  
code does nothing.

Trying to be constructive, I'd like to go back to what I said in my  
previous email about the natural organisation of GSTheme and the way  
it already has stuff split out into categories which could in turn be  
in separate files.  If we look at how that could  reasonably be made  
into separate classes too:
There is the theme loading mechanism .. this really is the heart of  
GSTheme and needs to be part of the base class.
There is the low level drawing code (tiling, drawing a box etc) which  
naturally falls into its own category.  This code is unlikely to be  
overridden by people wanting to subclass GSTheme, so it could be in a  
separate GSThemeDrawing class rather then a category (I don't see any  
advantage to spending time doing that, over simply putting it in a  
separate file).
Finally there are the control drawing methods (just NSButton so far)  
which could be split into a subclass and superclass simply to get the  
tiled/untiled drawing code into separate methods.
So you might have a class hierarchy:
NSObject->GSThemeLoading -> GSCutDownTheme ->GSTheme
and perhaps
NSObject->GSThemeDrawing

Then if someone wanted to write a code-based theme and wanted to  
ensure that people could not extend their theme using tiled images,  
they could inherit from GSCutDownTheme rather than GSTheme.

I'm not advocating that, but it's an option.

> As for the problems:
> - extra work -> I am offering to do it

OK ... but even if you do it, that's still time/energy spent doing  
something (like spending time on emails) which doesn't advance the  
actual development of theming :-(

> - code duplication -> I don't see it
> - less testing of theme engine -> This is quite possible, but then
> again, I don't think the current tiles theme gets used that much.

The code really doesn't get used at all because we haven't implemented  
the theme drawing routines yet. We need routines to draw all the  
common controls before we can do theming beyond controlling colors and  
menu layouts.

> We
> really should rather focus on providing a clear interface for themes,
> with that in place a full tiling theme will be as easy as any other  
> theme.

EXACTLY ... the thing to do is add those new drawing methods in the  
GSTheme class and get the controls in the gui to use them!
By far the most efficient way to do that and get it right is to:
1. implement each drawing method by copying the original code from the  
gui
2. test that each drawing method actually makes sense as a theme by  
adding tiling code drawn from Camaelon
3. make the new drawing method part of the public API.

You need (1) to ensure that the gui just keeps on working
You need (2) to check that the new drawing api is reasonably general  
and that you are less likely to need to change it later to add  
facilities that theme coders want.


> > > I
> > > don't understand, why this would force people to write code that  
> > > don't
> > > have to do it currently.
> >
> > I thought you meant code separation rather than code  
> > duplication ... if
> > things were separated then people would need to write code because
> > features they needed were not in the theme they were trying to  
> > use.  If
> > code ids duplicated, that particular issue goes away.
>
> I really don't understand this paragraph. As I explained below, you
> either inherit from the tile theme or the standard drawing theme and  
> as
> they inherit from each other I don't see how you could miss a feature.
> Other then inheriting from the standard drawing theme, but requiring
> tile and here a switch to the other super class resolves the issue.

Sounds like you are getting really fuzzy here ... you said these two  
things had separate drawing, but now they somehow 'inherit from each  
other' which is of course impossible.
The simplest solution is the current code.
Second simplest would be a superclass/subclass arrangement where the  
superclass uses original drawing code and the subclass offers tiling  
as an optional replacement.
Most complex/messy would be a common superclass with both styles of  
drawing but in separate methods, then separate subclasses selecting  
which of the methods from the common superclass they want to use.
I guess there are other even more esoteric options using delegation or  
multiple inheritance via manipulation of the runtime but we really  
don't want to go there: we want a simple, easy to understand  
organisation.



> > > - One non-standard tiled theme class, also delivered with gui  
> > > (perhaps
> > > as a bundle to demonstrate how to write theme bundles?). This will  
> > > be a
> > > subclass of the first and fall back to super calls when no tiles are
> > > found.
> >
> > This is what the existing theme engine already provides ... but it's
> > fundamentally not intended as a example of how to write theme  
> > bundles,
> > rather it's intended as a mechanism for non-technical people to be  
> > able
> > to design/develop themes *without having to write anything*!  This is
> > the key concept of theming ... to put it in the hands of the designer
> > rather than the programmer.
>
> This sounds as if you are against any other themes that want to do  
> more than
> just provide tiles.

> With my propose people would still have the same functionality to  
> write
> themes without any coding.

Nope ... if it sounds that way it's only because I'm emphasising a key  
feature that seems to have been forgotten.  Don't forget that the non- 
coding option also already includes changing system colors by  
providing new color lists, changing behavior etc using different  
interface styles (like horizontal/vertical menus for instance),  
changing particular images in the gui.  In future it could also  
contain other non-coding options we think of.

The key thing is that if someone wants to develop a theme using these  
options (rather than by writing code), then the options need to be  
available, so a philosophy of having separate branches of theming  
based on separate theme classes is a big problem.
What I want to see is a single fully featured class that theme  
developers can depend on.  It doesn't matter if features are all coded  
in one class or inherited from subclasses, but the person designing  
the theme must be be to depend on a single well defined and complete  
set of facilities provided by the gui library.

> > > - Multiple theme provided by other people either subclasses of the
> > > standard or the non-standard theme. (OK the later might get a little
> > > harder with a bundle)
> >
> > Again, this does not go far enough.  With the current design we allow
> > for these things but we also allow for theme bundles with are *not*
> > subclasses and do not contain code. Such bundles are pure data,
> > consisting images and defaults settings etc controlling how the  
> > standard
> > theme engine draws things (currently these are the only themes  
> > supported
> > by Thematic.app, but it would be nice to add support for subclassed
> > themes too).
> > However, they depend upon the facilities in the theme engine ... so
> > having a standard theme which did not support this would mean that  
> > we'd
> > need extra code to automatically switch to the other packaged theme  
> > when
> > necessary.
>
> No this is the only real problem I see, the one I mentioned in the  
> first
> paragraph of this mail. And now is that time to talk about it.
> Currently when a theme gets loaded that doesn't provide a principle
> class we create an instance of GSTheme and return that (see
> loadThemeNamed:). Wouldn't it be just as easy to create an instance of
> GSTiledTheme and use this?

Sure ... then GSTiledTheme would be our default public theme class as  
long as it includes all the facilities rather than being a cut-down  
version... just under a new name.  My concern is that theme developers  
should be able to depend on having all facilities available... so they  
should have a single theme class to work with ... doesn't really  
matter what it's called.

> > So, while none of the problems introduced by having a cut-down  
> > standard
> > theme would be huge, they do amount to a compelling reason not to  
> > do it,
> > given that (fro the point of view of theming at least) it has no
> > advantages to counter them.
>
> I don't follow you here. But there is one other thing that really
> surprises me in this debate: Nobody else seems to be interested in it.
> Perhaps it is just the mail subject that keeps people away. Or themes
> aren't that hot as the used to be?

I doubt that it's the subject ... people mostly do glance at content  
as most modern mail programs download the entire message routinely.

Maybe it will help to summarise my key points:

DESIGN
a. provide a single fully functional theme class for people to use  
rather than confusing them with multiple incomplete options
b. the class must allow for theme developer to produce good themes  
without coding
c. the methods in the class must be easy to override to make theme  
code as easy to add as possible
d. keep them implementation as simple as possible .

ACTION ... the thing to do is add those new drawing methods in the  
GSTheme class and get the controls in the gui to use them!
By far the most efficient way to do that and get it right is to:
1. implement each drawing method by copying the original code from the  
gui
2. test that each drawing method actually makes sense as a theme by  
adding tiling code drawn from Camaelon
3. make the new drawing method part of the public API.

You need (1) to ensure that the gui just keeps on working
You need (2) to check that the new drawing api is reasonably general  
and that you are less likely to need to change it later to add  
facilities that theme coders want (addressing design point 'c')