Fixes these glitches in options menu:

- Sort order not remembered during ES session - "Jump to ..." only correct on "name, asc." sort order - Metadata edit with rename name leaves list cursor misplaced - Scroll of long filename in gamelist restarts even if option dialog is closed without any changes - Sort order description "filename" changed to "name" - Some minor code smells
Gemba · Aug 6, 2023 · 2d46a0a · 2d46a0a
1 parent 30cbdeb
commit 2d46a0a
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 69 deletions.
diff --git a/es-app/src/CollectionSystemManager.cpp b/es-app/src/CollectionSystemManager.cpp
@@ -27,10 +27,10 @@ CollectionSystemManager::CollectionSystemManager(Window* window) : mWindow(windo
 {
 	CollectionSystemDecl systemDecls[] = {
 		//type                  name            long name            //default sort              // theme folder            // isCustom
-		{ AUTO_ALL_GAMES,       "all",          "all games",         "filename, ascending",      "auto-allgames",           false },
+		{ AUTO_ALL_GAMES,       "all",          "all games",         "name, ascending",          "auto-allgames",           false },
 		{ AUTO_LAST_PLAYED,     "recent",       "last played",       "last played, descending",  "auto-lastplayed",         false },
-		{ AUTO_FAVORITES,       "favorites",    "favorites",         "filename, ascending",      "auto-favorites",          false },
-		{ CUSTOM_COLLECTION,    myCollectionsName,  "collections",    "filename, ascending",      "custom-collections",      true }
+		{ AUTO_FAVORITES,       "favorites",    "favorites",         "name, ascending",          "auto-favorites",          false },
+		{ CUSTOM_COLLECTION,    myCollectionsName,  "collections",   "name, ascending",          "custom-collections",      true }
 	};
 
 	// create a map

diff --git a/es-app/src/FileData.cpp b/es-app/src/FileData.cpp
@@ -250,21 +250,30 @@ void FileData::removeChild(FileData* file)
 
 void FileData::sort(ComparisonFunction& comparator, bool ascending)
 {
-	std::stable_sort(mChildren.begin(), mChildren.end(), comparator);
-
-	for(auto it = mChildren.cbegin(); it != mChildren.cend(); it++)
+	if (ascending)
 	{
-		if((*it)->getChildren().size() > 0)
-			(*it)->sort(comparator, ascending);
+		std::stable_sort(mChildren.begin(), mChildren.end(), comparator);
+		for(auto it = mChildren.cbegin(); it != mChildren.cend(); it++)
+		{
+			if((*it)->getChildren().size() > 0)
+				(*it)->sort(comparator, ascending);
+		}
+	}
+	else
+	{
+		std::stable_sort(mChildren.rbegin(), mChildren.rend(), comparator);
+		for(auto it = mChildren.rbegin(); it != mChildren.rend(); it++)
+		{
+			if((*it)->getChildren().size() > 0)
+				(*it)->sort(comparator, ascending);
+		}
 	}
-
-	if(!ascending)
-		std::reverse(mChildren.begin(), mChildren.end());
 }
 
 void FileData::sort(const SortType& type)
 {
 	sort(*type.comparisonFunction, type.ascending);
+	mSortDesc = type.description;
 }
 
 void FileData::launchGame(Window* window)
@@ -377,6 +386,6 @@ FileData::SortType getSortTypeFromString(std::string desc) {
 			return sort;
 		}
 	}
-	// if not found default to name, ascending
+	// if not found default to "name, ascending"
 	return FileSorts::SortTypes.at(0);
 }
diff --git a/es-app/src/FileData.h b/es-app/src/FileData.h
@@ -86,8 +86,8 @@ class FileData
 			: comparisonFunction(sortFunction), ascending(sortAscending), description(sortDescription) {}
 	};
 
-	void sort(ComparisonFunction& comparator, bool ascending = true);
 	void sort(const SortType& type);
+	std::string getSortDescription() { return mSortDesc; }
 	MetaDataList metadata;
 
 protected:
@@ -96,13 +96,15 @@ class FileData
 	std::string mSystemName;
 
 private:
+	void sort(ComparisonFunction& comparator, bool ascending = true);
 	FileType mType;
 	std::string mPath;
 	SystemEnvironmentData* mEnvData;
 	SystemData* mSystem;
 	std::unordered_map<std::string,FileData*> mChildrenByFilename;
 	std::vector<FileData*> mChildren;
 	std::vector<FileData*> mFilteredChildren;
+	std::string mSortDesc;
 };
 
 class CollectionFileData : public FileData

diff --git a/es-app/src/FileSorts.cpp b/es-app/src/FileSorts.cpp
@@ -8,8 +8,8 @@ namespace FileSorts
 {
 
 	const FileData::SortType typesArr[] = {
-		FileData::SortType(&compareName, true, "filename, ascending"),
-		FileData::SortType(&compareName, false, "filename, descending"),
+		FileData::SortType(&compareName, true, "name, ascending"),
+		FileData::SortType(&compareName, false, "name, descending"),
 
 		FileData::SortType(&compareRating, true, "rating, ascending"),
 		FileData::SortType(&compareRating, false, "rating, descending"),

diff --git a/es-app/src/guis/GuiGamelistOptions.cpp b/es-app/src/guis/GuiGamelistOptions.cpp
@@ -12,71 +12,79 @@
 #include "SystemData.h"
 
 GuiGamelistOptions::GuiGamelistOptions(Window* window, SystemData* system) : GuiComponent(window),
-	mSystem(system), mMenu(window, "OPTIONS"), fromPlaceholder(false), mFiltersChanged(false)
+	mSystem(system), mMenu(window, "OPTIONS"), mFromPlaceholder(false), mFiltersChanged(false),
+	mJumpToSelected(false), mMetadataChanged(false)
 {
 	addChild(&mMenu);
 
 	// check it's not a placeholder folder - if it is, only show "Filter Options"
 	FileData* file = getGamelist()->getCursor();
-	fromPlaceholder = file->isPlaceHolder();
+	mFromPlaceholder = file->isPlaceHolder();
 	ComponentListRow row;
 
-	if (!fromPlaceholder) {
+	if (!mFromPlaceholder) {
 		// jump to letter
 		row.elements.clear();
 
-		// define supported character range
-		// this range includes all numbers, capital letters, and most reasonable symbols
-		char startChar = '!';
-		char endChar = '_';
+		std::string currentSort = mSystem->getRootFolder()->getSortDescription();
+		std::string reqSort = FileSorts::SortTypes.at(0).description;
 
-		char curChar = (char)toupper(getGamelist()->getCursor()->getSortName()[0]);
-		if(curChar < startChar || curChar > endChar)
-			curChar = startChar;
+		if (currentSort == reqSort) {
+			// "jump to" only avail and correct on sort order "name, asc"
 
-		mJumpToLetterList = std::make_shared<LetterList>(mWindow, "JUMP TO...", false);
-		for (char c = startChar; c <= endChar; c++)
-		{
-			// check if c is a valid first letter in current list
-			const std::vector<FileData*>& files = getGamelist()->getCursor()->getParent()->getChildrenListToDisplay();
-			for (auto file : files)
+			// define supported character range
+			// this range includes all numbers, capital letters, and most reasonable symbols
+			char startChar = '!';
+			char endChar = '_';
+
+			char curChar = (char)toupper(getGamelist()->getCursor()->getSortName()[0]);
+			if(curChar < startChar || curChar > endChar)
+				curChar = startChar;
+
+			mJumpToLetterList = std::make_shared<LetterList>(mWindow, "JUMP TO ...", false);
+			for (char c = startChar; c <= endChar; c++)
 			{
-				char candidate = (char)toupper(file->getSortName()[0]);
-				if (c == candidate)
+				// check if c is a valid first letter in current list
+				const std::vector<FileData*>& files = getGamelist()->getCursor()->getParent()->getChildrenListToDisplay();
+				for (auto file : files)
 				{
-					mJumpToLetterList->add(std::string(1, c), c, c == curChar);
-					break;
+					char candidate = (char)toupper(file->getSortName()[0]);
+					if (c == candidate)
+					{
+						mJumpToLetterList->add(std::string(1, c), c, c == curChar);
+						break;
+					}
 				}
 			}
-		}
 
-		row.addElement(std::make_shared<TextComponent>(mWindow, "JUMP TO...", Font::get(FONT_SIZE_MEDIUM), 0x777777FF), true);
-		row.addElement(mJumpToLetterList, false);
-		row.input_handler = [&](InputConfig* config, Input input) {
-			if(config->isMappedTo("a", input) && input.value)
-			{
-				jumpToLetter();
-				return true;
-			}
-			else if(mJumpToLetterList->input(config, input))
-			{
-				return true;
-			}
-			return false;
-		};
-		mMenu.addRow(row);
+			row.addElement(std::make_shared<TextComponent>(mWindow, "JUMP TO ...", Font::get(FONT_SIZE_MEDIUM), 0x777777FF), true);
+			row.addElement(mJumpToLetterList, false);
+			row.input_handler = [&](InputConfig* config, Input input) {
+				if(config->isMappedTo("a", input) && input.value)
+				{
+					jumpToLetter();
+					return true;
+				}
+				else if(mJumpToLetterList->input(config, input))
+				{
+					return true;
+				}
+				return false;
+			};
+			mMenu.addRow(row);
+		}
 
 		// sort list by
 		mListSort = std::make_shared<SortList>(mWindow, "SORT GAMES BY", false);
 		for(unsigned int i = 0; i < FileSorts::SortTypes.size(); i++)
 		{
 			const FileData::SortType& sort = FileSorts::SortTypes.at(i);
-			mListSort->add(sort.description, &sort, i == 0); // TODO - actually make the sort type persistent
+			mListSort->add(sort.description, &sort, sort.description == currentSort);
 		}
 
 		mMenu.addWithLabel("SORT GAMES BY", mListSort);
 	}
-	
+
 	// show filtered menu
 	if(!Settings::getInstance()->getBool("ForceDisableFilters"))
 	{
@@ -107,7 +115,7 @@ GuiGamelistOptions::GuiGamelistOptions(Window* window, SystemData* system) : Gui
 		mMenu.addRow(row);
 	}
 
-	if (UIModeController::getInstance()->isUIModeFull() && !fromPlaceholder && !(mSystem->isCollection() && file->getType() == FOLDER))
+	if (UIModeController::getInstance()->isUIModeFull() && !mFromPlaceholder && !(mSystem->isCollection() && file->getType() == FOLDER))
 	{
 		row.elements.clear();
 		row.addElement(std::make_shared<TextComponent>(mWindow, "EDIT THIS GAME'S METADATA", Font::get(FONT_SIZE_MEDIUM), 0x777777FF), true);
@@ -123,20 +131,27 @@ GuiGamelistOptions::GuiGamelistOptions(Window* window, SystemData* system) : Gui
 
 GuiGamelistOptions::~GuiGamelistOptions()
 {
+	FileData* root = mSystem->getRootFolder();
 	// apply sort
-	if (!fromPlaceholder) {
-		FileData* root = mSystem->getRootFolder();
-		root->sort(*mListSort->getSelected()); // will also recursively sort children
-
-		// notify that the root folder was sorted
-		getGamelist()->onFileChanged(root, FILE_SORTED);
+	if (!mFromPlaceholder) {
+		const FileData::SortType selectedSort = mJumpToSelected ? FileSorts::SortTypes.at(0) /* force "name, asc" */ : *mListSort->getSelected();
+		if (root->getSortDescription() != selectedSort.description) {
+			root->sort(selectedSort); // will also recursively sort children
+			// notify that the root folder was sorted
+			getGamelist()->onFileChanged(root, FILE_SORTED);
+		}
 	}
-	if (mFiltersChanged)
+
+	if (mFiltersChanged || mMetadataChanged)
 	{
-		// only reload full view if we came from a placeholder
-		// as we need to re-display the remaining elements for whatever new
-		// game is selected
+		// put cursor in the middle if list longer than one display page
+		ViewController::get()->getGameListView(mSystem)->setViewportTop(-1);
+		// re-display the elements for whatever new or renamed game is selected
 		ViewController::get()->reloadGameListView(mSystem);
+		if (mFiltersChanged) {
+			// trigger repaint of cursor and list detail
+			getGamelist()->onFileChanged(root, FILE_SORTED);
+		}
 	}
 }
 
@@ -184,8 +199,14 @@ void GuiGamelistOptions::openMetaDataEd()
 	p.game = file;
 	p.system = file->getSystem();
 
-	std::function<void()> deleteBtnFunc;
+	std::function<void()> saveBtnFunc;
+	saveBtnFunc = [this, file] {
+		ViewController::get()->getGameListView(mSystem)->setViewportTop(-1);
+		mMetadataChanged = true;
+		ViewController::get()->getGameListView(file->getSystem())->onFileChanged(file, FILE_METADATA_CHANGED);
+	};
 
+	std::function<void()> deleteBtnFunc;
 	if (file->getType() == FOLDER)
 	{
 		deleteBtnFunc = NULL;
@@ -198,8 +219,7 @@ void GuiGamelistOptions::openMetaDataEd()
 		};
 	}
 
-	mWindow->pushGui(new GuiMetaDataEd(mWindow, &file->metadata, file->metadata.getMDD(), p, Utils::FileSystem::getFileName(file->getPath()),
-		std::bind(&IGameListView::onFileChanged, ViewController::get()->getGameListView(file->getSystem()).get(), file, FILE_METADATA_CHANGED), deleteBtnFunc));
+	mWindow->pushGui(new GuiMetaDataEd(mWindow, &file->metadata, file->metadata.getMDD(), p, Utils::FileSystem::getFileName(file->getPath()), saveBtnFunc, deleteBtnFunc));
 }
 
 void GuiGamelistOptions::jumpToLetter()
@@ -234,6 +254,9 @@ void GuiGamelistOptions::jumpToLetter()
 
 	gamelist->setCursor(files.at(mid));
 
+	// flag to force default sort order "name, asc", if user changed the sortorder in the options dialog
+	mJumpToSelected = true;
+
 	delete this;
 }
 

diff --git a/es-app/src/guis/GuiGamelistOptions.h b/es-app/src/guis/GuiGamelistOptions.h
@@ -37,8 +37,10 @@ class GuiGamelistOptions : public GuiComponent
 
 	SystemData* mSystem;
 	IGameListView* getGamelist();
-	bool fromPlaceholder;
+	bool mFromPlaceholder;
 	bool mFiltersChanged;
+	bool mJumpToSelected;
+	bool mMetadataChanged;
 };
 
 #endif // ES_APP_GUIS_GUI_GAME_LIST_OPTIONS_H
diff --git a/es-app/src/guis/GuiMetaDataEd.cpp b/es-app/src/guis/GuiMetaDataEd.cpp
@@ -28,7 +28,8 @@ GuiMetaDataEd::GuiMetaDataEd(Window* window, MetaDataList* md, const std::vector
 
 	mMetaDataDecl(mdd),
 	mMetaData(md),
-	mSavedCallback(saveCallback), mDeleteFunc(deleteFunc)
+	mSavedCallback(saveCallback),
+	mDeleteFunc(deleteFunc)
 {
 	addChild(&mBackground);
 	addChild(&mGrid);