[PVR] support for fetching timers and recordings via json-rpc #4198

Closed
wants to merge 4 commits into
from

Projects

None yet

8 participants

@opdenkamp
Member

requested in http://forum.xbmc.org/showthread.php?tid=186072

ping @t-nelson @montellese @joethefox

just compile tested, i'll leave the actual testing up to you ;-)

@opdenkamp opdenkamp added the PVR label Feb 13, 2014
@opdenkamp
Member

jenkins build this please

@t-nelson
Member

loop checks for

!item && it != m_recordings.end()

Ah, missed it in the mess of STLisms. :)

We await word from the Kiwi.

@t-nelson

Am I sensing a bit of C&P? :)

Member

ofc :)

@t-nelson
Contributor

Seems reasonable. @jmarshallnz are you opposed to consideration for Gotham? With @Montellese's approval, ofc.

@da-anda

given you initialized m_iLastId as 0 you'd have a recording with ID 0 as well. Not sure if 0 is a good value for an ID, so I'd rather change it to ++m_iLastId. Also using m_iLastId++ would end in m_iLastId to not reflect the actual last used ID but last ID +1 unless C++ is handling this stuff differently than any other programming language I know.

Member

I consider '0' as valid identifier really, but i don't mind starting with 1 instead. i'll have a look at it in the evening

Member

as long as we don't store the value in the DB 0 is ofc valid, but who knows, maybe we start caching those identifiers in the local db for G+1 to speed up startup. I just like to be prepared for the future. But aside from 0 or not, m_iLastId doesn't currently hold the last used ID but the next free ID, and at least that should be changed.

Member

sure sure ;-)

@da-anda

same as with my comment in the other commit, you should probably change it to ++m_iLastId (also in line 290 further down)

@joethefox
Member

tested. compiles fine but at the moment it returns "method not found". Seems that also xbmc/interfaces/json-rpc/ServiceDescription.h , xbmc/interfaces/json-rpc/methods.json and xbmc/interfaces/json-rpc/types.json need to be updated according to the new methods/types.

ServiceDescription.h and addons/xbmc.json/addon.xml need a version update, could be 6.13.6.

@da-anda
Member
da-anda commented Feb 13, 2014

@joethefox correct, those changes are missing

@opdenkamp
Member

right, looks like it indeed. i don't know the json-rpc code really, i just added what the other methods were doing :) i'll add it to this PR and change @da-anda's cosmetics.

@Montellese Montellese commented on the diff Feb 16, 2014
xbmc/FileItem.cpp
@@ -227,6 +227,8 @@
m_strLabel2 = record.m_strPlot;
FillInMimeType(false);
+
+ SetProperty("recordingid", record.m_iRecordingId);
@Montellese
Montellese Feb 16, 2014 Member

While this (and the same logic for the constructor taking a CPVRTimerInfoTag) should work with JSON-RPC I'd prefer the way this works for video, music, epg etc i.e. CPVRRecording and CPVRTimerInfoTag implement the ISerializable interface and expose all their properties through that method which is then called by JSON-RPC to serialize the retrieved values. It prevents unnecessary cluttering of the properties map in CFileItem.

@Montellese
Member

As mentioned this requires the definition of the methods in the JSON schema API description. I can do that (and the changes to CPVRRecording and CPVRTimerInfoTag if you want me to.

I can't test the results however since I don't have a PVR backend/setup.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 17, 2014
@Tolriq
Contributor
Tolriq commented Feb 17, 2014

@Montellese You can just add demo pvr addons, it does works well with any platform and integrate well with JSON without the need for real PVR things.

If last time changes are allowed, would I'd be allowed to propose a PR to add some filtering to PVR.GetBroadcasts ? To allow returning only current or future items for faster answer.
And if possible a query for full data (IE without the filter on channel), since current API is quite slow and limited for efficient use :( Doing 300+ queries to display now playing on channels is bad :(

@opdenkamp
Member

I'll have a look at it tonight, but I'm also fine with you doing the changes, since I don't really know JSON-RPC and was just being a c+p monkey, so we don't end up with only a half working implementation.

@topfs2
Member
topfs2 commented Feb 17, 2014

@Tolriq This discussion is a bit off topic but anyways, It would be up to the release managers. Since the feature exist and you would mostly provide performance increase they might be reluctant. However they might be swaded (just don't overpress your point, if they say no its no). That being said, the feature sounds valid and if the implementation is fine I'd be surprised if we couldn't include it for G+1

@Tolriq
Contributor
Tolriq commented Feb 17, 2014

This is a PR for new pvr things with an ask for include in Gotham ;) So I try to ask for another one to have json pvr really usable in Gotham ;)

My point will not change on the fact I will only PR after validation and not before :)

@topfs2
Member
topfs2 commented Feb 17, 2014

Then you can disregard my comment. You will never get validation prior to
submitting code. Especially not this late.
Den 17 feb 2014 13:34 skrev "Tolriq" notifications@github.com:

This is a PR for new pvr things with an ask for include in Gotham ;) So I
try to ask for another one to have json pvr really usable in Gotham ;)

My point will not change on the fact I will only PR after validation and
not before :)

Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/4198#issuecomment-35252890
.

@Tolriq
Contributor
Tolriq commented Feb 17, 2014

Yes I'm sadly aware that validating a concept or the need for a feature before coding is so horrible to the Team :(
(Since I obviously do not ask for discussion on code content before submitting code ...)

Perhaps one day this will change and we can see more devs helping on Xbmc code :)
Not everyone can afford to loose time on something that whatever how it's coded will be refused after :)

Anyway if one day this change you know where to find me.

@da-anda
Member
da-anda commented Feb 17, 2014

@Tolriq we can give an answer if your suggestion in general is feasable for XBMC or not, but this won't be a guarantee that it'll be merged. It's still possible that devs don't like the way you implemented it and thus demand for changes or will deny it in favor of an entirely different approach your PR gave the inspiration for.
Also we can't tell you if it would make it into Gotham without a PR, because this depends on the code impact it has and it's possible side-effects. Highly self-contained code properly splitted into separate commits would increase the chances though.

To me your suggestions in general sound sane, but I'm not a dev, nor did I have a look at current implementation. Also fetching all PVR data in one go might not be the best solution - depending on the load it will cause (think about low power devices like the RasPi). So lazy loading of the data on your end in configurable junk sizes (like 10/50/100/... items in one go) might be preferred.
If you like to discuss further I suggest to move the discussion to the forum to not spam every dev.

@Tolriq
Contributor
Tolriq commented Feb 17, 2014

@da-anda I do understand your remarks and fight for that :)

But perhaps you missed http://forum.xbmc.org/showthread.php?tid=166398 but I'd like to continue to discuss this furthermore if some of the team does jump in this simple discussion I try to open since lots of months :)

As a side last remark about lazy loading, this is the goal of limits in JSON part and if I go with allowing getting all data limits would be added too since it's the only way to go with on rpi :) (Cf my accepted PR for Files.GetDirectory for example).

@t-nelson
Contributor

How about I clear it up and just say any addition to this would push it to
G+1? Exposing the timers via json was given an OK, before it was written,
so long as the resulting patch was minimal.

@t-nelson
Contributor

Where are we at on this? Just need the header/json thingy updated?

@Montellese
Member

Yeah but I didn't have the time yet. Maybe tonight.

@Montellese
Member

As promised, see #4246.

@t-nelson
Contributor

Closing in favor of #4246

@t-nelson t-nelson closed this Feb 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment