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

Remove TKM string code #11906

Merged
merged 11 commits into from Jan 28, 2024
Merged

Remove TKM string code #11906

merged 11 commits into from Jan 28, 2024

Conversation

frosch123
Copy link
Member

Motivation / Problem

#11341 added a {TKM "calendar" "wallclock"} string control code:

  • It would insert the strings calendar or wallclock into the string depending on a game setting value.
  • This command is not supported by other tools (eints).
  • The "calender" and "wallclock" texts do now allow any cases/genders/plurals, which may be required in certain languages and/or future strings.
  • In the case of vehicle service intervals: there are actually three cases ("days", "minutes", "percent"). TKM cannot handle this.
  • Game settings are rather complex, and should be handled in the code. Templating the strings depending on settings moves too much magic into the strings.

Description

This PR removes usage and support for TKM:

  • Strings are duplicated with slight alterations. This pattern was common before.
  • The UsingWallclockUnits is called from the individual GUI code to choose string variants.
  • UsingWallclockUnits must know, whether it should check the current-game or new-game settings. The invidual GUI code knows this better than the general string templating code.
  • UsingWallclockUnits is still called with the magic _game_mode == GM_MENU in the settings GUI. I am not aware of any case, where this assumption is false.
  • Settings now support callbacks to supply dynamic title, help and value strings.

Limitations

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

Looks okay to me, barring some minor coding style nitpicks.


auto *wid = this->GetWidget<NWidgetCore>(WID_GRAPH_FOOTER);
if (wid != nullptr && TimerGameEconomy::UsingWallclockUnits()) {
wid->SetDataTip(STR_GRAPH_LAST_72_MINUTES_TIME_LABEL, STR_NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we, for consistency, also have a string when it's not using wall clock units?
This PR keeps it as is, but it feels weird only having a tooltip some of the times. Possibly not for this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

There technically is, but different:

  • Calendar: show year numbers along the X axis.
  • Wallclock: there are no labels on the X axis, only this general text "72 minutes in total".

src/settings_internal.h Outdated Show resolved Hide resolved
src/settings_table.cpp Outdated Show resolved Hide resolved
src/settings_table.cpp Outdated Show resolved Hide resolved
src/settings_table.cpp Outdated Show resolved Hide resolved
src/table/settings.h.preamble Outdated Show resolved Hide resolved
src/table/settings/company_settings.ini Outdated Show resolved Hide resolved
src/table/settings/locale_settings.ini Outdated Show resolved Hide resolved
@frosch123 frosch123 merged commit 17dfc1a into OpenTTD:master Jan 28, 2024
20 checks passed
@frosch123 frosch123 deleted the no_tkm branch January 31, 2024 20:06
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