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

[addons] Preserve strings which are not string ids in enum lvalues #8271

Merged
merged 1 commit into from Jun 19, 2016

Conversation

@LS80
Copy link
Contributor

commented Oct 21, 2015

Sometimes with addon setting spinners some of the values may be untranslatable, e.g. a proper noun or some kind of identifier. Currently it is impossible to mix string ids to be localized and actual strings because the strings are converted to the string "Programs". This is a result of a lookup of the string id 0 as returned by atoi when it can't convert to an integer.

This PR fixes that so that the fallback is to preserve the original string.

The original thread with some more detail is at http://forum.kodi.tv/showthread.php?tid=244607 in case that's better for discussion.

@LS80 LS80 force-pushed the LS80:addon_settings_lvalues branch from 125fa38 to e341548 Oct 21, 2015
@LS80

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2015

Updated after comments for previous version LS80@125fa38

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Oct 25, 2015

@tamland mind having a look?

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 25, 2015

It's K** material at least since it's api change.

If we're adding implicit conversion for lvalues like this, it should be done for other settings types as well, not only enum/labelenum, for consistency.

@LS80

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2015

There are no api changes here. It might actually make more sense to change the api to just have values= and have it behave as lvalues= does with this change. That would make it K** material but this is just a fix, because if you try to mix string ids and literal strings in the same spinner using lvalues= you get the string "Programs" for all the literal strings. With this fix it will do nothing instead and leave the literal strings as they are.

It's a problem specific to the spinner settings labelenum and enum.

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 25, 2015

I just meant that it adds the ability to use literal in lvalues, something that hasn't been supported before, not that it breaks api backwards compatibility.

@MartijnKaijser MartijnKaijser modified the milestone: K***** 17.0-alpha1 Dec 4, 2015
@LS80 LS80 force-pushed the LS80:addon_settings_lvalues branch from e341548 to 2086cc1 May 8, 2016
@LS80

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2016

I rebased on master, hopefully this can now be merged.

@LS80

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

@tamland could you have another look?

@tamland

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

looks good. jenkins build and merge

@tamland

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

@LS80 build fails on android: http://jenkins.kodi.tv/job/Android-ARM/8330/console
you probably need to use atoi..

@tamland

This comment has been minimized.

Copy link
Member

commented Jun 18, 2016

@LS80 Friendly reminder: merge window ends in 2 days;) If it wasn't clear the atoi to std::stoi change need to be dropped.

@LS80 LS80 force-pushed the LS80:addon_settings_lvalues branch from 2086cc1 to c2bd593 Jun 19, 2016
@LS80

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2016

@tamland thanks for the heads up about the merge window, I've made the change.

@tamland

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

jenkins build this please

1 similar comment
@tamland

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

jenkins build this please

@tamland tamland merged commit 08ec69b into xbmc:master Jun 19, 2016
3 of 4 checks passed
3 of 4 checks passed
jenkins.kodi.tv Yeah yeah I'll get to it when i have some time
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default You did a great job. Have a cookie.
Details
@LS80 LS80 deleted the LS80:addon_settings_lvalues branch Jun 20, 2016
@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

I guess this change may be the reason this:

<setting id="items_perpage" type="labelenum" values="20|30|40|60|80|100|140|180|220|250" label="30039" default="40"/>

doesn't work anymore (Amazon VOD plugin - settings) in latest KODI nightlies.
It's a simple "items per page" selection with 20,30,40,... values

Since this commit the selection doesn't jump from "20" to "30", but from "20" to "November", "October", "40", "Oct", "SSW" and other strange language strings.

Any idea? (Tested on Linux and Win32 Nightly)

@ronie

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

confirmed. same issue in the picture slideshow screensaver.

we should not translate values, only lvalues.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 25, 2016

@LS80

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2016

At first glance I think the if (!lvalues.empty()) should not have been removed.

I'll check properly this evening.

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