[Freeciv-Dev] Re: Sound patches to be added (diffs'll come later)

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

David Weisgerber wrote:
> 
> Here are the newest and finest goodies from me. They now support parsing a
> soundspec file located in /usr/local/share/freeciv/ but have a look at the
> sources yourself I think you'll understand how the whole thing works. There
> is also a pretty option for modpackers so they don't have to worry I think.
> 
> With this sound patch there is a soundpack from me included. Note that it
> isn't really finished yet.

Some notes:

In sound.h, the unit sounds should be a separate enumeration from the
others.

When you "copy" the sound config file, you need to check for errors. 
Similarly, your sprintf's should be snprintf's (or
mysnprintf/fc_snprintf?).

I don't know how paths work in FreeCiv, but it seems inappropriate to
hardcode /usr/local/share/freeciv/.  Can't you just use the freeciv
"data" directory?

Your function braces aren't standard FreeCiv-styple.  The opening brace
should be on its own line.  Some of your other braces are off, too.  The
"if (a) if (b) {" line is quite odd.

What is the 52 magic number in sound.c?  I can't even figure out what
that's used for.

The "global" variables declared in sound.c should be declared static.

I'm also not quite sure what strcoll() is supposed to be doing.  Could
you add a comment on that one?

Rather than checking if the config file has been parsed each time a
sound is played, why not have parse_sound_config be called upon game
initialization?

Why is there a printf in parse_sound_config?  I assume you left this in
by accident?

You should probably check the return value of the system() call that
plays the sound.

Naming all of the units shouldn't be done in the sound file.  If you
want to do it outside of the sources, it should ultimately be put into
some other spec file.  In fact, a lot of the options you've put into the
sound spec file are inappropriate - for instance, defining the play
command in this file means anyone using an incompatible platform must
use a different sound spec file!

Listing all of the unused sounds in the spec file as '<soundname> = ""'
is IMO ugly and inelegant.  Can't you just assume there's no sound if
the sound isn't specified?

Many of these things can be taken care of later, of course.

jason