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

[Diego Biurrun]

On Thu, Dec 23, 2010 at 04:29:45PM +0100, Clément Bœsch wrote:
> 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:
> > > 
> > > @@ -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.

Feel free to change the rest of the file or any \ Doxygen syntax
you encounter.  I will at some point otherwise.

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

Whatever you prefer.  I just had a hunch we might have similar code in
other places, I did not check.

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

subtitle directories

BTW, no need to send a new patch for such nits, just change locally.

Diego