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

[PVR] Series recordings #7079

Merged
merged 4 commits into from Jul 3, 2015

Conversation

Projects
None yet
@ksooo
Member

ksooo commented May 7, 2015

This PR adds series recording support to Kodi.

The technical concept:

Basic concept is that PVR addons now define an arbitrary number of timer types they support, each type defining its own combination of timer type attributes (out of a set of attributes predefined by the PVR addon API).

Kodi PVR core picks up the different types (and their attributes) using a new PVR addon API function and strictly builds up the complete timer-related logic dynamically, according to the timer attributes. Timer type information is now available at every TimerInfoTag instance.

Essential timer attributes: Kodi distinguishes between manual (time-based) and epg-based timers. Also, there can be one-shot and repeating timers. All combinations of these attributes are allowed, e.g. "manual + one-shot" or "epg + repeating".

Examples for other timer attributes: "supports recording priority", "supports epg search string", "supports recording folders", ...

UI changes:

Timer window:

  • A "Type" column has been added
  • "Scheduled time" column now handles all combinations of weekdays, and start/stop time (incl. "any time" for timer schedules) correctly.

screenshot001

  • The timers scheduled by timer schedules (aka repeating timers) can be displayed as "children" of the timer schedule, similar to the "group items" feature of the recording window

screenshot004

  • This behaviour can be controlled using a new (level 4) setting available in the Confluence side blade, similar to the group items feature of the recordings window.

screenshot010

screenshot011

Timer settings dialog:

  • Completely rewritten from scratch, now acts completely dynamically upon the available timer types and their attributes
  • Main idea is to start creation of a new timer by selecting the appropriate timer type. Dialog content will automatically adjust itself according to the respective timer type attributes. As before, user fills in the relevant data on kicks of the new timer
  • Dialog can (like before) also used for editing existing timers
  • Couple of new/enhanced features, among them support for epg-based repeating timers, support for all weekday combinations, pre- and post-record time, ...
  • Finally, lots of bug fixes...

screenshot006

screenshot007

screenshot008

  • For weekday selection, a new dialog was implemented.

screenshot009

Context menu actions:

  • All relevant context menu items are now displayed only if the corresponding functionality is available according to the timer type attributes. Example: Activate/deactivate timer
  • A (from my point of view very cool) new menu item "Add advanced timer" is now available if an epg entry was selected, for example in the epg grid. This opens a timer settings dialog preset to create a an epg-based series recording based on the data of the selected epg entry.

screenshot012

screenshot013

Note: All this is implemented and tested against a real PVR addon (pvr.hts), not "just" pvr.demo. I consider pvr.hts as the reference implementation of this (larger) PVR addon API change. Code is currently here (https://github.com/ksooo/pvr.hts/tree/series-recording-support), PR will follow soon.

Feedback is much appreciated!

@Montellese

This comment has been minimized.

Member

Montellese commented May 7, 2015

I can't comment on the PVR stuff since I don't use it.
Why did you implement a new dialog for the weekdays selection? Why not simply use CGUIDialogSelect with multi-selection enabled?

@ksooo

This comment has been minimized.

Member

ksooo commented May 7, 2015

@Montellese As far as I can see, CGUIDialogSelect always works on a FileItemList. Weekdays are just an int. I don't see that CGUIDialogSelect fits here. Or is it a good approach to "wrap" the single weekdays to file items?

#. Label of pre-record spinner
#: xbmc/pvr/dialogs/GUIDialogPVRTimerSettings.cpp
msgctxt "#813"
msgid "Pre-record (minutes):"

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

please use "Start padding time" and "End padding time" so it's identical to what's shown in PVR settings (labels 19175 and 19176). IMO existing labels could be reused, as the colones should be added by skins.

This comment has been minimized.

@ksooo

ksooo May 8, 2015

Member

19175 and 19176 are both missing " (minutes)" at the end.
Will change the text of 813 and 814 to "Start padding time (minutes)" and "End padding time (minutes)"

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

can't you handle the timer setting the same way it's done in PVR->Recording settings and show "min" along with the values in the spinner?
Start padding time -> X min [ /\ | / ]

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

This is a really good idea, also for some other values in the new settings dialog. Will see what I can do.

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

@da-anda That was easy:

screenshot015

Regarding the now no longer present colons: From my point of view it looks "ugly" without the colons. There is no separation between label and value anymore. I think, until skin handles this better (which is out of scope of this PR, IMO), we should stick with the colons. What is your opinion on that?

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Not true. The current timer dialog does. That's why I did it the same way with the reimplemented timers settings dialg. But ofc I'm fine with fixing this inconsistency with the new timer settings dialog. ;-)

This comment has been minimized.

@da-anda

da-anda May 9, 2015

Member

by seeing this screenshot - can you change the spinner of the channel setting to a select list? Probably even with showing the channel logo? It'll be a real PITA to select a channel when having a lot of them. I suppose we can't browse by channel groups in this list (would probably require a new settings type)?

This comment has been minimized.

@phil65

phil65 May 9, 2015

Member

+1, would be great to use DialogSelect instead of spinners whenever possible.
Apart from that, nice work. :)

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Making it a list was easy. Adding icons currently goes (far) beyond my capabilities... AFAIKS, currently there is no appropriate guilib functionality I could use. @xhaggi?

This comment has been minimized.

@da-anda

da-anda May 9, 2015

Member

Channel icons are only a nice to have here, list vs spinner is already a big improvement IMO

#. Label of new episodes radio button (if user does not want to record the same episode multiple times)
#: xbmc/pvr/dialogs/GUIDialogPVRTimerSettings.cpp
msgctxt "#812"
msgid "Record only new episodes:"

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

wouldn't "Only new episodes" be enough? The label is shown in a timer dialog, so it should be obvious that it's related to the recording :) Reason why I ask is that I'd like to get the labels as short as possible in order to not get scrolling labels in translations

This comment has been minimized.

@ksooo

ksooo May 8, 2015

Member

Okay. Changed.

#. Label of spinner control for choosing the timer type (one time, repeating, ...)
#: xbmc/pvr/dialogs/GUIDialogPVRTimerSettings.cpp
msgctxt "#803"
msgid "Timer type:"

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

could label 146 "Type:" be reused? Mind checking where it's used to see if context matches?

This comment has been minimized.

@ksooo

ksooo May 8, 2015

Member

146 has a trailing colon. I just removed all the colons from the new timer settings dialog labels.

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

I sort of doubt this label is still used, but it ofc would have to be researched. Also, ideally we'd get rid of the colones all over the place, but that's a bigger change.

#. Label of a context menu entry to activate a currently deactivated timer
#: xbmc/pvr/windows/GUIWindowPVRTimers.cpp
msgctxt "#843"
msgid "Activate timer"

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

what's the difference between "activating" and "enabling" a timer? I suppose it's the same? IMO we should stick to one naming scheme (enable/disable like it's used all over Kodi)

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Changed.

@@ -8906,7 +9134,12 @@ msgctxt "#19294"
msgid "Delete this recording permanently?"
msgstr ""
#empty strings from id 19295 to 19498
#: system/settings/settings.xml

This comment has been minimized.

@da-anda

da-anda May 8, 2015

Member

Mind adding a contextual note? I suppose it's the description for the timer setting?

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Done.

@Tolriq

This comment has been minimized.

Contributor

Tolriq commented May 8, 2015

@Montellese already proposed a few time, but my offer to provide you needed equipment for PVR is still valid.

PVR is becoming better and better and more and more used and will be in the future and the API is not really following :(

@Jalle19

This comment has been minimized.

Member

Jalle19 commented May 8, 2015

Now my eyes hurt...

@ksooo

This comment has been minimized.

Member

ksooo commented May 8, 2015

@Jalle19 hehe. Need some glasses? Many thanks for the review. Nothing serious found so far, which makes me feel good...

Despite from the code comments, any feedback on the functionality?

@da-anda

This comment has been minimized.

Member

da-anda commented May 8, 2015

I'm not sure if it wouldn't be better to split up the type so that "guide based" would have it's own flag.

CDateTime now(CDateTime::GetCurrentDateTime());
CDateTime startLocalTime = m_bAnytime ? now : m_timerInfoTag->StartAsLocalTime();
CDateTime endLocalTime = m_bAnytime ? now : m_timerInfoTag->EndAsLocalTime();

This comment has been minimized.

@Glenn-1990

Glenn-1990 May 8, 2015

Contributor

Current time should only be used for manual timers.
Guide based timers should get their start and end time from the epg tag, even if any time is selected, users can unselect it and then the time would be wrong.

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Good catch. Fixed.

@@ -40,43 +40,53 @@ using namespace EPG;
CPVRTimerInfoTag::CPVRTimerInfoTag(bool bRadio /* = false */) :
m_strTitle(g_localizeStrings.Get(19056)), // New Timer
m_strDirectory("/")
m_bFullTextEpgSearch(true)

This comment has been minimized.

@Glenn-1990

Glenn-1990 May 8, 2015

Contributor

Why true as default? Probably most users will only record episodes based on title.
Maybe a setting for the default value?

This comment has been minimized.

@Jalle19

Jalle19 May 8, 2015

Member

I doubt a setting is needed, people got along fine all the years that tvheadend didn't support full text matching so I believe it's safe to just default to false.

This comment has been minimized.

@ksooo

ksooo May 8, 2015

Member

Okay. Default changed to false.

@ksooo

This comment has been minimized.

Member

ksooo commented May 8, 2015

;-) Feel free to send me a patch to add the setting. I know you have the skills for that.

@Glenn-1990

This comment has been minimized.

Contributor

Glenn-1990 commented May 8, 2015

Will see when I can find some time.

Nice work btw ;-)

@Jalle19

This comment has been minimized.

Member

Jalle19 commented May 9, 2015

Thanks for taking care of the comments so far!

@da-anda

This comment has been minimized.

Member

da-anda commented May 9, 2015

@Montellese would it be possible to put the timer settings in logical groups with the changes by alwinus that allow to define a group label? It might be a nice idea to clean up this dialog even more by having logical groups. Like one section for type, channel etc, the other for advanced schedule options, and one for secondary settings like "priority", "lifetime", ...

@ksooo I always wondered who is responsible for the available "lifetime" values, Kodi or backends? I also think this shouldn't be a spinner with the range from 1-356 days but rather a predefined list of values that make most sense + a "forever" and a "until disk space is required" option, like:
1 day, 3 days, 1 week, 2 weeks, 1 month, 2 months, 3 months, 5 months, 1 year, until diskspace is required, forever

edit: thanks for addressing all the bikeshed so far 👍

@ksooo

This comment has been minimized.

Member

ksooo commented May 9, 2015

@da-anda may I kindly ask to hold yer horses a little, please. I very appreciate all your ideas, but I get the feeling that there is so many room for small improvements everywhere that I will still work on this PR when we are in Beta phase for Kodi 20. ;-) I try my best, but having limited time. I opt for getting this out very soon and to improve over time. The change requests are at a very high level already which makes me think the important things are already in good shape.

Regarding lifetime: Can only speak for tvheadend. There, it is the backend which can handle every lifetime from 0 to ... days. "forever" and "until disk space is required" is not supported. At least with those last two values things will get complicated. How to determine which (kind of values) the backend supports? From the API point of view ... a mess. We have carefully to weight value against effort.

@da-anda

This comment has been minimized.

Member

da-anda commented May 9, 2015

@ksooo - sorry for getting crazy about features/changes. The things mentioned in my previous post are ofc only nice to haves and can also be addressed later. Don't want to bikeshed this PR to death, only got inspired by the improvements done so far.

Regarding lifetime API, well, not that complicated I think. Ask the PVR add-on for supported lifetime types and get a vector (or bitmask) back (PVR_TIMER_LIFETIME_DAYS, PVR_TIMER_LIFETIME_INFINITE, PVR_TIMER_LIFETIME_DISKUSAGE). The other option would be to register a settings value filler and within the filler optain the possible values from the PVR add-on (have the add-on fill the list). We could even support both ways, where with support for PVR_TIMER_LIFETIME_DAYS we use a predefined list like in my suggestion above and if backend wants to have custom settings have it fill the options on it's own. As said, not required for this PR in general, only a nice to have and can also be addressed later.

@ksooo

This comment has been minimized.

Member

ksooo commented May 9, 2015

@da-anda re lifetime. Your implementation suggestion is exactly what I'm saying. It bloats the PVR addon API (remember, every addon must implement it) for IMO not (too) much value gain. IMO, this goes far beyond the scope of this PR. Especially, as this "addons should be able to specify the values they support for their supported attributes" can be applied to almost(?) all(?) timer type attributes...

@da-anda

This comment has been minimized.

Member

da-anda commented May 9, 2015

@ksoo - I actually thought that when you mentioned the rewrite of the other PR that you're going the way that add-ons "simply" forward a timer settings structure and we're only the rendering GUI ;)

I don't see this as bloat of the API, but the way our API is handled is unfortunately causing additional bloat. So the real issue is the lack of Kodi not being able to talk to older API versions, which i know will make things even more complicated core wise, but this logic could probably be abstracted and reused for all (binary) interfaces. No more stub functions and PVR add-on breakage or update nightmare because of a minor API bump. But I can't judge the amount of work and complexity required for this, especially with binary stuff.
One way that I could think of this might work would be talking to add-ons via API connectors that either simply forward API calls 1:1 or translate older API versions to newer ones (+ having required stubs), while we support only the last X API versions via such connectors. But that's way off topic.

I really apreciate what you've done here and look forward to see this PR in action.

@ksooo

This comment has been minimized.

Member

ksooo commented May 9, 2015

I actually thought that when you mentioned the rewrite of the other PR that you're going the way that add-ons "simply" forward a timer settings structure and we're only the rendering GUI ;)

Yes, that's exactly the way I want to go. And this PR is the first step into this direction. 100% is still somehow far away, but with this PR we will have the basics. Looking at it with 80/20 rule in mind we're at 80 now, imo.

@ksooo

This comment has been minimized.

Member

ksooo commented May 9, 2015

All feedback from yesterday and today is now in.

  • strings
    • trailing colons removed from timer settings dialog setting labels
    • capitalization bugs fixed
    • wording, mostly consistency
  • windows
    • minor code changes and fixes
  • core
    • minor code changes and fixes
  • timer settings dialog
    • channels now as a list (vs. spinnner)
    • values with proper measurement units (vs. unit part of setting label)
    • minor code changes and fixes

Thanks to all reviewers so far.

Would really like to get some feedback re. PVR addon API changes. @opdenkamp? @FernetMenta? @xhaggi? all?

#. Representation of PVR timer state "disabled"
#: xbmc/pvr/timers/PVRTimerInfoTag.cpp
msgctxt "#825"
msgid "Disabled"

This comment has been minimized.

@da-anda

da-anda May 9, 2015

Member

me again, I really don't want to bikeshed, but even translators already complained about all the duplicates we're having, and there are already quite some "disabled" labels available. Mind checking if they can be reused? Like 1223, 13106, 13113, 13131, 24023. If the context they're used in doesn't match, keep yours, but I think at least the label used in settings.xml could match.

All this duplicates need to be cleaned up at some point, so the more duplicates we can avoid now the better.

edit: I'm soooo sorry, but after thinking more about it I came to the conclusion that a better wording for the timer states might be "active" and "inactive". What's more common @jjd-uk ?

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

active / inactive is more common. Changed.

msgstr ""
msgctxt "#19079"
msgid "Day:"
msgid "Weekdays"

This comment has been minimized.

@da-anda

da-anda May 9, 2015

Member

can't this label be reused and make 830 obsolete?

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Yes.

#: xbmc/pvr/windows/GUIWindowPVRGuide.cpp
#: xbmc/pvr/windows/GUIWindowPVRSearch.cpp
msgctxt "#841"
msgid "Add advanced timer"

This comment has been minimized.

@da-anda

da-anda May 9, 2015

Member

If I understood your description correctly this context item is automatically opening a timer dialog for a repeating timer, but the type can still be changed. Was just thinking if we should only call it "Add custom timer" which better indicates that the timer can be adjusted? Just an idea, whatever you prefer.

This comment has been minimized.

@ksooo

ksooo May 9, 2015

Member

Changed.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

@ksooo this seem to have broken instant recordings for backends not supporting those timer types:
http://forum.kodi.tv/showthread.php?tid=234605&pid=2073817#pid2073817

EDIT: not sure about this message, why does it trigger a warning?

WARNING: GetAddonProperties - Addon VDR-Network-Streaming-Interface (VNSI) Server:192.168.100.16:34890 does not support timer types. It will work, but not benefit from the timer features introduced with PVR Addon API 2.0.0
@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

@FernetMenta could you please check what happens in cVNSIData::AddTimer in this case? Cannot do myself as I do not have a working VDR setup.

@metaron-uk

This comment has been minimized.

Contributor

metaron-uk commented Aug 8, 2015

@ksooo , @FernetMenta - this doesn't appear to affect pvr.mythtv with only the 1.9.7/2.0.0, 2.0.1 and 3.0.0 'minimal support' patches applied.
Not saying it wasn't introduced by these changes, but it makes me think that the issue is probably VDR specific, rather than affecting all clients.

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

Can confirm that there are no issues with tvheadend.

@ryangribble

This comment has been minimized.

Contributor

ryangribble commented Aug 8, 2015

I can confirm that pvr.wmc Instant Record is working with current kodi master and pvr.wmc master (ie the 1.9.7 minimal support patches)

@ryangribble

This comment has been minimized.

Contributor

ryangribble commented Aug 8, 2015

also just to clarify, the users in that forum thread are running Kodi 15.1 but this change on this PR was only merged into Jarvis (Kodi 16 alpha) unless im missing something?

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

This is PR is Jarvis only. So, if the forum thread is about 15.1 there must be some other reason for the problem.

@ryangribble

This comment has been minimized.

Contributor

ryangribble commented Aug 8, 2015

looks like the OP on the forum thread was running Kodi 16, the 2nd guy in the thread with the "same problem" is on 15.1 which confused me

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

After reading the forum thread again, it actually is about Jarvis. Nevertheless, I think that this is most probably a problem with pvr.vdr.vnsi, not with Kodi core.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented on xbmc/pvr/timers/PVRTimerInfoTag.cpp in 36ad006 Aug 8, 2015

@ksooo this breaks instant recordings. instant recordings were used to be PVR_TIMER_STATE_SCHEDULED

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

@ksooo see my comment, this change broke it because of incorrect timer state for instant recordings

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

@FernetMenta okay, it's a change and obviously it broke vdr.vnsi. I was not aware of that this could break anything. Sorry.

But isn't state SCHEDULED wrong, according to the API docs (https://github.com/xbmc/xbmc/blob/Isengard/xbmc/addons/include/xbmc_pvr_types.h#L109)? SCHEDULED indicates that "timer is scheduled for recording" and as long as the backend has not confirmed that it successfully added a newly kodi-side created timer (AddTimer succeeded), the timer state should be NEW ("a new, unsaved timer"), imo.

So, I suggest to fix vdr.vnsi. What do you think?

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

vdr allows to create a timer which is not active. So what do you suggest?

  • Kodi creates an instant timer in new state
  • vdr acks that it has saved the timer
  • vnsi (backend) sends a message back to kodi
  • vnsi client sets the timer to scheduled
  • vnsi sends a message to vnsiserver to active the timer

This ping pong makes not much sense to me.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

Also note that PVR_TIMER_STATE_NEW was used to be never set from kodi. Your change may have broken more behaviour.

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

So the question is whether we consider the API spec broken and fix it or whether we consider some API implemenations broken because they violate the spec?

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

This ping pong makes not much sense to me.

  • Kodi creates an instant timer in new state
  • vdr acks that it has saved the timer

Why can't you just instruct vdr to create an active timer in this case (assuming that this is what you currently do if incoming state is SCHEDULED)?

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

why do you want to change the api? note that vdr/vnsi was the first pvr addon and many things of the API has beed derived from this.
we need to distinguish between a timer directly created on the backed (inactive, this is state new) and a timer which is supposed to start a recording.
implicitly assuming that a time state "new" from pvr means "scheduled" is wrong.
I don't see the need and the benefit why you changed this.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

So the question is whether we consider the API spec broken and fix it or whether we consider some API implemenations broken because they violate the spec?

how can the first addon with that the spec was created violate the spec?

@ksooo

This comment has been minimized.

Member

ksooo commented Aug 8, 2015

we need to distinguish between a timer directly created on the backed (inactive, this is state new) and a timer which is supposed to start a recording.

I see. But then the API docs for state NEW should definitely be revised. I read it as "just created on the Kodi side, not yet confirmed by the backend".

I will create a PR fixing both the misleading API docs and reverting the "misuse" (;-) of timer state new.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

thanks much!
btw: priority and lifetime come from vdr too.

@metaron-uk

This comment has been minimized.

Contributor

metaron-uk commented Aug 8, 2015

btw: priority and lifetime come from vdr too.

@FernetMenta anything which would affect this metaron-uk@a131127 in #7626 ?

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Aug 8, 2015

yes, lifetime goes untouched through vnsi and the definition is:

The number of days (0..99) a recording made through this timer is guaranteed to remain on disk before it is automatically removed to free up space for a new recording. Note that setting this parameter to very high values for all recordings may soon fill up the entire disk and cause new recordings to fail due to low disk space. The special value 99 means that this recording will live forever, and a value of 0 means that this recording can be deleted any time if a recording with a higher priority needs disk space.
@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Dec 25, 2015

@ksooo do you have plans to support vps timers? would this be a special type or just some flag?

@ksooo

This comment has been minimized.

Member

ksooo commented Dec 25, 2015

I think a special timer type would be the easiest way to implement this. Only difference to "normal" timers would be that start and end time are not fixed values, but approximated, right? Naming the timer respectively would make things easily understandable for the users.

Nothing additionally needed in Kodi core for this. All that is necessary can be done in the PVR add-ons - given that the vps functionality is something provided and controlled by the backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment