https://github.com/OpenMPT/openmpt/pull/30

Here's my attempt to implement this. It's not great, but I tried. Hopefully this (or a better version) gets merged.

This is not as easy as it looks from a first glance.

I can fully understand the frustration with autosave files getting placed inside collection folders. However, this is frankly not the use case we are most concerned about with the autosave feature. We already have the setting available to change that (https://wiki.openmpt.org/Manual:_Setup/Paths_/_Auto_Save#Backup_and_Auto_Save).

For autosaves of files a user is actually actively working on, the original file folder is the correct location, though. That's where I would expect autosaves to be. And there is also a technical limitation why this is arguably the only place they can be in case the user is using External Samples: If the file gets autosaved somewhere else, either (1) the relative filenames of external samples would need to get changed (making the autosaved file not an in-place replacement of the original file anymore), or (2) the autosaved file will not be openable from the fixed location where it got autosaved to (this is what currently happens), or (3) all relative external samples would (potentially) need to get saved to the autosave location as well.

Because of the (current) technical limitation of (2), IMHO the only viable default is what we are currently doing. And this is also why I do not think that providing a menu to browse autosaved file in an explicit location to be all that useful. It will fail for files with external samples.

Also, I do not think autosaving to %Temp% is a good idea. That folder is far too volatile. We should consider changing the current fallback for new files away from %Temp% and into %AppData%. That however has the problem of accumulating old data that never gets cleaned and which the user does not necessarily know about.

We can consider choosing the autosave location in the Welcome Dialog which gets shown only on the first run of OpenMPT.

I forgot about external samples. Should I close the PR or keep it up?

@cs127 I guess you can close that for now.

We first should come to a conclusion of what we actually want here, and then discuss which (if any) internal things need additional changes to be able to achieve that.

FWIW, we do have the option to just not use relative paths for external samples in autosaves. The logic exists. Right now the external paths are relative to the autosave location rather than the song location.

Also, if we were going to use a non-temporary folder, it should be in %localappdata% by default, not %appdata%, from my understanding of the rules. %localappdata% was originally meant for data that is not to be synced between workstations when a user profile is shared between multiple machines. Of course most people don't use this feature of Windows anyway, but still %localappdata% is usually treated more as an ephemeral location and I think auto-save fits that location a bit better.

So, we are not doing (2) at the moment. Looks like I misunderstood the logic somehow. In any case, for Autosave, the saved file really should be in-place replaceable for the original file IMHO, which is only the case when not saving to a fixed location, being another argument for not changing the current default.
Otherwise, we are talking about Recovery files (a feature we currently do not really have), that could be used to re-construct the original file. Recovery files, however, need additional metadata (in particular the path they are meant to get recovered to), which would need do get stored somewhere.

Yeah, %localappdata% is probably a better fit.

As a first step, we could disable relative file paths in autosaved files then.

As a first step, we could disable relative file paths in autosaved files then.

Only when saved to a fixed location. When saving to the same folder as the original file, the external paths should be in exactly the same format as for a regular save.

I do not care that much what we do for external samples when autosave is saved to a fixed location, but in any case, we should document what we do. and document the problems fixed-location-autosave can cause in general when using external samples (i.e. explicitly document directly-opening-from-fixed-location and replacing-original-file-with-autosaved-copy).

Suggested patch as step 1: Disable relative paths for external samples when using a user-specified folder to autosave into.

Index: common/mptPathString.cpp
===================================================================
--- common/mptPathString.cpp	(revision 22162)
+++ common/mptPathString.cpp	(working copy)
@@ -55,7 +55,7 @@
 	using namespace path_literals;
 	using char_type = RawPathString::value_type;
 	mpt::PathString result = path;
-	if(path.empty())
+	if(path.empty() || relativeTo.empty())
 	{
 		return result;
 	}
@@ -79,7 +79,7 @@
 	using namespace path_literals;
 	using char_type = RawPathString::value_type;
 	mpt::PathString result = path;
-	if(path.empty())
+	if(path.empty() || relativeTo.empty())
 	{
 		return result;
 	}
Index: mptrack/AutoSaver.cpp
===================================================================
--- mptrack/AutoSaver.cpp	(revision 22162)
+++ mptrack/AutoSaver.cpp	(working copy)
@@ -141,7 +141,7 @@
 {
 	mpt::PathString name = GetBasePath(modDoc, true) + GetBaseName(modDoc);
 	const CString timeStamp = CTime::GetCurrentTime().Format(_T(".AutoSave.%Y%m%d.%H%M%S."));
-	name += mpt::PathString::FromCString(timeStamp);  //append backtup tag + timestamp
+	name += mpt::PathString::FromCString(timeStamp);  // Append backtup tag + timestamp
 	name += mpt::PathString::FromUnicode(modDoc.GetSoundFile().GetModSpecifications().GetFileExtension());
 	return name;
 }
@@ -167,8 +167,8 @@
 		case MOD_TYPE_MOD: success = sndFile.SaveMod(f); break;
 		case MOD_TYPE_S3M: success = sndFile.SaveS3M(f); break;
 		case MOD_TYPE_XM:  success = sndFile.SaveXM(f); break;
-		case MOD_TYPE_IT:  success = sndFile.SaveIT(f, fileName); break;
-		case MOD_TYPE_MPT: success = sndFile.SaveIT(f, fileName); break;
+		case MOD_TYPE_IT:  success = sndFile.SaveIT(f, GetUseOriginalPath() ? fileName : mpt::PathString{}); break;
+		case MOD_TYPE_MPT: success = sndFile.SaveIT(f, GetUseOriginalPath() ? fileName : mpt::PathString{}); break;
 		default:
 			// nothing
 			break;
Index: test/test.cpp
===================================================================
--- test/test.cpp	(revision 22162)
+++ test/test.cpp	(working copy)
@@ -2406,6 +2406,8 @@
 	VERIFY_EQUAL(mpt::RelativePathToAbsolute(P_("\\foo"), exePath), P_("C:\\foo"));
 	VERIFY_EQUAL(mpt::AbsolutePathToRelative(P_("\\\\server\\path\\file"), exePath), P_("\\\\server\\path\\file"));
 	VERIFY_EQUAL(mpt::RelativePathToAbsolute(P_("\\\\server\\path\\file"), exePath), P_("\\\\server\\path\\file"));
+	VERIFY_EQUAL(mpt::AbsolutePathToRelative(P_("C:\\OpenMPT"), mpt::PathString{}), P_("C:\\OpenMPT"));
+	VERIFY_EQUAL(mpt::RelativePathToAbsolute(P_("C:\\OpenMPT"), mpt::PathString{}), P_("C:\\OpenMPT"));
 #endif
 
 #ifdef MODPLUG_TRACKER

Step 2: As of r22336, when using a custom folder to save autosaves to, there is now an option to delete autosaves older than X days (by default 30). With this possibility, it feels safer putting autosaves in a place the user normally never vists (such as %localappdata%), since the folder will not grow indefinitely. Also, autosaves are now moved to the recycling bin by default instead of being deleted permanently, just to be on the safe side.

Next step would be to choose a new default folder to save, and also make the uninstaller handle this folder. It's not quite clear yet what should happen with portable installations - should the default folder reside inside the portable installation folder?

Next step would be to choose a new default folder to save, and also make the uninstaller handle this folder.

I do not think the Uninstaller should touch user data, not even %LocalAppData%. For multi user system, we can only ever delete data from the uninstalling user, which would be inconsistent.

It's not quite clear yet what should happen with portable installations - should the default folder reside inside the portable installation folder?

yes

I do not think the Uninstaller should touch user data, not even %LocalAppData%. For multi user system, we can only ever delete data from the uninstalling user, which would be inconsistent.

Maybe not touch it, but at least tell the user where to find the autosave data, in case they want to delete it.

I do not think the Uninstaller should touch user data, not even %LocalAppData%. For multi user system, we can only ever delete data from the uninstalling user, which would be inconsistent.
Maybe not touch it, but at least tell the user where to find the autosave data, in case they want to delete it.

I guess documenting it (as part of the manual, together with where configuration settings are stored, in a new "uninstalling" chapter, ) is enough. This is a very general problem with all software on basically all platforms, and it is really only solved (and solvable) for single-user platforms (like Android/iOS) or per-user(-only) installations.

While OpenMPT can nowadays be installed per-user, I do not think implementing different behaviour with regards to how stuff is cleaned up on uninstallation between system-wide and per-user installs is worth the effort.

Step 3: As of r22351, Autosave defaults to either %localappdata%\OpenMPT\Autosave for normal installations, or in the Autosave sub folder within portable installations.

I guess documenting it (as part of the manual, together with where configuration settings are stored, in a new "uninstalling" chapter, ) is enoug

As the problem is technically not new, I have already added a help section on uninstallation for the current (1.31) manual: https://wiki.openmpt.org/Manual:_System_Setup#Uninstallation

As the problem is technically not new, I have already added a help section on uninstallation for the current (1.31) manual: https://wiki.openmpt.org/Manual:_System_Setup#Uninstallation

Looks good.