Added "oldest save" and "slots 1-5" as options for "auto load savestate" #11202
Conversation
Core/Config.cpp
Outdated
| @@ -352,7 +352,7 @@ static ConfigSetting generalSettings[] = { | |||
| ConfigSetting("ForceLagSync", &g_Config.bForceLagSync, false, true, true), | |||
|
|
|||
| ReportedConfigSetting("NumWorkerThreads", &g_Config.iNumWorkerThreads, &DefaultNumWorkers, true, true), | |||
| ConfigSetting("EnableAutoLoad", &g_Config.bEnableAutoLoad, false, true, true), | |||
| ConfigSetting("EnableAutoLoad", &g_Config.iEnableAutoLoad, false, true, true), | |||
There was a problem hiding this comment.
The default should be 0, rather than false, now. It would be preferable to add an enum to ConfigValues.h assuming #11187 gets merged.
Also, since we're changing the value anyway, maybe "AutoLoadSaveState" or something is a better name. AutoLoad makes me think about auto loading a game (and auto loading the last played game is not a totally crazy feature...)
-[Unknown]
There was a problem hiding this comment.
Ah yes of course, misunderstood the prototype there. I've gone the enum way as suggested.
Core/SaveState.cpp
Outdated
| } | ||
|
|
||
| bool operator ! (const tm &t1) { | ||
| if (t1.tm_year | t1.tm_mon | t1.tm_mday | t1.tm_hour | t1.tm_min | t1.tm_sec) return false; |
There was a problem hiding this comment.
Hmm, bitwise | seems odd here, although it might be fine. Maybe better to use || for clarity?
-[Unknown]
There was a problem hiding this comment.
Bitwise works. Was quite pleased with that solution. I'd brushed up on them the week before and as I was overloading I was thinking low level.
But as the members of the tm struct are only int I can use || just fine. And as you say that's more readable.
Core/SaveState.cpp
Outdated
| if (File::Exists(fn)) { | ||
| tm time; | ||
| bool success = File::GetModifTime(fn, time); | ||
| if ( (success && !oldestDate) || (success && oldestDate > time) ) { |
There was a problem hiding this comment.
nit: Would prefer (success && (!oldestDate || oldestDate > time)).
-[Unknown]
There was a problem hiding this comment.
Yes, that's much better. Like simplifying like terms in an equation.
Got carried away with overloading the ! and the >.
UI/EmuScreen.cpp
Outdated
| SaveState::LoadSlot(gamePath_, lastSlot, &AfterSaveStateAction); | ||
| g_Config.iCurrentStateSlot = lastSlot; | ||
| switch (g_Config.iEnableAutoLoad) { | ||
| case 0 : return; // "AutoLoad Off" |
There was a problem hiding this comment.
In general for switches, we use the convention:
switch (variable) {
case value1:
stuff();
break;
case value2:
case value3:
stuffFor2Or3();
default:
break;
}It's fine on the same line, but the cases shouldn't indent (like labels) and shoudln't have a space before the :.
All that said, maybe it could just be like...
switch (g_Config.iEnableAutoLoad) {
case (int)SaveStateAutoLoad::OFF: return;
case (int)SaveStateAutoLoad::OLDEST: autoSlot = SaveState::GetOldestSlot(gamePath_); break;
case (int)SaveStateAutoLoad::NEWEST: autoSlot = SaveState::GetNewestSlot(gamePath_); break;
default:
// The only other values supported are actual slot numbers.
// (code for that)
}That way it automatically handles if we add new slots later or whatever.
-[Unknown]
There was a problem hiding this comment.
The enum improves readability but makes the config file less intuitive. I guess users aren't meant to be editing it by hand so no matter.
And as you say, it more easily allows for more slots in future. With that in mind I spent some time looking into generating
static const char *autoLoadSaveStateChoices[] = { "Off", "Oldest Save", "Newest Save", "Slot 1", "Slot 2", "Slot 3", "Slot 4", "Slot 5" };
dynamically based on the
static const int NUM_SLOTS = 5;
so there's just that 1 constant to change, but as PopupMultiChoice takes a const array it can't be done, so far as I can tell.
There was a problem hiding this comment.
Right - I think it's okay for now. But PopupMultiChoiceDynamic exists for that purpose.
-[Unknown]
There was a problem hiding this comment.
I did see that and thought I'd cracked it, but didn't think it was going to be suitable for some reason. Likely I've misunderstood the prototype. Will have a another look at that for completeness.
|
Thank you for all of the constructive feedback. It's been educational. edit: just realised I've not cast my cases to |
Core/ConfigValues.h
Outdated
| @@ -71,3 +71,9 @@ enum class SmallDisplayZoom { | |||
| AUTO = 2, | |||
| MANUAL = 3, | |||
| }; | |||
|
|
|||
| enum AutoLoadSaveState { | |||
There was a problem hiding this comment.
This will "pollute" the global namespace so that OFF is 0. If you use enum class AutoLoadSaveState { instead, it will require AutoLoadSaveState::OFF and OFF alone won't mean anything.
But in that case, it will require a cast to (int).
-[Unknown]
There was a problem hiding this comment.
Ah, that explains your cast. I'll implement and test now.
For future reference should I be squashing my commits as I go along like this, or leaving them perhaps to show how we got here?
There was a problem hiding this comment.
Either way - usually I worry about if it would give me useful information 6 months from now. So a typo fix, definitely not. Whether something is a struct or a class, or enum vs enum class, probably not... but why it's slot - 3 or why something uses a mutex - probably so.
But generally I err on more commits than less, since bisecting 2000 commits vs 1000 commits is not a big difference anyway.
-[Unknown]
This adds some functionality to Auto Load Savestate. Previously it toggled on/off auto-loading of the newest savestate.
Now there is a choice of newest, oldest or a fixed slot number.
In other emulators I often use an oldest savestate that takes you straight to the main menu "press start" type of screen, to cut down on loading times as my default (good for guests too). The option of a set slot number just gave some additional flexibility whilst I was there.
In
UI/EmuScreen.cppI've used a switch/case and a ternary operator. I've tried a version that uses if/else instead but couldn't decide which was preferable for readability. I look forward to your guidance there.