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

Qt::Application resets numeric locale to "C" #51

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

f-fr
Copy link
Contributor

@f-fr f-fr commented Nov 11, 2021

This is necessary (and adviced by Qt) because Qt sets the locale to
the system default when creating an application object. However,
Crystal functions like to_f depend on the locale being set to "C" by
default.

Because the locale is Unix-specific the reset is done by adding an
additional C helper function, which is called from
Qt::Application#initialize.

For instance, the following example would raise an error with my system locale (de_DE):

require "qt5"

"4.2".to_f   # works
qapp = Qt::Application.new
"4.2".to_f   # fails because the locale has been changed such the decimal separator is "," now and not "."

Because the constant LC_NUMERIC is system dependent (although it's probably always 1 nowadays, but you can't be sure), we add a new C function (instead of just a lib-binding to setlocale from the Crystal side). This makes everything a little bit more complicated because we need to make sure that bindgen finds the new function and generates a binding for it.

This is necessary (and adviced by Qt) because Qt sets the locale to
the system default when creating an application object. However,
Crystal functions like `to_f` depend on the locale being set to "C" by
default.

Because the locale is Unix-specific the reset is done by adding an
additional C helper function, which is called from
`Qt::Application#initialize`.
@f-fr f-fr mentioned this pull request Nov 11, 2021
@docelic
Copy link
Collaborator

docelic commented Nov 11, 2021

Heya,
The patch looks OK.

Is your intention for the patch to have this happen automatically instead of users having to remember to reset the locale?

@f-fr
Copy link
Contributor Author

f-fr commented Nov 11, 2021 via email

@docelic
Copy link
Collaborator

docelic commented Nov 11, 2021

Would you mind also updating the README (maybe somewhere near the bottom?) to mention this?
If we include it, we should also document it since it is a non-standard addition.

If you don't want to bother with the README, no worries, I can do it a little bit later when I get a chance.

Thanks!

@docelic
Copy link
Collaborator

docelic commented Nov 12, 2021

Wonderful, thanks!

@docelic docelic merged commit bb908d5 into Papierkorb:master Nov 12, 2021
@f-fr f-fr deleted the locale-reset branch November 13, 2021 20:06
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