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

[Reimar Döffinger]

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.

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

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