byroot / pysrt

Python parser for SubRip (srt) files
GNU General Public License v3.0
446 stars 67 forks source link

Make character_per_seconds correctly deal with tags and introduce text_w... #54

Closed felagund closed 10 years ago

felagund commented 10 years ago

BTW: is not the commit message too long? Github truncates it.

This ignores tags that srt format allows. It could be done with one regular expression, but I wanted to be explicit in what is ignored (and replace is faster then re, but as it gets called several times, I guess this is overall slower).

SRT is also said to support position coordinates, but there are several ways to write them and I have never actually seen them in the wild, unlike other tags.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.47%) when pulling db554fec4be325cc0e2a537ba2d670bc679d3331 on felagund:improve_cps into 99b3066d92de65f0ff1047932078b3232b0d49cb on byroot:master.

byroot commented 10 years ago

Small comment, otherwise it looks good to me.

felagund commented 10 years ago

Yeah, that was my initial version. I am not sure about strapping just any tag: Consider this subtitle:

1
00:00:00,000 --> 00:02:50,435
<odd tag>tag</odd tag>

Mplayer plays it and displays "tag</odd tag>" . Totem and VLC display just "tag". Another gstreamer based player (Subtitleeditor) does not display this subtitle at all. So there does not seem to be an agreement what to do with broken tags. Overall it seems like hiding problems away. But this is not a big deal as this is a corner case, so if you insist, I will do asi you wish:-). I will compile the regexp, no problem.

byroot commented 10 years ago

So there does not seem to be an agreement what to do with broken tags.

That's my point. SRT is really badly specified. you better strip all tags ever than a whitelist.

felagund commented 10 years ago

Ok, changed as requested.

Still, this hides something like <i some really important subtitle , so I think it would be better to be specific in what we remove, but this really is a corner case:-).

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.23%) when pulling 2e6663ba9eff9167d62ca93759032e69bdbb764c on felagund:improve_cps into 99b3066d92de65f0ff1047932078b3232b0d49cb on byroot:master.

byroot commented 10 years ago

Thanks again for all your PRs!