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 separators from language addon #9794

Merged
merged 1 commit into from May 14, 2016

Conversation

Projects
None yet
4 participants
@phate89
Copy link
Contributor

commented May 11, 2016

Since some system doesn't have a locale I changed the number formatting to use an option in the language addon.
If there's not that option in the language addon it will fall back to en-us (like 100,000.45)
@stefansaraev @MilhouseVH This should make it work also for Open/LibreElec

I'm not sure if we want an option to let the user choose like for dates (and what other options there are, I know only 1,000.4 and 1.000,4)

@phate89 phate89 force-pushed the phate89:use_language_sep branch from c1ef11f to cca2d43 May 11, 2016

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

thanks! I dont remember what exacly was wrong with separators (how to reproduce), so I'll let @MilhouseVH test this ;)

btw, all posix systems should have POSIX (and C, which is guaranteed to be same as POSIX), not sure if this info may be useful to you.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

Thanks for this, it certainly seem to be working. I'm setup for English UK (24h), and now see the comma grouping for movie votes etc. Thanks.

Edit: Doh, I'd changed setting to English US (12h) to test this. It is in fact only working for English US (12h) at the moment and doesn't appear to be "falling back" to any default thousandsseparator (other than none at all), so if I change to English UK (24h) then I have no thousandsseparator. English US (24h) also has no thousandsseparator.

Presumably thousandsseparator and decimalseparator will eventually need to be added to for all regions in all langinfo.xml that use different values to en-US?

@phate89 phate89 force-pushed the phate89:use_language_sep branch from cca2d43 to 629dc0c May 11, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2016

oops I forgot to set the fallback also for grouping. Without that it wasn't working. Now the fallback works.
Right now I set it only for USA (12h) because I'm not sure all the regions use that method (and they all fall back to that format anyways). Anyone knows formats for that regions?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

Thanks, looks good - I've got the comma group separator for English UK (24h) now.

I'd have thought all native English-speaking regions would use comma as the group (thousands) separator and period as the decimal point, so as you have it now should be fine for the English regions. Although perhaps Central Europe and India could be different.

Obviously this isn't going to work so well for those languages/regions that use period/comma as group/decimal separator as they will now fall back to comma/period. This will need localising for all the regions that need it, maybe that should be handled by subsequent PR(s)?

@phate89 phate89 force-pushed the phate89:use_language_sep branch from 629dc0c to fcee3e0 May 11, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2016

I checked manually with windows locales (in english addon) and seems they're all the same except for india where they group first 3 numbers and then by 2 (something like 10,00,00,000).
I added all the values in the shipped english addon. All other addons needs to be updated but in the repository..

@phate89 phate89 force-pushed the phate89:use_language_sep branch from fcee3e0 to be3dd5b May 11, 2016

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

jenkins build this please

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

@MilhouseVH @phate89 ready to go ?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

I'm happy, although other non-English languages will need to their defaults added once this merges.

@stefansaraev stefansaraev merged commit 96c4c5d into xbmc:master May 14, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Merged build finished.
Details

@hudokkow hudokkow added this to the Krypton 17.0-alpha2 milestone May 14, 2016

@phate89 phate89 deleted the phate89:use_language_sep branch Jun 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.