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
858 stars 344 forks source link

Fix cornercase with cursor placement after screensaver 'A' btn press #850

Closed Gemba closed 8 months ago

Gemba commented 8 months ago

Context: https://retropie.org.uk/forum/topic/35041/

pjft commented 8 months ago

Thanks for this! I'll test later, but looks good. A question: should (or shouldn't) the code added to the SystemScreenSaver class rather be inside the setCursor function in general?

I kind of expected setCursor to work in general (i.e. set the cursor and not leave it in an invalid state), and it might end up being used elsewhere in the future (with people running into similar behavior without no recollection of the cause). Thoughts on that?

Gemba commented 8 months ago

From my understanding the ViewController is the controller, the ISimpleGameListView and subclasses are the view and they are delegating to IList (and TextListComponent). The SystemScreenSaver I see as additional controller. But in general I find it difficult to identify the architecture of ES. I would not move it around, as the refactoring/rewriting it can get really awkward. And there are not automated tests to vet for any regressions on this endeavour.

So, the setCursor on the View called from the SystemScreensaver delegates to the TextListComponent, which knows about the UI dimensions and visible list section aso. It is not that off for me. To have the logic transferred to setCursor() there must also be the context provided (ie. user navigation, launch/return from game, return (with 'A') from screensaver). I have not envisioned how this can be done in an elegant way, without breaking other stuff or introducing regressions. But I agree the setCursor function is used in contexts (features added over time) which haven't been foreseen by the original author and thus does not work as expected with calling it alone.

pjft commented 8 months ago

That's a fair assessment. Let me think about that a bit further later on - thank you!

Gemba commented 8 months ago

Superseded with #851. Closing it unmerged.