ENGINES: AUTOSAVE: Fix identification of autosaves by isAutosave() function.

[macca8]

As a guide, any test method for identifying the autosave file needs to meet the following test criteria:

• The method must be able to differentiate between an autosave and a regular save.
• The autosave file test must be strict enough to exclude regular saves from being incorrectly detected as autosaves.
• The autosave file test must be broad enough to cover all known variants relevant to the test, to reduce the risk of false positives.
• The method must complement any other methods in use, such that the strengths of one can cover the weaknesses of the other, to resolve false positives unique to a specific test method.

Autosaving depends on either the name test or the saveType test (which is based on interpretation of the autosave flag value, or an explicit setting) to determine the type of any save file stored in the autosave slot. This ensures that autosaving can proceed safely, without accidentally overwriting any user’s regular save which may be present.

Currently, isAutosave() applies these tests independently of each other, with priority given to the saveType test. The name test is only applied if the _saveType property hasn’t yet been specifically set (defaults to kSaveTypeUndetermined).

This means that the decision as to which type of save file is stored in the slot is always determined by performing one test only.

This arrangement is wrong for these reasons:

• While each test is capable of identifying an autosave file, and conversely, guaranteed to identify a regular save, each can also produce a false positive under circumstances unique to that test (i.e. an autosave identified as a regular save), unnecessarily triggering the in-game warning dialog.
• The saveType test, which predates the availability of the warning dialog, was implemented at a time when a failed test would cause an autosave to fail silently. It's introduction was intended to complement, rather than replace, the name test, by specifically handling those situations where the name test was expected to fail (unless a regular save is encountered, of course).
• The in-game warning dialog is designed to resolve issues created by the user, including clearing any regular save that may be stored in the autosave slot. Any false positives should be handled internally.

In practice, because each test is based on a different aspect of the save process, each false positive is offset by the other test correctly identifying the autosave file. Examples of these false positives, which were resolved by switching tests, can be found in #12363, #13432 & #13703.

Since switching the autosave test back to isAutosave() (​9b05c206), after using hasAutosaveName() exclusively, the bug in #12363 has been reinstated (applies to Myst III, and any game which offers write access in the autosave slot, and which also sets the _saveType value based on the autosave flag).

The name and saveType tests are already quite comprehensive, and don’t need any further adjustment. What’s needed to properly detect the autosave file is a restructure of the isAutosave() function’s definition, to prioritise the name test (the primary test), and follow up with the saveType test (the supplementary test) only if that test fails.. a genuine autosave file need only pass one of the tests, whereas a regular save will fail both.

With that in mind, I’d like to propose the following change to the isAutosave() function’s definition for your consideration.

// Proposed isAutosave() definition (in engines/savestate.cpp):

bool SaveStateDescriptor::isAutosave() const {
	if hasAutosaveName() {
		return true;
	} else {
		return _saveType == kSaveTypeAutosave;
	}
}

In summary:

• The name test always tests the actual save file, whereas the saveType test assumes the file’s type, based on whoever last interacted with the file (was it the user or was it an autosave?), except when it’s explicitly set by calling setAutosave(true).
• The name test allows a user to safely interact with the autosave file, whereas the saveType test allows an engine to choose an alternative name for the autosave (or dummy autosave), and offers limited support for identifying the autosave file after a language change.
• The kSaveTypeUndetermined test is no longer required since it's covered by the _saveType test.

This should resolve most, if not all, false positives involving an autosave file being detected as a regular save, but see #13841 for a new issue where a user’s save may be overwritten by an autosave.

Finally, when the autosave test switched back to using isAutosave(), only the in-game test was changed. The same change needs to be applied to the autosave test in the Global Options warning dialog as well, to ensure consistency between the results of both autosave tests.

Options warning dialog - gui/options.cpp/updateAutosavePeriod():

// replace hasAutosaveName() with isAutosave(), change L2639 to:
 
if (desc.getSaveSlot() != -1 && !desc.getDescription().empty() && !desc.isAutosave())

If approved, I would appreciate if someone could implement these changes on my behalf.
Thanks for your time.

[macca8]

Since there's been no response, I'm changing the component to something more generic, in the hope of stimulating some interest, and avoiding this issue finding its way into the 2.6.1 release (the same applies to #13841).

Not sure which is the correct component here. It's really a global issue affecting all engines, but there's no general "Engines" component listed (perhaps that should be added to the list, similar to Ports?), so I've settled on Other, rather than Common (which was used with ​9b05c206, which originally exposed this issue).

[macca8]

After a few false starts trying to create the PR, I've taken the punt and submitted ​PR4350 in the hope of resolving this before the 2.6.1 release.

[macca8]

Proposed ​PR4350 has been simplified & merged by bluegr.
Many thanks.

[macca8]

Just a heads up for anyone encountering this issue.
This fix missed the cut for the 2.6.1 maintenance release, so you can either fall back to the 2.6.0 release (which predates the bug), or try any of the latest 2.7.0git master builds.