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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Aug 18, 2001 at 10:28:10PM -0400, David Weisgerber wrote:
> Am Samstag, 18. August 2001 15:04 schrieben Sie:
> > David Weisgerber wrote:
> > > Am Samstag, 18. August 2001 10:25 schrieben Sie:
> >
> > <snip sound.[ch] files for adding sound support>
> >
> > > > I'm also not quite sure what strcoll() is supposed to be doing.  Could
> > > > you add a comment on that one?
> > >
> > > The strcoll() function compares the two strings s1 and s2.
> > >        It returns an integer less than, equal to, or greater than
> > >        zero  if  s1  is  found, respectively, to be less than, to
> > >        match, or be greater than s2.  The comparison is based  on
> > >        strings  interpreted as appropriate for the program's cur­
> > >        rent locale for category LC_COLLATE.  (See  setlocale(3)).
> >
> > Hmmm.  It seems odd that sound code should be checking translated
> > strings.  There should be a comment as to why it's better to do so.
> 
> I'll change it to strcmp() ...

Better.

> > > > You should probably check the return value of the system() call that
> > > > plays the sound.
> > >
> > > and what sould I do with the return value ??? That doesn't bring
> > > something as you can see from the discussion. Either play doesn't fail
> > > than there is a sound play or play fails and it is like it was until this
> > > patch came.
> >
> > My man page says that if the return value is 127 (and errno is set
> > appropriately) it means the program failed to execute.  An error message
> > (to freelog or possibly even stderr) should be sent in this case.
> > Otherwise sound will silently fail!
> >
> > Of course, even if it fails we're no worse off than we are now, but
> > that's no reason not to do things properly...
> 
> I think that play and the bash writes a good understandable error message to 
> stdout / stderr so we don't have to do this.

What if the freeciv log is redirected with "-l" and the user will only
look at it and not the normal std/stderr?

> > > > 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!
> > >
> > > No, I don't think so. First point: Naming the units in the sound file is
> > > good for mod makers.
> >
> > Ugh.
> 
> Why not ?!

The question is still: Why? Why include the names at all? Why is this
better for mod makers?

> > > Second point: Maybe you define a specfile which doesn't play wave.
> > > Instead it plays MP3s or AC3s than you have to use another program.
> >
> > Hmmm, good point.  How, then, will you deal with different platforms???
> > Can we have a separate config file that specifies different executables
> > to be used for different file types?  This could then be changed between
> > platforms without having to maintain separate versions of the main sound
> > spec file.  (This sounds like something for later patch to accomplish.)
> 
> Maybe we have to add a play command for other platforms ?!

As a configure option or maybe also as a command line argument.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "> WHY?! Isn't it better to put $(shell cat cscope.files) on the list of
  I only have a yellow belt in makefile kungfu.  These fancy gnu make things
  are relatively new to some of us..."
    -- Mark Frazer to Vassilii Khachaturov in linux-kernel