[Freeciv-Dev] Re: [PATCH] Customizable default ruleset

[Mike Kaufman <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

On Sat, May 04, 2002 at 10:07:25AM +0200, Raimar Falke wrote:
> On Fri, May 03, 2002 at 06:51:29PM -0400, Baumans wrote:
> > This patch allows you to customize what your default ruleset is by adding a
> > 'tileset="whatever"' to the "[client]" section of your .civclientrc .
> 
> The idea of the patch is ok. The patch itself:

I too like the idea quite a bit, but...

> 
> > +char *d_tileset;
> 
> Does "d" for default? If so please rename it to default_tileset.
> 
> > +  d_tileset = 
> > strdup(secfile_lookup_str_default(&sf,"","%s.tileset",prefix));
> 
> Why strdup?

yes, why?

> 
> > +extern char *d_tileset;
> 
> This is wrong. You have to declare the variable in civclient.h

well, the extern anyway.

> 
> > +  if (d_tileset == NULL) {
> > +   printf("d_tileset null\n");
> 
> Replace printf with freelog().

yep,

I wouldn't apply this in this state. This is a fine idea (one which
would benefit many), but it's more of a hack right now.

1. Should this option in particular and some options in general _not_
show up in the "Local Options" dialogs?
2. This should be extensible. add a COT_STR to the swtich and put the
var in GEN_OPTIONS. If the answer to #1 is yes, then another bool should 
be added to the options array that specifies whether a user can set the
option in the GUI. If the answer to #1 is no, then all the GUI's should
be updated to accomodate your new option "Tileset on startup"
3. your tilespec.c patch needs to be cleaned up.
4. I had a very good reason at the time for moving load_options(), but
for the life of me, I can't think of why now. Have you tested loading up
saved worklists with your patch? Either that is now screwed up or I was on
crack.

Please comment on these, and when you decide to send in your next
revision, send it to bugs so it doesn't get lost.

-mike