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

COMMON: Change TransMan builtin language to "en" instead of "C" #3165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@criezy
Copy link
Member

@criezy criezy commented Jul 16, 2021

The change in this pull request change Common::TranslationManager so that getCurrentLanguage and getLangById both return "en" instead of "C" for the built-in language.

This seems more logical to me and allows to simplify some code (which is also included in this pull request).
Advantages of this change include:

  • Better readability

For example

#ifdef USE_TRANSLATION
	Common::String cl = TransMan.getCurrentLanguage();
#else
	Common::String cl("en");
#endif

makes more sense than

#ifdef USE_TRANSLATION
	Common::String cl = TransMan.getCurrentLanguage();
#else
	Common::String cl("C");
#endif
  • No need for special cases

We can remove the following in various places

	if (currentLanguage == "C")
		currentLanguage = "en";
  • More language id in themes:

If we ever want to use a custom font for English in a theme, it makes more sense to write

<language id = 'en'  scalable_file = 'foo.ttf'/>

than

<language id = 'C'  scalable_file = 'foo.ttf'/>

If we want to provide an English translation that differs from the hardcoded strings in the source code, I would expect it to be something like "en_GB" or "en_ZA" for example. So that would still be possible with this change.

Also current config files are still correctly loaded as TransMan.parseLanguage("C") still returns kTranslationBuiltinId (as it does for any unrecognised language, such as "en").

@criezy
Copy link
Member Author

@criezy criezy commented Jul 16, 2021

I am not sure if I missed something and if there was a good reason to return "C", which is why I created this pull request. If you see a possible issue with this change, please speak up.

As indicated by SupSuper on Discord, the "C" almost certainly comes from the "C" locale. But TranslationManager::setLanguage() is expecting an ISO language_COUNTRY code, not a full locale. It for example wants fr_FR, not fr_FR.UTF8. So using a locale name for the built-in case seems strange to me.

I just saw that OSystem::getSystemLanguage() is actually documented as returning a POSIX locale, but the various implementations are explicitly only getting the language_COUNTRY part of it and stripping anything else. And I double checked that TranslationManager would indeed not properly handle cases were an encoding is included. So we might want to update the OSystem::getSystemLanguage() documentation as well to clarify that point.

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jul 17, 2021

What will happen if C locale is explicitly set by the user before running ScummVM?
For me, on SDL Linux build, it would return "C".
Shouldn't this be patched to change POSIX and C locales to en?

@criezy
Copy link
Member Author

@criezy criezy commented Jul 18, 2021

What will happen if C locale is explicitly set by the user before running ScummVM?
For me, on SDL Linux build, it would return "C".
Shouldn't this be patched to change POSIX and C locales to en?

You mean if OSystem::getSystemLanguage() returns "C" or "POSIX"? With the changes in this pull request it still works as before as TranslationManager::setLanguage() sets the languages to the builtin language (i.e. English) if the given string does not match the language for an existing translation. So passing "C", "en", or "foobar" would all have the same effect of selecting English.

But if we change the documentation of OSystem::getSystemLanguage() to say it should return a language or language_COUNTRY code, then yes, I think we should also change its implementation to be consistent (and return "en" instead of "C" or "POSIX".

@sev-
Copy link
Member

@sev- sev- commented Jul 27, 2021

This makes all sense to me and in the case of Linux, I'd modify OSystem::getSystemLanguage() behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants