Switch auto4lua to a transactional model which applies changes to the subtitle file only if a macro runs to completion without errors or the user cancelling. Significantly improves behavior when the user cancels and fixes a race condition caused by committing on threads other than the GUI thread.

Originally committed to SVN as r5650.

aegisub/src/auto4_lua.cpp

@@ -488,12 +488,7 @@ namespace Automation4 {
 	void LuaThreadedCall(lua_State *L, int nargs, int nresults, wxString const& title, wxWindow *parent, bool can_open_config)
 	{
 		BackgroundScriptRunner bsr(parent, title);
-		try {
+		bsr.Run(bind(lua_threaded_call, std::tr1::placeholders::_1, L, nargs, nresults, can_open_config));
-			bsr.Run(bind(lua_threaded_call, std::tr1::placeholders::_1, L, nargs, nresults, can_open_config));
-		}
-		catch (agi::UserCancelException const&) {
-			/// @todo perhaps this needs to continue up for exporting?
-		}
 	}
 
 	// LuaFeature
@@ -635,43 +630,46 @@ namespace Automation4 {
 		LuaAssFile *subsobj = new LuaAssFile(L, c->ass, true, true);
 		lua_pushinteger(L, transform_selection(L, c));
 
-		// do call
+		try {
-		// 3 args: subtitles, selected lines, active line
+			LuaThreadedCall(L, 3, 1, StrDisplay(c), c->parent, true);
-		// 1 result: new selected lines
-		LuaThreadedCall(L, 3, 1, StrDisplay(c), c->parent, true);
 
-		subsobj->ProcessingComplete(StrDisplay(c));
+			subsobj->ProcessingComplete(StrDisplay(c));
 
-		// top of stack will be selected lines array, if any was returned
+			// top of stack will be selected lines array, if any was returned
-		if (lua_istable(L, -1)) {
+			if (lua_istable(L, -1)) {
-			std::set<AssDialogue*> sel;
+				std::set<AssDialogue*> sel;
-			entryIter it = c->ass->Line.begin();
+				entryIter it = c->ass->Line.begin();
-			int last_idx = 1;
+				int last_idx = 1;
-			lua_pushnil(L);
+				lua_pushnil(L);
-			while (lua_next(L, -2)) {
+				while (lua_next(L, -2)) {
-				if (lua_isnumber(L, -1)) {
+					if (lua_isnumber(L, -1)) {
-					int cur = lua_tointeger(L, -1);
+						int cur = lua_tointeger(L, -1);
-					if (cur < 1 || cur > (int)c->ass->Line.size()) {
+						if (cur < 1 || cur > (int)c->ass->Line.size()) {
-						wxLogError("Selected row %d is out of bounds (must be 1-%u)", cur, c->ass->Line.size());
+							wxLogError("Selected row %d is out of bounds (must be 1-%u)", cur, c->ass->Line.size());
-						break;
+							break;
+						}
+
+						advance(it, cur - last_idx);
+
+						AssDialogue *diag = dynamic_cast<AssDialogue*>(*it);
+						if (!diag) {
+							wxLogError("Selected row %d is not a dialogue line", cur);
+							break;
+						}
+
+						sel.insert(diag);
+						last_idx = cur;
 					}
-
+					lua_pop(L, 1);
-					advance(it, cur - last_idx);
-
-					AssDialogue *diag = dynamic_cast<AssDialogue*>(*it);
-					if (!diag) {
-						wxLogError("Selected row %d is not a dialogue line", cur);
-						break;
-					}
-
-					sel.insert(diag);
-					last_idx = cur;
 				}
-				lua_pop(L, 1);
-			}
 
-			c->selectionController->SetSelectedSet(sel);
+				c->selectionController->SetSelectedSet(sel);
+			}
 		}
+		catch (agi::UserCancelException const&) {
+			subsobj->Cancel();
+		}
+
 		// either way, there will be something on the stack
 		lua_pop(L, 1);
 	}
@@ -762,11 +760,15 @@ namespace Automation4 {
 		assert(lua_istable(L, -1));
 		stackcheck.check_stack(3);
 
-		LuaThreadedCall(L, 2, 0, GetName(), export_dialog, false);
+		try {
-
+			LuaThreadedCall(L, 2, 0, GetName(), export_dialog, false);
-		stackcheck.check_stack(0);
+			stackcheck.check_stack(0);
-
+			subsobj->ProcessingComplete();
-		subsobj->ProcessingComplete();
+		}
+		catch (agi::UserCancelException const&) {
+			subsobj->Cancel();
+			throw;
+		}
 	}
 
 	ScriptDialog* LuaExportFilter::GenerateConfigDialog(wxWindow *parent, agi::Context *c)

aegisub/src/auto4_lua.h

@@ -35,6 +35,8 @@
 ///
 
 #ifndef AGI_PRE
+#include <deque>
+
 #include <wx/event.h>
 #include <wx/thread.h>
 #endif
@@ -53,6 +55,12 @@ namespace Automation4 {
 	/// @class LuaAssFile
 	/// @brief Object wrapping an AssFile object for modification through Lua
 	class LuaAssFile {
+		struct PendingCommit {
+			wxString mesage;
+			int modification_type;
+			std::list<AssEntry*> lines;
+		};
+
 		/// Pointer to file being modified
 		AssFile *ass;
 
@@ -73,6 +81,13 @@ namespace Automation4 {
 		/// calling C++ code are done with it
 		int references;
 
+		/// Set of subtitle lines being modified; initially a shallow copy of ass->Line
+		std::list<AssEntry*> lines;
+		/// Commits to apply once processing completes successfully
+		std::deque<PendingCommit> pending_commits;
+		/// Lines to delete once processing complete successfully
+		std::deque<AssEntry*> lines_to_delete;
+
 		/// Cursor for last access into file
 		std::list<AssEntry*>::iterator last_entry_ptr;
 		/// Index for last access into file
@@ -111,6 +126,9 @@ namespace Automation4 {
 		///                 description
 		void ProcessingComplete(wxString const& undo_description = "");
 
+		/// End processing without applying any changes made
+		void Cancel();
+
 		/// Constructor
 		/// @param L lua state
 		/// @param ass File to wrap
@@ -219,6 +237,14 @@ namespace Automation4 {
 		LuaFeature(lua_State *L);
 	};
 
+	/// Run a lua function on a background thread
+	/// @param L Lua state
+	/// @param nargs Number of arguments the function takes
+	/// @param nresults Number of values the function returns
+	/// @param title Title to use for the progress dialog
+	/// @param parent Parent window for the progress dialog
+	/// @param can_open_config Can the function open its own dialogs?
+	/// @throws agi::UserCancelException if the function fails to run to completion (either due to cancelling or errors)
 	void LuaThreadedCall(lua_State *L, int nargs, int nresults, wxString const& title, wxWindow *parent, bool can_open_config);
 
 	class LuaCommand : public cmd::Command, private LuaFeature {

aegisub/src/auto4_lua_assfile.cpp

@@ -380,18 +380,18 @@ namespace Automation4 {
 
 	void LuaAssFile::SeekCursorTo(int n)
 	{
-		if (n <= 0 || n > (int)ass->Line.size())
+		if (n <= 0 || n > (int)lines.size())
 			luaL_error(L, "Requested out-of-range line from subtitle file: %d", n);
 
 		if (n < last_entry_id - n) {
 			// fastest to search from start
-			last_entry_ptr = ass->Line.begin();
+			last_entry_ptr = lines.begin();
 			last_entry_id = 1;
 		}
-		else if ((int)ass->Line.size() - last_entry_id < abs(last_entry_id - n)) {
+		else if ((int)lines.size() - last_entry_id < abs(last_entry_id - n)) {
 			// fastest to search from end
-			last_entry_ptr = ass->Line.end();
+			last_entry_ptr = lines.end();
-			last_entry_id = ass->Line.size() + 1;
+			last_entry_id = lines.size() + 1;
 		}
 		// otherwise fastest to search from cursor
 
@@ -415,7 +415,7 @@ namespace Automation4 {
 
 				if (strcmp(idx, "n") == 0) {
 					// get number of items
-					lua_pushnumber(L, ass->Line.size());
+					lua_pushnumber(L, lines.size());
 					return 1;
 				}
 
@@ -474,7 +474,7 @@ namespace Automation4 {
 				AssEntry *e = LuaToAssEntry(L);
 				modification_type |= modification_mask(e);
 				SeekCursorTo(n);
-				delete *last_entry_ptr;
+				lines_to_delete.push_back(*last_entry_ptr);
 				*last_entry_ptr = e;
 			}
 			else {
@@ -488,7 +488,7 @@ namespace Automation4 {
 
 	int LuaAssFile::ObjectGetLen(lua_State *L)
 	{
-		lua_pushnumber(L, ass->Line.size());
+		lua_pushnumber(L, lines.size());
 		return 1;
 	}
 
@@ -503,7 +503,7 @@ namespace Automation4 {
 
 		while (itemcount > 0) {
 			int n = luaL_checkint(L, itemcount);
-			luaL_argcheck(L, n > 0 && n <= (int)ass->Line.size(), itemcount, "Out of range line index");
+			luaL_argcheck(L, n > 0 && n <= (int)lines.size(), itemcount, "Out of range line index");
 			ids.push_back(n);
 			--itemcount;
 		}
@@ -519,8 +519,8 @@ namespace Automation4 {
 				--last_entry_id;
 			}
 			modification_type |= modification_mask(*last_entry_ptr);
-			delete *last_entry_ptr;
+			lines_to_delete.push_back(*last_entry_ptr);
-			ass->Line.erase(last_entry_ptr++);
+			lines.erase(last_entry_ptr++);
 		}
 	}
 
@@ -529,14 +529,14 @@ namespace Automation4 {
 		CheckAllowModify();
 
 		int a = std::max<int>(luaL_checkinteger(L, 1), 1);
-		int b = std::min<int>(luaL_checkinteger(L, 2), ass->Line.size());
+		int b = std::min<int>(luaL_checkinteger(L, 2), lines.size());
 
 		SeekCursorTo(a);
 
 		while (a++ <= b) {
 			modification_type |= modification_mask(*last_entry_ptr);
-			delete *last_entry_ptr;
+			lines_to_delete.push_back(*last_entry_ptr);
-			ass->Line.erase(last_entry_ptr++);
+			lines.erase(last_entry_ptr++);
 		}
 	}
 
@@ -546,7 +546,7 @@ namespace Automation4 {
 
 		int n = lua_gettop(L);
 
-		if (last_entry_ptr != ass->Line.begin()) {
+		if (last_entry_ptr != lines.begin()) {
 			last_entry_ptr--;
 			last_entry_id--;
 		}
@@ -557,14 +557,14 @@ namespace Automation4 {
 			modification_type |= modification_mask(*last_entry_ptr);
 			if (e->GetType() == ENTRY_DIALOGUE) {
 				// find insertion point, looking backwards
-				std::list<AssEntry*>::iterator it = ass->Line.end();
+				std::list<AssEntry*>::iterator it = lines.end();
 				do { --it; } while ((*it)->GetType() != ENTRY_DIALOGUE);
 				// found last dialogue entry in file, move one past
 				++it;
-				ass->Line.insert(it, e);
+				lines.insert(it, e);
 			}
 			else {
-				ass->Line.push_back(e);
+				lines.push_back(e);
 			}
 		}
 	}
@@ -576,7 +576,7 @@ namespace Automation4 {
 		int before = luaL_checkinteger(L, 1);
 
 		// + 1 to allow appending at the end of the file
-		luaL_argcheck(L, before > 0 && before <= (int)ass->Line.size() + 1, 1,
+		luaL_argcheck(L, before > 0 && before <= (int)lines.size() + 1, 1,
 			"Out of range line index");
 
 		SeekCursorTo(before);
@@ -586,7 +586,7 @@ namespace Automation4 {
 			lua_pushvalue(L, i);
 			AssEntry *e = LuaToAssEntry(L);
 			modification_type |= modification_mask(e);
-			ass->Line.insert(last_entry_ptr, e);
+			lines.insert(last_entry_ptr, e);
 			lua_pop(L, 1);
 		}
 
@@ -641,7 +641,12 @@ namespace Automation4 {
 			luaL_error(L, "Attempt to set an undo point in a context where it makes no sense to do so.");
 
 		if (modification_type) {
-			ass->Commit(wxString(luaL_checkstring(L, 1), wxConvUTF8), modification_type);
+			pending_commits.push_back(PendingCommit());
+			PendingCommit& back = pending_commits.back();
+
+			back.modification_type = modification_type;
+			back.mesage = wxString(luaL_checkstring(L, 1), wxConvUTF8);
+			back.lines = lines;
 			modification_type = 0;
 		}
 	}
@@ -655,10 +660,26 @@ namespace Automation4 {
 
 	void LuaAssFile::ProcessingComplete(wxString const& undo_description)
 	{
-		if (modification_type && can_set_undo && !undo_description.empty()) {
+		// Apply any pending commits
-			ass->Commit(undo_description, modification_type);
+		for (std::deque<PendingCommit>::iterator it = pending_commits.begin(); it != pending_commits.end(); ++it) {
-			modification_type = 0;
+			swap(ass->Line, it->lines);
+			ass->Commit(it->mesage, it->modification_type);
 		}
+
+		// Commit any changes after the last undo point was set
+		if (modification_type)
+			swap(ass->Line, lines);
+		if (modification_type && can_set_undo && !undo_description.empty())
+			ass->Commit(undo_description, modification_type);
+
+		delete_clear(lines_to_delete);
+
+		references--;
+		if (!references) delete this;
+	}
+
+	void LuaAssFile::Cancel()
+	{
 		references--;
 		if (!references) delete this;
 	}
@@ -670,7 +691,8 @@ namespace Automation4 {
 	, can_set_undo(can_set_undo)
 	, modification_type(0)
 	, references(2)
-	, last_entry_ptr(ass->Line.begin())
+	, lines(ass->Line)
+	, last_entry_ptr(lines.begin())
 	, last_entry_id(1)
 	{
 		// prepare userdata object