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

[jsonrpc] set movie collection plots and get/set season titles #14523

Merged
merged 2 commits into from Oct 20, 2018

Conversation

Projects
None yet
4 participants
@rmrector
Copy link
Contributor

commented Oct 5, 2018

Description

Movie collection plots and season titles have been supported in the library, this adds the ability for JSON-RPC clients to set movie collection plots and get and set season titles. We can already get movie collection plot over JSON-RPC.

Motivation and Context

This request in the forum.

How Has This Been Tested?

Runtime tested.

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
@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Bump. @Montellese you implemented season titles initially, any thoughts on this?

Season title sets the sort title to match the existing implementation from GUI in #6041.

@@ -644,6 +644,8 @@ JSONRPC_STATUS CVideoLibrary::SetSeasonDetails(const std::string &method, ITrans
std::set<std::string> removedArtwork;
std::set<std::string> updatedDetails;
UpdateVideoTag(parameterObject, infos, artwork, removedArtwork, updatedDetails);
if (ParameterNotNull(parameterObject, "title"))
infos.SetSortTitle(parameterObject["title"].asString());

This comment has been minimized.

Copy link
@Razzeee

Razzeee Oct 17, 2018

Member

Should probably be SetTitle to be consistent with the code paths

This comment has been minimized.

Copy link
@rmrector

rmrector Oct 17, 2018

Author Contributor

This matches the existing implementation from #6041, should I change the way season title works in the rest of the code?

This comment has been minimized.

Copy link
@Razzeee

Razzeee Oct 18, 2018

Member

Keep it like it is, and let's move this forward this way. We will see if problems appear.
When I get some time I will look at why we're setting this and if this is an oversight.

Then again, this is about setting it from the outside and it might be very confusing for an api, if you try to set title and it get's set to sorttitle. While setting sorttitle will do nothing.

This comment has been minimized.

Copy link
@rmrector

rmrector Oct 18, 2018

Author Contributor

I'm up for changing it elsewhere, I wanted to confirm expanding the scope of the PR a bit beyond the JSON-RPC layer before I did.

sorttitle is not an option for seasons over JSON-RPC so either way the outside interface is the same, it's just some internal behavior that calls it sort title.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 19, 2018

Member

If kept like it is then it is possible to get this into v18 (additions to API acceptable under beta, will freeze API at RC) and further changes could then be in another PR for v19. It all depends how quickly this is wanted.

This comment has been minimized.

Copy link
@rmrector

rmrector Oct 19, 2018

Author Contributor

I'd say we're good as-is, then.

@Montellese

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

TBH I don't remember why I used SetSortTitle() and not SetTitle(). Using the sort title explicitly doesn't sound intuitive because the sorting happens based on the season number anyway.

@Razzeee Razzeee merged commit c83d227 into xbmc:master Oct 20, 2018

1 check passed

default You're awesome. Have a cookie
Details

@Razzeee Razzeee added this to the Leia 18.0-beta4 milestone Oct 20, 2018

@rmrector rmrector deleted the rmrector:json-setplot-seasontitle branch Oct 21, 2018

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.