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

[Clément Bœsch]

On Sat, Nov 20, 2010 at 11:19:47AM +0100, Reimar Döffinger wrote:
> On Sat, Nov 20, 2010 at 09:33:22AM +0100, Clément Bœsch wrote:
> > +/**
> > + * \brief Allocates a new buffer containing the directory name
> > + * \param path Original path. Must be a valid string.
> > + *
> > + * The path returned always contains a trailing slash '/'.
> > + * On systems supporting DOS paths, '\' is also considered as a directory
> > + * separator in addition to the '/'.
> > + */
> > +char *mp_dirname(const char *path)
> > +{
> > +    char *tmp, *s;
> > +    size_t len;
> > +
> > +    tmp = strrchr(path, '/');
> > +
> > +#if HAVE_DOS_PATHS
> > +    if (!tmp)
> > +        tmp = strrchr(path, '\\');
> > +#endif
> 
> Please test this with the samples for mp_basename.
> I think this is not working correctly at least with mixed DOS paths
> like c:/test\b\c.avi
> Actually I think you should try reusing mp_basename here,
> everything before what mp_basename returns should be the path,
> you should then only need to have a special-case for when there
> is nothing before it.
> 

Fixed, changed with mp_basename and exported in a standalone patch.

> > @@ -1889,17 +1897,35 @@ static int compare_sub_priority(const void *a, const void *b)
> >      }
> >  }
> >  
> > -char** sub_filenames(const char* path, char *fname)
> > +static void append_sub(struct sub_list *dst, struct subfn *src)
> > +{
> > +    if (dst->sid >= (int)dst->size - 1) {
> > +        dst->size += 32;
> > +        dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size);
> > +    }
> > +    memcpy(&dst->subs[dst->sid], src, sizeof(*src));
> > +    dst->sid++;
> > +}
> > +
> > +static void merge_subs(struct sub_list *dst, struct sub_list *src)
> > +{
> > +    if (src->sid == 0)
> > +        return;
> > +    dst->size = dst->sid + src->sid;
> > +    dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size);
> > +    memcpy(&dst->subs[dst->sid], src->subs, src->sid * sizeof(*src->subs));
> > +    dst->sid += src->sid;
> > +}
> 
> I think this is too much in one patch, I have a hard time understanding
> which part solves which issue.
> I'd strongly prefer it in more parts, e.g.
> 1) use mp_basename/mp_dirname
> 2) look at additional paths
> 3) support loading multiple subtitles (or does it just change it?
>    I don't really know, but I know your changes are at least 4-times
>    as large as for what I consider reasonable just to support
>    loading from one more path).
> Particularly 3) I consider likely to have subtle issues and I don't
> like having it mixed up with other changes at all.
> 

Done. 3 patches attached.

> > @@ -1954,9 +1958,9 @@ char** sub_filenames(const char* path, char *fname)
> >      // 1 = any subtitle file
> >      // 2 = any sub file containing movie name
> >      // 3 = sub file containing movie name and the lang extension
> > -    for (j = 0; j <= 1; j++) {
> > -	d = opendir(j == 0 ? f_dir : path);
> > +	d = opendir(path);
> >  	if (d) {
> > +            mp_msg(MSGT_SUBREADER, MSGL_INFO, "Load subtitles in %s\n", path);
> >  	    while ((de = readdir(d))) {
> 
> Indentation is off.

I was just using spaces instead of mixed tabs and spaces. It's now
consistent, but note I'll reindent at least this function after the
patches get applied.

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