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

Localization fixes follow-up #966

Merged
merged 6 commits into from
Sep 14, 2023
Merged

Localization fixes follow-up #966

merged 6 commits into from
Sep 14, 2023

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Sep 12, 2023

It turns out I've been a bit too eager in replacing .ToStdString() with .utf8_string() in #956, as it caused a few strings to be displayed with broken encoding (this was particularly noticeable on Windows when using languages other than English).
The problem was due to the fact that the implicit std::string -> wxString conversion assumes that the std::string contains data in the current locale encoding and not in UTF-8 (btw I have yet to understand what "current locale encoding" means, since I've observed slightly different behavior between Windows and Linux).
I've tried to avoid those implicit conversions as much as possible, generally by:

  • returning wxString directly from functions when there's no point in having a std::string
  • using formatWxString or a simple wxString append/concatenation in place of fmt::format

I have also taken the opportunity to replace the last two usages of the legacy wxStringFormat function with formatWxString, and also made the play time translatable in game list.
Regarding this last point, note that I've removed the "X seconds" text because the play time is stored in minutes and therefore it will never be displayed in seconds (it's likely a remnant of an older implementation).

@Exzap
Copy link
Member

Exzap commented Sep 12, 2023

We generally want to avoid .ToStdString() since it returns a locale dependent std::string. You were correct to change the download status messages to .utf8_string(), reverting that change is a step backwards imho. But it seems like on the displaying side we also still use implicit conversion:

titleManager->SetDownloadStatusText(statusText);
SetDownloadStatusText expects a wxString, but statusText is a std::string and thus an implicit conversion happens (expecting a locale dependent std::string instead of utf-8). This should be changed to titleManager->SetDownloadStatusText(to_wxString(statusText));

To reiterate, the current convention in our codebase is to use utf-8 everywhere and only convert to other (locale dependent) formats when necessary. Since wxWidgets also supports utf-8 it's not really something we have to worry about and utf-8 everywhere works just fine, as long as we avoid implicit conversion.

Unrelated to your changes but I also noticed we have lines like this:

setStatusMessage(std::string(_("Downloading system tickets...")), DLMGR_STATUS_CODE::CONNECTING);

This is essentially the same as .ToStdString(). Here .utf8_string() should be used instead of std::string(...).

btw I have yet to understand what "current locale encoding" means, since I've observed slightly different behavior between Windows and Linux

It not only differs by OS, but is also controlled by the user's locale and language settings. On Linux you usually get utf-8, but it can be configured to be something else. I wouldn't be able to tell you the behavior on Windows without researching it first, but it certainly isn't utf-8 for std::string, at least when I checked last time.

@Fs00
Copy link
Contributor Author

Fs00 commented Sep 13, 2023

But it seems like on the displaying side we also still use implicit conversion

Oook, fixed that! Thanks for pointing me to the correct place :)

I have also added a commit to disable implicit conversions from wxString to std::string, which caught two unwanted implicit conversions.
It is also possible to disable implicit conversions the other way (std::string -> wxString) using wxNO_IMPLICIT_WXSTRING_ENCODING. I've tried, but gave up soon after because there were compile errors all over the place.

the current convention in our codebase is to use utf-8 everywhere and only convert to other (locale dependent) formats when necessary

I feel that std::u8string should be used everywhere the code deals with UTF-8 encoded strings to make it very explicit, the only problem is that wxString doesn't yet support conversions to/from u8string. Time to open an issue on the wxWidgets repo...

@Exzap
Copy link
Member

Exzap commented Sep 14, 2023

We tried std::u8string initially but support for it is so severely under-cooked, even within the STL itself, that it just isn't useable on a larger scale. You end up converting between string types even more than if you just use the utf-8 in std::string trick. I wager that library adoption of u8string is slow for the same reason. Maybe this will be resolved by C++23/C++26.

wxNO_UNSAFE_WXSTRING_CONV was a nice find btw. Maybe we can also slowly work towards enabling wxNO_IMPLICIT_WXSTRING_ENCODING.

Anyway, PR looks good now. Thanks!

@Exzap Exzap merged commit 96800c6 into cemu-project:main Sep 14, 2023
5 checks passed
@Fs00 Fs00 deleted the more-i18n-fixes branch September 14, 2023 10:57
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.

2 participants