Emscripten doesn't persist all setting changes

[embeddedt]

Version of OpenTTD

Latest JGR commit on Emscripten, however, this is not a JGRPP-specific issue.

Expected result

openttd.cfg settings to be persisted across refreshes.

Actual result

Some settings (like the one controlling what category of settings are visible) do not appear to be persisted after the tab is closed and reopened.

This happens because many setting changes in OpenTTD do not trigger a write to openttd.cfg, and due to the limitations the browser environment provides, closing a tab is effectively the same as killing the OpenTTD process on Linux. Therefore, it is not possible to guarantee that settings are saved on exit like other platforms.

Here are some potential solutions (in no particular order):

• Add a SaveToConfig() call in every instance where a setting is changed.
• Save settings whenever the mouse cursor leaves the canvas (i.e. moves towards the back button).
• Aggressively save the config file every few seconds on Emscripten; this will hide the issue but won't solve the underlying problem.
• Save settings 1 second after the user clicks the canvas (since almost all setting changes are triggered by user interaction).
• Use an onbeforeunload handler to prevent the user from immediately closing the page. If they click OK, the page will be closed anyway and there's nothing we can do about it, however, if they click Cancel, OpenTTD will run again and have a chance to finish saving their settings. The problem with this solution is that there is no way I see to know whether the settings needs saving or not; therefore this handler will probably be perceived as annoying.

Steps to reproduce

1. Run OpenTTD in the browser with Fix: [Emscripten] Use non-XDG directories to simplify local storage (#9207) #9207 applied, otherwise no settings will be saved due to a bug with the storage path.
2. Click Settings.
3. Change the category of settings that are shown.
4. Close the settings dialog.
5. Refresh the page.
6. Open the Settings dialog again.
7. Notice that the category has reverted to its prior value.

[TrueBrain]

This happens because many setting changes in OpenTTD do not trigger a write to openttd.cfg

This is not true. Many settings do in fact trigger a write to openttd.cfg. I kinda made sure it did, as otherwise Emscripten wouldn't never save its config :D In fact, it is the reason this now happens on all platforms, that even if your game crashes, your change has been saved :)

What you run into here, is that we do have a few code-paths that do not use the Settings wrapper to change a setting. These are not stored after changed, and Emscripten is feeling the effects of that. In other words, we already do have code for this in place, some lines of code are just sneaky going behind its back ;)

The main issue: finding those places is difficult. This one is most likely https://github.com/OpenTTD/OpenTTD/blob/master/src/settings_gui.cpp#L2365 . But I am sure there are others that change the settings themselves instead of via the wrappers.

A quick grep suggests we have roughly 83 of those instances (this will miss some instances, like strecpy variants, for sure!)
grep -n -R "_settings_" src | grep -v "_settings_game" | grep -E "_settings[a-z_\.]+ =[^=]" | wc -l
But not all of them are stored in openttd.cfg.

These are 42 unique keys (again, there are a few more that use different syntax).
grep -n -R "_settings_" src | grep -v "_settings_game" | grep -E "_settings[a-z_\.]+ =[^=]" | sed -r 's/.*(_settings[a-z_\.]+ =).*/\1/' | sort | uniq

_settings_client.gui.autosave =
_settings_client.gui.last_newgrf_count =
_settings_client.gui.refresh_rate =
_settings_client.gui.settings_restriction_mode =
_settings_client.gui.station_dragdrop =
_settings_client.gui.station_gui_group_order =
_settings_client.gui.station_gui_sort_by =
_settings_client.gui.station_gui_sort_order =
_settings_client.gui.station_numtracks =
_settings_client.gui.station_platlength =
_settings_client.gui.station_show_coverage =
_settings_client.music.playing =
_settings_client.music.playlist =
_settings_client.music.shuffle =
_settings_client.network.max_spectators =
_settings_client.network.server_advertise =
_settings_client.network.server_port =
_settings_newgame.difficulty.industry_density =
_settings_newgame.difficulty.number_towns =
_settings_newgame.difficulty.quantity_sea_lakes =
_settings_newgame.difficulty.terrain_type =
_settings_newgame.game_config =
_settings_newgame.game_creation.amount_of_rivers =
_settings_newgame.game_creation.custom_sea_level =
_settings_newgame.game_creation.custom_terrain_type =
_settings_newgame.game_creation.custom_town_number =
_settings_newgame.game_creation.desert_coverage =
_settings_newgame.game_creation.generation_seed =
_settings_newgame.game_creation.heightmap_height =
_settings_newgame.game_creation.heightmap_rotation =
_settings_newgame.game_creation.land_generator =
_settings_newgame.game_creation.landscape =
_settings_newgame.game_creation.map_x =
_settings_newgame.game_creation.map_y =
_settings_newgame.game_creation.se_flat_world_height =
_settings_newgame.game_creation.snow_coverage =
_settings_newgame.game_creation.starting_year =
_settings_newgame.game_creation.tgen_smoothness =
_settings_newgame.game_creation.town_name =
_settings_newgame.game_creation.variety =
_settings_newgame.game_creation.water_borders =
_settings_profiles =

So that should be doable to fix :)

[embeddedt]

Thanks @TrueBrain; I see that this is in fact handled in most places.

I'd be happy to try and track down as many of the remaining settings I can find and submit a PR for that. I assume the proper fix is to refactor these to use SetSettingValue instead? If so, how do I convert a setting name (e.g. _settings_newgame.game_creation.heightmap_rotation) to an index for that function?

[TrueBrain]

Yeah, how to fix that .. that I was wondering about too. SetSettingValue is not a really nice function to use, and yeah .. I currently do not have an answer for you, sorry :) I think we need some glue for these cases .. but no clue how that should look.

Maybe one of the other devs have some ideas how to approach this :D

[embeddedt]

I wonder if I could add a simple helper function in settings.cpp that handles flushing to the config. Then the only change that's needed is to add a call to it after each manual setting adjustment in the other files.

Something like this, perhaps?

void FlushSettingsChange()
{
        SetWindowClassesDirty(WC_GAME_OPTIONS);

        if (_save_config) SaveToConfig();
}

[glx22]

Many SetSettingValue() calls use GetSettingFromName() to find the index.

[TrueBrain]

I wonder if I could add a simple helper function in settings.cpp that handles flushing to the config. Then the only change that's needed is to add a call to it after each manual setting adjustment in the other files.

No, that would be an anti-feature to me :) Currently the setting system does handle saving when it has to, but all those "illegal" writes are messing that up.

Mostly why they are an issue, at least for me, is that they bypass any validation set for the setting, and any post-callback set. They just change a setting and don't tell anyone. That, to me, is the real issue here. If they just would use the right functions, everything will happen automagicaly :)

So, no, please do not make a FlushSettingsChange :)

[rubidium42]

I've got an idea for a probably better solution where the act of (attempting) to set the setting triggers all kinds of things via Settings functions. This includes all kinds of validations, and would make changing settings from in the game's code much easier. I'm already working on parts, but it's progressing very slowly as it requires many other significant and complicated changes to happen first, and those take some time to get reviewed and accepted.

Mostly as "just" adding a simple helper function to do something is not going to fix your problem, it is going to fix the symptoms you see... and by doing it that way we will keep getting symptoms (i.e. places where that simple helper function is not called). In other words, with your "solution" we will keep getting bug reports for settings that someone changed that were not saved and the likes, and that is therefor not a solution to the bug.