[Scummvm-tracker] [ScummVM :: Bugs] #13842: ENGINES: Fix proper detection of autosaves by isAutosave() function.

[ScummVM :: Bugs]

#13842: ENGINES: Fix proper detection of autosaves by isAutosave() function.
-------------------+---------------------
Reporter:  macca8  |       Owner:  (none)
    Type:  defect  |      Status:  new
Priority:  normal  |   Component:  Common
 Version:          |  Resolution:
Keywords:          |        Game:
-------------------+---------------------
Description changed by macca8:

Old description:

> 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 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.
> - 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(), after using
> hasAutosaveName() exclusively
> ([https://github.com/scummvm/scummvm/commit/9b05c206993d3107f98ac5b437801e33ba3c79b7
> 9b05c206]), the bug in #12363 has been reinstated (applies to Myst III,
> and any game which offers write access in the autosave slot).
>
> 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())
> }}}
>
> Thanks for your time.

New description:

 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 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(), after using
 hasAutosaveName() exclusively
 ([https://github.com/scummvm/scummvm/commit/9b05c206993d3107f98ac5b437801e33ba3c79b7
 9b05c206]), 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.

--
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/13842#comment:1>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM