dosbox-staging icon indicating copy to clipboard operation
dosbox-staging copied to clipboard

Better handling for config settings that can/cannot be changed at runtime

Open weirddan455 opened this issue 2 years ago • 7 comments

Are you using the latest Dosbox-Staging Version?

  • [X] I have checked releases and am using the latest release.

Different version than latest?

latest main

What Operating System are you using?

Linux x86_64

If Other OS, please describe

Arch Linux

Is your feature request related to a problem? Please describe.

I was looking at issue #2527 where setting fullscreen=true does not work at runtime. Looking at the code, we have an enum that describes when a particular config setting is allowed to be changed.

struct Changeable {
	enum Value { Always, WhenIdle, OnlyAtStart, Deprecated };
};

Pbool = sdl_sec->Add_bool("fullscreen", always, false);

pbool = sdl_sec->Add_bool("pause_when_inactive", on_start, false);

Here we can see the usage code of two values. fullscreen is meant to always be changeable while pause_when_inactive is meant to not be changeable at runtime.

However, I can't find real enforcement of this nor any documentation to the user. Ideally, typing in pause_when_inactive=true would present the user with an error indicating that this value cannot be changed. However, as it currently stands, there is no error and the window actually flickers because it is running some re-init code. We shouldn't be trying to refresh the window when the user wants to change an unchangeable value.

Furthermore, it actually seems to update the config setting to the new value even though the pause functionality is unchanged.

pause

Describe the solution you'd like

Unchangeable values should report an error when the user tries to change them. We should not be running init functions when this happens. We should not be updating the value at all.

It would also be nice if this was documented somewhere. Perhaps in the config file, we write out when this setting is allowed to be changed.

Describe alternatives you've considered

I don't know but I am open to suggestions.

Add any other context or screenshots about the feature request here.

No response

Code of Conduct & Contributing Guidelines

  • [X] Yes, I agree.

weirddan455 avatar Nov 02 '23 05:11 weirddan455

Agree with all of the above. Is there anything we're missing here with @weirddan455 perhaps, @kcgen ?

johnnovak avatar Nov 02 '23 08:11 johnnovak

Related #1027

Torinde avatar Nov 02 '23 11:11 Torinde

Agree with all of the above. Is there anything we're missing here with @weirddan455 perhaps, @kcgen ?

Each module's Init() (and optional Destroy()) routines are triggered for key=value changes, and it's up to the module how fine-grained it wants to be about processing the new conf settings.

Definitely a bug here; the setup system should block and warn when the user tries to change an on_start key=value (and not call Init()).

kcgen avatar Nov 02 '23 15:11 kcgen

For me it is not clear what is the difference between Always and WhenIdle. What does idle precisely mean?

FeralChild64 avatar Nov 02 '23 15:11 FeralChild64

For me it is not clear what is the difference between Always and WhenIdle. What does idle precisely mean?

I asked @kcgen once but forgot the explanation. It's really inintuitive, at least the naming but maybe conceptually too. Probably time to address that too as it seems to confuse everybody.

johnnovak avatar Nov 02 '23 22:11 johnnovak

I also forget 😅; but from a user perspective the allowance is same: allow config changes when sitting at the command prompt, and so should be collapsed into a common 'anytime' enum at the code-level (and then we'll have 'onstart' as the other).

kcgen avatar Nov 02 '23 22:11 kcgen

I also forget 😅; but from a user perspective the allowance is same: allow config changes when sitting at the command prompt, and so should be collapsed into a common 'anytime' enum at the code-level (and then we'll have 'onstart' as the other).

With the OSD "at the command prompt" will no longer be true. We just need two options, yeah: only at startup, and anytime.

johnnovak avatar Nov 03 '23 00:11 johnnovak