Changes to maintain and persist <folder/> information in a gamelist: #841

Gemba · 2023-12-19T14:30:46Z

folder elements are loaded when present in a gamelist and if they match the filesystem representation
missing optional folder elements are rendered empty in theme (and not "unknown")
As before non-existing folder elements are generated by ES, with mandatory elements <path/> and <name/>, but with this PR, whenever metadata is edited for this folder object (FileData class) it is persisted
Editiing metadata on existing <folder/> gets persisted too
UI metadata edit: layout fix that renders entered metadata not centered when returning from editing
UI metadata edit: format tooltip for date format in release date field

Gemba · 2023-12-19T14:31:22Z

As it involved several files I will comment on the crucial changes.

es-app/src/Gamelist.cpp

Gemba · 2023-12-19T14:35:52Z

es-app/src/Gamelist.cpp

@@ -127,7 +137,7 @@ void parseGamelist(SystemData* system)
 			}

 			// Check whether the file's extension is allowed in the system
-			if (std::find(allowedExtensions.cbegin(), allowedExtensions.cend(), Utils::FileSystem::getExtension(path)) == allowedExtensions.cend())
+			if (i == 0 /*game*/ && std::find(allowedExtensions.cbegin(), allowedExtensions.cend(), Utils::FileSystem::getExtension(path)) == allowedExtensions.cend())


Only be strict on type game FileData, without this check no elements will be loaded into ES from the gamelist.

Gemba · 2023-12-19T14:37:54Z

es-app/src/Gamelist.cpp

@@ -142,7 +152,7 @@ void parseGamelist(SystemData* system)
 			else if(!file->isArcadeAsset())
 			{
 				std::string defaultName = file->metadata.get("name");
-				file->metadata = MetaDataList::createFromXML(GAME_METADATA, fileNode, relativeTo);
+				file->metadata = MetaDataList::createFromXML(i == 0 ? GAME_METADATA : FOLDER_METADATA, fileNode, relativeTo);


This flag got mandatory to differentiate between recognized subelements of and

Gemba · 2023-12-19T14:41:30Z

es-app/src/MetaData.cpp

-	{"publisher",   MD_STRING,              "unknown",          false,      "publisher",            "enter game publisher"},
-	{"genre",       MD_STRING,              "unknown",          false,      "genre",                "enter game genre"},
-	{"players",     MD_INT,                 "1",                false,      "players",              "enter number of players"}
+	{"rating",      MD_RATING,              "",                 true,       "rating",               "enter rating"},


Hide (statistics=true) some elements for editing and define default values when not defined in XML. 1970-01-02 was picked as 1970-01-01 is already taken for games without : "not-a-date-time" will put 1970-01-01 which then is rendered as "unknown". As a folder most of the time has not releasdate I wanted it to render as blank, this is what 1970-01-02 serves for.

I understand the decision, but I am not fully thrilled with the approach here. 1970-01-01 is, effectively, a null date. 1970-01-02 is an actual value that we are choosing to have a different behavior. It's pretty much begging to be caught in an edge case at some point in time when nobody else knows why it was here and what for :)
What other options could we have here, potentially even reusing 1970-01-01?
Also, happy new year!

Thanks, hope you started well too.

I have considered to reuse 1970-01-01 in some form, but the crucial part is that the flag folder/game is only available in the Metadata / Metadatalist class (which is used by the Filedata class). For rendering it is then applied to Datetimecomponent class (setValue()) e.g. in Detailgamelistview class. In the setValue() is then the display format decided (in getDisplayString()).

Which means to me one must either put render/display information in the Metadataclass and or overload the setValue() to allow a Game/Folder flag for reusing 1970-01-01 to be displayed as "" for folders.

Neither path did not make sense to me. Thus I have chosen 1970-01-02.

On a folder the releasedate information can not be edited from within ES. An user must set it deliberately via gamlist editing.

I also assume there will be no game released on one of these two dates for any system which are also supported by ES.

However, if users want one of these dates displayed (instead of "unknown"/""), they could append a non zero time to the releasedate (again via gamelist.xml editing).

I made the reference/usage of 1970-01-02 more coherent, by referring to the constant. The usage can be more easily reconstructed with grep or an IDE.

HTH

Epilogue: I found some corner cases which promted the user to "Save changes?" even when there are no changes (an efffect that the initial author assumed that any statistical (non editable) value are at the end of the MetaDataDecl (in Metadata.class). I fixed it to be more robust, when statistical values are in between.

I also noted that the releasedate in die ES editor rolls over at the end of the 32bit Unix-time-epoch. But that is a different story. ;-)

Gemba · 2023-12-19T14:43:14Z

es-app/src/guis/GuiMetaDataEd.cpp

 		// create ed and add it (and any related components) to mMenu
 		// ed's value will be set below
 		ComponentListRow row;
-		auto lbl = std::make_shared<TextComponent>(mWindow, Utils::String::toUpper(iter->displayName), Font::get(FONT_SIZE_SMALL), 0x777777FF);
+		auto lblTxt = Utils::String::toUpper(iter->displayName);


Add a hint about the date format, as the MM/DD/YY can be confusing if the day-of-month is less than 12.

Gemba · 2023-12-19T14:43:41Z

es-app/src/guis/GuiMetaDataEd.cpp

@@ -111,6 +117,8 @@ GuiMetaDataEd::GuiMetaDataEd(Window* window, MetaDataList* md, const std::vector
 			{
 				// MD_STRING
 				ed = std::make_shared<TextComponent>(window, "", Font::get(FONT_SIZE_SMALL, FONT_PATH_LIGHT), 0x777777FF, ALIGN_RIGHT);
+				const float height = lbl->getSize().y() * 0.71f;


This one fixes the text misalignment after editing a metadata.

es-app/src/guis/GuiMetaDataEd.cpp

Gemba · 2023-12-19T14:45:26Z

es-core/src/components/DateTimeComponent.cpp

@@ -47,9 +49,13 @@ void DateTimeComponent::onTextChanged()

 std::string DateTimeComponent::getDisplayString() const
 {
+	if(std::difftime(mTime.getTime(), Utils::Time::BLANK_DATE) == 0.0) {


If 1970-01-02 then render blank.

pjft · 2024-01-26T20:12:48Z

@Gemba pinging this, in case you were waiting for me to make progress here. I am ok with the changes, just had the couple minor questions I added there. No pressure though - just wanted to make sure this wasn't in a limbo because of me. Have a great weekend.

Gemba · 2024-01-27T10:05:02Z

Thanks. I have read through this issue twice. Did I miss a question to answer? I you can recap the one(s) which escaped my attention it would be nice.

pjft · 2024-01-27T13:22:58Z

Sure. Main questions I had:

Gamelist.cpp: std::string relative = Utils::FileSystem::removeCommonPath(path, root->getPath(), contains, true); - root->getPath() should be systempath, or am I missing something?
Why don't we allow ratings for the folders?

Gemba · 2024-01-28T08:50:27Z

* Gamelist.cpp: std::string relative = Utils::FileSystem::removeCommonPath(path, root->getPath(), contains, true); - root->getPath() should be systempath, or am I missing something?

You are right. Well spotted. Done.

* Why don't we allow ratings for the folders?

It was more an assumption of mine. I don't stick to that. Enabled the rating edit on folders. Thanks for nudging me.

pjft · 2024-02-06T20:18:30Z

Thanks. I just tested this and, from what I was able to test on my end, I'm good with merging. Just a minor question I asked regarding a label, but I'm good to go next. I don't have folders on my setup, though, so it should be test run by more in the community if you can share it there!

Gemba · 2024-02-06T21:49:34Z

Will ask for more testers in the forum. Just let me know: Before the merge or after the merge?

pjft · 2024-02-06T22:01:08Z

Afterwards is fine, it's just to make sure the different scenarios get covered.

Do check my comment on the variable and squash as appropriate. Let me know when it's good to go.

Gemba · 2024-02-07T07:59:27Z

Afterwards is fine, it's just to make sure the different scenarios get covered.

Ok.

Do check my comment on the variable and squash as appropriate. Let me know when it's good to go.

I sure will. But I am really sorry to bother you again, I cannot spot your comment on the label/variable. Would you be so kind to highlight/repeat it? Thanks.

pjft · 2024-02-07T08:39:56Z

GuiMetaDataEd.cpp:41 - why ROM and not GAME?

Gemba · 2024-02-07T09:10:57Z

GuiMetaDataEd.cpp:41 - why ROM and not GAME?

Ah. Thanks. I put "ROM" here deliberatly as the filename with extension will be shown. The ROM's display title is shown first in the edit fields.

pjft · 2024-02-07T10:12:21Z

If I'm reading this correctly, then we should probably keep GAME for a few reasons:

The ROM's display title shows up as "NAME", it doesn't mention ROM anywhere on the screen.
That's a new concept for the end user, as we never refer to the games as ROMs (searching the code will only yield one public instance of "ROM" as an exposed word, and it's in the command-line header ""EmulationStation, a graphical front-end for ROM browsing.". There's also something in an error message or something, and in Readme.md, but nothing really in the UI.
The edit entry for that screen says "EDIT GAME METADATA" (which, probably, would now say "GAME" or "FOLDER" depending on the type if you feel like it)
Technically, you can have games that aren't ROMs (executables, launch scripts, etc). But, at the end of the day, it really is about consistency with the rest of the UI - we refer to entries as "games" and "folders", and my preference would be staying as such.

A bit long, apologies, but hopefully will relay where I'm coming from here.

1. folder elements are loaded when present in a gamelist and if they match the filesystem representation 2. missing optional folder elements are rendered empty in theme (and not "unknown") 3. As before non-existing folder elements are generated by ES, with mandatory elements <path/> and <name/>, but with this PR, whenever metadata is edited for this folder object (FileData class) it is persisted 4. Editiing metadata on existing <folder/> gets persisted too 5. UI metadata edit: layout fix that renders entered metadata not centered when returning from editing 6. UI metadata edit: format tooltip for date format in release date field

Gemba · 2024-02-07T16:54:16Z

I am ok with your justification, better avoid "surprises" for the end-user and step-back a little on technical preciseness. Did adjust the subtitle to "GAME: ..."

pjft · 2024-02-07T18:01:05Z

Thank you.

Relates to the PR RetroPie#841 after the discussion in RetroPie#861, but code changes only needed in Skyscraper.

Gemba commented Dec 19, 2023

View reviewed changes

Gemba force-pushed the fb_gamelist_folder_persistence branch from 5f6acec to c87b0b7 Compare January 5, 2024 16:02

Gemba force-pushed the fb_gamelist_folder_persistence branch from c87b0b7 to 8c554e0 Compare January 28, 2024 08:43

Gemba force-pushed the fb_gamelist_folder_persistence branch from 8c554e0 to a113b22 Compare February 7, 2024 16:51

pjft merged commit aebbf19 into RetroPie:master Feb 7, 2024

Gemba added a commit to Gemba/EmulationStation that referenced this pull request Feb 18, 2024

More comments and a correction for a display label.

77d7e5b

Relates to the PR RetroPie#841 after the discussion in RetroPie#861, but code changes only needed in Skyscraper.

This was referenced Feb 18, 2024

Crash when loading gamelists xml #861

Closed

Fix initialization of metadata #862

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to maintain and persist <folder/> information in a gamelist: #841

Changes to maintain and persist <folder/> information in a gamelist: #841

Gemba commented Dec 19, 2023 •

edited

Loading

Gemba commented Dec 19, 2023

Gemba Dec 19, 2023

Gemba Dec 19, 2023

Gemba Dec 19, 2023

pjft Jan 2, 2024

Gemba Jan 5, 2024 •

edited

Loading

pjft Jan 12, 2024

Gemba Dec 19, 2023

Gemba Dec 19, 2023

Gemba Dec 19, 2023

pjft commented Jan 26, 2024

Gemba commented Jan 27, 2024

pjft commented Jan 27, 2024

Gemba commented Jan 28, 2024

pjft commented Feb 6, 2024

Gemba commented Feb 6, 2024

pjft commented Feb 6, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Changes to maintain and persist <folder/> information in a gamelist: #841

Changes to maintain and persist <folder/> information in a gamelist: #841

Conversation

Gemba commented Dec 19, 2023 • edited Loading

Gemba commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gemba Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjft commented Jan 26, 2024

Gemba commented Jan 27, 2024

pjft commented Jan 27, 2024

Gemba commented Jan 28, 2024

pjft commented Feb 6, 2024

Gemba commented Feb 6, 2024

pjft commented Feb 6, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Gemba commented Feb 7, 2024

pjft commented Feb 7, 2024

Gemba commented Dec 19, 2023 •

edited

Loading

Gemba Jan 5, 2024 •

edited

Loading