Skip to content

Conversation

KenV99
Copy link

@KenV99 KenV99 commented Mar 22, 2016

Please disregard mailing list pull request. Downloading from github removed as requested.
ken.vives@gmail.com

@KenV99
Copy link
Author

KenV99 commented Mar 22, 2016

for path in paths:
if not os.path.isdir(path):
try:
os.mkdir(path)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@ronie
Copy link
Member

ronie commented Mar 22, 2016

please use a x.y.z addon version.

@KenV99 KenV99 changed the title [script.service.kodi.callbacks] 0.9.8.5 [script.service.kodi.callbacks] 0.9.9 Mar 23, 2016
@KenV99
Copy link
Author

KenV99 commented Mar 23, 2016

No problem. Version changed. Pull request updated.

except threading.ThreadError as e:
log(msg=_('Error aborting: %s - Error: %s') % (str(p), str(e)))
dispatcher.abort()
xbmc.sleep(1000)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@KenV99
Copy link
Author

KenV99 commented Mar 24, 2016

tamland: I took your advice to heart and removed all sleep from shutdown. Any needed potential wait time is done via joins with timeouts. Currently passing testing on Windows and emulated/VM on OSX, Android and Linux.

@KenV99
Copy link
Author

KenV99 commented Apr 2, 2016

I updated my commit to include some added functionality with http verbs and content-type for http tasks and to emit additional info during playback events as was requested by users on the forum.

Edit: Also, looking through comments on other pull requests(@learningit), I renamed any variables using __xxx__, removed two print statements and removed .gitignore and .gitattributes.

KenV99 added a commit to KenV99/script.service.kodi.callbacks that referenced this pull request Apr 14, 2016
Include build number, correct addon.xml to use library extension point
@KenV99
Copy link
Author

KenV99 commented Apr 18, 2016

Hello,
I was wondering if there was anything I could do to assist in finalizing whether or not this addon may be added to the official repo? More code documentation? A few UML diagrams to help understand the core functionality?

I also noted only recently that perhaps I should have submitted this to the Isengard branch since the wiki states that helix is not accepting new addons (http://kodi.wiki/view/Submitting_Add-ons#Allowed_submissions)? Github seems to offer different information and perhaps the PR needs to go to Jarvis (https://github.com/xbmc/repo-scripts/tree/isengard)?

Thanks.

@learningit
Copy link
Member

This PR has been discussed among a few us for some time. What I explain below is my personal opinion:
Firstly, your code is good. I haven't looked through everything, but what I have is good. To fully test this addon will take a long time and even then, the test assumes that the executed scripts are always written correctly. While it's an interesting concept, it's way beyond a user that doesn't have a fair technical background. It's almost like a scripting control environment on it's own. We have not approved several wizard/backup/config add-ons because they use external scripts, write to other addon directories, access invalid video sources, etc. The primary concern that I have is that a user can do a fair amount of damage to both their Kodi install as well as the System OS if a script is rogue. To the same point this addon could become a vehicle to easily inject code into a Kodi install, as well as the OS which could infect the user's system. We generally do not include addons in the repo which can be functionally extended externally other than for content access.
An add-on does not have to be included in the kodi.org repo to be widely accepted by users - PTVL is a good example.

@KenV99
Copy link
Author

KenV99 commented Apr 18, 2016

@learningit Thank you for your comments.

I understand your point of view. If I may, I would like to offer an alternative way of possibly looking at things. I am not trying to be argumentative or defensive. I know that the Kodi team takes great pride and responsibility in the application. I hope that any replies to the comments below would be in the tone of a healthy, open discussion.

The submitted addon only simplifies things that anyone can do if they understand python and the built-in python addon interface. It allows users who do not wish to go through the process of understanding that interface to be able to run simple tasks in response to Kodi events, similar to the smartphone app IFTTT. I might argue that anyone sophisticated enough to write malicious code to be used via this addon is likely also sophisticated enough to write a full blown addon. Parenthetically, if it is the 'plugin' architecture interface (http://kodi.wiki/view/Add-on:Kodi_Callbacks#Create_your_own_custom_task) that is included in the addon that is a major cause for concern, that can be easily removed.

As best as I can tell from the forums (http://forum.kodi.tv/showthread.php?tid=256170 and http://forum.kodi.tv/showthread.php?tid=151011&page=7), the majority of people are using it to automate ambient lighting solutions, run library updates or external scrapers. Users can damage their Kodi installs or OS's with or without the addon and perhaps the addon may make that less likely than if they attempt to do that by writing an addon on their own. Regarding this addon running scripts, the only scripts that are included in the distro are example scripts to demonstrate how the user can have a script run after Kodi exits or run a script that requires sudo. They are not configured to do anything on their own. I can eliminate them and show that example code in the wiki, if desired. The only scripts that the addon does run are ones that the user themselves writes. Perhaps a statement such as 'User assumes all risks and responsibilities associated with running their own tasks' or similar in a popup each time the settings are saved may be appropriate.

If any of the Kodi team members feel that I can 'cripple' the addon by eliminating certain portions of it to make it suitable for the official repo, I would be willing to entertain doing that.

In any case, I will, of course, accept your final decision. My intent is only to expose as many users as I can to the possibilities of what the addon can do for them to make their Kodi experience the best that it can be.

@phil65
Copy link
Contributor

phil65 commented Apr 19, 2016

Thx for such a balanced response, we have seen much worse here. :)
Moving example scripts to a wiki sounds like a good solution to me. In general I would be fine with including this addon. @ronie what is your opinion?

@ronie
Copy link
Member

ronie commented Apr 19, 2016

i haven't reviewed the code, so couldn't give you a balanced opinion ;-)

if the addon doesn't write/overwrite stuff to places it shouldn't go
(outside of it's own addon_data subfolder), it should be acceptable i guess.

i agree script examples would be better fitted in the wiki yeah.

@learningit
Copy link
Member

learningit commented Apr 20, 2016

Ok. so if there aren't any more comments, I say we add this to the repo. The PR needs to change to at least Isengard as we are not accepting new add-ons into Helix any longer. The example scripts should be moved to the wiki. Once that's done, I'll do the merge.

@KenV99
Copy link
Author

KenV99 commented Apr 21, 2016

OK. Thank all for your time and consideration. The above changes have been made and a new PR for isengard has been made: https://github.com/xbmc/repo-scripts/pull/120

@razzeee
Copy link
Member

razzeee commented Apr 21, 2016

I don't think isendgard is supposed to accepts new addons? see https://github.com/xbmc/repo-scripts/tree/isengard#status

@KenV99
Copy link
Author

KenV99 commented Apr 21, 2016

@razzeee please see @learniGIT 's comment above. The wiki page and the GitHub page have conflicting info about which branches are accepting new PR's.

@razzeee
Copy link
Member

razzeee commented Apr 21, 2016

Wiki wasn't up to date, I just changed that. Thanks for pointing it out.

@learningit
Copy link
Member

I'm closing this PR as PR#120 supersedes this one (I hope).

@learningit learningit closed this Apr 22, 2016
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.

6 participants