frontend: Ensure pending profile changes are saved

[Warchamp7]

Description

Saves the current active profile config data to disk when:

• Renaming a profile
• Duplicating a profile
• Activating a profile

Motivation and Context

#11465 fixed an issue where profiles were loaded before all modules were ready, which would cause settings to 'reset' due to the encoders being missing.

However this seems to have caused the side effect noted in #12458 where renaming/duplicating a fresh profile would crash upon opening settings. This is due to null values for AdvOut -> AudioEncoder / RecAudioEncoder.

The defaults for those values come from InitBasicConfigDefaults2 and never get persisted to disk prior to the Rename/Duplicate. The Rename/Duplicate functions create a new profile file and then copy the current profile over it. This means any unsaved profile data which are not saved to disk yet (In the example scenario, pretty much all of them) is lost. We re-initialize defaults in ActivateProfile which means most of those values are transparently set up again but we do not call InitBasicConfigDefaults2. Thus anything set up there is not initialized in the copied profile.

The fix in this PR addresses the problem above and also solves a theoretical scenario where unsaved profile data is currently lost when doing Rename/Duplicate. I'm not actually sure if there's anywhere we are guilty of this, but it is solved all the same.

How Has This Been Tested?

Launched OBS in Portable mode. With the freshly created Untitled profile, I can open Settings. If I Rename or Duplicate the profile and then open Settings, OBS will crash. With this fix it no longer crashes.

This crash can also be triggered by creating a brand new profile and then Renaming or Duplicating that new profile.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[PatTheMav]

Because I've been bitten by this behaviour a few times while working on this code in the past: Are the profiles always in a "complete" state before calling the save function here?

Particularly during initialisation, some code paths can be (unexpectedly) taken which would write (or overwrite) a default profile with partial data and thus corrupt the profile, leading to knock-on effects (either OBS crashes on next launch, or crashes on some other interaction later) and the only fix is to magically detect this "half-baked" state of the application and skip saving.

[Warchamp7]

Because I've been bitten by this behaviour a few times while working on this code in the past: Are the profiles always in a "complete" state before calling the save function here?

We don't really have any mechanism for communicating that currently, so it cannot be said with certainty.

Particularly during initialisation, some code paths can be (unexpectedly) taken which would write (or overwrite) a default profile with partial data and thus corrupt the profile, leading to knock-on effects (either OBS crashes on next launch, or crashes on some other interaction later) and the only fix is to magically detect this "half-baked" state of the application and skip saving.

I feel like that is a problem of it's own and related to the above.

I'm not sure what the expected behaviour should be if you try to duplicate a profile that is in a partial state.

[PatTheMav]

After a second look I feel like this change is a bad idea, as it throws some "pls save" calls at the wall to fix something unrelated to the profile API here.

This is partially born out of the legacy behaviour of OBS to treat the state of the file on disk as the "source of truth", so duplicating a profile literally just means "copy the directory for the current profile to a new directory". The entire class doesn't care about the "contents", it cares about files/directories.

IMO the correct fix is that whoever calls SetupDuplicateProfile (or its callers) to ensure the profile is saved or in a "valid state" before asking it to be duplicated.

Otherwise you're enhancing the "stateless" implementation of SetupDuplicateProfile to behave as if it were stateful. It's messy because the APIs are not properly separated: Some methods like OBSBasic::ActivateProfile are implemented in OBSBasic_Profiles as they are profile-specific code the application code wants to handle (indeed this is where data is saved before the new profile is loaded and all), but other methods in the same file like OBSBasic::SetupDuplicateProfile would be more appropriate on an actual OBSProfile object with its own API (e.g. OBSProfile::duplicate(std::string newName) - no const reference because the duplicate will inherit the local copy create for the function).

But until we have that, we have to treat some methods as if they would belong to an encapsulated profile class and others are just OBSBasic-specific code handling the profile "objects".

(And in the "proper" implementation, just having a profile create a duplicate of itself should not automatically mean that either is persisted to disk - neither the existing nor new profile should always be "saved" because of that, instead OBSProfile::save would need to be called explicitly.)

[Warchamp7]

After a second look I feel like this change is a bad idea, as it throws some "pls save" calls at the wall to fix something unrelated to the profile API here.

This is partially born out of the legacy behaviour of OBS to treat the state of the file on disk as the "source of truth", so duplicating a profile literally just means "copy the directory for the current profile to a new directory". The entire class doesn't care about the "contents", it cares about files/directories.

IMO the correct fix is that whoever calls SetupDuplicateProfile (or its callers) to ensure the profile is saved or in a "valid state" before asking it to be duplicated.

Otherwise you're enhancing the "stateless" implementation of SetupDuplicateProfile to behave as if it were stateful. It's messy because the APIs are not properly separated: Some methods like OBSBasic::ActivateProfile are implemented in OBSBasic_Profiles as they are profile-specific code the application code wants to handle (indeed this is where data is saved before the new profile is loaded and all), but other methods in the same file like OBSBasic::SetupDuplicateProfile would be more appropriate on an actual OBSProfile object with its own API (e.g. OBSProfile::duplicate(std::string newName) - no const reference because the duplicate will inherit the local copy create for the function).

But until we have that, we have to treat some methods as if they would belong to an encapsulated profile class and others are just OBSBasic-specific code handling the profile "objects".

(And in the "proper" implementation, just having a profile create a duplicate of itself should not automatically mean that either is persisted to disk - neither the existing nor new profile should always be "saved" because of that, instead OBSProfile::save would need to be called explicitly.)

Updated this to be only the single save call inside ActivateProfile.