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

["esr@xxxxxxxxxxx via RT" <rt@xxxxxxxxxxxxxx>]

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.

Well...my thinking was that if I committed the data sirectory, then
not requiring the specfile change would mean people could easily swap
the one C patch in and out for testing.
 
> Instead you should change the capstring so that old tilesets will just fail.

Done. It's tilespec3 now.
 
> > -  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?).

Yup.  I should have followed through on that the first time through.
 
> >    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.

Done.

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

Yes.  I'm a little puzzled that this wasn't done sooner; the registry.c code
indicates that all that's needed here is a sipmple free().

> > -  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).

Done.

> > +  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).

I must be missing something.  I reread the style guide without finding
anything that looked relevant to this line.
 
> > +    /* 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.

Done.
 
> >  /**********************************************************************
> > -  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).

Doesn't seem like a biggie.  What problem case *can't* be solved by putting a 
directory with overriding sprites at the end of the list?

> > +    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?

No.  Unless the directory name happens to end in a graphics extention
and match a spritename.  Unlikely...
 
> > +    strcpy(spritename, entry->d_name);
> 
> Someone will have to compile freeciv on HURD some day, so all these 
> little shortcuts will break :-).

Eh?  This is straight POSIX, I think.  What looks breakable to you?

> > +         /* don't free old sprite, could be many tags pointing to it */
> 
> Except that there can't (anymore), right?

Right...but it also looks to me like the only time you actually want to
free hashes is just before a reread.

> > +     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.

This is a pretty important patch. :-)  I figured I'd just replatch that line
later if I did anything more significant

> > +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).

I didn't edit very carefully because I was already planning to write a much 
more detailed HOWTO.  That's been done.
 
> 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.

I wasn't planning to remove the old files in the first patch.  Time
enough for that once this has been tested.

I'm not sure I understabd the issue with Makefile.am files.  Since
the graphics files have no dependency relationships that will show
up in a makre production, why do they need to appear in an automake file?
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>