Make the MRU code not so bizzarely overcomplicated

Originally committed to SVN as r5502.

aegisub/libaegisub/common/mru.cpp

@@ -18,18 +18,13 @@
 /// @brief Most Recently Used (MRU) Lists
 /// @ingroup libaegisub
 
-#ifndef LAGI_PRE
-#include <fstream>
-#include <time.h>
-#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_t, std::string>(time(NULL), entry));
-
+	MRUListMap &map = Find(key);
+	map.remove(entry);
+	map.push_front(entry);
 	Prune(map);
-
-	} else {
-		throw MRUErrorInvalidKey("Invalid key value");
-	}
 }
 
 
 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;
-
-	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());
+	map.resize(std::min(16u, map.size()));
 }
+
+static json::String cast_str(json::UnknownElement const& e) {
+	return static_cast<json::String>(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));
+	transform(array.Begin(), array.End(), back_inserter(mru[key]), cast_str);
+	Prune(mru[key]);
 }
 
-	mru[key] = map;
-	Prune(*map);
-}
-
-
 }

aegisub/libaegisub/include/libaegisub/mru.h

@@ -20,6 +20,7 @@
 
 #ifndef LAGI_PRE
 #include <fstream>
+#include <list>
 #include <map>
 #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<time_t, std::string, std::greater_equal<time_t> > MRUListMap;
+	typedef std::list<std::string> 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<std::string, MRUListMap*> MRUMap;
+	typedef std::map<std::string, MRUListMap> 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

aegisub/src/compat.cpp

@@ -1,14 +1,14 @@
 #include "compat.h"
 #include "main.h"
 
+#ifndef AGI_PRE
+#include <algorithm>
+#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;
 }

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()));

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";
 }