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

[plugin.video.youtube] v6.1.2 #2001

Merged
merged 1 commit into from
Aug 23, 2018
Merged

[plugin.video.youtube] v6.1.2 #2001

merged 1 commit into from
Aug 23, 2018

Conversation

anxdpanic
Copy link
Member

Description

Changelog: https://github.com/jdf76/plugin.video.youtube/blob/6.x.x/changelog.txt#L1-L38

Checklist:

  • My code follows the add-on rules and piracy stance of this project.
  • I have read the CONTRIBUTING document
  • Each add-on submission should be a single commit with using the following style: [plugin.video.foo] v1.0.0

Additional information :

  • Submitting your add-on to this specific branch makes it available to any Kodi version equal or higher than the branch name with the applicable Kodi dependencies limits.
  • add-on development wiki page.
  • Kodi pydocs provide information about the Python API
  • PEP8 codingstyle which is considered best practice but not mandatory.
  • This add-on repository has automated code guideline check which could help you improve your coding. You can find the results of these check at Codacy. You can create your own account as well to continuously monitor your python coding before submitting to repo.
  • Development questions can be asked in the add-on development section on the Kodi forum.

@TravisBuddy
Copy link

Travis tests have failed

Hey @anxdpanic,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

Expand here
INFO: Checking add-on plugin.video.youtube
INFO: Created by bromix
INFO: Addon id matches folder name
WARN: Required addon script.module.six does not require a fixed version Available: 1.11.0 
ERROR: Required addon script.module.inputstreamhelper not available in current repository.
INFO: Image icon exists
INFO: Icon dimensions are fine 256x256
INFO: Image fanart exists
INFO: Fanart dimensions are fine 1280x720
WARN: Complex entry point. Check: resources/lib/startup.py | Counted lines: 52 | Lines allowed: 15
ERROR: We found 1 problems and 2 warnings, please check the logfile.

@TravisBuddy
Copy link

Travis tests have failed

Hey @anxdpanic,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

Expand here
INFO: Checking add-on plugin.video.youtube
INFO: Created by bromix
INFO: Addon id matches folder name
WARN: Required addon script.module.six does not require a fixed version Available: 1.11.0 
ERROR: Required addon script.module.inputstreamhelper not available in current repository.
INFO: Image icon exists
INFO: Icon dimensions are fine 256x256
INFO: Image fanart exists
INFO: Fanart dimensions are fine 1280x720
ERROR: We found 1 problems and 1 warnings, please check the logfile.

@anxdpanic
Copy link
Member Author

I've fixed the complex entry warning and the codacy issue, leaving the commit separate in case anyone has started reviewing. Will bump version and squash when given the go ahead.

list_item.setProperty("Fanart_Image", fanart)
elif major_version <= 15:
list_item.setArt({'thumb': thumb, 'fanart': fanart})
list_item.setIconImage(thumb)

This comment was marked as spam.

This comment was marked as spam.

play_item.get_headers() and play_item.get_uri().startswith('http'):
play_item.set_uri(play_item.get_uri() + '|' + play_item.get_headers())

list_item.setProperty('inputstreamaddon', '')

This comment was marked as spam.

This comment was marked as spam.


monitor.remove_temp_dir()

while not monitor.abortRequested():

This comment was marked as spam.

This comment was marked as spam.

@@ -106,16 +105,4 @@ def _seconds_difference(_first, _last):
return cached_data

def _optimize_item_count(self):
clear = False

This comment was marked as spam.

This comment was marked as spam.



__API_KEY = 'ZGFiODVhMzM5NTI2ZmVhNmEwZDc5ZjkzMjM2M2FjY2Q='
__API_BASE_URL = 'http://api.ipstack.com'

This comment was marked as spam.

self.current_video_total_time = self.getTotalTime()

def onAVStarted(self):
if self.context.get_settings().use_playback_history():

This comment was marked as spam.

else:
context.log_warning('Missing video ID for post play event')
return True

@kodion.RegisterProviderPath('^/users/(?P<action>[^/]+)/$')
def _on_users(self, context, re_match):

This comment was marked as spam.

This comment was marked as spam.

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Globally all is good. I don't like the ipstack stuff nor understand why you need it. Is it only used for the initial setup wizzard? Why is it required?
Note it can be confused by some sort of tracking. I don't think anyone will be confortable to have a youtube plugin that performs requests to other API's rather than youtube itself.
Thanks for the update and sorry for the time taken to review (changes are big). Don't mind about addon-check failure, we'll try to fix it soon.

@anxdpanic
Copy link
Member Author

Thanks for the review. No worries on the time, was expecting a few day. I will try to respond and address the issues.

As for the ipstack code, a 'My Location' folder was added that uses latitude, longitude(parameters in youtube api call) to provide geo based results and nothing more. During the setup wizard, the user is asked "Perform geolocation? Used for My Location"

If this is not okay, is manual input of this information allowed?

@enen92
Copy link
Member

enen92 commented Aug 23, 2018

@anxdpanic thanks for understanding. The problem here is that relying on an API key will also enable api usage stats and along with it...user tracking. I was pointed by @Lunatixz that http://ip-api.com/json could be an alternative (used in plutotv). Would this work for you?

@anxdpanic
Copy link
Member Author

@enen92
ip-api looks like it will do the trick. Will get that changed.

While I'm making these changes, is there anything that needs to be done to identify py3 compatibility?

@enen92
Copy link
Member

enen92 commented Aug 23, 2018

When Leia is final a new field will be available in addon.xml to identify the ability of the addon to support both py2 and py3, but as of now it is not yet available. Support for both will be required when submitting to the Leia branch. For M*** py3 is required (py2 no longer used).

You can try to use the addon-check project (https://github.com/xbmc/addon-check) locally against your addon by setting --branch=leia. It includes support for 2to3, a python module that warns about deprecated python usage and suggestions for py3 compatibility. We are only enforcing this check for Krypton and Leia branches of repo-plugins and repo-scripts IIRC.

@anxdpanic
Copy link
Member Author

Thank you for the information. I will check to see if there are any suggestions that come from running --branch=leia. IDE shows all clear and been working in py3 nightlies since 6.0.0 I believe.

@TravisBuddy
Copy link

Travis tests have failed

Hey @anxdpanic,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

Expand here
INFO: Checking add-on plugin.video.youtube
INFO: Created by bromix
INFO: Addon id matches folder name
WARN: Required addon script.module.six does not require a fixed version Available: 1.11.0 
ERROR: Required addon script.module.inputstreamhelper not available in current repository.
INFO: Image icon exists
INFO: Icon dimensions are fine 256x256
INFO: Image fanart exists
INFO: Fanart dimensions are fine 1280x720
ERROR: We found 1 problems and 1 warnings, please check the logfile.

@anxdpanic
Copy link
Member Author

Changes made and using ip-api now.

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anxdpanic can you squash for merging? Thanks for adjusting

@anxdpanic anxdpanic changed the title [plugin.video.youtube] v6.1.1 [plugin.video.youtube] v6.1.2 Aug 23, 2018
@TravisBuddy
Copy link

Travis tests have failed

Hey @anxdpanic,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

Expand here
INFO: Checking add-on plugin.video.youtube
INFO: Created by bromix
INFO: Addon id matches folder name
WARN: Required addon script.module.six does not require a fixed version Available: 1.11.0 
ERROR: Required addon script.module.inputstreamhelper not available in current repository.
INFO: Image icon exists
INFO: Icon dimensions are fine 256x256
INFO: Image fanart exists
INFO: Fanart dimensions are fine 1280x720
ERROR: We found 1 problems and 1 warnings, please check the logfile.

@anxdpanic
Copy link
Member Author

Thanks for taking the time to review, squashed and updated titles

@enen92
Copy link
Member

enen92 commented Aug 23, 2018

CI failure is already reported in addon-check project and should be addressed soon (xbmc/addon-check#110). Merging.

@enen92 enen92 merged commit 53f0541 into xbmc:isengard Aug 23, 2018
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