Core: save default settings before opening file for users

[qwint]

What is this fixing or adding?

saves the settings before opening host.yaml so all defaults are populated

How was this tested?

had an outdated host.yaml for TUNIC, installed the new one, clicked the open host.yaml button in the launcher, saw the new settings values populated

If this makes graphical changes, please attach screenshots.

[Jouramie]

Shouldn't settings.get_settings handle that? Since it already saves the file if it does not exist, I think it would make sense if the function would also update if when it's missing default properties. With the current changes, the file will be saved two times if it didn't exist.

[qwint]

oh nice catch,
my worry though about moving it upstream to get_settings is when do you choose to save or not?
do you iterate every key to see if there is any key missing? do you pass in an optional parameter?
most get_settings calls do not care about the contents of the file and are happy to just pull back defaults if the file is missing info, and some even intend to update and save some data meaning you'd be double saving in those cases instead of in this case

I'd be happy if you had a solution to these worries 😅 but I don't so I'm personally content with some redundant saving on a user action

[Jouramie]

The more I look at get_settings, the more I think that the fact that it can save is an undesirable side effect in most cases. This function does two things, it should only do one, accessing the current settings (not saving them). As you said, in most cases, callers are happy with just the values and don't need the actual file to be up-to-date. It's only in the case where we actually want to open or modify the settings that it needs to be saved.

That being said, changing that seems like a medium size and maybe impacting refactor. I've checked only a handful of the calls and have no idea if it would impact something to move the save out of get_settings. All things considered, the unnecessary save seems pretty harmless.

[qwint]

so is that an approval? 😅

[black-sliver]

I said something alone those lines in discord:

get_settings() is not supposed to save.
atexit hook is supposed to save if and only if the file will be changed.
The biggest reason for that is the race when you open multiple AP programs.
The reason why we can create a file if it doesn't exist yet, is because that is most likely safe. Multiple programs opened at the same time in a racy fashion would save identical files.

So, the solution presented in this PR is mostly correct - you only safe if the user explicitly asks to have the current file.
The thing this PR gets "wrong" is that it also saves if it hasn't changed, however the world cache to detect if saving would be required or not does not exist yet, so this is a good stop gap.

[black-sliver]

Thanks!