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

Add support for all product families ("VB", "VJ", "VZ" etc) #112

Merged
merged 1 commit into from
Jan 31, 2016
Merged

Add support for all product families ("VB", "VJ", "VZ" etc) #112

merged 1 commit into from
Jan 31, 2016

Conversation

relipo
Copy link
Collaborator

@relipo relipo commented Jan 31, 2016

Hi, I'm an R&D software engineer for VBox Comm - that's the company that owns the product that this add-on integrates into Kodi.
SO far the only product family was "VB", which is the reason it's been hard-coded in this function.
Now we have a line of a new product that's being tested right now ("VJ"), and we need the add-on to recognize those devices as well. Other future products will also be in that format ("V"+another capital letter).

Thanks a lot,
Ariel

Hi, I'm an R&D software engineer for VBox Comm - that's the company that owns the product that this add-on integrates into Kodi.
SO far the only product family was "VB", which is the reason it's been hard-coded in this function.
Now we have a line of a new product that's being tested right now ("VJ"), and we need the add-on to recognize those devices as well. Other future products will also be in that format ("V"+another capital letter).

Thanks a lot,
Ariel
@Jalle19
Copy link
Contributor

Jalle19 commented Jan 31, 2016

Looks good, don't mind the Travis failures for now.

Are you able to do some maintenance on this addon? I'm the original coder but I don't have enough time or interest at the moment to keep up with development, and there's always the risk that addons that don't build or work properly for some time get dropped from the official repository.

Jalle19 added a commit that referenced this pull request Jan 31, 2016
Add support for all product families ("VB", "VJ", "VZ" etc)
@Jalle19 Jalle19 merged commit f80b799 into kodi-pvr:master Jan 31, 2016
@relipo
Copy link
Collaborator Author

relipo commented Feb 1, 2016

Hi Sam,

As you know by now (and as I wrote in the original code suggestion), I'm a member of the VBox Comm R&D team.

My name is Ariel, by the way. First of all, the addon (in my opinion) - very well done.

Second - we will take control of the code, since we know how important Kodi users (and addons) are.

Actually, we went into GitHub just for that. So to help us do that, I have a question for you:

What exactly does is require for our addon to be in the nightly build of Kodi?

Because currently, after installing the nightly build, it seems our addon isn't included. And what does this mean for the next Kodi release?

How do we make sure our addon is included in it?

Thanks,

Ariel

From: Sam Stenvall [mailto:notifications@github.com]
Sent: 31 January 2016 14:01
To: kodi-pvr/pvr.vbox
Cc: relipo
Subject: Re: [pvr.vbox] Add support for all product families ("VB", "VJ", "VZ" etc) (#112)

Looks good, don't mind the Travis failures for now.

Are you able to do some maintenance on this addon? I'm the original coder but I don't have enough time or interest at the moment to keep up with development, and there's always the risk that addons that don't build or work properly for some time get dropped from the official repository.


Reply to this email directly or view it on GitHub #112 (comment) . https://github.com/notifications/beacon/AQFNZ21FmF6r1soWSvyh7bP-_ssil1Gaks5pfe75gaJpZM4HQAjx.gif

@MartijnKaijser
Copy link
Member

Hi @relipo it currently fails to build on windows only. Once that is solved it should be included automaticly

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 1, 2016

My name is Ariel, by the way. First of all, the addon (in my opinion) - very well done.

Thank you, I really tried to do my best since this was commissioned work.

What exactly does is require for our addon to be in the nightly build of Kodi? Because currently, after installing the nightly build, it seems our addon isn't included. And what does this mean for the next Kodi release?

I suspect the Windows build failure (and the Travis failures on OS X) is due to an API change that hasn't been implemented in the addon yet. The PVR API (which is the API between Kodi and PVR addons) was bumped to 4.2.0 while the addon currently implements 4.1.0.

The missing change is implementing the changes done in xbmc/xbmc#8896. For this addon I think it's enough to return true when playing live TV and false when playing recordings.

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 1, 2016

@relipo I added you as a collaborator so you can push to this repository, but I'd prefer if you create PRs instead of pushing to master directly. This way Travis gets a chance to catch build errors and I get a chance to do a review. I hope that's okay.

@relipo
Copy link
Collaborator Author

relipo commented Feb 2, 2016

O.K, so just making sure:

I have the patch so the VBox Gateway addon works with the 4.2.0 PVR API.

  1.    AS for the v16.0 "Jarvis" version - the PVR API for that version will still be 4.1.0, yes? Was it bumped to 4.2.0 for v16.0 or v17.0?
    

· If it was bumped for v16.0, I need it to be in the v16.0. Possible?

· If it was bumped for v17.0 - will my VB / VJ / VZ patch be in v16.0 (works regardless of the PVR API)?

  1.   As for the v17.0 "Krypton" version, the PVR API will be 4.2.0, right? 
    

I'm considering adding some more changes, so just a quick question: how long do you think before that version closes as well?

Thanks again,

Ariel

From: Sam Stenvall [mailto:notifications@github.com]
Sent: 01 February 2016 16:38
To: kodi-pvr/pvr.vbox
Cc: relipo
Subject: Re: [pvr.vbox] Add support for all product families ("VB", "VJ", "VZ" etc) (#112)

My name is Ariel, by the way. First of all, the addon (in my opinion) - very well done.

Thank you, I really tried to do my best since this was commissioned work.

What exactly does is require for our addon to be in the nightly build of Kodi? Because currently, after installing the nightly build, it seems our addon isn't included. And what does this mean for the next Kodi release?

I suspect the Windows build failure (and the Travis failures on OS X) is due to an API change that hasn't been implemented in the addon yet. The PVR API (which is the API between Kodi and PVR addons) was bumped to 4.2.0 while the addon currently implements 4.1.0.

The missing change is implementing the changes done in xbmc/xbmc#8896 xbmc/xbmc#8896 . For this addon I think it's enough to return true when playing live TV and false when playing recordings.


Reply to this email directly or view it on GitHub #112 (comment) . https://github.com/notifications/beacon/AQFNZ_y-VR6nMC1umbsbHGwaHDg6wGO6ks5pf2U-gaJpZM4HQAjx.gif

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 2, 2016

The API was only bumped in Kodi master so there's no need to change anything in that regard for Jarvis.

@MartijnKaijser
Copy link
Member

@relipo to be clear you would need to made anew PR against jarvis branch with this patch (including a minor version bump and changelog) if you also want this in the Kodi v16 release

@relipo relipo deleted the patch-1 branch February 3, 2016 08:38
@relipo
Copy link
Collaborator Author

relipo commented Feb 4, 2016

O.K, great.

I'll just ask this: I know the nightly builds refer to the development of v17.0, but is there a way for me to make sure my fix is a part of the v16.0 release?

(except building that branch from source, which I already did & seems fine)

Thanks again, Ariel

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 4, 2016

We're going to bump all addons to the latest revision from the Jarvis branches soon (or it could be done already, I haven't checked). @MartijnKaiser knows the details of how it's done better than me.

@MartijnKaijser
Copy link
Member

We just point to Jarvis branch for PVR while for all other we point to specific git SHA. So all stuff added to jarvis will be auto included in whatever is latest build

@Jalle19
Copy link
Contributor

Jalle19 commented Feb 5, 2016

Very nice, thanks for clearing that up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants