ENGINES: Prevent autosave overwriting regular save without warning

[macca8]

Due to very slow connection speeds, I've opted to close, rather than attempt to rebase, #4358, and reopen with this appropriately named PR, and the commits relevant to the issue.

This replaces the content of #4355 & #4358, which contained previously merged commits from #4350.

For an overview of the proposed changes in 73f48d3 & ae1c1db, please refer to the description in #4355.

For an overview of the proposed changes in 52ceabe, 4f304bd & 0c7ea96, please refer to the description in #4358.

For a preliminary discussion regarding the resolution of this issue, please refer to #4358.

In summary, the issue is that a saveType initialization based on slot type is currently overriding the original method of setting the saveType value based on the isAutosave flag, that's set whenever a save is performed. As a result, affected engines are identifying any save stored in the autosave slot as an autosave, regardless of its true type.

The effect is that any user's regular save which happens to be stored in the autosave slot is now automatically overwritten by the autosave, instead of generating the in-game warning dialog, denying the user the opportunity to recover their save file.

The key is that this saveType-based-on-slot-type initialization is NOT required for the actual autosave process, originally being introduced only to determine if the autosave slot was empty, for the sole purpose of generating a dummy autosave if required. At the time of its introduction, the saveType property was NOT being used for autosave testing.

The risk was always that someday the saveType would again be restored to its intended purpose of identifying the type of save stored in the autosave slot, and this occurred with 9b05c20, revealing the previously hidden consequences of coercing the use of saveType for something other than its intended purpose.

This PR resolves this issue by removing this unnecessary saveType initialization, restoring the previous default initialization to the SaveStateDescriptor methods, and removes the usage of the saveType in determining if the autosave slot is indeed empty, by replacing calls to isAutosave() in varous listSaves() methods with their original "slot == autosaveSlot" type calls.

[bluegr]

I won't go into details again in this PR, since we've been through a long discussion in #4358.
I'd just like to add that the autosave slot should be protected at all times, not only when autosaves are enabled. Allowing it to be ovewritten by regular saves when autosaves are disabled doesn't make sense, and will only intensify confusion in users.

[macca8]

Thanks. I must admit I'm surprised by your apparent failure to recognise that 092a6ff & the current initSaveType() method produce the same autosave slot set-up, but I guess that highlights just how oversimplified the initSaveType() method’s definition really is, and how well it hides what’s actually going on... you did check this before posting, right?

To put 092a6ff into its proper perspective, it’s simply a clearer and cleaner implementation of how the initSaveType() method sets up the autosave slot, and has been doing since it was implemented some 15 months ago. The only minor variation is that it identifies the autosave slot before running the autosave enabled test, which is now applied only when accessing the autosave slot, but that doesn't change how the autosave slot is set up.

Apart from that, there’s nothing changed here, except for the removal of the saveType initialization.

It doesn’t help to clarify the situation that this method is named initSaveType(), where its primary purpose is to streamline the setting of the writeProtected & Deletable flags, for setting up slots in the Save/Load dialogs.

It also doesn’t help that this alternative _saveType initialization, which simply takes the slot type (as defined by how it's set up) and coerces it into a saveType value, is included in this method.

For comparison purposes, here’s the problematic saveType initialization, replacing the current autosave variable with the equivalent autosaveEnabled variable in 092a6ff :
_saveType = autosaveEnabled ? kSaveTypeAutosave : kSaveTypeRegular;

This clearly shows that, whether it’s an autosave or a regular save, it’s identified as an autosave when autosaving is turned on, and as a regular save when autosaving is turned off. As such, it can’t be relied upon for determining the type of save stored in the autosave slot, and can also fail to determine if a save is indeed stored in the autosave slot (if autosaving is disabled).

It offers no value to the autosave process, and is directly responsible for overwriting a user’s save, so what's the point of keeping it?

Are you concerned about what happens to the _saveType property once this alternative initialization method is removed? If so, then let's break this down.

Realistically, there’s only a single edge case where this property is required to identify the type of save stored in the slot, and that’s when the user changes the game language after the autosave file has already been created (see bug #13703).

Not surprisingly, the original saveType-based-on-isAutosave-flag method was introduced in #2050, after a discussion about this contingency. It's initialized in SSD definitions as _saveType(kSaveTypeUndetermined), then updated based on who creates the save, which in the current default implementation, can only be the autosave.

In virtually all other cases, the name test can be relied upon to correctly identify the type of save stored in the autosave slot, so the saveType test has minimal effect in determining whether or not an autosave can proceed.

In my opinion, the commits already submitted with this PR are all that's needed to resolve this issue.

I honestly don’t understand why you’re procrastinating over this, or even what you’re actually waiting for, because any reasonable person would consider an autosave overwriting a user’s regular save without warning to be totally unacceptable behaviour, particularly given the alternative is that the user will be presented with a warning dialog, allowing him to clear the save from the slot himself.

As for what users should, or shouldn’t, do, and properly protecting the autosave slot, I'm not seeing that has any relevance to the issue we're handling here… which is, there’s a user’s save stored in the autosave slot, and it needs to be handled respectfully.

I’ve given you all the relevant info. I don’t know what more I can do!

[macca8]

Commit ece55fd denies the user access to an empty autosave slot, thus preventing the user from creating a regular save in the slot. In this case, the autosave slot remains inaccessible until after an autosave creates the autosave file.

The user can still access the autosave slot by disabling autosaving, but only if a save is present. This allows the user to handle any maintenance issues involving the autosave file (including deleting the file if necessary), or to access an existing regular save that the user may have opted to retain.

In normal usage, where the user starts with an empty autosave slot, and autosaving is enabled by default, if autosaving behaves as expected, it's unlikely that a user would consider disabling autosaving. Nevertheless, it's prudent to have user support available as backup, in the event that autosaving unexpectedly fails for any reason.

Note that engines which create their own custon dummy autosaves are not affected by these changes, since they have their own special needs and behaviours.

[sev-]

I was abstaining for a while from reading all of these comments in both PRs, and now finally done it. I feel a bit lost, so let me ask a couple of stupid questions.

The current approach is the following:

1. The autosave slot must always be skipped when offering a save, regardless if the autosave period is set or not set.
2. In a very, very rare case, when somebody managed to create a save in the autosave slot (the functionality was added in 2002), which happens when an engine has autosave added later, we want to warn the user about save slot and offer it to be moved.

Now, does this PR takes these two into account or changes some of the approaches?

What problem does it try to solve, e.g. could you please describe a use-case?

[macca8]

Thanks for your insight into the current approach, and your questions. I'll follow up when I have the time.

[sev-]

What is the status of this PR?

[macca8]

What is the status of this PR?

This PR's purpose, and proposed resolution, is adequately described in the opening comment at the beginning of this PR, and is ready to go with ALL the included commits. It's really quite simple... IF a regular save is occupying the autosave slot (regardless of how it got there), and an autosave is pending, it should be detected by displaying the in-game warning dialog to the user, instead of allowing the pending autosave to automatically overwrite the regular save without warning the user, which is currently the behaviour for those engines which access the initSaveType() method (through creating an instance of SSD(metaengine, slot, description) when calling their local querySaveMetaInfos() method).

It's worth noting that the problematic saveType initialization included in the initSaveType() method, which was introduced by PR #3261, isn't documented in any of that PR's discussions or commented in the code, so you need to review the actual commits from that PR individually to fully appreciate the changes made, and more importantly, the circumstances prevailing at the time of those changes. There's no shortcuts available here.

However, there appears to be a reluctance on the part of those reviewing this PR to believe that it's possible for a user's regular save to be occupying the autosave slot. This is extremely frustrating, because if I offer an example (refer #4358 (comment)), I'm either being told a user shouldn't do that, or the example is simply being ignored. This also includes the example quoted in your Point 2, which bluegr was very quick to dismiss as irrelevant when I raised it earlier. If you guys can't agree on expected behaviour, then what hope have I got of convincing you otherwise?

With regard to the current approach, both your points are contradicted by the current implementation of the initSaveType() method. Interestingly (and adding to this confusion), you personally approved, and subsequently committed, the implementation of this method as part of PR #3261, effectively making Point 1 redundant as the default approach for setting up the autosave slot. To be fair, at that time, Point 2 wasn't directly affected, but was set up to subsequently fail at a later date.

This PR is about reinstating the expected approach highlighted in Point 2, which also applies to handling the more recent example created by allowing the user access to the autosave slot when disabled.

With respect to handling the autosave slot itself (i.e. your Point 1), this PR supports the current behaviour as determined by the initSaveType() method (as related to the setting of the writeProtected & Deletable flags), since it's not dependent on resolving the underlying issue (i.e. the proper treatment of a regular save stored in the autosave slot).

However, because user access currently may present a normal empty slot to the user when autosaving is disabled, and this creates an opportunity for the user to create a regular save in the autosave slot, I've added ece55fd to prevent this from happening in the future.

The end result is that, once any existing regular saves are removed from the slot, the user will be presented with either an inaccessible dummy autosave in the autosave slot if the autosave file is yet to be created (regardless of whether autosaving is enabled or disabled), or with the actual autosave file. In the latter case, this file is inaccessible to the user when autosaving is enabled. However, if for some reason the autosave file becomes unusable, the user can remove the old autosave file (if deemed necessary) by disabling autosaving, which allows the autosave to create a new file when autosaving is re-enabled.

Ultimately, the only change from your Point 1 is that user access is available only when autosaving is disabled, and then only if the actual autosave file is present... and the user should only need to disable autosaving if there's an autosaving problem.

[sev-]

One of the biggest problems of these PRs is your communication style.

Could you please (a) abstain from personal attacks, as you did twice just in your last comment and (b) write it in a comprehensive and concise format?

I do not want to follow cross-references, go deep and back into pages of texts in the previous PRs (been there), but I want a pragmatic engineering approach: 1. A problem statement 2. outlined proposed solution. Not yet another history lesson, please.

Put it in a user-story format, bullet list, or whatever, but please stay brief. Keep in mind that few people here are native English speakers, and we're not very good with prose.

And to add a bit of tech detail. Please, do not change the meaning of a save slot depending on the "autosave" setting.

[macca8]

Communication works both ways. As frustrating as it is, there doesn't appear to be any way that we can communicate on a common level here, so we may as well call it quits.

Please feel free to close this PR if you feel there's insufficient info to proceed.

[sev-]

@macca8, perhaps there is sufficient info provided. I even feel it. But I was reading through your long-long posts with cross-references several times, and I am too stupid to understand that.

I asked you to explain it in simpler steps, but that seems to have offended you. Maybe it is my bad English. It was not intended.

So, would you please write something in simple words? The code is not crystal clear to me either, I asked a couple of clarifying questions.

[sev-]

Why are we changing this slot behaviour when autosaves are not enabled?

[macca8]

You will need to follow this up with orgads, since he initiated this change in behaviour when he created the initSaveType() method. This code simply clarifies that behaviour.

[sev-]

Since you're putting this feature in order, why not fix it?

[macca8]

While this behaviour doesn't comply with your current approach, I don't believe that any further changes are necessary at this time, for a couple of reasons.

My understanding of your current approach is that it's meant to protect the autosave slot by preventing the user from accidentally storing a regular saved game in the autosave slot.

If that's correct, the changes in commit ece55fd already comply with the spirit of this approach by overriding these settings whenever the slot is empty, which means that the initial save to the autosave slot will always be performed by an autosave, which obviously creates the autosave file.
In simple terms, the above slot behaviour is only enforced when an autosave file already exists in the autosave slot.

I dom't know why @orgads changed to this slot behaviour, but I favour that the autosave file can be accessed. With access currently restricted to when autosaving is disabled, if autosaving performs as expected, most users won't even be aware that this access exists. The thing is, it doesn't break autosaving, so why not leave it in place? On those (hopefully) rare occasions when autosaving fails, troubleshooting the autosave file can often fix the problem (iI've had situations where deleting the autosave file successfully resets autosaving).

[orgads]

IIRC I kept the old behavior, only streamlined it, and made it consistent.

I don't have a strong opinion about this issue anyway.

[sev-]

However, why not take this opportunity to review the current approach for handling the autosave slot, by enforcing it only when absolutely necessary?

The problem is that the saves rename is a very risky operation and with a potentially confusing GUI message. Imagine when you accidentally turn the autosave off, and then you suddenly see the message about renames.

There is no significant advantage for the save slot 0, since we often provide hundreds or even thousands of save slots. Thus, it is best to have it designated for autosave.

As a result, the rename option will pop up only for a tiny fraction of users who happened to have non-autosave slots from many years ago.

As you went this far in improving the feature, may I ask you to straighten this behaviour so we finally merge this PR?

[macca8]

The problem is that the saves rename is a very risky operation and with a potentially confusing GUI message. Imagine when you accidentally turn the autosave off, and then you suddenly see the message about renames.

This isn't possible. The GUI message is only activated when autosaving is on (it's also why I prefer any user access to be limited to when autosaving is on, so that issues like this can be handled if and when they happen).

There is no significant advantage for the save slot 0, since we often provide hundreds or even thousands of save slots. Thus, it is best to have it designated for autosave.

Just to be clear, allowing user access is meant to support the autosave process, not replace it. It's an insurance policy for situations which autosaving can't handle on its own, such as where autosave-on-exit capabilty isn't available (and the user is prompted to save), or if the autosave file becomes corrupted. May not be needed, but it's there when we do.

And just so there's no misunderstanding, your reason for requesting this change is because you believe that users will want to rename an existing saved game named "Autosave" to something else, and you're not satisfied with the current GUI message that fixes this issue.

If that's still the case, please confirm, and I'll revert this code tp your preferred behaviour.

However, I'm not sure this is a good long term solution. Do you have any objections if I include, as commented out code, my preferred solution for future consideration (I really don't wanr to go through all this again)? If that's not OK, then fair enough.

[macca8]

I haven't received a response, but you raised concerns about renaming saves being a very risky operation. I'm not seeing any renaming saves issues with any games I have installed, whether it's the autosave file, or a save stored in any other slot, so I'm not understanding what the problem is.
Some games don't allow their save files to be renamed, so I've addressed this by adding a commit which changes the behaviour of the in-game warning dialog. From now on, any save detected by the warning dialog will be deleted, allowing the autosave to create a new save file, instead of trying to overwrite the existing saved game.
I've also tweaked the messaging to prioritise clearing the slot.

As for setting up the autosave slot, well, you've asked me to fix it. I'm sorry, but going back to the old behaviour is a backward step in my opinion. Instead I've opted to commit my preferred solution, which allows the user access when autosaving is enabled, instead of when it's disabled.

The key point for me is that, with this setup, I can access the autosave file for testing the autosave system when things go wrong. The only problem the autosave system can handle on its own is an improperly named save file.
As far as the average user is concerned, for most it will be business as usual, with usage probably limited to manually updating the autosave file when exiting a game. Importantly, the autosave system is already properly set up to handle any action a user may perform.

It's also worth remembering that not all engines use the default behaviour for setting up the autosave slot, so changes made here won't necessarily change the setup for all engines. Indeed, I currently have two games nstalled which already allow full user access when autosaving is enabled, so clearly they're not using the current defaults to set up the autosave slot, or for that matter, you're preferred method.

[sev-]

The problem is that the saves rename is a very risky operation and with a potentially confusing GUI message. Imagine when you accidentally turn the autosave off, and then you suddenly see the message about renames.

This isn't possible. The GUI message is only activated when autosaving is on (it's also why I prefer any user access to be limited to when autosaving is on, so that issues like this can be handled if and when they happen).

You misunderstood me.

1. Somebody turns off autosaves. That makes slot 0 writeable
2. They create a save to saveslot 0
3. They turn on autosaves. Now they have to deal with the save renames

Save renames are part of the functionality that "salvages" a save which is in the way of the autosave, e.g. sits in that relevant slot but is not an autosave.

So yes, please revert this functionality.

As for you, as a dev, wanting to debug autosaves, well, you can then disable the code during your development, or rename the file: you're a dev, and you know what you're doing.

PS. I do not review PRs every day or even every week. Please bear with me

[macca8]

As requested, I've reverted the default behaviour to comply with your preferred current approach.

you're a dev, and you know what you're doing.

Not a dev, just an end user able to analyse situations and read & write basic code. I'm clear on what I'm trying to do, but I depend on you guys to ensure that my code is correct.
Probably why we also have trouble communicating.

[sev-]

We have only one open question still, and then this PR is good to go. Thank you for your patience, @macca8

[sev-]

Thank you very much, merging!