[PVR] Replace startTime=endTime=0 with bAnyTime boolean for Timers #7635
Conversation
|
@ksooo Here's my bAnyTime change for your consideration. |
|
In general I like the idea (yes, finally you convinced me). Really unclutters the code and is easy to implement on the addon-side. What we are loosing with the current approach is the possibility to distinguish between "start at any time" and "end at any time", which is a real use case, at least for tvheadend (and not supported by the current timer settings dialog). In tvheadend webui you can set up timer schedules starting at any time, ending at a concrete time and vice versa. To not to loose this use case we will need to have two bools in the new API. |
ksooo
added
API change
Type: Cleanup
|
I like the bAnyTime boolean better than inferring anytime from both times being set to 0. If other addons require 2 bools (1 for start, 1 for end) I dont have a problem with that, although in my case (pvr.wmc) the AnyTime flag is used only on Epg or keyword based recordings and indicates to record a given show AnyTime it airs as opposed to the specific time being entered, so there is no concept of any start or eny end, but I would just set both bools to true in my case. |
|
Another thing I'm uncertain about is that from previous discussions I know that for mythtv it is possible to always supply a meaningful value for start and end time that you don't want to loose when toggling "any time" because you want to switch back and forth without loosing the time values. For tvheadend it is not possible to always supply meaningful start and end times as "any time" in tvheadend is implemented as ... you already guess it, right ... 0. EDIT: Which start and end times should I set in PVR_TIMER instances in case tvheadend just gives me 0? Any ideas? EDIT2: Just passing 0 to Kodi will have bad effects after your PR got merged, as for instance in timer settings dialog you now simply init the start and end time from what you get from the timer info tag which just takes over what it gets from PVR_TIMER which results in displaying 1/1/1970 in the timer dialog. Don't get me wrong, I'm not against the new booleans, just looking for a solution that also works for tvheadend. |
|
On mythtv, the default for most rules (apart from manual ones) is AnyTime = true. It's only certain types (weekly/daily) which you can choose to record 'any time that day' / 'any time that week' or at specific times. (Mythtv actually does a search within +/- 10 mins of the specified start time for the specified title, so end time is actually ignored although it is stored). @ksooo has protyped a separate change to hide the 'Any Time' radio button if start/end clocks aren't supported by a timer type to reduce clutter. @ryangribble : does that also make sense on wmc? Two bools
Maybe: This would actually allow mythtv to remove the 'EndTime' clock to save space from daily/weekly timers, so that would be good. |
|
Yes, this makes all perfect sense for me. Would improve things nicely. |
|
@ksooo regarding which values to set when TV headend says '0' what about 'today@00:00' - That way you get a semi-meaningfull date in the list which is refresehed when the timers list is, and a default of 00:00 on the clock when turning off 'Any Time'. I'll give it a day or so for the dust to settle. If there aren't any further suggestions/objections I'll re-work this PR (and the associated API change commit) to do as suggested above. I'll start tinkering with Max Recordings in the meantime as that will (hopefully) be a more straightforward change... |
|
Yeah, go ahead. My tvheadend problem is not a blocker. I'm sure I will find a working solution. Regarding max recordings: I have another weird idea for this. Hint: define a small number of generic properties that are not interpreted by Kodi, but displayed and filled by the timer settings dialog. All what is missing conceptually in the existing API is that the add-on must be able to define a name for the property along with data values and default. |
|
Yeh - I though of that one too. While I'm sure the addon maintainers would love you, the UI consistency of the Timer Settings dialog with different frontends would disappear instantly. I've seen @opdenkamp and @xhaggi express opinions on this type of issue before so maybe they ought to comment. This might actually be of benefit though. There are already lots of options to play with, so consistency isn't likely to be wondefull anyhow. It would also finally allow 100% accurate backend terminology to be used for some of the more 'unusual' backend options which was one of my initial frustrations with the series recordings change. If you go that route, I would ask that some method of adjusting the order/list position of these 'user defined selection lists' be included. To my mind 'Recording Group' and 'Lifetime' (potential candidates for this approach) should appear above 'padding' and 'priority' settings - currently they are right at the bottom :(. Doing this now however might be a blessing. You can see how many conflicts there are when only three pvr clients are actively working on an implementation. Imagine when another 15 jump on the band waggon. It might be considerd corragious to say this on an open forum, but ultimately it depends on how much control team Kodi wants to keep over the UI and how much faith it has that addon maintaners will construct a 'good' UI with the extra flexibilty this provides. |
I think UI consistency is important. This is the reason why I want to have just a small (fixed, 3 or 5?) number of generic properties only for "special" functionality unique to one or two addons. And I would not go down the road for a complete generic UI, including resorting and such. But this just my personal opinion. I'd like the KISS approach as much as possible here. I have had my very personal experiences with implementing and maintaining a completely generic UI for a larger application. Technically it worked like charm, but user experience never was called good by all stakeholders... |
|
@ksooo what do you think about this as a crazy approach ( metaron-uk@5f85981 ) |
|
@metaron-uk Don't like that. Don't even fully understand it. Make it as you described it originally, I'd say - two booleans, PVR_TIMER_SUPPORTS_START, PVR_TIMER_SUPPORTS_END, PVR_TIMER_SUPPORTS_START_ANYTIME / PVR_TIMER_SUPPORTS_END_ANYTIME Do you see how things easily get complicated (no KISS)? Now my question: Honestly, is not loosing start and end time for the only use case changing an "any-time" timer back to a "non-any-time" timer really worth the effort? Given, that not even all backends support this? EDIT: Okay, there is one functional improvement compared to what we have now - separate control over availability of start and end time. But still not worth the effort, if you ask me. |
|
KISS - definately. I was trying to minimize the number of SUPPORTS bits... not sure why. Duh! A question occrred to me. Can you think of a way the SUPPORTS flags could be defined without having to specify an exact 0x00 value for each? It's a pain as more than one API change has to add flags and I also want to change the order. |
|
The "single boolean version" lacks support for separate start and stop any time which is possible today and actually used for tvheadend. So, this would be a step back. |
|
Here's a revised version which implements separate 'Start any time' and 'End any time' radio buttons for Start time and End time respectively. IMO it should work for mythtv as 'End Time' doesn't get used by the backend for scheduling 'weekly' or 'daily' rules so they can be removed from the timer settings dialog display. @janbar any comments? Hopefully it should work for tvheadend as it allows separate 'Anytime' control for both start and end clocks. @ksooo, do you think this will work? @ryangribble - I know you were happy with the single boolean version but does this work for you now? I did wonder if anyone might need the old behavour back (i.e. one 'anytime' switch which hides both start and end settings). This could be done I think, but the KISS principal makes me not want to try unless it is essential. The start/end anytime radio buttons can now be displayed or hidden completely independently of the start or end clocks. |
| @@ -443,7 +445,7 @@ bool CPVRClient::GetAddonProperties(void) | |||
| types_array[size].iAttributes = PVR_TIMER_TYPE_IS_MANUAL | | |||
| PVR_TIMER_TYPE_SUPPORTS_ENABLE_DISABLE | | |||
| PVR_TIMER_TYPE_SUPPORTS_CHANNELS | | |||
| PVR_TIMER_TYPE_SUPPORTS_START_END_TIME | | |||
| PVR_TIMER_TYPE_SUPPORTS_START_TIME | | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Yeah! This is exactly what I suggested. If you take care about my comemnts we're done with the kodi-side part of the story. Next would be PRs with the related changes for all addons. |
|
@metaron-uk the flag AnyTime is great. But i don't understand why there is 2 flags knowing a timer SHOULD have always a duration when time is specified. |
|
@metaron-uk there is only one "internal" case where end time is not used: to find showing in EPG i use only the start time and the channel. But the timer should always has a end time if it has a start time. |
|
@janbar as I wrote earlier in this conversation:
|
|
@ksooo nothing loosed. I have just to take care of always end time is filled when start time is filled. Is there an attribute to force that ? |
|
I think having 2 flags breaks the initial concept of @metaron-uk. The initial concept was to know if the timer is scheduled at any time even if we fill the start and end time which show the next upcoming. |
|
@janbar as @ksooo says, the second bool is for tvheadend use. I was thinking that the pvr.mythtv client can simply adjust: newEndTime = oldEndTime + (newStartTime - oldStartTime) for weekly and daily rules on save. Am I missing something? |
|
To be tested ;) ... also with manual weekly or daily which are based on time |
|
So i have tried a schedule, and end time is always required and should greater than start time: adding one second at least. In the addon for manual timer, the end time could be defaulted to start time + 2 hours if user select "end at any time". It is dirty but no other way. |
|
That's what I found as well. For manual timers, the 'anytime' fields aren't shown, so that isn't a problem. I've pushed my working code to https://github.com/metaron-uk/pvr.mythtv/tree/API3.0.0 . If you want to run it against my https://github.com/metaron-uk/xbmc/tree/anytime branch it will need to be rebased against https://github.com/metaron-uk/xbmc/tree/apiChanges first. Probably just looking at MythVersionHelper75::GetTimerTypes() will give you and idea though. NB: I've stopped updating the 75' versions since you agreed to the idea of dropping 0.26 support in Jarvis. Also, I consider this a 'working test hack' not a master version, so don't be surprised if something looks odd! |
|
@metaron-uk , weekly and daily are not manual in Kodi, but in addon we have these type of timer. |
|
sorry - I'm mixing my terminology - very confusing ! |
|
@ksooo if TVheadend need it then go ahead. I will see later how it can be managed in pvr.mythtv without returning the "not implemented" error to the user ;) |
|
@janbar okay. But I will go nowhere. ;-) @metaron-uk is the one pushing things forward. |
|
@janbar - I'm happy to submit a PR against your doityourself branch to implement this and anything else which makes it into API3.0.0 for your consideration. |
|
Since you asked for my opinion, In WMC concepts the AnyTime flag is only used on EPG based recordings. If user creates a timer for "Friends" at 8pm from the EPG, by default it will record "Friends" at 8pm on whatever days of week are input. If AnyTime is enabled then it will record Friends on Any Timeslot (eg a rerun at 2am or whatever). There is no ability to set a specific start or end time, it is either the specific airtime of the show, or anytime that show airs. In general to maintain my desired behaviour, assuming you add attributes for 4 things (start time, end time, and anytime boolean for each) I would just want to be able to enable 1 boolean and disable the other 3 fields. Then I think it would just maintain the behaviour as it is right now of having a AnyTime radio button on my Epg based timer types that can be toggled on/off, but never triggers a start or end time to be presented to the user. Hope that clarifies my usage, let me know if this is how your changes will behave. If not I can still likely work around thigns although it isnt ideal to present the user with start/end time boxes they can edit, when those actually will have no effect |
|
@ryangribble - It should be possible to make it work exactly as you suggested. I'd propose that you set PVR_TIMER_SUPPORTS_START_TIME and PVR_TIMER_SUPPORTS_START_ANYTIME for this timer type. Alternatively you could set only PVR_TIMER_SUPPORTS_START_ANYTIME and get just the Start any time radio button without the clock. |
|
@ryangribble - Re-reading your description, what would be really nice is if you could get the clock to display but be greyed out - but the API can't do that (yet). If you can think of a way to grey-out options selectively by type, I'd like to see this for mythtv as well... |
|
Well just like we are using attributes to control if a field is hidden or shown, I assume it'd be possible to grey out fields based on yet more attributes (just like how when readonly attribute is set, all fields are greyed out). Eg StartTimeIsReadOnly and EndTimeIsReadOnly. These could cause the field to be made readonly then when it is shown/hidden by radio buttons it is still in the control of the addon if it can be edited. A similar case could exist for other fields eg for me the Timer Name field would be uneditable on Epg based timers but editable on manual ones This would be ideal but is definitely a bell or whistle |
|
Sorry, but I will unsubscribe from this PR the next time someone posts another "good idea" here that is not strictly related to this PR. I have no other idea how to handle the "spam" otherwise. Same holds true for "our" other PRs. Use the forum to discuss the ideas! Keep the PR conversation clean, please. |
Allow startTime/endTime to be retained if AnyTime is turned on and back off again to see how this affects the shows a timer will record. Also splits 'Any Time' so start/end can be controlled individually
Allow Any Time to be toggled on/off without affecting start/end clocks Replace startTime=endTime=0 with booleans bStartAnyTime and bEndAnyTime Remove functions and variables required for the old method Use startTime not FirstDay in strSummary as startTime now always valid Add short strSummary format if end is anytime
|
Rebased and squashed including feedback from @ksooo, code & comment tidy-ups and strSummary revision to make the most of the new interface. |
|
Looking good now. Thanks a bunch. |
Allows startTime/endTime to be retained if AnyTime is turned on and back off again to see how this affects the shows a timer will record.
Also: Retains meaningfull 'startTime' based 'sort by date' order for timers with 'Any Time' selected.
PVR Client Changes Requried
In places where an addon currently sets 'startTime=endTime=0' to indicate Any Time, this should be removed and replaced by setting bAnyTime = true.
In places where 'startTime=endTime=0' is used to control backend recording rule settings, this should be replaced with something based on the bAnyTime state.
Context
This PR relies on the changes in PR #7626 which groups several API changes forming API 2.2.0.
Comments
Please make comments regarding the bAnyTime change itself here.
For comments regarding any of the other 2.2.0 API changes, please use #7626