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

Use the C locale for non-Windows CharArrayFromFormatV() and StringFromFormat() #2004

Merged
merged 1 commit into from
Feb 25, 2015
Merged

Use the C locale for non-Windows CharArrayFromFormatV() and StringFromFormat() #2004

merged 1 commit into from
Feb 25, 2015

Conversation

randomstuff
Copy link
Contributor

The Windows implementations of CharArrayFromFormatV() and StringFromFormat() use the "C"/".1252" locale instead of the user locale (using _vsnprintf_l). On non-Windows, the user locale was used.

This leads to bugs on non-Windows: the Overclock parameter was serialised with the user locale (0,279322 in some locale) and was interpreted back as 0 (because the "C" locale is used for parsing the string).

Make non-Windows CharArrayFromFormatV() and StringFromFormat() consistent with their Windows counterpart.

@BhaaLseN
Copy link
Member

BhaaLseN commented Feb 2, 2015

Hmm, shouldn't we consistently use the C locale everywhere, for everything? User locale is always bound to make problems when inter-opping, or when assuming something/nothing about the values and their representation.

@randomstuff
Copy link
Contributor Author

The non-C locale is used (and setup) by the GUI toolkit (wxWidgets via wxLocale) in order to handle localisation.

@CrossVR
Copy link
Contributor

CrossVR commented Feb 4, 2015

@dolphin-emu-bot rebuild

The code looks good to me.

@randomstuff
Copy link
Contributor Author

uselocale and friends are available in Android API 21. Since dolphin is build with android:minSdkVersion="18", I guess I should use the old code (without locale_t) for Android.

…mFormat()

The Windows implementations of CharArrayFromFormatV() and
StringFromFormat() use the "C"/".1252" locale instead of the user
locale (using _vsnprintf_l). On non-Windows, the user locale was used.

This leads to bugs on non-Windows: the Overclock parameter was
serialised with the user locale ("0,279322" in some locale) and was
interpreted back as "0" (because the C locale is used for parsing the
string).

Make non-Windows CharArrayFromFormatV() and StringFromFormat()
consistent with their Windows counterpart.

The locale code is not enables for Android:: uselocale is only
available since API 21 and API 21 only supports C and C.UTF-8.
@CrossVR
Copy link
Contributor

CrossVR commented Feb 4, 2015

@randomstuff IIRC uselocale() in the NDK doesn't do anything, as far as I know Android always assumes C locale.

@CrossVR
Copy link
Contributor

CrossVR commented Feb 4, 2015

@dolphin-emu-bot rebuild

@randomstuff
Copy link
Contributor Author

Yes, recent version of bionic have locale management fucntions but they only support "C" vs "C.UTF-8" (the default).

@Mullin
Copy link
Contributor

Mullin commented Feb 12, 2015

LGTM

@randomstuff
Copy link
Contributor Author

This bug is stil on master. Can this be merged or should another fix be used ?

@degasus
Copy link
Member

degasus commented Feb 25, 2015

I'm for merging as it is.

degasus added a commit that referenced this pull request Feb 25, 2015
Use the C locale for non-Windows CharArrayFromFormatV() and StringFromFormat()
@degasus degasus merged commit d6ccbb1 into dolphin-emu:master Feb 25, 2015
@randomstuff randomstuff deleted the stringutil-c-locale branch February 28, 2015 10:17
@Nucleoprotein
Copy link

This was reverted ? I get problem with saving CPU Override in r6963 on Windows.

@randomstuff
Copy link
Contributor Author

@Tapcio: This specific patch does not change the behaviour on Windows. I think, it was supposed to have been fixed on Windows.

@Nucleoprotein
Copy link

It's not fixed on Windows.
My Windows is set to Polish locale and it saves coma instead of dot when saving CPU Override to ini.. Then when I re-run Dolphin CPU Override is set to 0%, so then I cant run game - I need to uncheck option. This issue is really even more annoying when I want to enable CPU Override, because it is always 0 and slider is disabled (grayed out) when option is disabled so enabling it causes game freeze (ie. sets slider value - 0% ...).

// sorry for bad english

@BhaaLseN
Copy link
Member

BhaaLseN commented Jul 8, 2015

Can you please create an issue on our issue tracker, instead of posting on a closed PR? This would help us more in keeping track of issues that are still present.

@Nucleoprotein
Copy link

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