Tidy subrip parsing a bit and fix failure to persist

italic/bold etc. across multiple lines of one subtitle
(DCP-o-matic bug #837).

diff --git a/src/subrip_reader.cc b/src/subrip_reader.cc
index 6c0c63a26130877f87adfa6c0bb27847af7e3064..a82862cb5bc060000773d3b1edaa172976086bd3 100644 (file)
--- a/src/subrip_reader.cc
+++ b/src/subrip_reader.cc
@@ -66,11 +66,13 @@ SubripReader::read (function<optional<string> ()> get_line)
                CONTENT
        } state = COUNTER;
 
-       Time from;
-       Time to;
-
-       string line;
-       int line_number = 0;
+       RawSubtitle rs;
+       rs.font = "Arial";
+       rs.font_size.set_points (48);
+       rs.vertical_position.line = 0;
+       /* XXX: arbitrary */
+       rs.vertical_position.lines = 32;
+       rs.vertical_position.reference = TOP_OF_SUBTITLE;
 
        while (true) {
                optional<string> line = get_line ();
@@ -100,6 +102,12 @@ SubripReader::read (function<optional<string> ()> get_line)
                        }
 
                        state = METADATA;
+
+                       /* Reset stuff that should not persist across separate subtitles */
+                       rs.bold = false;
+                       rs.italic = false;
+                       rs.underline = false;
+                       rs.vertical_position.line = 0;
                }
                break;
                case METADATA:
@@ -110,8 +118,8 @@ SubripReader::read (function<optional<string> ()> get_line)
                                throw SubripError (*line, "a time/position line");
                        }
 
-                       from = convert_time (p[0]);
-                       to = convert_time (p[2]);
+                       rs.from = convert_time (p[0]);
+                       rs.to = convert_time (p[2]);
 
                        /* XXX: should not ignore coordinate specifications */
 
@@ -121,10 +129,9 @@ SubripReader::read (function<optional<string> ()> get_line)
                case CONTENT:
                        if (line->empty ()) {
                                state = COUNTER;
-                               line_number = 0;
                        } else {
-                               convert_line (*line, line_number, from, to);
-                               line_number++;
+                               convert_line (*line, rs);
+                               rs.vertical_position.line = rs.vertical_position.line.get() + 1;
                        }
                        break;
                }
@@ -152,7 +159,7 @@ SubripReader::convert_time (string t)
 }
 
 void
-SubripReader::convert_line (string t, int line_number, Time from, Time to)
+SubripReader::convert_line (string t, RawSubtitle& p)
 {
        enum {
                TEXT,
@@ -161,16 +168,6 @@ SubripReader::convert_line (string t, int line_number, Time from, Time to)
 
        string tag;
 
-       RawSubtitle p;
-       p.font = "Arial";
-       p.font_size.set_points (48);
-       p.from = from;
-       p.to = to;
-       p.vertical_position.line = line_number;
-       /* XXX: arbitrary */
-       p.vertical_position.lines = 32;
-       p.vertical_position.reference = TOP_OF_SUBTITLE;
-
        list<Colour> colours;
        colours.push_back (Colour (1, 1, 1));
 
@@ -233,6 +230,7 @@ SubripReader::convert_line (string t, int line_number, Time from, Time to)
        maybe_content (p);
 }
 
+/* Push p into _subs if it has some text, and clear the text out of p */
 void
 SubripReader::maybe_content (RawSubtitle& p)
 {

diff --git a/src/subrip_reader.h b/src/subrip_reader.h
index e9a9b35594140c3f1993c10ac6f49f4ee578f739..5f5f343f48ca7e56f46d7571785ae8d014a6a4e9 100644 (file)
--- a/src/subrip_reader.h
+++ b/src/subrip_reader.h
@@ -45,7 +45,7 @@ private:
        SubripReader () {}
 
        static Time convert_time (std::string t);
-       void convert_line (std::string t, int line_number, Time from, Time to);
+       void convert_line (std::string t, RawSubtitle& p);
        void maybe_content (RawSubtitle& p);
        void read (boost::function<boost::optional<std::string> ()> get_line);
 };

diff --git a/test/data/test2.srt b/test/data/test2.srt
index 94251d4891ca5a625be9e31d18173dda32745600..3e0e47831594016b75bae400806c2cb034f58753 100644 (file)
--- a/test/data/test2.srt
+++ b/test/data/test2.srt
@@ -22,3 +22,8 @@ Is this a dagger I see before me?
 6
 00:03:54,560 --> 00:03:56,471
 Hello world.
+
+7
+00:04:50,123 --> 00:04:55,023
+<i>Some italics over
+multiple lines</i>

diff --git a/test/subrip_reader_test.cc b/test/subrip_reader_test.cc
index 0e7f50db2d72781c89b68d9b2c52506f9e42ee43..a20cc07bccb48348bfb93df68f977c7d535a1514 100644 (file)
--- a/test/subrip_reader_test.cc
+++ b/test/subrip_reader_test.cc
@@ -212,6 +212,16 @@ BOOST_AUTO_TEST_CASE (subrip_reader_test2)
        BOOST_CHECK_EQUAL (i->lines.size(), 1);
        BOOST_CHECK_EQUAL (i->lines.front().blocks.front().text, "Hello world.");
 
+       ++i;
+       BOOST_CHECK (i != subs.end ());
+       BOOST_CHECK_EQUAL (i->from, sub::Time::from_hms (0, 4, 50, 123));
+       BOOST_CHECK_EQUAL (i->to, sub::Time::from_hms (0, 4, 55, 23));
+       BOOST_CHECK_EQUAL (i->lines.size(), 2);
+       BOOST_CHECK_EQUAL (i->lines.front().blocks.front().text, "Some italics over");
+       BOOST_CHECK_EQUAL (i->lines.front().blocks.front().italic, true);
+       BOOST_CHECK_EQUAL (i->lines.back().blocks.front().text, "multiple lines");
+       BOOST_CHECK_EQUAL (i->lines.back().blocks.front().italic, true);
+
        ++i;
        BOOST_CHECK (i == subs.end ());
 }
@@ -221,48 +231,56 @@ BOOST_AUTO_TEST_CASE (subrip_reader_convert_line_test)
 {
        sub::SubripReader r;
 
-       r.convert_line ("Hello world", 0, sub::Time (), sub::Time ());
+       sub::RawSubtitle rs;
+       r.convert_line ("Hello world", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        r._subs.clear ();
 
-       r.convert_line ("<b>Hello world</b>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<b>Hello world</b>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().bold, true);
        r._subs.clear ();
 
-       r.convert_line ("<i>Hello world</i>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<i>Hello world</i>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().italic, true);
        r._subs.clear ();
 
-       r.convert_line ("<u>Hello world</u>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<u>Hello world</u>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().underline, true);
        r._subs.clear ();
 
-       r.convert_line ("{b}Hello world{/b}", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("{b}Hello world{/b}", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().bold, true);
        r._subs.clear ();
 
-       r.convert_line ("{i}Hello world{/i}", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("{i}Hello world{/i}", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().italic, true);
        r._subs.clear ();
 
-       r.convert_line ("{u}Hello world{/u}", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("{u}Hello world{/u}", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "Hello world");
        BOOST_CHECK_EQUAL (r._subs.front().underline, true);
        r._subs.clear ();
 
-       r.convert_line ("<b>This is <i>nesting</i> of subtitles</b>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<b>This is <i>nesting</i> of subtitles</b>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 3);
        list<sub::RawSubtitle>::iterator i = r._subs.begin ();
        BOOST_CHECK_EQUAL (i->text, "This is ");
@@ -279,7 +297,8 @@ BOOST_AUTO_TEST_CASE (subrip_reader_convert_line_test)
        ++i;
        r._subs.clear ();
 
-       r.convert_line ("<font color=\"#ff00ff\">simple color</font>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<font color=\"#ff00ff\">simple color</font>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 1);
        BOOST_CHECK_EQUAL (r._subs.front().text, "simple color");
        BOOST_CHECK_EQUAL (r._subs.front().bold, false);
@@ -288,7 +307,8 @@ BOOST_AUTO_TEST_CASE (subrip_reader_convert_line_test)
        BOOST_CHECK_CLOSE (r._subs.front().colour.b, 1, 0.1);
        r._subs.clear ();
 
-       r.convert_line ("<font color=\"#ff0000\">some red text <b>in bold</b></font>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<font color=\"#ff0000\">some red text <b>in bold</b></font>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 2);
        i = r._subs.begin ();
        BOOST_CHECK_EQUAL (i->text, "some red text ");
@@ -304,7 +324,8 @@ BOOST_AUTO_TEST_CASE (subrip_reader_convert_line_test)
        BOOST_CHECK (fabs (i->colour.b) < 0.01);
        r._subs.clear ();
 
-       r.convert_line ("<font color=\"#0000ff\">some blue text <b>in bold</b></font>", 0, sub::Time (), sub::Time ());
+       rs = sub::RawSubtitle();
+       r.convert_line ("<font color=\"#0000ff\">some blue text <b>in bold</b></font>", rs);
        BOOST_CHECK_EQUAL (r._subs.size(), 2);
        i = r._subs.begin ();
        BOOST_CHECK_EQUAL (i->text, "some blue text ");