From 87abcddd874c211b2f5db80c2292cc35e7d9807f Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 26 Jul 2011 19:51:56 +0000 Subject: [PATCH] Make the MRU code not so bizzarely overcomplicated Originally committed to SVN as r5502. --- aegisub/libaegisub/common/mru.cpp | 120 +++++--------------- aegisub/libaegisub/include/libaegisub/mru.h | 10 +- aegisub/src/compat.cpp | 14 +-- aegisub/src/frame_main.cpp | 2 +- aegisub/src/menu.cpp | 30 ++--- aegisub/tests/libaegisub_mru.cpp | 29 +++-- 6 files changed, 78 insertions(+), 127 deletions(-) diff --git a/aegisub/libaegisub/common/mru.cpp b/aegisub/libaegisub/common/mru.cpp index c02e49d80..6b7259951 100644 --- a/aegisub/libaegisub/common/mru.cpp +++ b/aegisub/libaegisub/common/mru.cpp @@ -18,18 +18,13 @@ /// @brief Most Recently Used (MRU) Lists /// @ingroup libaegisub -#ifndef LAGI_PRE -#include -#include -#endif - #include "libaegisub/cajun/writer.h" #include "libaegisub/access.h" +#include "libaegisub/io.h" #include "libaegisub/json.h" #include "libaegisub/log.h" #include "libaegisub/mru.h" -#include "libaegisub/io.h" namespace agi { @@ -53,72 +48,42 @@ MRUManager::MRUManager(const std::string &config, const std::string &default_con MRUManager::~MRUManager() { Flush(); +} - for (MRUMap::iterator i = mru.begin(); i != mru.end(); ++i) { - delete i->second; - } +MRUManager::MRUListMap &MRUManager::Find(std::string const& key) { + MRUMap::iterator index = mru.find(key); + if (index == mru.end()) + throw MRUErrorInvalidKey("Invalid key value"); + return index->second; } void MRUManager::Add(const std::string &key, const std::string &entry) { - MRUMap::iterator index; - - if ((index = mru.find(key)) != mru.end()) { - MRUListMap &map = *index->second; - - // Remove the file before adding it. - Remove(key, entry); - - map.insert(std::pair(time(NULL), entry)); - - Prune(map); - - } else { - throw MRUErrorInvalidKey("Invalid key value"); - } + MRUListMap &map = Find(key); + map.remove(entry); + map.push_front(entry); + Prune(map); } void MRUManager::Remove(const std::string &key, const std::string &entry) { - MRUMap::iterator index; - - if ((index = mru.find(key)) != mru.end()) { - MRUListMap &map = *index->second; - for (MRUListMap::iterator map_idx = map.begin(); map_idx != map.end();) { - if (map_idx->second == entry) - map.erase(map_idx++); - else - ++map_idx; - } - } else { - throw MRUErrorInvalidKey("Invalid key value"); - } - + Find(key).remove(entry); } const MRUManager::MRUListMap* MRUManager::Get(const std::string &key) { - MRUMap::iterator index; - - if ((index = mru.find(key)) != mru.end()) { - return index->second; - } else { - throw MRUErrorInvalidKey("Invalid key value"); - } + return &Find(key); } - -const std::string MRUManager::GetEntry(const std::string &key, const int entry) { +std::string const& MRUManager::GetEntry(const std::string &key, size_t entry) { const MRUManager::MRUListMap *map = Get(key); - MRUListMap::const_iterator index = map->begin(); - - if ((unsigned int)entry > map->size()) + if (entry > map->size()) throw MRUErrorIndexOutOfRange("Requested element index is out of range."); - std::advance(index, entry); - - return index->second; + MRUListMap::const_iterator index = map->begin(); + advance(index, entry); + return *index; } @@ -126,61 +91,34 @@ void MRUManager::Flush() { json::Object out; for (MRUMap::const_iterator i = mru.begin(); i != mru.end(); ++i) { - json::Array array; - MRUListMap *map_list = i->second; + json::Array &array = out[i->first]; + const MRUListMap &map_list = i->second; - for (MRUListMap::const_iterator i_lst = map_list->begin(); i_lst != map_list->end(); ++i_lst) { - json::Object obj; - obj["time"] = json::Number((double)i_lst->first); - obj["entry"] = json::String(i_lst->second); - array.Insert(obj); + for (MRUListMap::const_iterator i_lst = map_list.begin(); i_lst != map_list.end(); ++i_lst) { + array.Insert(json::String(*i_lst)); } - - out[i->first] = array; } - io::Save file(config_name); - std::ofstream& ofp = file.Get(); - json::Writer::Write(out, ofp); - + json::Writer::Write(out, io::Save(config_name).Get()); } /// @brief Prune MRUListMap to the desired length. /// This uses the user-set values for MRU list length. inline void MRUManager::Prune(MRUListMap& map) { - unsigned int size = 16; + map.resize(std::min(16u, map.size())); +} - MRUListMap::iterator index = map.begin();; - - if (map.size() >= size) { - std::advance(index, size); - - // Use a range incase the storage number shrinks. - map.erase(index, map.end()); - } +static json::String cast_str(json::UnknownElement const& e) { + return static_cast(e); } /// @brief Load MRU Lists. /// @param key List name. /// @param array json::Array of values. void MRUManager::Load(const std::string &key, const json::Array& array) { - json::Array::const_iterator index(array.Begin()), indexEnd(array.End()); - - MRUListMap *map = new MRUListMap(); - - for (; index != indexEnd; ++index) { - const json::Object& obj = *index; - - time_t time = (time_t)(json::Number)obj["time"]; - std::string entry = (json::String)obj["entry"]; - - map->insert(make_pair(time, entry)); - } - - mru[key] = map; - Prune(*map); + transform(array.Begin(), array.End(), back_inserter(mru[key]), cast_str); + Prune(mru[key]); } - } diff --git a/aegisub/libaegisub/include/libaegisub/mru.h b/aegisub/libaegisub/include/libaegisub/mru.h index 2e363c3db..4458d9e01 100644 --- a/aegisub/libaegisub/include/libaegisub/mru.h +++ b/aegisub/libaegisub/include/libaegisub/mru.h @@ -20,6 +20,7 @@ #ifndef LAGI_PRE #include +#include #include #endif @@ -47,9 +48,7 @@ class MRUManager { public: /// @brief Map for time->value pairs. - /// @param int Last time loaded - /// @param std::string File or value that was last loaded. - typedef std::multimap > MRUListMap; + typedef std::list MRUListMap; /// @brief Constructor /// @param config File to load MRU values from @@ -79,7 +78,7 @@ public: /// @param key List name /// @param entry 0-base position of entry /// @exception MRUErrorInvalidKey thrown when an invalid key is used. - const std::string GetEntry(const std::string &key, const int entry); + std::string const& GetEntry(const std::string &key, size_t entry); /// Write MRU lists to disk. void Flush(); @@ -92,13 +91,14 @@ private: /// @brief Map for MRUListMap values. /// @param std::string Name /// @param MRUListMap instance. - typedef std::map MRUMap; + typedef std::map MRUMap; /// Internal MRUMap values. MRUMap mru; void Load(const std::string &key, const ::json::Array& array); inline void Prune(MRUListMap& map); + MRUListMap &Find(std::string const& key); }; } // namespace agi diff --git a/aegisub/src/compat.cpp b/aegisub/src/compat.cpp index d5ee1f356..ad579061f 100644 --- a/aegisub/src/compat.cpp +++ b/aegisub/src/compat.cpp @@ -1,14 +1,14 @@ #include "compat.h" #include "main.h" +#ifndef AGI_PRE +#include +#endif + wxArrayString lagi_MRU_wxAS(const wxString &list) { + const agi::MRUManager::MRUListMap *map = config::mru->Get(STD_STR(list)); wxArrayString work; - - const agi::MRUManager::MRUListMap *map_list = config::mru->Get(STD_STR(list)); - - for (agi::MRUManager::MRUListMap::const_iterator i_lst = map_list->begin(); i_lst != map_list->end(); ++i_lst) { - work.Add(wxString(i_lst->second.c_str(), wxConvUTF8)); - } - + work.reserve(map->size()); + transform(map->begin(), map->end(), std::back_inserter(work), lagi_wxString); return work; } diff --git a/aegisub/src/frame_main.cpp b/aegisub/src/frame_main.cpp index f5eafba4c..041d399ea 100644 --- a/aegisub/src/frame_main.cpp +++ b/aegisub/src/frame_main.cpp @@ -648,7 +648,7 @@ void FrameMain::RebuildRecentList(const char *root_command, const char *mru_name ss << "/"; ss << i; - wxFileName shortname(lagi_wxString(it->second)); + wxFileName shortname(lagi_wxString(*it)); menu->Append(cmd::id(ss.str()), wxString::Format("%s%d %s", i <= 9 ? "&" : "", i + 1, shortname.GetFullName())); diff --git a/aegisub/src/menu.cpp b/aegisub/src/menu.cpp index 938ecf97b..cd8806549 100644 --- a/aegisub/src/menu.cpp +++ b/aegisub/src/menu.cpp @@ -57,8 +57,8 @@ Menu::Menu() { const json::Object::Member& member = *index; const json::Array& array = member.element; delete BuildMenu(member.name, array); - } -} + } + } Menu::~Menu() {} @@ -69,11 +69,11 @@ wxMenu* Menu::GetMenu(std::string name) { if ((index = map.find(name)) != map.end()) { return index->second; - } + } LOG_E("menu/invalid") << "Invalid index name: " << name; throw MenuInvalidName("Unknown index"); -} + } @@ -92,7 +92,7 @@ wxMenu* Menu::BuildMenu(std::string name, const json::Array& array, int submenu) if (type == Menu::Spacer) { menu->AppendSeparator(); continue; - } + } const json::String& command = obj["command"]; @@ -101,13 +101,13 @@ wxMenu* Menu::BuildMenu(std::string name, const json::Array& array, int submenu) std::string cmd_name = type == Menu::Submenu ? name_submenu : command.Value(); cmd::Command *cmd; - try { + try { cmd = cmd::get(cmd_name); - } + } catch (CommandNotFound const&) { LOG_W("menu/command/not_found") << "Command '" << cmd_name << "' not found; skipping"; continue; - } + } wxString display = cmd->StrMenu() + "\t" + hotkey::get_hotkey_str_first("Default", cmd_name); wxString descr = cmd->StrHelp(); @@ -116,17 +116,17 @@ wxMenu* Menu::BuildMenu(std::string name, const json::Array& array, int submenu) case Menu::Option: { wxMenuItem *menu_item = new wxMenuItem(menu, cmd::id(command.Value()), display, descr, wxITEM_NORMAL); menu->Append(menu_item); - } + } break; case Menu::Check: { menu->AppendCheckItem(cmd::id(command.Value()), display, descr); - } +} break; case Menu::Radio: { menu->AppendRadioItem(cmd::id(command.Value()), display, descr); - } + } break; case Menu::Recent: { @@ -135,7 +135,7 @@ wxMenu* Menu::BuildMenu(std::string name, const json::Array& array, int submenu) menu->Append(menu_item); map.insert(MTPair(command.Value(), menu_new)); - } + } break; case Menu::Submenu: { @@ -150,10 +150,10 @@ wxMenu* Menu::BuildMenu(std::string name, const json::Array& array, int submenu) menu->Append(menu_item); } else { main_menu->Append(menu_new, display); - } - } - break; } + } + break; +} } // for index diff --git a/aegisub/tests/libaegisub_mru.cpp b/aegisub/tests/libaegisub_mru.cpp index e8248475e..cb6fac917 100644 --- a/aegisub/tests/libaegisub_mru.cpp +++ b/aegisub/tests/libaegisub_mru.cpp @@ -48,13 +48,7 @@ TEST_F(lagi_mru, MRUConstructFromString) { TEST_F(lagi_mru, MRUConstructInvalid) { util::copy("data/mru_invalid.json", "data/mru_tmp"); - - const std::string nonexistent("data/mru_tmp"); - agi::MRUManager mru(nonexistent, default_mru); - - // Make sure it didn't load from the file. - EXPECT_THROW(mru.Add("Invalid", "/path/to/file"), agi::MRUErrorInvalidKey); - EXPECT_NO_THROW(mru.Add("Valid_Int", "/path/to/file")); + EXPECT_ANY_THROW(agi::MRUManager("data/mru_tmp", default_mru)); } TEST_F(lagi_mru, MRUEntryAdd) { @@ -82,6 +76,25 @@ TEST_F(lagi_mru, MRUKeyValid) { EXPECT_NO_THROW(mru.Add("Valid", "/path/to/file")); } +TEST_F(lagi_mru, MRUAddSeveral) { + util::copy("data/mru_ok.json", "data/mru_tmp"); + agi::MRUManager mru("data/mru_tmp", default_mru); + + EXPECT_NO_THROW(mru.Add("Valid", "/file/1")); + EXPECT_NO_THROW(mru.Add("Valid", "/file/2")); + EXPECT_NO_THROW(mru.Add("Valid", "/file/3")); + EXPECT_NO_THROW(mru.Add("Valid", "/file/1")); + EXPECT_NO_THROW(mru.Add("Valid", "/file/1")); + EXPECT_NO_THROW(mru.Add("Valid", "/file/3")); + + agi::MRUManager::MRUListMap::const_iterator entry = mru.Get("Valid")->begin(); + EXPECT_STREQ("/file/3", (*entry++).c_str()); + EXPECT_STREQ("/file/1", (*entry++).c_str()); + EXPECT_STREQ("/file/2", (*entry++).c_str()); + EXPECT_EQ(mru.Get("Valid")->end(), entry); +} + + // Check to make sure an entry is really removed. This was fixed in // r4347, the entry was being removed from a copy of the map internally. @@ -94,6 +107,6 @@ TEST_F(lagi_mru, MRUEntryRemove_r4347) { const agi::MRUManager::MRUListMap *map_list = mru.Get("Valid"); agi::MRUManager::MRUListMap::const_iterator i_lst = map_list->begin(); - if ((i_lst != map_list->end()) && (i_lst->second == "/path/to/file")) + if ((i_lst != map_list->end()) && (*i_lst == "/path/to/file")) FAIL() << "r4347 regression, Entry exists after remove"; }