kodi-pvr / pvr.stalker

A PVR Client that connects Kodi to Stalker Middleware
GNU General Public License v2.0
41 stars 64 forks source link

(Matrix) Fix issue #189 : "year" and "starRating" are optional in xmltv contract #190

Closed Macadoshis closed 1 year ago

Macadoshis commented 1 year ago

Didn't try it though. It's my very first project with Kofi development. (I followed cmake instructions to compile but I didn't make it to built a dylib release to test irl)

phunkyfish commented 1 year ago

Nice. If you also add a commit similar to this one but instead for 20.2.3 to this PR then I can release it for you as well.

Macadoshis commented 1 year ago

Hello @phunkyfish , thanks for the feedback. Would I still be able to test it on my running Kodi (matrix v19.3). As far as I understood, v20.x is for the upcoming Kodi v20 (Nexus) not yet released publicly.

Or otherwise would it be possible to build me an installable (zip w/ dylib) if I'd PR this cherrypick to the matrix branch ?

Macadoshis commented 1 year ago

(I've added a bump commit to the PR if that's what you meant)

Macadoshis commented 1 year ago

Btw I followed the azurepipelines and managed to create locally a 19.0.4 release (dylib + zip) from Matrix branches (both pvr.staker and xmbc).

I've tested locally and I confirmed my fix works. For the same EPG that was in error, I finally got :

image

But I guess something should be rework about the default values set from the struct. It shouldn't always be valued but empty/missing data should be handled more properly imo.

You see in the capture, the episode is proposed because it's set by default to "0" from :

https://github.com/kodi-pvr/pvr.stalker/blob/d0170c5b3fc496d8a51fe894661babcd5d7b4347/src/GuideManager.cpp#L195

https://github.com/kodi-pvr/pvr.stalker/blob/1ee127a3735b31cdf1a1a7bc7b5b526f5bf9c96b/src/XMLTV.h#L49

The tv guide for the same exact same EPG.xml is working and displayed fine with ipv.iptvsimple, maybe some code should be inspired from the latter :)

phunkyfish commented 1 year ago

What platform are you using, is it a Mac? You can always get the builds of addons from Jenkins link under checks on this PR main page.

You can create a similar PR for Matrix if you like.

Regarding the default value for Episode or Season Number, it should be set to EPG_TAG_INVALID_SERIES_EPISODE if it's not valid. So in your case, if the value is <= 0 then set to EPG_TAG_INVALID_SERIES_EPISODE otherwise set it to the value.

Feel free to add a commit for this and I'm happy to review it.

And thanks for contributing, there are a lot of stalker users who don't have a working add-on today and they will soon!

Macadoshis commented 1 year ago

@phunkyfish yes I'm on MacOS both for running Kodi and developing. (Though my main usage of Kodi is from my Android-based firetv).

I thought I could download the released runnable from the azure pipeline but it's only generated locally on the azure agent (D:\...). I'll try investigating from Jenkins workspace if I have access, so I could also get the plugin from my PR to test on FireTV, thanks for the clue.

I'm happy to contribute, it'd be less time-consuming if there were unit-tests (also java/c# guy, I haven't coded with c++ for ages) :)

Took good note of your hint for the episode, and I'll try to fix the fallback logic, give me few extra days.

phunkyfish commented 1 year ago

If you expand the checks link below just click the Jenkins link you see.

Screenshot 2022-09-13 at 13 59 47

Here is the link to the Jenkins page for your PR: https://jenkins.kodi.tv/blue/organizations/jenkins/kodi-pvr%2Fpvr.stalker/detail/PR-190/2/pipeline And if you click artifacts on the same page you'll see all the builds: https://jenkins.kodi.tv/blue/organizations/jenkins/kodi-pvr%2Fpvr.stalker/detail/PR-190/2/artifacts

Azure is only the Windows based platforms, targeting the unusual builds (UWP / ARM etc.).

Jenkins has all the common platforms that most people use (Windows, Mac, Android).

You can always get the latest Kodi builds from our download server: https://mirrors.kodi.tv/nightlies. Take your time with the change for episode, I can review your addition once you are ready, just post back here.

AlwinEsch commented 1 year ago

This needs an rebase after the API update request was come in.

Macadoshis commented 1 year ago

@phunkyfish done and tested with 2 <programme> : one simple (with required minimum fields) and a second one with all possible fields. No error founds. Episode is correctly hidden for the simple one and displayed for the second one.

(For the second full programme, I noticed lot of fields are not displaying even if present in the <programme> : <category>, <icon>, <subtitles>, <previously-shown>, ... don't know if it's related to not even being supported by kodi core or another issue in the actual GuideManager:Event structure from GuideManager:AddEvents(..))

Btw the azure pipelines didn't trigger or went to error/cancelled. Is it related to my forcepush ? (moved from/to Matrix branch and rebased as well).

phunkyfish commented 1 year ago

Just worry about Jenkins on the Matrix branch. Azure has deprecated the older Visual Studio version but we can’t upgrade because it’s the highest version possible with Matrix.

phunkyfish commented 1 year ago

We will need to create a Nexus PR first though. We need to merge that before we can merge any Matrix change.

phunkyfish commented 1 year ago

Also I think those fields just are not support in kodi core.

phunkyfish commented 1 year ago

You could probably translate the categories for the programmes to genres in Kodi. There are quite a few examples of this being done in other PVR addons. Happy to share examples of you are interested. The end effort is that you have genre text and colouring in the EPG if you get it working. But likely something for a separate PR.

Macadoshis commented 1 year ago

Just worry about Jenkins on the Matrix branch. Azure has deprecated the older Visual Studio version but we can’t upgrade because it’s the highest version possible with Matrix.

Thanks I didn't see Jenkins in the GH workflow but from the direct link I managed to download the android targeted binaries. I'm beta-testing my 19.0.4 fix in early access since yesterday on my firetv (EPG working so far).

I'll rebase again my PR to the Nexus branch as wished.

Macadoshis commented 1 year ago

You could probably translate the categories for the programmes to genres in Kodi. There are quite a few examples of this being done in other PVR addons. Happy to share examples of you are interested. The end effort is that you have genre text and colouring in the EPG if you get it working. But likely something for a separate PR.

Nice advice. I'll compare with iptvsimple which seems to be more reliable and up-to-date regarding xmltv mapping with Kodi core. Anyway I agree it'd definitely be part of another fix and PR.

phunkyfish commented 1 year ago

Just worry about Jenkins on the Matrix branch. Azure has deprecated the older Visual Studio version but we can’t upgrade because it’s the highest version possible with Matrix.

Thanks I didn't see Jenkins in the GH workflow but from the direct link I managed to download the android targeted binaries. I'm beta-testing my 19.0.4 fix in early access since yesterday on my firetv (EPG working so far).

I'll rebase again my PR to the Nexus branch as wished.

If you’d like just cherry pick your commits to another nexus based branch and create a second PR. I can merge them one after another.

phunkyfish commented 1 year ago

Please add a commit like this: https://github.com/kodi-pvr/pvr.stalker/commit/3ffcaba9ce1828e8557ddf44bc15809807b755d3

But for 19.0.4, then I can merge and release your PR.

Macadoshis commented 1 year ago

Please add a commit like this: 3ffcaba

But for 19.0.4, then I can merge and release your PR.

I actually had this in the first version in this branch, as you suggested in the beginning, but I reverted this recently thinking it was just for the purpose of building a release for my own tests. Didn't know you really meant your contributors to mess with your release management policy already in contributions PRs :) Thought the bump commit were up to regular members only, in a separate and secondary commit (content of change logs, frequency of releases, versioning policy, ...).

phunkyfish commented 1 year ago

Thanks for the contribution!!!