[Ready to Merge] UIModeController Class

[zigurana]

Refactoring: Split PasskeyListener and UI mode monitoring into new separate class: UIModeController (singleton).

The UIMode code started to end up in too many different locations for my liking, so I practiced implementing a singleton class for it.
The one functional improvement this brings, is that the passkey phrase (as reported by the new popup when changing the UI mode) is now pulled from the passkey actually stored in settings.xml:

Otherwise, this PR should have no functional changes.

I have one open question for anyone with an interest in this:
I was trying to insert the unicode glyph for the arrow symbols here, but I have no idea how to get that working, anyone has an idea?

[tomaz82]

I can get that working for you, later tonight, should add an UTF8Util.cpp/h that does the conversion both ways. If it might be useful in other places, if it's just for these 4 then I can easely get you the utf-8 hex for those 4 arrows. ( that u-2191 is utf-16 hex and as utf-8 hex it's e28691 )

You could try doing:
out += "\xe2\x86\x91"; // up 2191
out += "\xe2\x86\x93"; // down 2193
out += "\xe2\x86\x90"; // left 2190
out += "\xe2\x86\x92"; // right 2192

And no, it's more complicated than just replacing 21 with e286, that the 90, 91, 92 and 93 stayed the same was just pure conincidence.

[zigurana]

Whohoo!

Regarding the support functions for UTF-8, some of that might already be part of font.cpp

[tomaz82]

@zigurana Yeah I know, and I think it should be split out to a Unicode.cpp or whatever, with the ability to convert both ways. It does utf-8 to utf-16 ( kinda ), and for this you needed utf-16 to utf-8 ( kinda )

I'll look into it tonight.

[joolswills]

Note this need rebasing currently due to other PRs. Also #306 if accepted would probably affect this PR.

[zigurana]

The unicode characters for arrows are not included in the default ES font, and the default fallback font is not installed.
And so I am currently investigating suitable alternatives for the default fallback fonts, hopefully I will make some progress this week.

[pjft]

An odd suggestion, but perhaps worth considering. Have you tried actually pasting the arrow character in the string in the code directly? I recall doing something like that for a special character for the help popup in OMX Player (I think the "bullet" one) and it worked well.

See here:

https://github.com/RetroPie/EmulationStation/blob/master/es-app/src/guis/GuiVideoScreensaverOptions.cpp#L74

Just a thought.

[zigurana]

Yeah, I tried it, but it causes the whole file to be saved with a different code-page character encoding, and I had no idea what effect that could have. Also, it still does not work, if the font used for rendering does not support the glyph.

But thank you for the suggestion, it is indeed the easiest way to get the 'code' for that specific glyph in de string.

[zigurana]

Ok, this is now ready to go, I've fixed the fallback fonts to include some that are installed by default on the pi.

[tomaz82]

Not sure what this PR does, but at a quick glance it does one big nono in coding, you access es-app code from es-core, that's something that should never be allowed :/

es-core is a support library for es-app, es-app requires es-core to function, but es-core should not require anything from es-app.

[hex007]

es-core should not be dependent on es-app. I agree.

[zigurana]

I agree, but I have difficulty finding a good solution.

To explain : I need to use the ViewController methods (es-app) to reload all views, but I also need the inputs at a very early stage (before they can be consumed by Guicomponents). That happens now in Window (es-core).

As most of these components live in es-core (but not all!), it's difficult to find a clean cut there.

As a side note : what is the benefit of this split between es-core and es-app? There is only a single user of core, and it seems unlikely that there will be any others soon.

So, I am open to fix this, but I need some pointers.

[zigurana]

Ok, I think that I can safely move it to es-app, and hook into the ViewController::input() method, as that is the top of the GuiComponent stack anyways. I'll come with an alternative later today

[zigurana]

Ok, that was not as complicated as I thought. I've moved the UIModeController class to the same directory as ViewController (with which it has the closest interactions anyways). For the UIModeController::listener(), method, I intercept the inputs from ViewController, as it is the top of the stack anyways, so all inputs should still be available.
Thanks for kicking my butt, I hope this time the PR is acceptable.

[zigurana]

Wait, I think I have the non-saving settings thing as well, hold this for now please.

[tomaz82]

Okay, just a few personal opinions:

The CMakeLists, why tabs when everything else there use 4 spaces?
Why was the ResourceManager::GetInstance::fileExists changed?

And a really personal preference, when I cleaned up the header includes, I set it up in alphabetical order, folders first, then any external includes last. Also, always include files based on root folder, so folder/whatever.h instead of just whatever.h, it helps when several work on a project, and when a project grows, if everything is based on the root folder. Again, just my personal preference.

[zigurana]

Hi Tomaz, thanks for looking at this.

Regarding the spaces: short reason: because my IDE at work has tabs set to 4 spaces, and I forgot. Longer reason, because we should decide on a type of formatting, start using Clang or something, and -never- talk about whitespaces again (only slightly joking).

The ResourceManager::getInstance()->fileExists(path) is a check on the availability of a file (designated by its path), as a resource already in memory, rather than available as a file on the system. We want the latter, because the fallback fonts are not loaded into memory be default, but we want to be able to get them when someone uses a fancy symbol.

I'll change the includes, I have zero preference, so I'll follow yours.

[tomaz82]

ResourceManager::getInstance()->fileExists(path) does check if it's an embedded file that's being asked for, then it does a fs::exists, so I'd suggest you revert those changes for now.

[zigurana]

Ok, updated the PR, reverted the changes to Font.cpp to call ResourceManager's fileExists() method again. UIMode settings are now saved after the popupbox has been shown (and accepted).

[zigurana]

@tomaz82, if you don't have any other concerns, I think this is ready to merge, agreed?

[hex007]

As a side note : what is the benefit of this split between es-core and es-app? There is only a single user of core, and it seems unlikely that there will be any others soon.

@joolswills Might know why the split was introduced

[joolswills]

I don't recall anything. Probably was before we forked it?

[zigurana]

No matter, I am not changing it for this PR, that's for sure!

[joolswills]

I'm going to merge this a bit later - so if anyone has any comments - let me know asap. Thanks for your work on this @zigurana

[joolswills]

Thanks