AlexPoint / SubtitlesParser

Multi formats subtitles parser in C#
MIT License
134 stars 40 forks source link

MicroDvd recognized as SRT #28

Closed OoDeLally closed 4 years ago

OoDeLally commented 4 years ago

Thanks for your great work! I am using the universal parser, which is supposed to recognize the format.

var parser = new SubtitlesParser.Classes.Parsers.SubParser();
using (var fileStream = File.OpenRead(pathToSrtFile)){
    var items = parser.ParseStream(fileStream);
}

I use as test a MicroDvd file https://www.opensubtitles.org/fr/subtitleserve/sub/117068

However it is recognized as a SRT, and therefore I get the following error: Stream is not a valid Srt format

My current workaround is to use the filename, and GetMostLikelyFormat, which correctly guesses MicroDvd. However since there are many formats using .sub extension, I am not sure how solid this method is.

I would be happy with any help :-)

OoDeLally commented 4 years ago

I had a look at the code, and I think the error comes from this line: https://github.com/AlexPoint/SubtitlesParser/commit/ef43fea34084af9a0ab4aeac11559694a6ac81a0#diff-3800832dd951a1e46961d425c0a3c4c7R125

It should not throw here, but rather continue the for loop. Any particuliar reason of that change?

AlexPoint commented 4 years ago

Looking at this old commit, I realize that we made a mistake then. We tried to handle the case of empty subtitle files - a case that should not happen - but by doing so, we broke things in the SubParser. You're right, we should continue the loop in the place you pointed out (and not throw an exception). I can't do any change in the next two weeks ; you can submit a pull request or I can do it later. Whichever suits you!

OoDeLally commented 4 years ago

I guess I can do the PR. Is the PR just about removing that line? Or is there something to make up for it?

AlexPoint commented 4 years ago

For that PR, let's just remove that line. I'll take some time later to review the logic and update the code if necessary.

OoDeLally commented 4 years ago

Done.

OoDeLally commented 4 years ago

Can you please update the nuget to 1.4.9 after merging ? I believe I cannot to that on your behalf. When you have time of course 😉