On Mon, Nov 22, 2010 at 12:05:08AM +0100, Reimar Döffinger wrote:
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.
After the first patch, it will still be the same: only two directories are tracked: ~/.mplayer/sub and the movie one. About the fuzziness behaviour with the subdirs patch, I think it should also honor the value of 2 since subdirs patch is mainly for relative paths. When you have a sub-fuzziness set to 2, you just want to load all the subtitles in the movie directory not matching the movie name, so you have a tree like that for example: ./movie1/ ./movie1/movie1.avi ./movie1/subtitle-en.srt ./movie1/subtitle-fr.srt ./movie1/subtitle-de.srt ./movie2/ ./movie2/movie2.avi ./movie2/subtitle-en.srt ./movie2/subtitle-fr.srt ./movie2/subtitle-de.srt ... ...with a common subtitle format name for example, total random, a hash or whatever. The subdirs option just extend this behaviour with this kind of tree: ./movie1/ ./movie1/movie1.avi ./movie1/sub/subtitle-en.srt ./movie1/sub/subtitle-fr.srt ./movie1/sub/subtitle-de.srt ./movie2/ ./movie2/movie2.avi ./movie2/sub/subtitle-en.srt ./movie2/sub/subtitle-fr.srt ./movie2/sub/subtitle-de.srt ... All the subtitles are always in a sub/ (or anything else) directory where you don't have to care about the filenames format. ~/.mplayer/sub is an exception since it was made to centralize *all* the subtitles all over the place, so it does not make any sense to load them all.
+ 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.
I see. Changed with a strcat.
+ // 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.
Yes, I changed the patch to stay consistent with the old code: -sub subtitles are directly loaded now.
And actually, -sub did disable auto-loading before.
Huh? I don't think so; the old code do that: if (sub_name) { ... } if (sub_auto) { ... } And I kept this.
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.
OK, sorry about that. I simplified the patch all over the place so it should make more sense. So let me resume this again with the new code: Everything starts in load_subtitles which load all the possible subtitles. This function first add subtitles specified by -sub. After that, if automatic detection is disabled, it stops there. Then a range of subtitles is allocated (this was previously done in sub_filenames). The movie directory is then tracked using the helper track_sub_directory to build the correct path. This helper will be used a second time with the subdirs patch. Next, it tries to load subtitles in ~/.mplayer/sub directory. The list is now complete, so subtitles are sorted by priority and then added. End of the function. sub_filenames now only loads a single path and append the subtitles to a list, so its prototype changed and it was renamed to append_dir_subtitles. Is this better? Note: I also added a workaround for absolute paths in the subdirs patch. -- Clément B. Not sent from a jesusPhone.