From b55ff3f8031f122a3a462e8ee3a0797835b9d147 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Wed, 28 Sep 2011 19:50:14 +0000 Subject: [PATCH] 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 | 86 ++++++++++++++++--------------- aegisub/src/auto4_lua.h | 26 ++++++++++ aegisub/src/auto4_lua_assfile.cpp | 72 +++++++++++++++++--------- 3 files changed, 117 insertions(+), 67 deletions(-) diff --git a/aegisub/src/auto4_lua.cpp b/aegisub/src/auto4_lua.cpp index aaf9d16a1..eb9312546 100644 --- a/aegisub/src/auto4_lua.cpp +++ b/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)); - } - catch (agi::UserCancelException const&) { - /// @todo perhaps this needs to continue up for exporting? - } + bsr.Run(bind(lua_threaded_call, std::tr1::placeholders::_1, L, nargs, nresults, can_open_config)); } // 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 - // 3 args: subtitles, selected lines, active line - // 1 result: new selected lines - LuaThreadedCall(L, 3, 1, StrDisplay(c), c->parent, true); + try { + 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 - if (lua_istable(L, -1)) { - std::set sel; - entryIter it = c->ass->Line.begin(); - int last_idx = 1; - lua_pushnil(L); - while (lua_next(L, -2)) { - if (lua_isnumber(L, -1)) { - int cur = lua_tointeger(L, -1); - 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()); - break; + // top of stack will be selected lines array, if any was returned + if (lua_istable(L, -1)) { + std::set sel; + entryIter it = c->ass->Line.begin(); + int last_idx = 1; + lua_pushnil(L); + while (lua_next(L, -2)) { + if (lua_isnumber(L, -1)) { + int cur = lua_tointeger(L, -1); + 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()); + break; + } + + advance(it, cur - last_idx); + + AssDialogue *diag = dynamic_cast(*it); + if (!diag) { + wxLogError("Selected row %d is not a dialogue line", cur); + break; + } + + sel.insert(diag); + last_idx = cur; } - - advance(it, cur - last_idx); - - AssDialogue *diag = dynamic_cast(*it); - if (!diag) { - wxLogError("Selected row %d is not a dialogue line", cur); - break; - } - - sel.insert(diag); - last_idx = cur; + lua_pop(L, 1); } - 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); - - stackcheck.check_stack(0); - - subsobj->ProcessingComplete(); + try { + LuaThreadedCall(L, 2, 0, GetName(), export_dialog, false); + stackcheck.check_stack(0); + subsobj->ProcessingComplete(); + } + catch (agi::UserCancelException const&) { + subsobj->Cancel(); + throw; + } } ScriptDialog* LuaExportFilter::GenerateConfigDialog(wxWindow *parent, agi::Context *c) diff --git a/aegisub/src/auto4_lua.h b/aegisub/src/auto4_lua.h index 52a081e24..0cb5a4c97 100644 --- a/aegisub/src/auto4_lua.h +++ b/aegisub/src/auto4_lua.h @@ -35,6 +35,8 @@ /// #ifndef AGI_PRE +#include + #include #include #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 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 lines; + /// Commits to apply once processing completes successfully + std::deque pending_commits; + /// Lines to delete once processing complete successfully + std::deque lines_to_delete; + /// Cursor for last access into file std::list::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 { diff --git a/aegisub/src/auto4_lua_assfile.cpp b/aegisub/src/auto4_lua_assfile.cpp index 49d244165..05d7aed22 100644 --- a/aegisub/src/auto4_lua_assfile.cpp +++ b/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_id = ass->Line.size() + 1; + last_entry_ptr = lines.end(); + 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; - ass->Line.erase(last_entry_ptr++); + lines_to_delete.push_back(*last_entry_ptr); + lines.erase(last_entry_ptr++); } } @@ -529,14 +529,14 @@ namespace Automation4 { CheckAllowModify(); int a = std::max(luaL_checkinteger(L, 1), 1); - int b = std::min(luaL_checkinteger(L, 2), ass->Line.size()); + int b = std::min(luaL_checkinteger(L, 2), lines.size()); SeekCursorTo(a); while (a++ <= b) { modification_type |= modification_mask(*last_entry_ptr); - delete *last_entry_ptr; - ass->Line.erase(last_entry_ptr++); + lines_to_delete.push_back(*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::iterator it = ass->Line.end(); + std::list::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()) { - ass->Commit(undo_description, modification_type); - modification_type = 0; + // Apply any pending commits + for (std::deque::iterator it = pending_commits.begin(); it != pending_commits.end(); ++it) { + 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