[Freeciv-Dev] Re: (PR#2922) Revised version of data-reorganizattion patch

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

Eric S. Raymond via RT wrote:
> This revised version doesn't require top-level specfiles to be patched
> in order to work.  See the third FIXME comment for discussion

But it does require that the tileset graphics be updated.  Trying to 
preserve backward-compatability without really doing so isn't very 
helpful, and in this case it prevents compatability checking.

Instead you should change the capstring so that old tilesets will just fail.

> -  char *file_capstr = secfile_lookup_str(file, "%s.options", which);
> +  char *file_capstr = secfile_lookup_str(file, "%s.options", "tilespec");

No %s is necessary here anymore, just lookup "tilespec.options" (right?).

>    if (!has_capabilities(us_capstr, file_capstr)) {
> -    freelog(LOG_FATAL, _("%s file appears incompatible:"), which);
> +    freelog(LOG_FATAL, _("tilespec file appears incompatible:"));
>      freelog(LOG_FATAL, _("file: \"%s\""), filename);
>      freelog(LOG_FATAL, _("file options: %s"), file_capstr);
>      freelog(LOG_FATAL, _("supported options: %s"), us_capstr);
>      exit(EXIT_FAILURE);

As long as we're changing one of these strings, can you merge it all 
into one single log?  i.e.

freelog(LOG_FATAL, _(".....\n"
                      ".....\n"
                      ".....\n"
                      "....."), ...);

this just seems better to me (mostly for translators, since they get a 
full sentence instead of random phrases to translate).

>    if (!has_capabilities(file_capstr, us_capstr)) {
> -    freelog(LOG_FATAL, _("%s file claims required option(s)"
> -                      " which we don't support:"), which);
> +    freelog(LOG_FATAL, _("tilespec file claims required option(s)"
> +                      " which we don't support:"));
>      freelog(LOG_FATAL, _("file: \"%s\""), filename);
>      freelog(LOG_FATAL, _("file options: %s"), file_capstr);
>      freelog(LOG_FATAL, _("supported options: %s"), us_capstr);

Same here.

> @@ -265,7 +259,7 @@
>      free(minimap_intro_filename);
>      minimap_intro_filename = NULL;
>    }
> -  /* FIXME: free spec_filenames */
> +  /* FIXME: free sprite_dirs */
>  }

Can this be FIXed?

> -  spec_filenames = secfile_lookup_str_vec(file, &num_spec_files,
> +  /* FIXME: this should look up in tilespec.directories */
> +  sprite_dirs = secfile_lookup_str_vec(file, &num_sprite_dirs,
>                                         "tilespec.files");

As I said above, preserving compatability of the .tilespec file doesn't 
gain much if the tileset itself won't work.  It allows a script to more 
easily update the tileset, but is much worse for users who may try to 
use old tilesets.

So, go ahead and FIX this (and don't forget to change the capstring).

> +  for(i=0; i<num_sprite_dirs; i++) {

Please follow the style guide (doc/CodingStyle).  If you end up 
committing this patch yourself you need to follow it.  If someone else 
commits it they will make sure it is 'correct'.

This may seem rather anal-retentive of us.  But the more I follow the 
consistent style guidelines the more I like having consistent style 
(other projects I'm in use different style, which is fine too).

> +    /* FIXME: remove this kluge!
> +     * The clean way to do this is to call
> +     * sprite_dirs[i] = mystrdup(datafilename_required(sprite_dirs[i]));
> +     * but we do things in a slihghtly grubbier way here so as not to have
> +     * to change the top-level specfiles.  Eventually, we want to lop
> +     * off the .spec ending and prefix with graphics *there*.
> +     */
> +    char dirname[PATH_MAX];
> +    strcpy(dirname, "graphics/");
> +    strcat(dirname, sprite_dirs[i]);
> +    if (strcmp(dirname + strlen(dirname) - 5, ".spec") == 0)
> +     dirname[strlen(dirname) - 5] = '\0';
> +    sprite_dirs[i] = mystrdup(datafilename_required(dirname));
> +    freelog(LOG_DEBUG, "spec directory %s", sprite_dirs[i]);
>    }

Err, yeah, so you can FIX this too.

>  /**********************************************************************
> -  Load one specfile and specified xpm file; splits xpm into tiles,
> -  and save sprites in sprite_hash.
> +  Load sprites from a specified image directory, 
> +  and save them in sprite_hash.
>  ***********************************************************************/
> -static void tilespec_load_one(const char *spec_filename)
> +static void tilespec_load_one(const char *image_dir)

The problem here - and it's a big one - is that it is impossible to 
include *part*, but not all, of a directory.  With the grid-png system 
you can take just part of an exiting PNG by writing your own specfile 
for it.  But under your system this isn't possible, although it is still 
possible to have one sprite be overwritten by another (just make sure 
the importance of ordering the directories is clear).

> +    if (strcmp(entry->d_name, ".")==0 || strcmp(entry->d_name, "..")==0)
> +     continue;
> +    if (!strchr(entry->d_name, '.')) /* skip artists file and README */
> +      continue;

What about other subdirectories?  Will they present problems?

> +    strcpy(spritename, entry->d_name);

Someone will have to compile freeciv on HURD some day, so all these 
little shortcuts will break :-).

> +           /* don't free old sprite, could be many tags pointing to it */

Except that there can't (anymore), right?

> +     Eric S. Raymond <esr@xxxxxxxxxxx> (new data file organization)

Heh.  PEOPLE is usually updated at release time, not with each new patch 
:-).  Otherwise the commits would really add up.

> +The purpose of the 'tilespec' file and related directories is to allow
> +choice of graphics flexible and not hard-coded into Freeciv, and to
> +allow add-ons to conveniently provide additional graphics.

This sounds wrong.

> -To work properly tags should correspond to appropriately sized
> -graphics.  (The basic size may vary, as specified in the top-level
> -tilespec file, but the individual tiles should be consistent with
> -those sizes and/or the usage of those graphics.)

Hmm, this part still applies (although it is badly written).

This looks like a reasonable patch.  But note that its application needs 
to be associated with the addition of 1454 new PNG files, lots of 
Makefile.am files (don't forget those!), lots of removed .PNG and .spec 
files, and some changed .tilespec files.

jason