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

[Reimar Döffinger]

On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
> > > @@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname)
> > >  		    }
> > >  		    if (!prio) {
> > >  			// doesn't contain the movie name
> > > -			// don't try in the mplayer subtitle directory
> > > -			if ((j == 0) && (sub_match_fuzziness >= 2)) {
> > > +			if (sub_match_fuzziness >= 2) {
> > 
> > Huh? Where did handling of this end up? Loading any subtitle file anywhere in the
> > path doesn't sound to me like it would ever be desireable.
> > 
> 
> I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on
> ~/.mplayer/sub directory. subdirs patch updates a little the documentation
> about this point too.

Sure it doesn't make more sense to only apply it to the movie
directory and nothing else at all?
Doesn't make sense to have a path where there's only the subtitle(s)
for one single movie - and in all other cases it will end up
with wrong subtitles.

> > > +    strcpy(path + plen, dir);
> > 
> > av_strlcpy might make it more obvious.
> > 
> 
> Why? It would either add a second strlen call or a new variable. Maybe you
> meant strlcat; but even so, the problem is the same. Or maybe I missed
> something.

Yes I meant strlcat. Doesn't matter much, it's just that this is
kind of a reimplementation of strcat, which is not so great from
a code-readability standpoint.

> > > +    // Load subtitles specified by sub option with highest priority
> > > +    if (sub_name) {
> > > +        int i;
> > > +        for (i = 0; sub_name[i]; i++) {
> > > +            struct subfn sub = {
> > > +                .fname    = strdup(sub_name[i]),
> > > +                .priority = INT_MAX - i,
> > > +                .noerr    = 0
> > > +            };
> > > +            append_sub(&slist, &sub);
> > > +        }
> > 
> > Seems a bit overkill to push them through qsort.
> > Though it might be the simplest way.
> 
> Yes but qsort was already in use, I didn't want to change that.

qsort wasn't used on the subtitles specified with -sub.
And actually, -sub did disable auto-loading before.
I was assuming you just added more search paths, but you changed
the behaviour all over the place, that makes this review really
a major pain.