Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change: split check and update callbacks for settings #9302

Merged
merged 5 commits into from May 29, 2021

Conversation

rubidium42
Copy link
Contributor

Motivation / Problem

In the settings there is currently one callback that does both check whether a change is allowed, and it updates when a change is performed. This means that one first needs to set the new value, then call that callback and when that fails the value needs to be reset again.
Similarly there is no useful callback for strings, as the callback will be called with 0 as parameter and that cannot be used.

The goal was to be able to validate the server and client name when they are entered from the console, and not be in a situation where the old name is lost once the validation of the name is happening.

Description

Split the validation/check/pre change callback from the update/post change callback.
The pre-change callback will be of type bool Func(std::string &new_value) or bool Func(int32 &new_value) which allows the pre change to update the value if needed. For example removing/clearing a bit that should never be set, or more useful for strings trimming white space or replacing * with an empty string.
The post-change callback will be of type void Func(const std::string &new_value) or void Func(int32 new_value).

The effect of splitting these changes is that it becomes way clearer on which settings there are actual checks, and for which there is merely a post-change callback.

As added bonus with this precheck it becomes impossible to clear the server and client name via the console or the UI. The only viable path is clearing it in the configuration, but that should only be feasible for the dedicated server and there are guards in place to replace the empty names with something.

Limitations

None that I know of.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@rubidium42 rubidium42 marked this pull request as draft May 27, 2021 17:50
@rubidium42 rubidium42 force-pushed the setting-prechange-check branch 2 times, most recently from 3c01b2b to 526decc Compare May 27, 2021 19:39
@rubidium42 rubidium42 marked this pull request as ready for review May 27, 2021 20:04
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully understand some comments might be out-of-scope of this PR, but they also might be easy to fix while at it. I leave that up to you :)

src/settings.cpp Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Show resolved Hide resolved
src/settings.cpp Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
src/settings.cpp Show resolved Hide resolved
src/table/settings.ini Show resolved Hide resolved
src/table/settings.ini Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"It seems fine" :D

@rubidium42 rubidium42 merged commit 4144e94 into OpenTTD:master May 29, 2021
@rubidium42 rubidium42 deleted the setting-prechange-check branch May 29, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants