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

Feature: support non-ASCII currency separator #9121

Merged
merged 3 commits into from Apr 27, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 27, 2021

Motivation / Problem

The currency separator, in the configuration for the INI file is a single character which means only ASCII characters may be used as separator. The separator field itself is 8 bytes long, so it feels like someone did consider non-ASCII characters but did not follow through.
Next to this the string length limiting that is happening when writing the string into the configuration could cause a valid multibyte encoded Utf8 character to only be placed there partially. As a result, the configuration has become an "invalid" string.

Description

Solved by:

  • changing the configuration for the INI file to use SDT_STR instead of SDT_CHR and the same type as prefix and suffix.
  • changing the settings GUI to support the number of bytes allowed by the buffers in CurrencySpec; remove the magic constants in the code with sizeof. Changes separator from 1 to 7 bytes and prefix/suffix from 12 to 15.
  • running str_validate after strecpy since strecpy might have trimmed the string
  • some refactoring of writing strings, meaning the same validation is also applied for strings coming from the console/set functions.

Limitations

Support for a single character in a configuration file is lost, though technically that was already effectively a 2 byte string as it will always set the byte after the character to '\0'. However, the currency separator was the only thing that made use of it.

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')
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 27, 2021

Not that I'm particularly opposed to it, but what motivation is there to support non-ASCII currency separators at all?

@rubidium42
Copy link
Contributor Author

@rubidium42 rubidium42 commented Apr 27, 2021

Not that I'm particularly opposed to it, but what motivation is there to support non-ASCII currency separators at all?

Mostly consistency; for the custom currency and there you are limited to 1 byte/ASCII-character. With the locale digit separators you can actually have any length separator, i.e. also non-ASCII. So why should I be forbidden from using a non-breaking space as separator for my custom currency but I can use one for normal numbers?

Copy link
Member

@LordAro LordAro left a comment

¯\_(ツ)_/¯

@rubidium42 rubidium42 merged commit 31c87ba into OpenTTD:master Apr 27, 2021
11 checks passed
@rubidium42 rubidium42 deleted the currency_cleanup branch Apr 27, 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

2 participants