Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to ignore common leading articles in Filename sort #729

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

PhilaPhan80
Copy link

@PhilaPhan80 PhilaPhan80 commented Jan 29, 2021

This enhancement replaces #726 by adding an option called Ignore Articles (Name Sort Only) to the START menu. When enabled, common leading articles are ignored in both ascending and descending order while sorting each system's game titles, similar to popular multimedia-based systems. The comma-delimited list of articles is stored within the es_settings.cfg file as LeadingArticles. By default, common English articles "a", "an", and "the" are ignored, and others may be added to the list manually. (This also allows more articles to be easily added to the default setting going forward.) The option itself is persisted as IgnoreLeadingArticles.

Additionally, all file sorting options are consolidated into one application-wide location by moving the Sort Games By selection from the SELECT menu to the START menu (see attached image). Upon value change, the selection is applied to all systems across the board, and the SortType ID is stored within the settings file so it can be restored upon restart/reboot.

Lastly, a popup message is displayed whenever sorting has been re-performed in response to either of the above selections.

Additionally, a popup message is displayed whenever sorting has been re-performed in response to the selection above.

Examples:

[SortType = FILENAME ASC, IgnoreLeadingArticles = false]

A Boy and His Blob
Adventures of Lolo
Marble Madness
The Legend of Zelda
Yoshi's Cookie

[SortType = FILENAME ASC, IgnoreLeadingArticles = true]

Adventures of Lolo
A Boy and His Blob   <-- article ignored
The Legend of Zelda   <-- article ignored
Marble Madness
Yoshi's Cookie

Changes:

  • Adds Ignore Articles (Name Sort Only) option to the UI Settings (START) menu
  • Adds IgnoreLeadingArticles boolean to settings
  • Adds LeadingArticles string to settings
  • Applies IgnoreLeadingArticles selection across all systems on value change and ES startup
  • Moves Sort Games By option from the Options (SELECT) menu to the UI Settings (START) menu
  • Adds SortType enum to FileSorts namespace so values can be uniquely referenced by ID
  • Adds SortType int to settings
  • Applies SortType selection across all systems on value change and ES startup

Notes:

  • IgnoreLeadingArticles logic cannot be applied to sort types other than Filename ASC/DESC due to the way the sorting logic compares two files without any outside knowledge of broader intent or scope. Currently, other sort types group files by their associated comparator (e.g. Rating, Times Played, Developer, etc.). Ideally, though, the names within each group would be alphabetized ASC/DESC. For example, sorting by year would display something like 1998 A-Z, 1995 A-Z, 1986 A-Z, etc. (or 1998 Z-A, 1995 Z-A, 1986 Z-A if DESC). Without that logic in place, IgnoreLeadingArticles currently doesn't make sense outside the scope of Filename.

image

@pjft
Copy link
Collaborator

pjft commented Jan 29, 2021

Wow. Ok, a lot to unpack here - thanks for putting this together!

I have not yet read through the code, much less tested it, but I wanted to add some remarks and ask about some testing scenarios, as the sorting logic isn't always as trivial as it seems from my previous interactions with it.

  1. There is a use case for a session-only, system-only sorting functionality. Moving it to the start menu, from what you describe, seems to both violate the current use case, unless I'm reading it wrong. It seems to persist it in options, and apply it to all systems at once. I would personally not be supporting of making this change lightly myself, as it is a departure from long-standing functionality that users have come to expect. That being said, having an option for "default sorting" that's system-wide (and defaults to "alphabetical") would certainly be a welcome addition.
  2. How does this affect/respect the "Last Played" auto-system sorting? That's meant to always be sorted by "last played, descending". Have you tested that, and how is it addressed?
  3. How does your sorting persist when using filters? I recall that filters or anything in the "select" menu would by default change the sorting back to alphabetical. Probably it's no longer an issue now that you moved it to "Start", but wanted to ask the question.

Other than these remarks - and the item in point number 1, this looks like something that can be useful. Thanks for putting it together, and do share your feedback and notes on these remarks, especially as I may have misunderstood some of them.

Have a great weekend!

@PhilaPhan80
Copy link
Author

Thanks for checking this so quickly. I agree that the sorting logic can be complex in some ways.

Allow me to break my answers down a bit...

1a. I was not aware that there are existing use cases for session-only nor system-only sorting. I actually tried to think of reasons why someone would want to do either or both, and it seemed (on the surface) that it wasn't as useful as retaining the information and restoring it. I also noticed there's a TODO remark on line 74 of /es-app/src/guis/GuiGamelistOptions.cpp that reads, actually make the sort type persistent so I figured I'd take a shot at making it work.

1b. With that said, I'm curious. What are the use cases for both of the above (session-only and system-only)? I'd like to understand them better.

1c. I concur that it's a departure from long-standing functionality, and though I take that very seriously from a UX perspective, it seemed like the right shift to make given the scenario.

1d. When you mention an option for "default sorting", are you suggesting that the two would work in tandem? e.g. The START menu would retain the added field and apply it as I'm doing it within this PR, but the SELECT menu would also retain its current field which would act as a session-only, system-only override? If so, I think something like that could work.

  1. I assume you're referring to the "Last Played, Ascending" and "Last Played, Descending" SortType options currently within the SELECT menu. Yes, I've tested the new IgnoreLeadingArticles option against these, and they work well together. Basically, the new option only applies to the "Filename, Ascending" and "Filename, Descending" options due to the way the sorting logic currently works (see description above). Since it's not even called for the other SortType options, they continue to work the same way they do today (with the exception of them persisting, which we discussed above).

  2. I did not test the new option with any filtering, as I'm not intimately familiar with that feature. I'd be happy to address any feedback regarding test results.

I'm interested in learning more and looking forward to your feedback, especially on point 1d.

Enjoy the rest of your weekend as well!

@pjft
Copy link
Collaborator

pjft commented Jan 31, 2021

Hi. Thanks for the reply, let me clarify what I was raising.

1a, 1b and 1c. Well, I mean, yes: the current sorting has been session-only and system-only, and meant to address session specific needs. For instance, in a session the user wants to sort the games by rating to find a game to play that is highly rates, or sort by release date to play games from a specific date range. I'm just sharing the two I use the most (other than alphabetical, of course, which is the easiest to navigate by default, I imagine). If you'd want to learn more, certainly asking in the forums for "what do users use the sorting option for" could help better understand what is (and what isn't) used. But right now, having it always persist means that if the user changes the sorting for whatever reason on its session, he needs to deliberately go back and revert it every time, otherwise risking having the sort of every single system persist in a different manner.

1d. Yes, that was my immediate suggestion, but I didn't really think this through in any extensive manner. I was just looking for a way for this change to work, while respecting the current use cases. I'm not saying it's the best solution or that it should be done - just raising it as a possibility.

  1. I'm not asking about the IgnoreLeadingArticles working with the Last Played collection, but rather the fact that there's an automatic collection - "Last Played" that is by default sorted by "Last Played, descending". How does the system-wide-default-sorting-selection affect that specific system, that should always be sorted by Last Played, descending (unless the user explicitly, in the session, chooses to sort it some other way for whatever reason). See more here: https://retropie.org.uk/docs/EmulationStation/

  2. In the current implementation - likely because of it being in the Select menu - whenever the user opens the select menu and exists the list gets automatically sorted alphabetically, unless you explicitly choose a different sorting order every time you use the menu. It might be related to the sorting being applied on exit of the Select menu, but wanted to check if that messed up the now-system-wide-start-menu sorting option.

Just that. Let me know if this helps, or if anything isn't clear.

Thanks, and have a great week ahead!

@PhilaPhan80
Copy link
Author

More follow-ups:

1a-c. Thanks, I understand what you mean now.

1d. Understood that it was an off-the-cuff solution. The more I think of it, though, I believe it could work. Functionally, it makes sense, and I think it could be separated in such a way that it would work.

  1. I just tested this, and there is no conflict. Line 739 within \es-app\src\CollectionSystemManager.cpp runs a sort on the collection that's being built, and that sort is hard-coded as last played, descending (as you noted). Since this doesn't look to the SortType setting within es_settings.cfg, it's completely unaware that the user has chosen an application-wide sort type.

  2. Correct. The reason it sorts alphabetically is because on exit of the SELECT menu, it applies whatever is selected as the sort type, and since the sort type is defaulted to Filename, Ascending, exiting the menu without choosing a different option applies that selection as the new sort type. Since the option has been moved to the START menu, this action never takes place, and thus, no conflict. However, if we were to restore it as a "temporary override" (1d above), I could see it affecting the sorting of the filtered list (which, I imagine is what it would do today anyway).

With all of that said, it sounds like there are two available options:

  1. Update this PR to include/restore the "temporary override" scenario discussed in 1d above
  2. Split this PR into two -- one for the IgnoreLeadingArticles option, and one for the scenario discussed in 1d above

What are your thoughts on that?

@pjft
Copy link
Collaborator

pjft commented Feb 1, 2021

Thanks for the reply - and thanks for checking those scenarios.

I agree with the split of the PRs, and then submitting those as different PRs indeed for the sake of traceability and future maintainability.

In regards to the Collection Manager, see if it should be updated in any way to conform with your enum changes.

Best.

@PhilaPhan80 PhilaPhan80 changed the title Add option to ignore common leading articles in Filename sort (and group SortType selection within the same menu) Add option to ignore common leading articles in Filename sort Feb 1, 2021
@PhilaPhan80
Copy link
Author

PhilaPhan80 commented Feb 1, 2021

@pjft I've isolated the IgnoreLeadingArticles code and updated the original post to reflect the changes. I've only struck things out to retain context for the conversation. This one should be ready to merge.

I'll submit a separate PR for the other piece once I get a chance to test it further.

Thanks for stepping through this with me!

@pjft
Copy link
Collaborator

pjft commented Feb 9, 2021

Thanks.

I currently don't have access to a Pi, but I will go through the code soon.

{
//Apply sort recursively
FileData* root = (*it)->getRootFolder();
root->sort(FileSorts::SortTypes.at(Settings::getInstance()->getInt("SortType")));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PhilaPhan80 I was testing this today, and it works well but this setting should be part of the future PR, not this one, correct?

If you can fix this I'll test it again and merge it, as I have a Pi3 available for a while for these.

Apologies for the delay, and thanks for putting this together.

@PhilaPhan80
Copy link
Author

@pjft Thanks for diving deeper into this. The section you highlighted is intended to immediately sort all systems so that the user's choice is reflected. I couldn't find an existing way to trigger a sort across all systems, so I copied some of the sorting code and placed it here within a for loop. If there's a more streamlined way to do this, I'd be happy to replace what I have with the better solution. It's definitely for this PR, though.

@pjft
Copy link
Collaborator

pjft commented Feb 28, 2021

Apologies - what I meant was that "Settings::getInstance()->getInt("SortType"))" isn't defined in this PR. I got that error when turning on the option - "lvl0: Tried to use unset setting SortType!". :)

@PhilaPhan80
Copy link
Author

Oh, gotcha. Sorry about that. I've replaced it with the following so it picks up on each system's specific sort preference.
root->sort(FileSorts::SortTypes.at(getSortTypeFromString(root->getSortName())));

@pjft
Copy link
Collaborator

pjft commented Feb 28, 2021

Thanks. Tested it, works fine.

If you'd want to squash the commits into a single one, I'll merge it tomorrow. If you want to announce it in the forums for testing it'd be awesome for further testing then.

Thank you.

@PhilaPhan80
Copy link
Author

Great! Just squashed it. Is there a specific place where I should announce that? (I've never used the forums here.)

@pjft
Copy link
Collaborator

pjft commented Mar 1, 2021

In the RetroPie forums, at https://retropie.org.uk/forum/ - just create a post in "Ideas and Development", and let them know it's on emulationstation-dev (after we merge it).

Apologies for nitpicking, but can you change the commit name and description to be reflective of what's being added here, as that's what will stay in the history for others to trace things down? :)

@PhilaPhan80
Copy link
Author

@pjft Thanks! I'll wait to hear from you before posting anything to the forum.

I'm sorry, I'm not well-versed in Git, and I'm doing everything through GitHub.com and VS Code. I'm not sure how to change the commit name and description. If you can offer some sample commands, I'll be happy to perform them.

@pjft
Copy link
Collaborator

pjft commented Mar 2, 2021

Of course, not a bother.

From https://docs.github.com/en/github/committing-changes-to-your-project/changing-a-commit-message , if you have git installed on your computer I suspect you'd do the following on the project directory:

git --amend 

them write the message in the right commit, and finally

git push --force

But let me know if you run into any trouble. Thank you!

@PhilaPhan80
Copy link
Author

@pjft Thanks! I think that worked. Let me know if there's anything else you need me to do.

@pjft pjft merged commit 28faf89 into RetroPie:master Mar 2, 2021
@pjft
Copy link
Collaborator

pjft commented Mar 2, 2021

Thank you for sending this over! Feel free to share the good news in the forums, I'm sure others will benefit from this change.

It's available in the emulationstation-dev package.

Thanks.

@HVR88
Copy link

HVR88 commented Jul 16, 2021

Any ideas of when an up-to-date public build of ES is coming? Ignoring articles on sort is a game changer (and part of basic title sorting 101)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants