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

Branch script for Kodi 18 #210

Open
ghost opened this issue Dec 17, 2016 · 21 comments
Open

Branch script for Kodi 18 #210

ghost opened this issue Dec 17, 2016 · 21 comments

Comments

@ghost
Copy link

ghost commented Dec 17, 2016

Having given some thought to Phil's comment on recent PR for compatibility for Kodi 18, I'd like to branch the script for v18 of Kodi but would appreciate others thoughts before I do this (which won't be until after Christmas at the earliest ;)) and have begun extended my RetroPlayer branch at https://github.com/Ignoble61/script.skinshortcuts/tree/games in preparation

The advantage of doing this is it simplifies code (rather than having to use the workarounds currently on git for keeping compatibility with v18 - this also gives a good opportunity to make a 'clean break' of it, as it were, where the current master will only work on v15-17, the new branch only v18). It also gives a good opportunity to refactor and remove old code knowing that this may cause breakages in the short term, but with plenty of time to fix before v18 becomes mainstream.

Jobs already done on the branch:

  • Retroplayer support (not complete - Kodi api has not yet been updated to supply all information)
  • Remove all backwards support (with the preference being to remove 'forward compatibility code' from existing branch)
  • Remove gui 309 (old widget select code)
  • Remove auto-playlist generation for viewing sources in library
  • Refactor various library loading code - sources, static items (common, commands, settings, pvr)

Jobs that it would be nice to do:

  • Remove gui's 101/102/110 - these are only used by Nox 5. @BigNoid if I were to submit a PR which replaced these with gui 401 to your skin (as used by others), would you be happy to lose these?
  • Move 'Default widget' listing from Skin Helper to Skin Shortcuts, with the possibility to backport this to existing branch. @marcelveldt any objections to this?
  • Keep refactoring complex/repetitive code

Jobs that it may not be practical to do, but would let the code be massively simplified in places

  • Remove [skin.id].properties file - save properties directly in the .DATA.xml file
  • Remove various 'default' overrides - have these provided directly in the skins default .DATA.xml files
  • Move available shortcut loading to separate module

Any thoughts on the idea in general, the ideas presented above or other areas of the script where the opportunity could be taken to remove/refactor?

@BigNoid
Copy link
Owner

BigNoid commented Dec 17, 2016

@Ignoble61 I am not aware I am using deprecated code :)
I will have a look to see what needs updating.
I dont use much of the rest of the script as i still havent found a nice way to migrate to skin shortcuts without users losing their customization, so can't really comment on the rest of the proposed changes.

@ghost
Copy link
Author

ghost commented Dec 17, 2016

You're not using deprecated features - which is to say, features which have been replaced and specifically deprecated - in this case, it's more a case of you're the only skinner using the specified gui controls. As it is, three separate controls (and their associated code) are being maintained for a single skin which isn't great - though, in fairness, the code for those controls hasn't changed in a long time, it just removes the need to keep them updated if the code changes in the future :)

I'm also not fixed on any of this - one of the aims of the script, and the reason I've never forced a branch before this - is to never require skinners to change their implementation unless absolutely necessary. This, specifically, is why I'm happy to keep the controls if you prefer them or to PR the necessary changes to your skin myself :) (I skin myself, and know the hassle where specific script integration needs to be regularly revised between Kodi versions.)

Honestly I don't consider a full-Skin-Shortcuts implementation to be the right solution for every skin but this would be a good opportunity to look at implementing some sort of migration system if you did want to go for the full integration - there's so much time before v18 that this is the time for such things to be discussed :)

@smitchell6879
Copy link

I know I really don't have any known experience to comment on much but I am plus one for a v18... I am starting a new skin and would like to implement skin shortcuts and considering I am just start it will probably be a v18 out by the time I finish and so far I can only make this script work with Krypton.... May just be my noobieness to skinning..... But the script does save a lot of writing.

@braz96
Copy link

braz96 commented Dec 19, 2016

@Ignoble61
Just my two cents, but I say go for it, especially if it will make the script easier to maintain into the future. I'm sure skinners will be quick to adapt to any changes that are required as a result of the refactor.

@marcelveldt
Copy link
Contributor

I will help out with the refactor and if you agree I'd like to make sure it meeds the pep8 guidelines making it more readable and maintainable. Once that's all done we can have codacy automatically do some basic tests

@ghost
Copy link
Author

ghost commented Dec 30, 2016

I don't think I can resist pep8 any longer :) I'm not doing anything more code-wise this year, so in the new year we'll look at where's best to continue forward with the refactor - whether you want push rights to my fork, or if we move it over here at this point. (I'll also do a list of what I've already done, and the already needed skinning changes)

@BigNoid
Copy link
Owner

BigNoid commented Dec 30, 2016

Just as a tip, install Anaconda package in sublime text. That's a great tool for python coding in general and also helps for keeping the code within the pep-8 guidelines.

@marcelveldt
Copy link
Contributor

Thanks for the tip! Currently I'm still using plain-old Notepad++ but with my current move to MacOS I definitely have to go for the Sublime route

@BigNoid
Copy link
Owner

BigNoid commented Dec 30, 2016

I'm sure you'll never look back to Notepad++ :)

@ghost
Copy link
Author

ghost commented Jan 5, 2017

@marcelveldt you're already authorised to push changes to my fork, so I think it makes sense to keep development there for the moment. There are basically two breaking changes to the script so far - the directory for skin integration has changed from special://skin/shortcuts to special://skin/extras/script.skinshortcuts; and there's a new dependancy - simpleeval. There are also variously removed gui controls, but I don't think they'll affect anything but Nox5 (and I'll PR an update to that skin before we're done).

My own priorities for the next week are to finish refactoring the gui onClick; move custom properties to the .DATA.xml files and to refactor the library-node code in library.py. Please have-at any element you want to look at, from Pep8 upwards :)

Note: Change of plan, v18 now being primarily developed here - which allows my owns 'games' branch to be used for WIP cope.

@ghost
Copy link
Author

ghost commented Jan 13, 2017

@BigNoid script version for v18 of Kodi is now ready for initial testing. As your skin is the one most affected by script changes, if you let me know which branch Leia changes are being made against I'll PR relevant script changes against such (bearing in mind skin-breaking changes to the script should be done, even if all changes are nowhere near - though there is now the possibility of an early Leia release whilst full changes are being finalised)

@BigNoid
Copy link
Owner

BigNoid commented Jan 27, 2017

cript version for v18 of Kodi is now ready for initial testing. As your skin is the one most affected by script changes, if you let me know which branch Leia changes are being made against I'll PR relevant script changes against such (bearing in mind skin-breaking changes to the script should be done, even if all changes are nowhere near - though there is now the possibility of an early Leia release whilst full changes are being finalised)

Sorry @Ignoble61 I missed your ping :(
Please feel free to break Aeon Nox with your script. I'll catch up if you do. It will be my own fault for stubbornly keeping on to the skin coded main menu :)
If you want to do a PR, my master branch is Leia, but please don't feel obliged to do the changes for me.

@marcelveldt
Copy link
Contributor

marcelveldt commented Feb 9, 2017

@Ignoble61 sorry, have been busy lately. Where are you with this now ?
Shall I go ahead and start moving the skinhelper widget code to the skinshortcuts branch ?

@ghost
Copy link
Author

ghost commented Feb 16, 2017

@BigNoid - I think I've already broken Nox5 in the development branch. If not (and I'm not currently up-to-date on exactly where things are), I'll certainly feel free to do so :)

@marcelveldt - I'm just starting to re-focus on Kodi after some health issues, so things are kind of stuck at the moment from my end until I can get the time to get back to things. As such, feel free to look at any area that interests you - if you wanted to go ahead and move the widget code, that would be great :) I'm going to try and get a to-do list together over the next week or two so I can try and structure the work that still needs doing.

@ghost
Copy link
Author

ghost commented Feb 16, 2017

To attempt to separate discussion of changes for Leia as opposed to identified issues and to-do's for Leia: #217

@marcelveldt
Copy link
Contributor

@Ignoble61 Sorry to hear about that, I hope you feel better soon!

For the TODO list:

  1. Make code PEP8 compatible
  2. Move widget code
  3. New script version which will be Kodi 18+ only
  4. Maybe integrate the image pickers (with resource addon support) from skin helper tools

What branch should I use to start going ?

@ghost
Copy link
Author

ghost commented Feb 24, 2017

@marcelveldt - the Leia branch here is the most up-to-date published version, so best to work against :)

@marcelveldt
Copy link
Contributor

@Ignoble61 okay, will start working on it as of next week. this week still busy with providing support after large updates of SH and skin.

@marcelveldt
Copy link
Contributor

@Ignoble61 if you don't mind, I will push out another update to the kodi repo as the current version is a bit outdated and missing some features I currently rely on in my skin.

@ghost
Copy link
Author

ghost commented Mar 25, 2017

No problem, and I apologise my time is so limited at present to make any updates myself. I am going to try to fix Braz and JurialMunkey's issues in the next couple of days - if I get them fixed before the update, I'm sure they would appreciate if you could pull the fixes into the update assuming it hasn't already been approved :)

@Ch1llb0
Copy link

Ch1llb0 commented Sep 27, 2018

As the v18 release is not so far off, will the v18 branch be made ready for that realease? Until now it's just alpha and depends on an un-realeased secondary script, I believe. Skins depending on this amazing script are in great need of this ;)

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

No branches or pull requests

5 participants