Refactorings to #857 in C++11 standard

[Gemba]

As discussed. In short:

• Stick to C++11
• Support map storage for current feature (Random Collection) in Settings

[pjft]

Thank you. This looks good. I left a minor comment on a variable name, and a general comment on the use of auto types in general - unless there's a strong reason for it, if we could use the expected types there it'd be preferrable.

[Gemba]

Forced push with your requested changes.

[pjft]

Thanks. Let me test tomorrow.

[pjft]

Oh, there seems to be a conflict now that I merged your other PRs. Sorry about that.

[Gemba]

Oh, there seems to be a conflict now that I merged your other PRs. Sorry about that.

Nvm. I will address that today or tomorrow.

Edit: Should be mergable now.

[pjft]

Thanks.

So, I was testing this but it didn't seem to load the currently existing settings from the selected collections and values. The MaxGames was expected as we changed the variable name (though, in hindsight, since we're going to change code, I am leaning towards keeping the same variable name to keep compatibility with the folks who already have it running).

But the following strings weren't read:

<string name="RandomCollectionMaxItems" value="15" />
<string name="RandomCollectionSystems" value="Commodore Amiga:1,Arcade:4,Commodore 64:1,Daphne:1,Dreamcast:1,Famicom Disk System:0,Game and Watch:0,Sega Gamegear:0,Game Boy:1,Game Boy Advance:1,Game Boy Color:1,Nintendo Gamecube:0,Multiple Arcade Machine Emulator:0,Multiple Arcade Machine Emulator:0,Sega Master System:1,Sega Mega Drive:2,MSX:0,Nintendo 64:0,Nintendo DS:0,Neo Geo:2,Nintendo Entertainment System:2,Neo Geo Pocket Color:0,PC:0,PC Engine:2,Pico-8:2,Ports:0,PlayStation Portable:1,PlayStation:1,Sega Saturn:1,Sega 32X:1,Mega CD:1,Sega Model 3:1,Singe:1,Super Nintendo:2,Wonderswan Color:1,ZX Spectrum:2" />
<string name="RandomCollectionSystemsAuto" value="all games:0,favorites:3,last played:0" />
<string name="RandomCollectionSystemsCustom" value="+Now Playing:0,+today arcade:0,+today selection:0,16bit:0,32bit:0,8bit:0,Backlog:0,Francisco:0,Games of the Month:0,Games to Try Out:3,Konami:0,MAME ROW Vol 1:0,MAME ROW Vol 2:0,Marlene:0,Now Playing:0,Party:0,Run and Guns:2,Top Games:0,arcade-2p:0,arcade-2p-kids:0,arcade-3p-kids:0,best of:0,btmups:2,capcom:0,classics:0,cps1:0,cps2:0,cps3:0,data east:0,exclude:0,fighting:2,gems:0,handhelds:0,kid friendly:0,kids:0,lightgun:0,lightgun-all:0,mario:0,neogeo MVS:0,nesmini:0,new games:0,other-systems:0,racing:2,roguelikes:0,sega:0,shmups:2,snesmini:0,sports:1,taito:0,working:0,zxspectrum-new:1" />

The systems initialized as 1 for all, and 0 for all the collections. Could you look into it?

Thanks.

[Gemba]

Sure. No big thing.

Do you want the conversion logic for the four settings baked into ES and no action needed for the testers (but someone of the maintainers should remove the conversion code in the over-next release of ES).

or

will a small Python script be sufficient and a note to the testers in the forum to run the script before rebuilding the ES with these changes merged in?

[pjft]

Sorry, maybe I missed something. I wasn't expecting any conversion logic to be needed, what conversion needs to take place? Has the format been changed? I was just reporting that the settings (which I expected would still be honored) weren't anymore so I assumed it was a bug.

The only setting that could change would be the maxItems though I'm leaning to keeping it with the same name.

If it's supposed to change, what's the new format it expects? Can we not keep the same as before?

Edit: meaning, I was literally looking at this purely as a welcome code refactoring effort, not necessarily changing any of the behavior.

[Gemba]

I did not make this explicit, as I assumed you will notice it on a review. I don't want to go down that road if changing the settings format in a dev version is a refactoring or not. However, if it would have been an already rolled out version of ES I would have been highlighting it to you and assured conversion logic in place.

My motivation was that XML supports nesting anyway why not utilizing this instead of custom parsing of a "key:value;..." string? I assumed it to be more maintainable codewise and more readable format for the es_settings.cfg. What was your rationale to use the "key:value;..." string format despite the smaller footprint in es_settings.cfg?

The settings format as with PR is now like this:

<map name="RandomCollectionSystems" type="int">
        <int name="Apple II" value="3" />
        <int name="Atari 2600" value="5" />
        <int name="Commodore 64" value="5" />
...

same for RandomCollectionSystemsAuto and RandomCollectionSystemsCustom. The type="int" is a marker for the nesting types to do the proper cast (pugixml:: ... as_int() in this case).

[pjft]

Got it. That's fair, regarding the format. Still, breaking changes - even in a dev version, and no matter how small - would be highlighted here for the sake of the review. It's a development build in the sense that it is the master branch from the repository, but there are still users who use this version rather than the main ES as it is slightly more up-to-date on bugfixes and features.

I don't think it needs a conversion at this stage, though do update the forum thread with that change, for the folks who were using it.

Thanks.

[pjft]

I'm ok with merging - could you squash the two commits then? Thank you.

[Gemba]

I created an conversion script, to automate the tedious task of re-applying the selected values: https://gist.github.com/Gemba/4729e3560264511ea06aadb18f439112

[Gemba]

[...] Still, breaking changes - even in a dev version, and no matter how small - would be highlighted here for the sake of the review. It's a development build in the sense that it is the master branch from the repository [...]

Duly noted.

I will announce a heads up on the forum about the changed settings format.

Commits are squashed.

[pjft]

Thank you!

Have a great weekend.