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

Change ratings to reflect json art (and future uniqueid) structure #10020

Merged
merged 2 commits into from Jun 23, 2016

Conversation

Projects
None yet
6 participants
@phate89
Copy link
Contributor

commented Jun 21, 2016

Ratings are exactly like uniqueids and this brings the same structure uniqueids will have.
@Montellese @Tolriq Is this api change ok even if it was already added differently? I don't think anyone implemented it yet

@Tolriq

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

I have not yet implemented those but it's on Todo :)

From quick reading definition looks ok with just one caveat maybe I have not read all code.
How do you handle the default ? If the default is deleted should it move to something else ?
If you add a new rating with default true, should it set false to previous true ?

During v17 alpha it was decided to ease JSON consumers, that breaking changes are handled with a little details.

The update to version.txt should be in a separated commit with title containing [Breaking Change] see #9300 for history.

But there's no problems for breaking changes before Beta 1.

Since this is related to something in my TODO I ask my question about it here :)
How is all that managed in the GUI ? (Coupled with userrating ?)

Even if there's a rating with default true, if there's a user rating it shows the user rating ?
Or is there's a global Kodi settings somewhere that toggle between ratings and userratings ?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

A separate commit is plain stupid to do. Period

Reason: if you don't follow or read git then doing the bump for these changes in separate is completely worthless. Even following git makes it still pointless.

@Tolriq

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

Well maybe you should read the full discussion in #9300

This was decided and acted, because breaking changes should bump major version, but during alphas it was also decided to not bump version to avoid a too big number.

version.txt is the file that any API consumer have to look to know what PR made changes.

You can't ask someone to read 60 or 70 PR to know when the breaking change was made. Specially when JSON change is hidden in the middle of other changes.

Not indicating a breaking change in a way for API consumers to detect it is plain stupid. Period.

Please stop endless debating on acted things.

Either major version bump, or commit title on version.txt that was the choice and it was made.
If you have a better idea that would revolution the problem then previous discussion would have been the right timing for that.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

userratings are a separate value in a separate field.
If you delete the default one without setting a new one it will use the first one available as default (to avoid .
If you set a new default the other one is set to false.. There's only one default value
one specific only for the version change seems unnecessary.. I can add [breaking change] to the commit breaker so it's actually easier to spot the breaker code

@Tolriq

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

As long as version.txt is contained in a commit that have that tag in the tittle it's OK.

A separate commit was easier to avoid polluting the global git history to not fear everyone specially skinners.
But if the whole change only impact JSON adding [breaking change] and [json] as @Montellese loves is perfect.

About ratings, how do skin choose between userrating and ratings ?

Edit : To avoid spam. Ok thanks I'll guess I'll have add a setting on remote side so.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

I'm not sure but they can do what they want. If they think it's better to replace rating with userrating they can, if they think it's better to show them separate they have both

@phate89 phate89 force-pushed the phate89:ratings_as_art branch from ea46ba5 to 4b9fb67 Jun 21, 2016

},
"Video.Ratings.Set": {
"type": "object",
"additionalProperties": { "type": [ "null", { "$ref": "Video.Rating", "required": true } ] }

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Jun 21, 2016

Contributor

Should this be Video.Rating or Video.Ratings? As it is, the Video.Ratings type (403:406) appears to be redundant.

This comment has been minimized.

Copy link
@phate89

phate89 Jun 21, 2016

Author Contributor

If I didn't something wrong they're not redundant.
Video.Rating should be:

{ "rating": 1, "votes": 2 }

Video.Ratings should be:

{ "imdb": (Video.Rating), "tvdb": (Video.Rating) }

And Video.Ratings.Set should be:

{ "imdb": (null | Video.Rating), "tvdb": (null | Video.Rating) }

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Jun 21, 2016

Contributor

In that case, I don't see where the Video.Ratings (plural) type is used or referenced by a field so it appears (with my limited understanding of this JSON syntax) to be redundant/unnecessary as only Video.Rating is used by Video.Ratings.Set, which in turn is used only by the ratings field/property.

This comment has been minimized.

Copy link
@phate89

phate89 Jun 21, 2016

Author Contributor

That's because the "Video.Ratings" was already there before this change. I just moved and changed the definition. So all the times it was used (except 3 I changed now to Video.Ratings.Set) are still there. You just can't see it from the diff ;)
Look at the full file and you will find them
https://github.com/phate89/xbmc/blob/4b9fb67521205adf2c48a675d13db0f8e96200e5/xbmc/interfaces/json-rpc/schema/types.json

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Jun 21, 2016

Contributor

Doh, sorry for the noise!

},
"Video.Ratings": {
"type": "object",
"additionalProperties": { "type": "$ref": "Video.Rating" }

This comment has been minimized.

Copy link
@Montellese

Montellese Jun 22, 2016

Member

This looks wrong. It should be

"additionalProperties": { "$ref": "Video.Rating" }
for (const auto& i : m_ratings)
{
CVariant rating;

This comment has been minimized.

Copy link
@Montellese

Montellese Jun 22, 2016

Member

I would change this a bit to reduce having to type all the indices multiple times to something like

CVariant ratings = CVariant(CVariant::VariantTypeObject);
for (const auto& i : m_ratings)
{
  CVariant rating;
  rating["rating"] = i.second.rating;
  rating["votes"] = i.second.votes;
  rating["default"] = i.first == m_strDefaultRating;

  ratings[i.first] = rating;
}
value["ratings"] = ratings;

This way you only have to use every index once.

@Montellese

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Looks good apart from my comments. Thanks for thinking of this (based on the discussion in the other PR) and taking care of it.

@phate89 phate89 force-pushed the phate89:ratings_as_art branch from 4b9fb67 to 9373279 Jun 22, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2016

@Montellese updated according to your comments

@Montellese

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Looks good.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2016

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-alpha3 milestone Jun 22, 2016

@phate89 phate89 merged commit 2731311 into xbmc:master Jun 23, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@sualfred

This comment has been minimized.

Copy link

commented Jul 19, 2016

@phate89 @Montellese @MartijnKaijser

I'm no dev and I'm just guessing so please be gentle with me:
Artwork Downloader, and other addons which are going to update the Art value via JSON, will break/remove the cast of the updated TV show. Movies are fine.
The nightly before this commit is working, so I just guess it has something to do with this change?

See: http://forum.kodi.tv/showthread.php?tid=282596

@phate89 phate89 deleted the phate89:ratings_as_art branch Nov 30, 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.