[MPlayer-dev-eng] [PATCH] Subtitles directories

[Clément Bœsch]

On Wed, Dec 22, 2010 at 01:06:11PM +0100, Diego Biurrun wrote:
> On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
> > 
> > Well, since I still don't have any feedback, I tried to split it more. So
> > There is now 3 patches. I wonder how I can split it more. Please review
> > this so I can move on :)
> > 
> > --- a/sub/subreader.c
> > +++ b/sub/subreader.c
> > @@ -1889,7 +1890,7 @@ static int compare_sub_priority(const void *a, const void *b)
> >  
> > -char** sub_filenames(const char* path, char *fname)
> > +static char** sub_filenames(const char* path, const char *fname)
> 
> Please fix the * placement to K&R while you're changing this line anyway.
> 

Fixed. Btw, I didn't feel the need to since the prototype is changed in a
later commit.

> > @@ -2070,6 +2071,30 @@ char** sub_filenames(const char* path, char *fname)
> >  
> > +/**
> > + * \brief Load all subtitles matching the subtitle filename
> > + * \param fname Path to subtitle filename
> > + * \param fps FPS parameter for the add subtitle function
> > + * \param add_f Add subtitle function to call for each sub
> 
> Please use @ syntax with Doxygen.  I'm afraid we have a mix of both in
> MPlayer, but @ is more common and easier to grep for :)
> 

Changed. I also prefer the '@' syntax, but it was just to be consistent
with the rest of the file. New syntax is now in use in the 3 commits.

> > --- a/DOCS/man/en/mplayer.1
> > +++ b/DOCS/man/en/mplayer.1
> > @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250.
> >  .
> >  .TP
> > +.B \-subdirs <dirname1,dirname2,...>
> > +Specify extra subtitles directories to track in the media directory.
> > +.sp 1
> > +.I EXAMPLE:
> > +Assuming that /path/\:to/\:movie/\:movie.avi is played and \-subdirs
> > +sub,subtitles,/tmp/subs is specified, MPlayer searches for subtitles files in these
> 
> subtitle files
> 

Fixed.

> And please break this line earlier.
> 

Done.

> > --- a/sub/subreader.c
> > +++ b/sub/subreader.c
> > @@ -2123,6 +2123,18 @@ void load_subtitles(const char *fname, int fps, void add_f(char *, float, int))
> >  
> > +    // Load subtitles in dirs specified by subdirs option
> > +    if (sub_dirs)
> > +        for (i = 0; sub_dirs[i]; i++)
> > +#if HAVE_DOS_PATHS
> > +            if (sub_dirs[i][1] == ':')
> > +#else
> > +            if (sub_dirs[i][0] == '/')
> > +#endif
> 
> Maybe this can be factorized into a global PATH_SEP #define or similar?
> I suspect we have something similar in other places...
> 

The condition does not check the same character, but I since this
condition may not be explicit about what it does, I could make an helper
in path.c, something like:

  int mp_path_is_absolute(const char *path)
  {
  #if HAVE_DOS_PATHS
      return path[0] && path[1] == ':';
  #else
      return path[0] == '/';
  #endif
  }

Would you prefer this?

I can't find any other place where it can be used atm so...

-- 
Clément B.
Not sent from a jesusPhone.