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

[pvr] basic series recording support #5208

Closed
wants to merge 3 commits into from

Conversation

Glenn-1990
Copy link
Contributor

@Jalle19, @xhaggi
I've extended the pvr api for some basic serie recording support.

This is not very well tested as no pvr client supports this yet of cource :-)
(Basic testing with faked timers and logging events sended to the pvr client)

So about the implementation:

  1. The most common serie types are supported and the pvr client can set them through a bitflag.

  2. No new dialog/window added, so every skin should stay working!

  3. Timersettings dialog is reworked so we can use that one now for creating serie recordings.
    It's a lot more flexible now, only the appropriate settings are showed to the user and the others are hided.

  4. When an epg based serie timer is opened with the 'timersetting' dialog, a few settings will be greyed out (read only) These are: strart time, end time, title, day, channel, they are set by the epg tag so it's not smart to let the user edit them (I guess). If a unknown type of timer is opened, everything will be read only.

  5. It would be nice if this dialog gets auto sized depending on the amount of items, but I've no idea how.

  6. Adding a recording through the epg guide will still add a normal 'once' recording like it was before.
    Personally I don't want to be asked for a serie recording every time :-)

  7. So, adding an serie recording has to be done through the contex menu from the 'tv guide' or 'timer search' window. The user will find an "add advanced timer" button for this.

schermafdruk van 2014-09-17 12 09 59

@Jalle19
Copy link
Member

Jalle19 commented Aug 13, 2014

Can you please split the commit into pieces? Especially the skin changes should go into one commit, it's much easier to review it that way.


CGUIDialogPVRTimerSettings::CGUIDialogPVRTimerSettings(void)
: CGUIDialogSettingsManualBase(WINDOW_DIALOG_PVR_TIMER_SETTING, "DialogPVRTimerSettings.xml"),
m_tmp_iFirstDay(0),
m_tmp_day(11),

This comment was marked as spam.

This comment was marked as spam.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Aug 13, 2014
@@ -269,20 +263,6 @@ bool CGUIWindowPVRTimers::ActionDeleteTimer(CFileItem *item)
if (!timerTag || timerTag->m_iClientIndex < 0)
return false;

/* show a confirmation dialog */

This comment was marked as spam.

This comment was marked as spam.

@xhaggi
Copy link
Member

xhaggi commented Aug 13, 2014

first of all nice work, but as @Jalle19 said please split your code changes into logical parts (commits). if you removes some confirmation dialogs, this have to go in a separate commit. I'll do the review after you split up your code changes.

EDIT and please split the API changes and the API version bump too.

msgstr ""

msgctxt "#807"
msgid "Repeating timer delete"

This comment was marked as spam.

@Jalle19
Copy link
Member

Jalle19 commented Aug 13, 2014

Yeah, nice work indeed, forgot to say that :-)

msgstr ""

msgctxt "#821"
msgid "Unsupported repeating timer"

This comment was marked as spam.

This comment was marked as spam.

@da-anda
Copy link
Member

da-anda commented Aug 13, 2014

general note on labels. We require for every new label to have a comment on where it's being used (which files) and also a contextual help for translators (as well as devs that might want to reuse a label) in which context the label is used. As an example the label "record" could be both, a verb or a subject, but without contextual help translator don't know how to correctly translate to their language.

Using your new label 803 as example, this could look like this:

#. Label of a context menu entry to create a new recording timer with advanced configuration options
#: xbmc/pvr/windows/GUIWindowPVRGuide.cpp
#: xbmc/pvr/windows/GUIWindowPVRSearch.cpp
msgctxt "#803"
msgid "Add advanced timer"
msgstr ""

@xhaggi xhaggi added PVR and removed API change labels Aug 13, 2014
msgstr ""

msgctxt "#816"
msgid "Weekdays"

This comment was marked as spam.

@Glenn-1990
Copy link
Contributor Author

@Jalle19 rebased and small confluence mod

@ryangribble
Copy link
Contributor

hey so I've actually been working a bit against this PR working on implementing support for it in our pvr.wmc addon. This mostly consisted of removing our custom series timer support and dialogs and extra backend calls etc. Testing has been fine and havent found anything not working so great job!

I had quite a few comments and discussion points but realise they are way too much to clog up this PR discussion so I have started a thread on the forums and strongly encourage all PVR developers to please come to the thread and discuss my proposals

http://forum.kodi.tv/showthread.php?tid=214469

In a nut shell my comments are around the following

  • How to approach the changes to all existing addons in order to merge this change (or moreso, the companion change on the pvr repo)
  • Recording Priority field
  • Recording Type weekday/weekend not having any/this channel modes
  • Recording Lifetime field
  • specifying defaults for all these values

The reason i make all these suggestions is that once this change is merged, all PVR addons need to be changed to even compile. and those with active development that actually want to support the kodi series recording implementation need to make more changes. So while we are all in the process of doing that, let's get it RIGHT and as user intuitive and flexible as we can etc... eg by having more sensible concepts around Priority and Lifetime, making their lives easy by being able to specify their preferred defaults and so on

Ping list: @xhaggi @Glenn-1990 @Jalle19 @janbar @opdenkamp @krustyreturns @da-anda @margro @FernetMenta @dvblogic @anssih

@@ -75,10 +75,10 @@ struct DemuxPacket;
#define PVR_STREAM_MAX_STREAMS 20

/* current PVR API version */
#define XBMC_PVR_API_VERSION "1.9.2"
#define XBMC_PVR_API_VERSION "1.9.3"

This comment was marked as spam.

@Jalle19
Copy link
Member

Jalle19 commented Jan 23, 2015

@Glenn-1990 I did a new review, some minor issues came up but nothing big. Hope you have time to look at them!

@Glenn-1990
Copy link
Contributor Author

@Jalle19 Actually I'm working on an alternative PR which is more flexible with advanced schedules.

@Jalle19
Copy link
Member

Jalle19 commented Jan 23, 2015

@Glenn-1990 okay. Did you read my latest post in the forum thread? I vote for keeping the interface unified regardless of backend (less dynamic but also more simple).

@dreamcat4
Copy link

On Fri, Jan 23, 2015 at 10:50 AM, Glenn-1990 notifications@github.com
wrote:

@Jalle19 https://github.com/Jalle19 Actually I'm working on an
alternative PR which is more flexible with advanced schedules.

FYI @Jalle19 you can see a sneak peek in Glenn's v2 branch:

https://github.com/Glenn-1990/xbmc/tree/series_rec_v2

Just please be aware that right now seems to be 1 transient changing
commit, being updated by git push --force… hmm. Reminds me of me. It is a
small crime which I am also often guilty of. :)

@ryangribble
Copy link
Contributor

I have to say i do like the look of the more flexible/dynamic approach Glenn is working on. the various PVR backends dont conform to one set of recording types etc, so allowing each backend to define the types of recordings it has, and whether they involve channel/time/day selector or not, is pretty cool

@dreamcat4
Copy link

It would be great if the interested parties could please post here and report a status update on this feature / PR. Many thanks.

@FernetMenta
Copy link
Contributor

due to changing build system for pvr addons all PVR related PRs may face a delay.

@Glenn-1990
Copy link
Contributor Author

better implemantation: #6281

@Glenn-1990 Glenn-1990 closed this Feb 28, 2015
@MartijnKaijser MartijnKaijser modified the milestones: Pending for inclusion, Abandoned, obsolete or superseeded Mar 1, 2015
@Glenn-1990 Glenn-1990 deleted the series_rec branch January 29, 2016 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Improvement non-breaking change which improves existing functionality v15 Isengard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet