RetroPie / EmulationStation

A Fork of Emulation Station for RetroPie. Emulation Station is a flexible emulator front-end supporting keyboardless navigation and custom system themes.
Other
850 stars 340 forks source link

Minor suggestions for "Collection use during Screensaver" #853

Closed Gemba closed 5 months ago

Gemba commented 5 months ago

Minor suggestions:

  1. Menuitem text shortened to fit on 4:3 or 5:4 displays.
  2. renamed to "Favorites".
  3. Simplified applySettings() as I could not manage to defaultScreenSaverCollection->getSelected() to be not set.
  4. setupScreenSaverEditingCollection() renamed to handleScreenSaverEditingCollection().

Could you have a look @pjft ? Most crucial is likely no.3.

pjft commented 5 months ago

Thanks for this. I am fine with 1 and 4. Please check my comments for 2 and 3 - if I understood correctly, 2. is not correct. For 3, I completely follow the intent - that's what it was supposed to be. I just played it safe here as I managed to get inconsistent scenarios during testing. One may say "well, but that requires users to do things outside of ES", but I'd rather be safe than sorry. Still, I'll test the changes later and report back, regarding 3.

Gemba commented 5 months ago

Another thing I noticed, but is not related to the recent features added, is that the custom collections are sorted case sensitive. This may confuse the user as ES uses uppercase for displaying.

This method could get an optional parameter or the class gets an separate method for sorting to remediate this if needed/you think it is useful. (This would also make the first iteration for always having a valid selection for the screensaver collection obsolete. I am referring to my suggestion above).

pjft commented 5 months ago

Thanks for the comments - I replied to them, I believe.

I like the suggestion for case insensitiveness. The main reason for hesitation is that ES is only case insensitive if the theme chooses to be, via <forceUppercase>1</forceUppercase>, though indeed, most themes we use are like that, so I see why you're suggesting the change. Also, the menu is, by default, case insensitive, so maybe it would make sense to fix the sorting on the menus, regardless of the rest of the ES experience.

I'd be open to that change as a separate PR, for sure, and we'd assess. I'm thinking, though: wouldn't this also be solvable simply by adding the elements in the order we want, though? We could probably just sort them before adding them, and not have to mess with the component itself, as that may introduce unintended changes. Thoughts?

Gemba commented 5 months ago

Reverted 2. and 3.

About the sorting: I am leaning towards subclassing OptionsListComponent for this and for this Dialog/Menu. That way the signalling to the 'collection during screensaver' and keeping that selection correct can be implemented (and the user can get a toast when 'collection during screensaver' has been altered due to change in the selection of enabled custom collections), eventually it makes the applySettings() straightforward.

pjft commented 5 months ago

Thanks. Happy with merging this. Can you squash all of these into a single commit?

Gemba commented 5 months ago

Done. :)

pjft commented 5 months ago

Thank you!