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

New options: stream over HTTPS & stream with login #119

Merged
merged 1 commit into from Oct 22, 2018

Conversation

@knackebrot
Copy link
Contributor

commented Oct 22, 2018

Good for people with webif behind a reverse proxy

Copy link
Member

left a comment

Please add change log and add-on minor version number bump.

Thanks

@@ -58,6 +58,8 @@ namespace enigma2
const std::string& GetPassword() const { return m_strPassword; }
bool GetAutoConfigLiveStreamsEnabled() const { return m_bAutoConfig; }
int GetStreamPortNum() const { return m_iPortStream; }
bool GetUseSecureConnectionStream() const { return m_bUseSecureHTTPStream; }
bool GetUseLoginStream() const { return m_bUseLoginStream; }

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 22, 2018

Member

One verb is enough. Please remove the "Get"s.

This comment has been minimized.

Copy link
@phunkyfish

phunkyfish Oct 22, 2018

Member

All the other "Use" functions use the same double verb. So at the very least what's in the PR is consistent ;) Will fix others in a future PR.

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 22, 2018

Member

fine by me.

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Similar to streamport this setting would be overridden if autoconfig is enabled. Therefore it should have a dependency on autoconfig in settings.xml. So each settings should have a <dependency> block like streamport. This will make it clear to users that this won't have an effect if autoconfig is enabled.

<dependencies> <dependency type="enable" setting="autoconfig">false</dependency> </dependencies>

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Looks good to me

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

@ksooo would you usually request the review changes be fixed up into the first commit or are you OK with a separate review commit?

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

fixed up, please

@knackebrot knackebrot force-pushed the knackebrot:master branch 2 times, most recently from 6b37093 to 83c44ef Oct 22, 2018
@knackebrot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

squashed.

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Well done, all looks good now ;)

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Shouldn't the minor addon version number get increased instead of the micro version number? This is a new feature, not a bug fix only release.

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Fair point should be: 3.13.0

@knackebrot knackebrot force-pushed the knackebrot:master branch from 83c44ef to f2f7b2a Oct 22, 2018
@knackebrot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

We are now on v3.13.0 :)

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

You're nearly there. Building failing. Your StringUtils call returns a std::string. You need to put .c_str() at the end of it.

So like this:
Settings::GetInstance().UseLoginStream() ? StringUtils::Format("%s:%s@", Settings::GetInstance().GetUsername().c_str(), Settings::GetInstance().GetPassword().c_str()).c_str() : "",

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

It's the final .c_str() that you are missing.

@knackebrot knackebrot force-pushed the knackebrot:master branch from f2f7b2a to 60aa65c Oct 22, 2018
@knackebrot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

I see that it failed on every platform I didn't try (I only tried MSVC). Should be fixed now. Thank you.

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Ok, you are looking good. Again 😉

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

@ksooo are you Ok for me to approve and merge?

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Sure, go ahead.

Please be aware that if you want the new version to appear in the new Kodi binary addon repo you need to tag the master HEAD after merge => 3.13.0-Leia

@phunkyfish

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Cool, will do. Thanks

@phunkyfish phunkyfish merged commit 6bb1af4 into kodi-pvr:master Oct 22, 2018
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.