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

Add rating and votes label for specific rating #8788

Merged
merged 2 commits into from Jan 10, 2016

Conversation

phate89
Copy link
Contributor

@phate89 phate89 commented Jan 6, 2016

Now skins can use directly a label with both votes and ratings.
do someone know if there is a number formatting method in kodi to transform for example
10000
in
10.000?

@razzeee razzeee added Type: Feature non-breaking change which adds functionality v17 Krypton Component: Video labels Jan 7, 2016
@razzeee
Copy link
Member

razzeee commented Jan 7, 2016

I would guess it's best to keep it in line with the other ratings and votes labels and not add 10.000 if the others don't behave that way.

@phate89
Copy link
Contributor Author

phate89 commented Jan 7, 2016

The problem is that in Jarvis (at least in my setup) votes was a string with already built in punctuation (100,000). With my pr andrating as integer I changed how votes are displayed (and IMHO it looks weird too)

@zag2me
Copy link
Contributor

zag2me commented Jan 8, 2016

I always prefer to show big numbers with the comma. I do that with all vote counts on the metadata sites so I think Kodi should do the same.

if (rating.votes == 0)
return StringUtils::Format("%.1f", rating.rating);
else
return StringUtils::Format("%.1f (%i %s)", rating.rating, rating.votes, g_localizeStrings.Get(20350).c_str());

This comment was marked as spam.

This comment was marked as spam.

@tamland
Copy link
Member

tamland commented Jan 8, 2016

Number formatting is language specific and must be handled in such way. Possibly with std::ios_base::imbue?

@phate89
Copy link
Contributor Author

phate89 commented Jan 8, 2016

@tamland
I'm looking into using a stringstream and imbue but apparently kodi sets a global "*" locale that I can't override so it doesn't work (at least in my win 10). Someone knows better this part of code?

@tamland
Copy link
Member

tamland commented Jan 8, 2016

See SetGlobalLocale in LangInfo.cpp, but for some reason it only sets collate because it "breaks atof() others similar functions" (?)

@phate89
Copy link
Contributor Author

phate89 commented Jan 9, 2016

After investigating better I discovered that it's possible to override global locale. It only doesn't work in windows. The problem is here:
https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L636
before that line everything works well. After it doesn't.
I don't understand why that code also because in windows we use std::locale::global or wsetlocale and not setlocale..
Who knows better? @Karlson2k @Paxxi ?

@phate89
Copy link
Contributor Author

phate89 commented Jan 10, 2016

I guess this could go in since the existing label is already without formatting style. The formatting could be posted in a new pr (once I figure it out how :| )

@@ -11430,7 +11430,7 @@ msgid "No video files found in this path!"
msgstr ""

msgctxt "#20350"
msgid "votes"
msgid "%.1f (%i votes)"

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@phate89
Copy link
Contributor Author

phate89 commented Jan 10, 2016

Updated with explanation string

@razzeee razzeee added this to the Krypton 17.0-alpha1 milestone Jan 10, 2016
@razzeee
Copy link
Member

razzeee commented Jan 10, 2016

jenkins build and merge

@jenkins4kodi jenkins4kodi merged commit 03c80c5 into xbmc:master Jan 10, 2016
@phate89 phate89 deleted the ratingandvotes_label branch January 11, 2016 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Feature non-breaking change which adds functionality v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants