[PVR] PVR Addon API 4.0.0

[ksooo]

This PR contains cleanup changes for the PVR Addon API, including the related Kodi core changes

Remove parameter bDeleteScheduled from function DeleteTimer

• Rational: Parameter is not needed, functionality can easily be implemented in Kodi core
• Core changes: Implement "on delete of a child of a repeating timer offer to delete the repeating timer including all children or just the child. Requested by @da-anda in [PVR] Series recordings #7079 and in the forum. Remove "on delete of a repeating timer offer to keep the children of that timer". Functionality was considered senseless by several users in the forum. ;-)

Introduce constant PVR_TIMER_NO_CLIENT_INDEX

• Rational: Magic numbers "everywhere" in the core and in the addons, signed/unsigned problems, bugs (-1 vs. 0 as null value)
• Core changes: No functional changes. Use the new constant instead of magic numbers. Change data types signed<->unsigned to align Kodi core and PVR Addon API data types.

Changed type of PVR_TIMER.iEpgUid to unsigned int. Introduce constant PVR_TIMER_NO_EPG_UID

• Rational: EPG event ids in PVR_RECORDING and EPG_TAG are represented as unsigned int, but in PVR_TIMER as int. Magic numbers "everywhere" in the core and in the addons, signed/unsigned problems, bugs (-1 vs. 0 as null value).
• Core changes: No functional changes. Use the new constant instead of magic numbers. Change data types signed<->unsigned to align Kodi core and PVR Addon API data types.

Please note that this PR does not solve all "magic number" problems we have with the current API and that it is not my attention to achieve this with this PR. So, please, refrain from pulling in other problems, not related to PVR_TIMER.iClientIndex and PVR_TIMER.iEpgUid, into this PR. ;-)

I will open PRs for bumping all PVR addons to version 4.0.0, including the necessary code changes, the next days.

@metaron-uk, @ryangribble, @janbar, for your information, feedback welcome.
@Jalle19 mind doing the review?

[ksooo]

Silence means agreement? ;-)

[ryangribble]

Proposed code changes are good with me... I tend to agree with @metaron-uk point about the comment

[metaron-uk]

I've a working prototype of pvr.mythtv working against your pvr-api-4-0-0 branch.
Want me to do the mythtv add-on support PR?

[ksooo]

@metaron-uk Mind taking a look at https://github.com/ksooo/pvr.mythtv/tree/pvr-api-4-0-0

[metaron-uk]

@ksooo, I know you didn't want other things in this change, but I've got frustrated that strEpisodeName isn't part of the PVR_TIMER structure. I dropped it from the 3.0.0 API changes, but now I've changed my mind. As you're updating the API again, could this be added now?

Reasons

1. The current behaviour means if you want EpisodeName for a subordinate timer in the timer list view (pretty much essential IMO), you have to squeeze it into strTitle field in the client which breaks the 'Find similar' search feature.
2. The skin has control over the subtitle presentation format for recordings and EPG. Currently the client has for the recordings list (but not the EPG info presented immediately below it)
3. Just getting the info from the EPG tag for the list view doesn't work too well either as EPG data won't necessarily be available more than 3 days into the future...

The mythtv client also tries to squeeze season/episode number into the title field as well, so I'd suggest adding these at the same time (although maybe not include them in the skin yet)

(Happy to do a commit for this and the associated confluence change for you to cherry-pick if you agree)

Will take a look at https://github.com/ksooo/pvr.mythtv/tree/pvr-api-4-0-0

[janbar]

@ksooo, good for me.
@metaron-uk, not agree, the episode name is part of epg tag or recording, so nothing to do with timer record which is already linked with epg tag. So no need to add this field.

[metaron-uk]

@janbar - then why is it currently squeezed into strTitle in the client (yes I know I did the PR for that...)

This would look pretty wierd if some of the upcoming recordings had 'subtitles' and some didn't.

Forget about season and episode. It's only the close relationship between title and episode title which makes it important to have strEpisodeName here as well imo.

[janbar]

@metaron-uk , i would agree with a new field named "strSubtitle" which is more generic and could be used by other client to show various info. But episode name is too restricted and as i said above it doesn't relate the timer itself (as a timer class).

[metaron-uk]

@janbar: The API names are a bit messed up IMHO.
strEpisodeName really ought to be strSubtitle, iSeriesNumber ought to be iSeasonNumber etc...

@ksooo - what do you think. Should we use Jarvis as an opportunity to sanitize the API variable names (not in this PR!), or do we live with the current variables names for legacy reasons....

[ksooo]

@ksooo - what do you think. Should we use Jarvis as an opportunity to sanitize the API variable names (not in this PR!), or do we live with the current variables names for legacy reasons....

Although in principal a good idea, we're running out of time for Jarvis, I'm afraid. Merge window for alpha 3 closes Sept 20 and I don't think that there will be another alpha. And in beta phase, API changes are a no go.

[ryangribble]

Ive run tested this and it functions as expected. Thanks for making this change and responding to the feedback in the forum about the previous "preserve children timers" idea not making sense :)

A comment on the latest change:
When deleting a child timer, the message displayed is very large

"You are about to delete a timer that was scheduled by a repeating timer. Do you also want to delete the repeating timer and all timers it has scheduled?

Timer Name"

This not only causes scrolling in the dialog, but the initial state of the dialog already has the text scrolled to the end (so you cant read the start of the sentence for a while).

Personally I still think this question is redundant. Due to the fantatsic hierarchical display of parent/child timers in Kodi now, I think that if a user is deleting a single timer, then just delete the one timer... there is no need to ask them if they wanted to delete the parent timer. If they want to delete the parent timer, they would just go back out one level and then select delete on the parent.

If you feel the question MUST be asked, perhaps we need more succint wording as that length of wording in a dialog breaks user experience, IMO?

PS I have a pvr.wmc PR ready to go for API 4.0, so you wont need to worry about pvr.wmc: kodi-pvr/pvr.wmc#23

[metaron-uk]

When deleting a child timer, the message displayed is very large

I agree the message is a bit long. What about making the title of the dialog 'Timer Name' and removing it from the end of the message. That way the message fits in the box.

 Personally I still think this question is redundant.

I disagree, there are plenty of places a user can delete a timer from which are not so obvious (e.g. EPG view, search results, timers list when not grouped)

[ksooo]

When deleting a child timer, the message displayed is very large

Yep. Will try to find better (shorter) wording.

What about making the title of the dialog 'Timer Name'

Not a good idea, imo as timer name can be pretty long and scrolling dialog title is really very ugly.

Personally I still think this question is redundant.

I disagree, there are plenty of places a user can delete a timer from which are not so obvious (e.g. EPG view, search results, timers list when not grouped)

Full ack to what metaron-uk said.

[ksooo]

jenkins build this please

[ksooo]

Jenkins build error on LINUX-64 is not related to this PR.

[ksooo]

All review comments are addressed, all addons are updated. Ready to go...