Revamp AssFile::AddLine and related functions to eliminate the statics and hopefully make it less brittle

Originally committed to SVN as r6231.

aegisub/src/ass_file.cpp

@@ -273,90 +273,92 @@ bool AssFile::CanSave() {
 	return true;
 }
 
-// I strongly advice you against touching this function unless you know what you're doing;
+void AssFile::AddLine(wxString data, int *version, AssAttachment **attach) {
-// even moving things out of order might break ASS parsing - AMZ.
+	// Is this line an attachment filename?
-void AssFile::AddLine(wxString data,wxString group,int &version,wxString *outGroup) {
+	bool isFilename = data.StartsWith("fontname: ") || data.StartsWith("filename: ");
+
+	// If there's an attachment in progress, deal with it first as an
+	// attachment data line can appear to be other things
+	if (*attach) {
+		// Check if it's valid data
+		bool validData = data.size() > 0 && data.size() <= 80;
+		for (size_t i = 0; i < data.size(); ++i) {
+			if (data[i] < 33 || data[i] >= 97) {
+				validData = false;
+				break;
+			}
+		}
+
+		// Data is over, add attachment to the file
+		if (!validData || isFilename) {
+			(*attach)->Finish();
+			Line.push_back(*attach);
+			*attach = NULL;
+		}
+		else {
+			// Insert data
+			(*attach)->AddData(data);
+
+			// Done building
+			if (data.Length() < 80) {
+				(*attach)->Finish();
+				Line.push_back(*attach);
+				*attach = NULL;
+				return;
+			}
+		}
+	}
+
 	if (data.empty()) return;
 
-	// Group
+	// Section header
-	wxString origGroup = group;
+	if (data[0] == '[' && data.Last() == ']') {
-	static wxString keepGroup;
+		// Ugly hacks to allow intermixed v4 and v4+ style sections
-	if (!keepGroup.empty()) group = keepGroup;
+		wxString low = data.Lower();
-	if (outGroup) *outGroup = group;
+		if (low == "[v4 styles]") {
+			data = "[V4+ Styles]";
+			*version = 0;
+		}
+		else if (low == "[v4+ styles]") {
+			data = "[V4+ Styles]";
+			*version = 1;
+		}
+		else if (low == "[v4++ styles]") {
+			data = "[V4+ Styles]";
+			*version = 2;
+		}
+
+		Line.push_back(new AssEntry(data, data));
+		return;
+	}
+
+	// If the first nonblank line isn't a header pretend it starts with [Script Info]
+	if (Line.empty())
+		Line.push_back(new AssEntry("[Script Info]", "[Script Info]"));
+
+	wxString group = Line.back()->group;
 	wxString lowGroup = group.Lower();
 
 	// Attachment
 	if (lowGroup == "[fonts]" || lowGroup == "[graphics]") {
-		// Check if it's valid data
-		size_t dataLen = data.Length();
-		bool validData = (dataLen > 0) && (dataLen <= 80);
-		for (size_t i=0;i<dataLen;i++) {
-			if (data[i] < 33 || data[i] >= 97) validData = false;
-		}
-
-		// Is the filename line?
-		bool isFilename = (data.StartsWith("fontname: ") || data.StartsWith("filename: "));
-
-		// The attachment file is static, since it is built through several calls to this
-		// After it's done building, it's reset to NULL
-		static AssAttachment *attach = NULL;
-
-		// Attachment exists, and data is over
-		if (attach && (!validData || isFilename)) {
-			attach->Finish();
-			keepGroup.Clear();
-			group = origGroup;
-			lowGroup = group.Lower();
-			Line.push_back(attach);
-			attach = NULL;
-		}
-
-		// Create attachment if needed
 		if (isFilename) {
-			attach = new AssAttachment(data.Mid(10));
+			*attach = new AssAttachment(data.Mid(10));
-			attach->group = group;
+			(*attach)->group = group;
-			keepGroup = group;
-			return;
-		}
-
-		// Valid data?
-		if (attach && validData) {
-			// Insert data
-			attach->AddData(data);
-
-			// Done building
-			if (data.Length() < 80) {
-				attach->Finish();
-				keepGroup.Clear();
-				group = origGroup;
-				lowGroup = group.Lower();
-				Line.push_back(attach);
-				attach = NULL;
-			}
-
-			// Not done
-			else {
-				return;
-			}
 		}
 	}
 	// Dialogue
 	else if (lowGroup == "[events]") {
 		if (data.StartsWith("Dialogue:") || data.StartsWith("Comment:"))
-			Line.push_back(new AssDialogue(data,version));
+			Line.push_back(new AssDialogue(data, *version));
 		else if (data.StartsWith("Format:"))
 			Line.push_back(new AssEntry("Format: Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text", group));
-		else
-			Line.push_back(new AssEntry(data, group));
 	}
 	// Style
 	else if (lowGroup == "[v4+ styles]") {
 		if (data.StartsWith("Style:"))
-			Line.push_back(new AssStyle(data,version));
+			Line.push_back(new AssStyle(data, *version));
 		else if (data.StartsWith("Format:"))
 			Line.push_back(new AssEntry("Format: Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding", group));
-		else
-			Line.push_back(new AssEntry(data, group));
 	}
 	// Script info
 	else if (lowGroup == "[script info]") {
@@ -367,20 +369,21 @@ void AssFile::AddLine(wxString data,wxString group,int &version,wxString *outGro
 			return;
 		}
 
-		// Version
 		if (data.StartsWith("ScriptType:")) {
-			wxString versionString = data.Mid(11);
+			wxString versionString = data.Mid(11).Trim(true).Trim(false).Lower();
-			versionString.Trim(true);
-			versionString.Trim(false);
-			versionString.MakeLower();
 			int trueVersion;
-			if (versionString == "v4.00") trueVersion = 0;
+			if (versionString == "v4.00")
-			else if (versionString == "v4.00+") trueVersion = 1;
+				trueVersion = 0;
-			else if (versionString == "v4.00++") trueVersion = 2;
+			else if (versionString == "v4.00+")
-			else throw "Unknown SSA file format version";
+				trueVersion = 1;
-			if (trueVersion != version) {
+			else if (versionString == "v4.00++")
-				if (!(trueVersion == 2 && version == 1)) wxLogMessage("Warning: File has the wrong extension.");
+				trueVersion = 2;
-				version = trueVersion;
+			else throw
+				"Unknown SSA file format version";
+			if (trueVersion != *version) {
+				if (!(trueVersion == 2 && *version == 1))
+					wxLogMessage("Warning: File has the wrong extension.");
+				*version = trueVersion;
 			}
 		}
 
@@ -407,30 +410,26 @@ void AssFile::LoadDefault(bool defline) {
 	Clear();
 
 	// Write headers
-	AssStyle defstyle;
+	AssAttachment *attach = 0;
 	int version = 1;
-	AddLine("[Script Info]","[Script Info]",version);
+	AddLine("[Script Info]", &version, &attach);
-	AddLine("Title: Default Aegisub file","[Script Info]",version);
+	AddLine("Title: Default Aegisub file", &version, &attach);
-	AddLine("ScriptType: v4.00+","[Script Info]",version);
+	AddLine("ScriptType: v4.00+", &version, &attach);
-	AddLine("WrapStyle: 0", "[Script Info]",version);
+	AddLine("WrapStyle: 0", &version, &attach);
-	AddLine("ScaledBorderAndShadow: yes","[Script Info]",version);
+	AddLine("ScaledBorderAndShadow: yes", &version, &attach);
-	AddLine("Collisions: Normal","[Script Info]",version);
+	AddLine("Collisions: Normal", &version, &attach);
 	if (!OPT_GET("Subtitle/Default Resolution/Auto")->GetBool()) {
-		AddLine(wxString::Format("PlayResX: %" PRId64, OPT_GET("Subtitle/Default Resolution/Width")->GetInt()),"[Script Info]",version);
+		AddLine(wxString::Format("PlayResX: %" PRId64, OPT_GET("Subtitle/Default Resolution/Width")->GetInt()), &version, &attach);
-		AddLine(wxString::Format("PlayResY: %" PRId64, OPT_GET("Subtitle/Default Resolution/Height")->GetInt()),"[Script Info]",version);
+		AddLine(wxString::Format("PlayResY: %" PRId64, OPT_GET("Subtitle/Default Resolution/Height")->GetInt()), &version, &attach);
 	}
-	AddLine("","[Script Info]",version);
+	AddLine("[V4+ Styles]", &version, &attach);
-	AddLine("[V4+ Styles]","[V4+ Styles]",version);
+	AddLine("Format:  Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding", &version, &attach);
-	AddLine("Format:  Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding","[V4+ Styles]",version);
+	AddLine(AssStyle().GetEntryData(), &version, &attach);
-	AddLine(defstyle.GetEntryData(),"[V4+ Styles]",version);
+	AddLine("[Events]", &version, &attach);
-	AddLine("","[V4+ Styles]",version);
+	AddLine("Format:  Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text", &version, &attach);
-	AddLine("[Events]","[Events]",version);
-	AddLine("Format:  Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text","[Events]",version);
 
-	if (defline) {
+	if (defline)
-		AssDialogue def;
+		AddLine(AssDialogue().GetEntryData(), &version, &attach);
-		AddLine(def.GetEntryData(),"[Events]",version);
-	}
 
 	Commit("", COMMIT_NEW);
 	savedCommitId = commitId;
@@ -460,102 +459,50 @@ AssFile& AssFile::operator=(AssFile from) {
 	return *this;
 }
 
-void AssFile::InsertStyle (AssStyle *style) {
+void AssFile::InsertStyle(AssStyle *style) {
-	using std::list;
+	// Search for insertion point
-	AssEntry *curEntry;
+	std::list<AssEntry*>::iterator it = Line.end();
-	list<AssEntry*>::iterator lastStyle = Line.end();
+	do {
-	list<AssEntry*>::iterator cur;
+		--it;
-	wxString lastGroup;
+		if ((*it)->group == style->group) {
-
+			Line.insert(++it, style);
-	// Look for insert position
+			return;
-	for (cur=Line.begin();cur!=Line.end();cur++) {
-		curEntry = *cur;
-		if (curEntry->GetType() == ENTRY_STYLE || (lastGroup == "[V4+ Styles]" && curEntry->GetEntryData().substr(0,7) == "Format:")) {
-			lastStyle = cur;
-		}
-		lastGroup = curEntry->group;
 		}
+	} while (it != Line.begin());
 
 	// No styles found, add them
-	if (lastStyle == Line.end()) {
+	Line.push_back(new AssEntry("[V4+ Styles]", "[V4+ Styles]"));
-		// Add space
+	Line.push_back(new AssEntry("Format: Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding", "[V4+ Styles]"));
-		curEntry = new AssEntry("");
-		curEntry->group = lastGroup;
-		Line.push_back(curEntry);
-
-		// Add header
-		curEntry = new AssEntry("[V4+ Styles]");
-		curEntry->group = "[V4+ Styles]";
-		Line.push_back(curEntry);
-
-		// Add format line
-		curEntry = new AssEntry("Format: Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding");
-		curEntry->group = "[V4+ Styles]";
-		Line.push_back(curEntry);
-
-		// Add style
-		style->group = "[V4+ Styles]";
 	Line.push_back(style);
-	}
-
-	// Add to end of list
-	else {
-		lastStyle++;
-		style->group = (*lastStyle)->group;
-		Line.insert(lastStyle,style);
-	}
 }
 
-void AssFile::InsertAttachment (AssAttachment *attach) {
+void AssFile::InsertAttachment(AssAttachment *attach) {
 	// Search for insertion point
-	std::list<AssEntry*>::iterator insPoint=Line.end(),cur;
+	std::list<AssEntry*>::iterator it = Line.end();
-	for (cur=Line.begin();cur!=Line.end();cur++) {
+	do {
-		// Check if it's another attachment
+		--it;
-		AssAttachment *att = dynamic_cast<AssAttachment*>(*cur);
+		if ((*it)->group == attach->group) {
-		if (att) {
+			Line.insert(++it, attach);
-			if (attach->group == att->group) insPoint = cur;
+			return;
 		}
+	} while (it != Line.begin());
 
-		// See if it's the start of group
+	// Didn't find a group of the appropriate type so create it
-		else if ((*cur)->GetType() == ENTRY_BASE) {
+	Line.push_back(new AssEntry(attach->group, attach->group));
-			AssEntry *entry = (AssEntry*) (*cur);
-			if (entry->GetEntryData() == attach->group) insPoint = cur;
-		}
-	}
-
-	// Found point, insert there
-	if (insPoint != Line.end()) {
-		insPoint++;
-		Line.insert(insPoint,attach);
-	}
-
-	// Otherwise, create the [Fonts] group and insert
-	else {
-		int version=1;
-		AddLine("",Line.back()->group,version);
-		AddLine(attach->group,attach->group,version);
 	Line.push_back(attach);
-		AddLine("",attach->group,version);
-	}
 }
 
 void AssFile::InsertAttachment (wxString filename) {
-	wxFileName fname(filename);
+	std::auto_ptr<AssAttachment> newAttach(new AssAttachment(wxFileName(filename).GetFullName()));
-	AssAttachment *newAttach = new AssAttachment(fname.GetFullName());
-
-	try {
 	newAttach->Import(filename);
-	}
-	catch (...) {
-		delete newAttach;
-		throw;
-	}
 
 	// Insert
 	wxString ext = filename.Right(4).Lower();
-	if (ext == ".ttf" || ext == ".ttc" || ext == ".pfb") newAttach->group = "[Fonts]";
+	if (ext == ".ttf" || ext == ".ttc" || ext == ".pfb")
-	else newAttach->group = "[Graphics]";
+		newAttach->group = "[Fonts]";
-	InsertAttachment(newAttach);
+	else
+		newAttach->group = "[Graphics]";
+	InsertAttachment(newAttach.release());
 }
 
 wxString AssFile::GetScriptInfo(wxString key) {
@@ -638,22 +585,18 @@ void AssFile::GetResolution(int &sw,int &sh) {
 	}
 }
 
-void AssFile::AddComment(const wxString _comment) {
+void AssFile::AddComment(wxString comment) {
-	wxString comment = "; ";
+	comment.Prepend("; ");
-	comment += _comment;
-	std::list<AssEntry*>::iterator cur;
 	int step = 0;
 
-	for (cur=Line.begin();cur!=Line.end();cur++) {
+	for (std::list<AssEntry*>::iterator cur = Line.begin(); cur != Line.end(); ++cur) {
 		// Start of group
-		if (step == 0 && (*cur)->group == "[Script Info]") step = 1;
+		if (step == 0 && (*cur)->group == "[Script Info]")
+			step = 1;
 
 		// First line after a ;
 		else if (step == 1 && !(*cur)->GetEntryData().StartsWith(";")) {
-			AssEntry *prev = *cur;
+			Line.insert(cur, new AssEntry(comment, "[Script Info]"));
-			AssEntry *comm = new AssEntry(comment);
-			comm->group = prev->group;
-			Line.insert(cur,comm);
 			break;
 		}
 	}
@@ -661,13 +604,10 @@ void AssFile::AddComment(const wxString _comment) {
 
 wxArrayString AssFile::GetStyles() {
 	wxArrayString styles;
-	AssStyle *curstyle;
+	for (entryIter cur = Line.begin(); cur != Line.end(); ++cur) {
-	for (entryIter cur=Line.begin();cur!=Line.end();cur++) {
+		if (AssStyle *curstyle = dynamic_cast<AssStyle*>(*cur))
-		curstyle = dynamic_cast<AssStyle*>(*cur);
-		if (curstyle) {
 			styles.Add(curstyle->name);
 	}
-	}
 	return styles;
 }
 

aegisub/src/ass_file.h

@@ -159,13 +159,12 @@ public:
 	/// Set the value of a [Script Info] key. Adds it if it doesn't exist.
 	void SetScriptInfo(wxString const& key, wxString const& value);
 	// Add a ";" comment in the [Script Info] section
-	void AddComment(const wxString comment);
+	void AddComment(wxString comment);
 	/// @brief Add a line to the file
 	/// @param data Full text of ASS line
-	/// @param group Section of the file to add the line to
+	/// @param[in,out] version ASS version the line was parsed as
-	/// @param[out] version ASS version the line was parsed as
+	/// @param[in,out] attach Accumulator for attachment parsing
-	/// @param[out] outGroup Group it was actually added to; attachments do something strange here
+	void AddLine(wxString data, int *version, AssAttachment **attach);
-	void AddLine(wxString data,wxString group,int &version,wxString *outGroup=NULL);
 
 	/// Type of changes made in a commit
 	enum CommitType {

aegisub/src/ass_style.cpp

@@ -173,35 +173,6 @@ AssStyle::AssStyle()
 	UpdateData();
 }
 
-AssStyle::AssStyle(const AssStyle& s)
-: AssEntry(s)
-, name(s.name)
-, font(s.font)
-, fontsize(s.fontsize)
-, primary(s.primary)
-, secondary(s.secondary)
-, outline(s.outline)
-, shadow(s.shadow)
-, bold(s.bold)
-, italic(s.italic)
-, underline(s.underline)
-, strikeout(s.strikeout)
-, scalex(s.scalex)
-, scaley(s.scaley)
-, spacing(s.spacing)
-, angle(s.angle)
-, borderstyle(s.borderstyle)
-, outline_w(s.outline_w)
-, shadow_w(s.shadow_w)
-, alignment(s.alignment)
-, encoding(s.encoding)
-, relativeTo(s.relativeTo)
-{
-	group = "[V4+ Styles]";
-	memcpy(Margin, s.Margin, sizeof(Margin));
-	SetEntryData(s.GetEntryData());
-}
-
 static wxString get_next_string(wxStringTokenizer &tok) {
 	if (!tok.HasMoreTokens()) throw "Malformed style: not enough fields";
 	return tok.GetNextToken();
@@ -222,6 +193,8 @@ static int get_next_double(wxStringTokenizer &tok) {
 }
 
 AssStyle::AssStyle(wxString rawData,int version) {
+	group = "[V4+ Styles]";
+
 	wxStringTokenizer tkn(rawData.Trim(false).Mid(6), ",", wxTOKEN_RET_EMPTY_ALL);
 
 	name = get_next_string(tkn).Trim(true).Trim(false);

aegisub/src/ass_style.h

@@ -107,7 +107,6 @@ public:
 	static void GetEncodings(wxArrayString &encodingStrings);
 
 	AssStyle();
-	AssStyle(AssStyle const&);
 	AssStyle(wxString data,int version=1);
 
 	wxString GetSSAText() const;

aegisub/src/mkv_wrap.cpp

@@ -126,10 +126,10 @@ static void read_subtitles(agi::ProgressSink *ps, MatroskaFile *file, MkvStdIO *
 	delete[] readBuf;
 
 	// Insert into file
-	wxString group = "[Events]";
 	int version = ssa;
+	AssAttachment *attach = 0;
 	for (std::map<int, wxString>::iterator it = subList.begin(); it != subList.end(); ++it) {
-		target->AddLine(it->second, group, version, &group);
+		target->AddLine(it->second, &version, &attach);
 	}
 }
 
@@ -196,15 +196,11 @@ void MatroskaWrapper::GetSubtitles(wxString const& filename, AssFile *target) {
 			wxString privString((const char *)trackInfo->CodecPrivate, wxConvUTF8, trackInfo->CodecPrivateSize);
 
 			// Load into file
-			wxString group = "[Script Info]";
+			int version = !ssa;
-			int version = 1;
+			AssAttachment *attach = 0;
-			if (CodecID == "S_TEXT/SSA") version = 0;
+			wxStringTokenizer token(privString, "\r\n", wxTOKEN_STRTOK);
-			wxStringTokenizer token(privString,"\r\n",wxTOKEN_STRTOK);
+			while (token.HasMoreTokens())
-			while (token.HasMoreTokens()) {
+				target->AddLine(token.GetNextToken(), &version, &attach);
-				wxString next = token.GetNextToken();
-				if (next[0] == '[') group = next;
-				target->AddLine(next,group,version,&group);
-			}
 		}
 		// Load default if it's SRT
 		else {

aegisub/src/subtitle_format.cpp

@@ -104,8 +104,8 @@ void SubtitleFormat::LoadDefault(bool defline) {
 	assFile->LoadDefault(defline);
 }
 
-void SubtitleFormat::AddLine(wxString data, wxString group, int &version, wxString *outgroup) {
+void SubtitleFormat::AddLine(wxString data, int *version, AssAttachment **attach) {
-	assFile->AddLine(data, group, version, outgroup);
+	assFile->AddLine(data, version, attach);
 }
 
 /// @brief Ask the user to enter the FPS 

aegisub/src/subtitle_format.h

@@ -45,8 +45,9 @@
 
 #include <libaegisub/exception.h>
 
-class AssFile;
+class AssAttachment;
 class AssEntry;
+class AssFile;
 class FractionalTime;
 
 /// DOCME
@@ -102,9 +103,10 @@ protected:
 
 	AssFile *GetAssFile() { return assFile; }
 	/// Add a line to the output file
-	/// @param data Line data
+	/// @param data Full text of ASS line
-	/// @param group File section
+	/// @param[in,out] version ASS version the line was parsed as
-	void AddLine(wxString data,wxString group,int &version,wxString *outgroup=NULL);
+	/// @param[in,out] attach Accumulator for attachment parsing
+	void AddLine(wxString data, int *version, AssAttachment **attach);
 	/// Prompt the user for a framerate to use
 	/// @param showSMPTE Include SMPTE as an option?
 	FractionalTime AskForFPS(bool showSMPTE=false);

aegisub/src/subtitle_format_ass.cpp

@@ -69,45 +69,12 @@ void ASSSubtitleFormat::ReadFile(wxString const& filename, wxString const& encod
 
 	TextFileReader file(filename, encoding);
 	int version = filename.Right(4).Lower() != ".ssa";
+	AssAttachment *attach = 0;
 
-	wxString curgroup;
 	while (file.HasMoreLines()) {
 		wxString line = file.ReadLineFromFile();
-
-		// Make sure that the first non-blank non-comment non-group-header line
-		// is really [Script Info]
-		if (curgroup.empty() && !line.empty() && line[0] != ';' && line[0] != '[') {
-			curgroup = "[Script Info]";
-			AddLine(curgroup, curgroup, version, &curgroup);
-		}
-
-		// Convert v4 styles to v4+ styles
-		if (!line.empty() && line[0] == '[') {
-			// Ugly hacks to allow intermixed v4 and v4+ style sections
-			wxString low = line.Lower();
-			if (low == "[v4 styles]") {
-				line = "[V4+ Styles]";
-				curgroup = line;
-				version = 0;
-			}
-			else if (low == "[v4+ styles]") {
-				line = "[V4+ Styles]";
-				curgroup = line;
-				version = 1;
-			}
-			else if (low == "[v4++ styles]") {
-				line = "[V4+ Styles]";
-				curgroup = line;
-				version = 2;
-			}
-			// Not-so-special case for other groups, just set it
-			else {
-				curgroup = line;
-			}
-		}
-
 		try {
-			AddLine(line, curgroup, version, &curgroup);
+			AddLine(line, &version, &attach);
 		}
 		catch (const char *err) {
 			Clear();