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

Implement PVR API 5.0.0 #195

Merged
merged 8 commits into from
Feb 16, 2016
Merged

Implement PVR API 5.0.0 #195

merged 8 commits into from
Feb 16, 2016

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Feb 7, 2016

Implementation of PVR Addon API 5.0.0 as defined here xbmc/xbmc#8736

Note: SetEPGTimeFrae currently is a stub only, to be implemented later. Other stuff is fully implemented and runtime-tested for some weeks now.

@Jalle19 Your opinion?

@ksooo ksooo added the Feature label Feb 7, 2016
@ksooo ksooo changed the title IMplement PVR API 5.0.0 Implement PVR API 5.0.0 Feb 7, 2016
@ksooo
Copy link
Member Author

ksooo commented Feb 7, 2016

The build errors are expected as ci builds against old API headers.

if (prevState != newState)
{
/* Notify connection state change (callback!) */
PVR->ConnectionStateChange(GetServerString().c_str(), newState, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be done in the same if block as above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, callback must not be called with mutex locked. #195 (diff)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I missed the mutex part.

@L-S-D
Copy link

L-S-D commented Feb 7, 2016

don't forget #169

@ksooo
Copy link
Member Author

ksooo commented Feb 7, 2016

@Jalle19 by any chance, I would like to get this PR merged before all the other refactoring PRs. The code is well-tested and I will have not much time during the next weeks for rebasing and retesting all this. What do you think?

Ofc, I will address the other - non-refactoring related - points you raised before merging this PR.

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 7, 2016

@ksooo fine by me

@ksooo
Copy link
Member Author

ksooo commented Feb 7, 2016

@Jalle19 Thanks, Sam.

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 8, 2016

@ksooo I guess it's good to go once the API bump in Kodi gets merged. @L-S-D #169 is not usable yet without more changes to the API (never had time to implement them since I'm not yet sure about how exactly it should be done).

@ksooo
Copy link
Member Author

ksooo commented Feb 8, 2016

I guess it's good to go once the API bump in Kodi gets merged.

Cool. My plan is to merge the Kodi API bump PR mid to end of Feb.

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 15, 2016

@ksooo wasn't it agreed that every addon maintainer has to implement addon changes on their own instead of having one guy do a reference implementation for all?

@ksooo
Copy link
Member Author

ksooo commented Feb 15, 2016

wasn't it agreed that every addon maintainer has to implement addon changes on their own instead of having one guy do a reference implementation for all?

I think if the implementation is trivial (in the sense of not requiring real knowledge of addon internals) if somebody steps up and implements everything (as I did here) all is fine.

However, what we must avoid is providing implementations if we are not really know what we are doing - happened more than one time in the past with implementation of new functions - for instance with IsTimeshifting.

That's why I refrained from implementing API 4.2.0 for pvr.dvlink and pvr.dvbviewer. These addons support timeshifting and I have no clue how to implement IsRealTimeStream for those.

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 15, 2016

@ksooo fair point. I have nothing against "outsiders" keeping addons working, just wanted to check if I missed something in the previous discussion.

ksooo added a commit that referenced this pull request Feb 16, 2016
@ksooo ksooo merged commit 0814da4 into kodi-pvr:master Feb 16, 2016
@ksooo ksooo deleted the pvr-api-5-0-0 branch February 16, 2016 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants