[PVR] Echo up important Status info to Timer Rules #8732

metaron-uk · 2016-01-01T16:24:06Z

Number of 'Will Record' Child Timers
Presence of InProg Child Timers
Presence of Conflict_NOK or Error Child Timers

Also adds 'Completed' status as distinct from 'Enabled'

No sure how this will play with all existing pvr clients, but I can't see it breaking anything.
Small tweaks may need to be made to the reported status of 'Timer Rules' in each client to get it to work fully as intended. I'm hoping the logic is sound so it should just 'work'.

If a client already echos up 'Conflict / Error / Recording' status the behaviour should be unchanged.
Only a status of 'NEW' or 'SCHEDULED' and in the case of recording in prog, 'COMPLETED' will be replaced with the new information based on child timer states.
(I made some changes to pvr.mythtv to get 'COMPLETED' to work as shown which I will PR if this is accepted)

I'm a little uncomfortable about putting the child timer status reset in CPVRTimerInfoTag::UpdateEntry and the updates in CPVRTimers::UpdateEntries. Happy to move it all to CPVRTimers::UpdateEntries if there are strong opinions.

pings: @ksooo, @ryangribble, @janbar, @Jalle19

xbmc/pvr/timers/PVRTimers.cpp

+          case PVR_TIMER_STATE_NEW:
+          case PVR_TIMER_STATE_SCHEDULED:
+          case PVR_TIMER_STATE_CONFLICT_OK:
+            parentTimer->m_iActiveChildTimers ++;


Glenn-1990 · 2016-01-01T22:41:08Z

@metaron-uk First of all nice work.

I'm not sure about the "completed" status, in my eyes a completed timer is no timer anymore, it's a recording. You seems to depend on PVR_TIMER_STATE_COMPLETED for the completed state, but do clients use that state for repeating schedules? At least tvh will only use PVR_TIMER_STATE_SCHEDULED or PVR_TIMER_STATE_DISABLED for repeating schedules as "completed" is a bit misleading here.

EDIT:
What about "%i upcoming recordings" and "No upcoming recordings" (instead of completed)?
maybe too long though...

@ksooo Maybe the api should define which timer states are for parents and which are for childs?

addons/resource.language.en_gb/resources/strings.po

-#empty strings from id 19242 to 19243
+#: xbmc/pvr/timers/PVRTimerInfoTag.cpp
+msgctxt "#19242"
+msgid "%d will record"


metaron-uk · 2016-01-02T17:01:54Z

My idea with using the 'Completed' status is to allow the client to indicate (if it has the information) that a rule has actually carried out a recording successfully (see https://github.com/metaron-uk/xbmc/blob/sorting/xbmc/addons/include/xbmc_pvr_types.h#L180).

Mythv includes 'will next record' and 'last recorded' timestamps for each rule updated by the scheduler. When the scheduler finds a match for the rule, it updates the 'will next record' timestamp with the first match. When a recording starts, the 'last recorded' timestamp is updated.

By setting rules with a 'last recorded' timestamp but no 'will next record' timestamp to 'COMPLETED', rules for which a recording was performed, state 'Completed', rules which are about to do something say 'x will record' and those which haven't done anything yet say 'Enabled'.

This way you can easily spot rules which have already done something and aren't planning to do anything else within available guide data and remove/edit them if you want.
Rules which are still waiting for a match (e.g. because the latest starwars hasn't been shown on TV yet) show 'Enabled' until they have done something.

If your backend cleans up after itself and deletes 'Completed' rules automatically, you'll never see this status, but if not, it helps separate 'finished' rules from 'not yet started' ones.

ksooo · 2016-01-02T17:55:45Z

By setting rules with a 'last recorded' timestamp but no 'will next record' timestamp to 'COMPLETED'

Sorry, but imo using "Completed" for timer rules does not fit. As you explain it it is more "Currently nothing scheduled". "Completed" for me means that there will be never ever new timers scheduled by this rule which could only be the case if you set an end day and time for the rule.

metaron-uk · 2016-01-02T18:39:10Z

By setting rules with a 'last recorded' timestamp but no 'will next record' timestamp to 'COMPLETED'

Sorry, but imo using "Completed" for timer rules does not fit. As you explain it it is more "Currently nothing scheduled". "Completed" for me means that there will be never ever new timers scheduled by this rule which could only be the case if you set an end day and time for the rule.

What about a 'Record one' rule? pvr.mythtv has these, so assuming 'one' has been recorded successfully, the rule is 'completed' without needing an end date. The definition in the API "/*!< @brief the recording completed successfully */" seems fairly clear and seems to be compatible with displaying 'Completed' in this scenario.

I agree the logic I've described for setting 'Completed' status for a rule is less than perfect (especially for 'record all' rules), but IMO how best to decide if a rule is 'complete' is something for the client/backend.

The other states seem to be:

'NEW' "/*!< @brief the timer was just created on the backend and is not yet active. This state must not be used for timers just created on the client side. */" and
'SCHEDULED' "/*!< @brief the timer is scheduled for recording */"

New covers 'hasn't resulted in any intended recordings yet' and Scheduled covers 'is currently resulting in intended recordings'.

There isn't a 'has caused recordings before, might do so again, but isn't currently planning to do so' status. The choice would appear to be 'New' , 'Completed', or changing the API to add 'Resting'.

With the logic in this PR, if the client sets 'NEW' or 'SCHEDULED without any child timers the display will show 'Enabled'. If it sets 'COMPLETED' the display shows 'Completed' unless it has children currently recording.

I agree allowing in-prog child recordings to over-ride 'COMPLETED' status might be considered superfluous, but it does make the code in pvr.mythv for 'Completed' easier 😉. I can take this bit out if you like.

I'd be happy to raise a separate PR to add PVR_TIMER_STATE_RESTING = 10 /*!< @brief the timer rule is active, has previously recorded, but currently can't find a match in available guide data */ if you think this state is worth separating out from 'New' or 'Completed'.

metaron-uk · 2016-01-03T12:34:20Z

Rebased and squashed (the 'spare' strings were used by another merged PR).

I agree my criteria for setting 'Completed' rule status was flawed (I'm pondering more valid alternatives), but I still think 'Completed' is valid for either a rule (i.e. for completed manual / once rules) or an individual timer to report. I believe if the client reports it, displaying it would be helpful and informative.

I hope my '%d will record' explanation was good enough to justify not re-naming it '%d upcoming(s)'

@ksooo / @xhaggi / @Glenn-1990 / others - have I convinced you, or do you have use cases which won't work with these names which I can ponder?

Any opinions on adding a new 'RESTING' timer state (in a separate PR)?

ksooo · 2016-01-03T13:34:54Z

but I still think 'Completed' is valid for either a rule (i.e. for completed manual / once rules)

Agree to "record one" rule.
Do not understand what a "completed manual rule" is.

Any opinions on adding a new 'RESTING' timer state (in a separate PR)?
There isn't a 'has caused recordings before, might do so again, but isn't currently planning to do so' status.

I do not see the need for this new state. "RESTING" can easily be calculated by Kodi from existing information: state == SCHEDULED && rule_has_no_children. No need for another PVR API timer state. What you want to achieve imo is just about user interface, not PVR API. UI representation of the kodi-calculated "state" could be something like "resting", but again, no need to extend API for this.

metaron-uk · 2016-01-03T14:06:12Z

Do not understand what a "completed manual rule" is.

In mythtv all 'upcoming' timers have a parent rule on the backend, even manual or 'this showing' ones. Currently pvr.mythtv client doesn't transfer oneshot 'manual' rules to kodi core for consideration unless it has an associated 'upcoming', but they could potentially still accumulate on the backend as 'completed manual rules' (I'm not 100% sure about this as I don't seem to have any languishing on my backend, maybe it deletes them). The mythtv client could be extended to transfer these rules to kodi core for housekeeping purposes. Can't get too excited about this though.

I do not see the need for this new state. "RESTING"...

OK by me, one less API change PR :-)

Let me know if anything else occurs to you.

addons/resource.language.en_gb/resources/strings.po

@@ -9553,7 +9553,15 @@ msgctxt "#19254"
 msgid "Guide update failed for channel"
 msgstr ""

-#empty strings from id 19255 to 19256
+#: xbmc/pvr/timers/PVRTimerInfoTag.cpp


metaron-uk · 2016-01-03T18:10:53Z

Comments incorporated. Let me know if I've missed anything.

metaron-uk · 2016-01-03T18:44:34Z

OK now?

- Number of Scheduled Child Timers (including New/Conflict_OK) - Presence of Child Timers currently recording - Presence of Conflict_NOK or Error Child Timers Also adds 'Completed' status as distinct from 'Enabled'

metaron-uk · 2016-01-03T19:54:11Z

Fingers crossed! Many thanks for the help and and the constructive criticism @ksooo.

ksooo · 2016-01-03T20:54:43Z

You're welcome and thanks for this nice improvement.

ksooo · 2016-01-03T20:54:53Z

jenkins build this please

ksooo · 2016-01-03T21:48:37Z

WIN-32 jenkins error is unrelated.

ksooo · 2016-01-03T21:49:03Z

Runtime-tested. Works like expected.

[PVR] Echo up important Status info to Timer Rules

xhaggi reviewed Jan 1, 2016
View reviewed changes

xbmc/pvr/timers/PVRTimers.cpp

case PVR_TIMER_STATE_NEW:

case PVR_TIMER_STATE_SCHEDULED:

case PVR_TIMER_STATE_CONFLICT_OK:

parentTimer->m_iActiveChildTimers ++;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR v17 Krypton labels Jan 1, 2016

Jalle19 reviewed Jan 2, 2016
View reviewed changes

addons/resource.language.en_gb/resources/strings.po

#empty strings from id 19242 to 19243

#: xbmc/pvr/timers/PVRTimerInfoTag.cpp

msgctxt "#19242"

msgid "%d will record"

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

metaron-uk force-pushed the sorting branch from f0bb3b8 to 6d0f52a Compare January 3, 2016 12:18

ksooo reviewed Jan 3, 2016
View reviewed changes

addons/resource.language.en_gb/resources/strings.po

@@ -9553,7 +9553,15 @@ msgctxt "#19254"

msgid "Guide update failed for channel"

msgstr ""

#empty strings from id 19255 to 19256

#: xbmc/pvr/timers/PVRTimerInfoTag.cpp

This comment was marked as spam.

Sign in to view

metaron-uk force-pushed the sorting branch 2 times, most recently from 45d70db to 4c461ab Compare January 3, 2016 18:08

metaron-uk force-pushed the sorting branch from 4c461ab to 73edcb0 Compare January 3, 2016 18:43

[PVR] Echo up important Status info to Timer Rules

7e02761

- Number of Scheduled Child Timers (including New/Conflict_OK) - Presence of Child Timers currently recording - Presence of Conflict_NOK or Error Child Timers Also adds 'Completed' status as distinct from 'Enabled'

metaron-uk force-pushed the sorting branch from 73edcb0 to 7e02761 Compare January 3, 2016 19:51

ksooo added a commit that referenced this pull request Jan 3, 2016

Merge pull request #8732 from metaron-uk/sorting

3bdfc28

[PVR] Echo up important Status info to Timer Rules

ksooo merged commit 3bdfc28 into xbmc:master Jan 3, 2016

metaron-uk deleted the sorting branch January 3, 2016 21:50

razzeee added this to the Krypton 17.0-alpha1 milestone Jan 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PVR] Echo up important Status info to Timer Rules #8732

[PVR] Echo up important Status info to Timer Rules #8732

metaron-uk commented Jan 1, 2016

This comment was marked as spam.

This comment was marked as spam.

Glenn-1990 commented Jan 1, 2016

This comment was marked as spam.

This comment was marked as spam.

metaron-uk commented Jan 2, 2016

ksooo commented Jan 2, 2016

metaron-uk commented Jan 2, 2016

metaron-uk commented Jan 3, 2016

ksooo commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

This comment was marked as spam.

metaron-uk commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016

[PVR] Echo up important Status info to Timer Rules #8732

[PVR] Echo up important Status info to Timer Rules #8732

Conversation

metaron-uk commented Jan 1, 2016

This comment was marked as spam.

This comment was marked as spam.

Glenn-1990 commented Jan 1, 2016

This comment was marked as spam.

This comment was marked as spam.

metaron-uk commented Jan 2, 2016

ksooo commented Jan 2, 2016

metaron-uk commented Jan 2, 2016

metaron-uk commented Jan 3, 2016

ksooo commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

This comment was marked as spam.

metaron-uk commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

metaron-uk commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016

ksooo commented Jan 3, 2016