search

kodi-pvr / pvr.nextpvr

Kodi's NextPVR client addon
GNU General Public License v2.0
22 stars 33 forks source link

V3.3.15 #93

Closed emveepee closed 5 years ago

emveepee commented 5 years ago

Initial changes for NextPVR v5 (subtitle, season, episode, disable timeshift).

sub3 commented 5 years ago

Thanks Martin!

ksooo commented 5 years ago

Let me know if you guys don't care about "stylistic" code enhancements, then I will no longer look over your PRs. Personally, I think the last PRs could have gotten some more love before merge.

sub3 commented 5 years ago

Thanks ksooo - even this stylistic feedback is appreciated. Sorry it wasn't a cleaner PR from us. When we're focusing on application function, some times we forget this type of stuff. We will work through your feedback items, and address with the next PR. Cheers.

emveepee commented 5 years ago

Yes I was looking for feedback, the PR got tagged a bit earlier than I expected. I do want to move more of the old C code to C++ as I go so don't know all the conventions. Things like NULL to nullptr will be more for a style cleanup PR.

The reason for some of the odd looking code is the API from NextPVR supports multiple clients beyond Kodi and sub didn't want to bump the server API version. Currently the description is a combination of "episode name: description." Now that the episode name is coming in a separate field in v5, I have to parse it with the colon from the description. This means at times there will be no description.

ksooo commented 5 years ago

I do want to move more of the old C code to C++ as I go so don't know all the conventions. Things like NULL to nullptr will be more for a style cleanup PR.

The second sentence contradicts the first. Either you want to make the changes as you go or with dedicated cleanup commits. ;-)

emveepee commented 5 years ago

All the the other XML parsing code uses NULL, I thought it would be odd changing them one at a time but if that is better style I will do that in the future.

ksooo commented 5 years ago

All the the other XML parsing code uses NULL, I thought it would be odd changing them one at a time

There is no right or wrong here. For Kodi, we fix coding guideline violations whenever we touch a code line (this is how I take "as we go"). Addons can handle this differently.

I just commented where I thought the code could be improved. It's completely up to you what you make out of this.

emveepee commented 5 years ago

Excellent comment on fixing the code as we touch the line so I will do that going forward. Much appreciated once again.