script.skinshortcuts icon indicating copy to clipboard operation
script.skinshortcuts copied to clipboard

Branch script for Kodi 18

Open ghost opened this issue 8 years ago • 21 comments

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?

ghost avatar Dec 17 '16 18:12 ghost

@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.

BigNoid avatar Dec 17 '16 21:12 BigNoid

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 :)

ghost avatar Dec 17 '16 21:12 ghost

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.

smitchell6879 avatar Dec 18 '16 06:12 smitchell6879

@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.

braz96 avatar Dec 19 '16 01:12 braz96

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

marcelveldt avatar Dec 30 '16 09:12 marcelveldt

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)

ghost avatar Dec 30 '16 09:12 ghost

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.

BigNoid avatar Dec 30 '16 10:12 BigNoid

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

marcelveldt avatar Dec 30 '16 10:12 marcelveldt

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

BigNoid avatar Dec 30 '16 10:12 BigNoid

@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 avatar Jan 05 '17 01:01 ghost

@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)

ghost avatar Jan 13 '17 23:01 ghost

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.

BigNoid avatar Jan 27 '17 14:01 BigNoid

@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 ?

marcelveldt avatar Feb 09 '17 10:02 marcelveldt

@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 avatar Feb 16 '17 17:02 ghost

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

ghost avatar Feb 16 '17 23:02 ghost

@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 ?

marcelveldt avatar Feb 20 '17 08:02 marcelveldt

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

ghost avatar Feb 24 '17 19:02 ghost

@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 avatar Mar 20 '17 21:03 marcelveldt

@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.

marcelveldt avatar Mar 20 '17 21:03 marcelveldt

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 :)

ghost avatar Mar 25 '17 01:03 ghost

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 ;)

Ch1llb0 avatar Sep 27 '18 08:09 Ch1llb0