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

Fix: Missing 'Town names:' colon in map gen GUI #8986

Merged
merged 1 commit into from Apr 25, 2021

Conversation

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Apr 10, 2021

Motivation / Problem

The map generation window has a missing colon after Town names.

missing_colon

Description

I added the missing colon.

Limitations

There was some discussion in #8566 about renaming the strings to reflect the fact that they are no longer in Options. This would become STR_MAPGEN_TOWN_NAMES. However, I assume this would break all existing translations. Thoughts?

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')
@perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Apr 10, 2021

Given that 1.11 just went out, maybe moving the string to where the rest of the World Generation window strings are makes more sense. I don't know what implications removing a string has though. Thanks for doing this!

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Apr 10, 2021

Yeah the string identifier ought to be renamed now that it is used in a different window.

@LordAro
Copy link
Member

@LordAro LordAro commented Apr 10, 2021

There are implications with eints & backporting translations to consider when renaming strings, of course

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Apr 10, 2021

Perhaps a solution would be to backport this fix, then open a separate codechange PR to rename the strings? (There are at least four: this one plus its tooltip and a title/tooltip pair for which side of the road to drive on, mentioned in #8566.)

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 10, 2021

The main implication is that backporting will no longer pick up this string if anyone would fix their translation. I would say, after our next point-release, merge this, and so be it.

Over time, this will happen and more often anyway, till we do the next real release :P Let's not make our lives a lot more difficult over a single string :)

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 12, 2021

Or, possibly I was thinking to complex: just rename the string in this PR, and backport this PR in full. Solves any backport issues with translations in the future :P Wouldn't that work?

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Apr 12, 2021

(Not sure if you're asking a rhetorical question or not): I don't fully understand backporting or eints, so just tell me what you decide and I'll do it! 😀

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 25, 2021

With respect to the renaming/moving of the strings I've gone with a slightly more complicated strategy: assemble a list of strings that need to be changed before the next release branch, and then do it before the next release. See #9101. That keeps this PR, and the backporting of language updates for the coming point releases much easier until then.

@rubidium42 rubidium42 merged commit 9d6ff1c into OpenTTD:master Apr 25, 2021
11 checks passed
@2TallTyler 2TallTyler deleted the missing_townnames_colon branch Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants