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

Abitily to use host system language/regional settings #2831

Closed
wants to merge 16 commits into from

Conversation

Karlson2k
Copy link
Member

I believe that any serious program must use system-wide settings, like language/regional settings. No such settings is used in XBMC for now.
That might confuse inexperienced users that are forced to find and change language after first XBMC installation. Problem is even worse if user doesn't know English.
This PR add support for using host system regional settings as default XBMC language/regional settings. User is still free to change XBMC regional settings, default settings are used only at first XBMC run and after XBMC settings reset.
Code is trying to find best matching language and region. If no suitable language is found, code defaulting to "English", If system region is not available in XBMC, than language default region is used.

Code in this PR detects host system setting on Win32, Linux, FreeBSD and Android (by @Montellese) platforms, but platform support can be easily extended.

SetCurrentRegion(strName);
if (m_globalObject)
{
const CStdString& strName=CSettings::Get().GetString("locale.country");

This comment was marked as spam.

This comment was marked as spam.

@ace20022
Copy link
Member

ace20022 commented Jun 5, 2013

Just had a brief look, great idea. There are quite long parameter lists, maybe a struct could be useful.

@Fneufneu
Copy link
Member

Fneufneu commented Jun 5, 2013

FreeBSD work exactly like Linux here

@Karlson2k
Copy link
Member Author

@jmarshallnz Rewritten according to your comments

@Karlson2k
Copy link
Member Author

@jmarshallnz Should I move CLanguageTag to separate .h/.cpp?

@jenkins4xbmc
Copy link
Contributor

Is this PR ment to be tested by jenkins?

@Karlson2k
Copy link
Member Author

jenkins test this please

@garbear
Copy link
Member

garbear commented Jun 16, 2013

after several frustrating false-starts, the new command has been changed to "jenkins test this, you annoying piece of crap"

@Karlson2k
Copy link
Member Author

@jmarshallnz ?

@jmarshallnz
Copy link
Contributor

jenkins build this please

@jmarshallnz
Copy link
Contributor

Ideally the extra class would move to another file, but I'm aware this will have build implications, so could be done in another round so that this gets in and gets tested.

@Memphiz
Copy link
Member

Memphiz commented Jul 7, 2013

@jmarshallnz BTW - the jenkins trigger doesn't Need to be a single comment - you can just add it somewhere in a comment and it will trigger aswell :)

@Memphiz
Copy link
Member

Memphiz commented Jul 7, 2013

Build failed on ios because the pr is not rebased to Master (and has outdated jenkins scripts) - on droid it failed because of something Else i have Seen before but can't remember what (maybe rebase Fixed that one too)

@Karlson2k
Copy link
Member Author

@jmarshallnz Done.
jenkins build this please

@MartijnKaijser
Copy link
Member

Only team members can initiate a build.

jenkins build this please

@Karlson2k
Copy link
Member Author

Note to Android devs:
to add android support you just need to set sysLang at Karlson2k@ee7af92#L0R620 to result of java function Locale.getDefault().toString().

@Montellese
Copy link
Member

Is this good to be merged? I volunteer to look at the android side over the weekend if this gets in during this merge window.

@Karlson2k
Copy link
Member Author

@Montellese nice! Android support should be simple now.
Meantime could you ask jenkins to rebuild this with last commit?

@Montellese
Copy link
Member

jenkins build this please

@Montellese
Copy link
Member

You can get the top three commits from Montellese/xbmc@master...autoset_lang for android support.

The last commit can be squashed into your other commits. IMO it doesn't make sense to only do the "set default language" for specific platforms. Any platform not supporting it will return "English" anyway.

Maybe it makes sense to retrieve the default language returned from CLangInfo::GetAvailableDefaultLanguage() from the actual default value of the locale.language setting so "English" is not hardcoded.

@Karlson2k
Copy link
Member Author

@Montellese Thanks for nice idea! And for android code too. :)

@MartijnKaijser
Copy link
Member

jenkins build this please

@jmarshallnz / @Montellese
ok for next window?

@Montellese
Copy link
Member

I haven't looked at the changes in detail but I tested it on win32 and it works and I implemented it on android and it works (TM). IMO it's a very valid and valuable addition as it's something I expect of almost every application I install.

@@ -0,0 +1,41 @@
/*
* Copyright (C) 2013 Team XBMC
* http://www.xbmc.org

This comment was marked as spam.

@ghost ghost assigned Montellese Jul 30, 2013
@Montellese
Copy link
Member

Please squash your last cleanup commit into the proper commit.

@Karlson2k
Copy link
Member Author

@Montellese done

@theuni
Copy link
Contributor

theuni commented Jul 31, 2013

@Montellese: beautifully done, thanks.

Though, AConfiguration_getCountry() and AConfiguration_getLanguage() might've been a bit easier ;)

@Karlson2k: Have you verified that asian/rtl languages have working default fonts? If a Chinese user runs XBMC and gets Chinese by default, it's crucial that they not see blocks.

@Montellese
Copy link
Member

@theuni: yeah I considered those as well but it would have required changes to the rest of the parsing logic and it was a chance to look into your JNI refactoring and how it works now ;-)

@ace20022
Copy link
Member

ace20022 commented Oct 3, 2013

It's not yet ready for merge. I'm working on solution for languages scripts missing in base fonts.

Could you please open a separate pr with bug-fixes only? So we can merge them asap?

@ace20022
Copy link
Member

@Karlson2k Gentle reminder, could you please separate the bugfix(es)?

@Karlson2k
Copy link
Member Author

@ace20022 Soon

@MartijnKaijser
Copy link
Member

@jmarshallnz @t-nelson
Although merge window is closed I still think this would be potential good candidate for user friendliness. Thoughts?

@Karlson2k
Copy link
Member Author

@MartijnKaijser, this PR is really good for users, but to merge it we need to implement detection of character presence in font. I know how to implement it, but this in turn may require to extend language .xml to include string with full alphabet.


bool CLanguageTag::operator!=(const CLanguageTag& other) const
{
return !operator==(other);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@MartijnKaijser
Copy link
Member

@Karlson2k Any progress on this?

@Karlson2k
Copy link
Member Author

@MartijnKaijser This should be rebased, but before merging it some companion PR for fonts handling must be merged.

I'm thinking about implementing "fallback" font for missing chars. General idea: if some glyph is missing in current font, try to substitute it from "fallback" font defined in skin and, if missing in skin fallback font, substitute glyph from system "arial" font. @jmarshallnz What do you think?

@jmarshallnz
Copy link
Contributor

@Karlson2k if it can be done easily, sure. I'm not sure how easy it is to get the information out into textlayout object which I think is where it will be needed.

@topfs2
Copy link
Contributor

topfs2 commented Aug 11, 2014

Just from a quick glance at the discussion here, Isn't the font issue rather unrelated to this PR? I mean we had that problem if the user selected the language in the settings too?

@Karlson2k
Copy link
Member Author

@topfs2 Font issue makes Kodi with this PR unusable on some systems if system default language alphabet is not present in default font.

@topfs2
Copy link
Contributor

topfs2 commented Aug 11, 2014

But wouldn't that still happen if you pick that language in settings anyways?

@Karlson2k
Copy link
Member Author

Of course. But in this case it will be much simpler to blindly switch it back.
If Kodi is started with only blanks and digits, it should be not so easy to open settings, find required setting and switch it to something readable.

@MartijnKaijser MartijnKaijser removed this from the Pending for inclusion milestone Jun 14, 2015
@da-anda
Copy link
Member

da-anda commented Jan 22, 2017

are there any interests in resurrecting this PR?

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.

None yet