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.

Sign up for GitHub

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

Jump to bottom

xbmc.getLanguage() remediation to provide correct Language & Region information. #23110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

xbmc.getLanguage() remediation to provide correct Language & Region information. #23110

wants to merge 1 commit into from
+514 −26

Conversation

DeltaMikeCharlie
Copy link
Contributor

Description

The CLangCodeExpander class has been expanded to include new ISO 3166-1 (Country Code) lookup capabilities for Alpha-2, Alpha-3 and Name.

CLangInfo::Load and CLangInfo::SetCurrentRegion have been modified to perform additional lookups (ISO_639_X [language] and ISO 3166-1 [country]) as the langinfo.xml file is loaded with the results being stored as new properties of their respective objects. New getter methods were added to obtain these new property values.

GetLanguageConfigCode()
GetLanguageISO6391()
GetLanguageISO6392()
GetLanguageISOEnglishName()
GetCurrentRegionISO31661Alpha2()
GetCurrentRegionISO31661Alpha3()
GetCurrentRegionISO31661EnglishName()

xbmc.getLanguage() has been updated as follows:

xbmc.getLanguage(ENGLISH_NAME) - Remains unchanged for backwards compatibility.

xbmc.getLanguage(ISO_639_1) - Updated to return values based on the 'locale' properties in the langinfo.xml file. An ISO_639_2 value is returned where an ISO_639_1 does not exist for a language.

xbmc.getLanguage(ISO_639_2) - Updated to return values based on the 'locale' properties in the langinfo.xml file. The 'locale' properties in the langinfo.xml file are used to return the ISO_639_2 and ISO 3166-1 alpha-3 codes.

xbmc.getLanguage(ISO_NAME) - Added to return the 'clean' names for the language and regions based on a lookup table using the 'locale' properties in the langinfo.xml file.

For example: 'English-Australia' instead of 'English (Australia) - Australia (24h)'

Motivation and context

Provide correct language information to addons.

#22715

How has this been tested?

Custom addon used to assess the returned values for a number of language and region combinations.

Log extract BEFORE:

xbmc.getLanguage(xbmc.ENGLISH_NAME, True) = English (Australia)-Australia (24h)
xbmc.getLanguage(xbmc.ENGLISH_NAME, False) = English (Australia)
xbmc.getLanguage(xbmc.ISO_639_2, True) = en-au-
xbmc.getLanguage(xbmc.ISO_639_2, False) = en-au
xbmc.getLanguage(xbmc.ISO_639_1, True) = -
xbmc.getLanguage(xbmc.ISO_639_1, False) =

Log extract AFTER:

xbmc.getLanguage(xbmc.ENGLISH_NAME, True) = English (Australia)-Australia (24h)
xbmc.getLanguage(xbmc.ENGLISH_NAME, False) = English (Australia)
xbmc.getLanguage(xbmc.ISO_639_2, True) = eng-AUS
xbmc.getLanguage(xbmc.ISO_639_2, False) = eng
xbmc.getLanguage(xbmc.ISO_639_1, True) = en-AU
xbmc.getLanguage(xbmc.ISO_639_1, False) = en
xbmc.getLanguage(xbmc.ISO_NAME, False) = English
xbmc.getLanguage(xbmc.ISO_NAME, True) = English-Australia

Language with ISO_639_2 only:

BEFORE:

xbmc.getLanguage(xbmc.ENGLISH_NAME, True) = Filipino (Talagog)-Philippines (12h)
xbmc.getLanguage(xbmc.ENGLISH_NAME, False) = Filipino (Talagog)
xbmc.getLanguage(xbmc.ISO_639_2, True) = fil-
xbmc.getLanguage(xbmc.ISO_639_2, False) = fil
xbmc.getLanguage(xbmc.ISO_639_1, True) = -
xbmc.getLanguage(xbmc.ISO_639_1, False) =

AFTER:

xbmc.getLanguage(xbmc.ENGLISH_NAME, True) = Filipino (Talagog)-Philippines (12h)
xbmc.getLanguage(xbmc.ENGLISH_NAME, False) = Filipino (Talagog)
xbmc.getLanguage(xbmc.ISO_639_2, True) = fil-PHL
xbmc.getLanguage(xbmc.ISO_639_2, False) = fil
xbmc.getLanguage(xbmc.ISO_639_1, True) = fil-PH
xbmc.getLanguage(xbmc.ISO_639_1, False) = fil
xbmc.getLanguage(xbmc.ISO_NAME, False) = Filipino
xbmc.getLanguage(xbmc.ISO_NAME, True) = Filipino-Philippines

What is the effect on users?

Standard Kodi users should see no change to the UI. Python add-on developers will now be able to obtain the correct language and region codes and names.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@DeltaMikeCharlie
Copy link
Contributor Author

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

* [PR23110-0001-b1bdd0d170-xbmc.getLanguage-remediation-to-pr.diff](https://jenkins.kodi.tv/job/BuildMulti-PR/26195/artifact/PR23110-0001-b1bdd0d170-xbmc.getLanguage-remediation-to-pr.diff)

* [PR23110.diff](https://jenkins.kodi.tv/job/BuildMulti-PR/26195/artifact/PR23110.diff)

Changes applied.

CastagnaIT
CastagnaIT reviewed Apr 7, 2023
View reviewed changes
xbmc/LangInfo.cpp Outdated Show resolved Hide resolved
CastagnaIT
CastagnaIT reviewed Apr 7, 2023
View reviewed changes
xbmc/LangInfo.cpp Outdated Show resolved Hide resolved
@DeltaMikeCharlie
Copy link
Contributor Author

@CastagnaIT - Thanks for your valuable feedback.

Do you have any comments on the logic or method applied to remediate this bug?

@DeltaMikeCharlie DeltaMikeCharlie force-pushed the getLanguage-2023-04 branch 2 times, most recently from 06b91ae to b4da8ba Compare April 7, 2023 19:37
@DeltaMikeCharlie
Copy link
Contributor Author

@CastagnaIT - Are you awaiting any input or action from me? I'm still learning how the PR process works.

@CastagnaIT
Copy link
Collaborator

PR process guidelines are here: https://github.com/xbmc/xbmc/blob/master/docs/CONTRIBUTING.md#pull-request-merge-guidelines

i dont have so much time lately and im currently involved on another repository so i can't give you a timing
changes seem reasonable (but i dont have give a good look at code yet)
there is also to check if python scrapers and subtitles providers addons works as expected

@DeltaMikeCharlie
Copy link
Contributor Author

@CastagnaIT - Thank your for your response. My main concern was that I was meant to do something that I was unaware of. I will wait until you have some free time.

fuzzard
fuzzard reviewed Aug 9, 2023
View reviewed changes
xbmc/LangInfo.cpp Outdated Show resolved Hide resolved
@DeltaMikeCharlie
Copy link
Contributor Author

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

* [PR23110-0001-dc08041deb-xbmc.getLanguage-remediation-to-pr.diff](https://jenkins.kodi.tv/job/BuildMulti-PR/28341/artifact/PR23110-0001-dc08041deb-xbmc.getLanguage-remediation-to-pr.diff)

* [PR23110.diff](https://jenkins.kodi.tv/job/BuildMulti-PR/28341/artifact/PR23110.diff)

For more information please see our current code style guidelines.

Done

@DeltaMikeCharlie
Copy link
Contributor Author

I would like to attempt to get this PR moving with a suggestion.

My understanding is that the main issue blocking this PR is finding a resource with expertise in this area of the code in order to perform a review.

In the absence of such a resource, I would like to suggest that once v22 “P” is open for development, this PR is merged into the master branch. This will see the change added to the ‘Nightly Development Builds’. Any severe issues should become apparent relatively quickly.

Although the first strategy would be remediation, at any point, the PR could be rolled-back should it be shown to be the source of any new instability. Once it has passed the nightly builds, the ‘Pre release Builds’ should also ensure that no addons or other user features are adversely impacted.

@garbear garbear added this to the "P" 22.0 Alpha 1 milestone Feb 8, 2024
@garbear garbear added Type: Improvement non-breaking change which improves existing functionality API change: Python API change: Binary add-ons v22 "P" labels Feb 8, 2024
@DeltaMikeCharlie
Copy link
Contributor Author

I would like to suggest that once v22 “P” is open for development, this PR is merged into the master branch. This will see the change added to the ‘Nightly Development Builds’. Any severe issues should become apparent relatively quickly.

Are there any other comments regarding my suggestion to merge this PR so that it can be evaluated via the nightly builds?

@scott967
Copy link
Contributor

Sorry, just now saw this. I will have to spend some time on this, but currently xbmc.getLanguage() with region = True returns the locale, not the language code. Which I think the user of this function would rather get the actual language code and use a different function for locale. But that's just IMHO.

So for example, if the active language resource is resource.language.en_gb would expect to get "en-gb" returned for the 639 2-alpha with region, not something like en-de which currently is possible.

@DeltaMikeCharlie
Copy link
Contributor Author

@scott967 - Thanks for your interest. For some additional background on this issue: I created a thread on the forum about a year ago addressing the underlying cause of this issue.

https://forum.kodi.tv/showthread.php?tid=372626

The accuracy of the values that the xbmc.getLanguage() function currently returns depends on how well-formed the text descriptions in selected language addon are.

@scott967
Copy link
Contributor

I picked the commit on top of master and from quick testing seemed to fix getting the language code 2 or 3 alpha, which I had a "band-aid" fix for, but ISO_NAME didn't work and it seemed like it didn't build correctly on my system. So went back and tried to create a clean build environment but managed to kill my buildsteps chain somehow, so ATM can't do more testing.

@scott967 scott967 mentioned this pull request May 20, 2024
Closed
14 tasks
@DeltaMikeCharlie
Copy link
Contributor Author

but ISO_NAME didn't work and it seemed like it didn't build correctly on my system

@scott967 - I noticed that for PR#25062, you tested on Windows 10. Did you also build this PR on Win10?

I seem to recall that in the code there are a number of TARGET_WINDOWS platform-dependent sections. I develop and build on Ubuntu, so perhaps some of the Windows-specific stuff is not quite right.

@scott967
Copy link
Contributor

scott967 commented May 21, 2024 •
edited

Yes, VS2022 on Win10. But I build using kodi.sln generated by cmake and run the code in VS debugger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuzzard fuzzard fuzzard left review comments

@CastagnaIT CastagnaIT Awaiting requested review from CastagnaIT

Assignees
No one assigned
Labels
API change: Binary add-ons API change: Python Type: Improvement non-breaking change which improves existing functionality v22 "P"
Projects
None yet
Milestone
"P" 22.0 Alpha 1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DeltaMikeCharlie @CastagnaIT @scott967 @fuzzard @garbear