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

[add-ons/settings] migrate add-on settings to settings library #12125

Merged
merged 33 commits into from Jun 3, 2017

Conversation

@Montellese
Copy link
Member

commented May 18, 2017

Description

So this is it. I've been "dreaming" of this final step ever since I worked on the original settings rework in core but it turned out to be much more complicated than expected which is also why it took so long. This PR introduces (almost) all the necessary bits to be able to use the same settings system for add-ons that we already in core. Furthermore it contains a backwards compatibility layer which reads in old settings definitions (and values) and translates those into settings from the new system. After this PR settings will always be saved in the same format as guisettings.xml but it can read the settings definition either from the old add-on settings format or from the "new" core settings system format.

Since I'm not that much of an add-on user myself and since I've never written an add-on myself I'm pretty sure I missed a few things in the backwards compatibility layer. So it would be greatly appreciated if people with more experience in this area could give this a try. But beware once Kodi has written the setting values in the new format you can't go back to the old format without losing all your add-on settings. So best make a backup of your add-on data before giving this a try.

There are a few parts that are known not to be backwards compatible but I've tried to add log messages in those places so that it becomes obvious. I hope that those cases are edge cases that can live without backwards compatibility but who knows.

There are also parts that I wasn't able to test like binary add-ons defining settings through the binary add-on API since I don't know if and which add-on is using this at all.

In the end the major benefit is that GUIDialogAddonSettings has become very simple because it can derive from the existing settings related base classes. The major part of the work is in CAddonSettings which also contains the backwards compatibility layer which we can hopefully drop someday (a few releases into the future).

I'm not sure if we want to "mark" the old add-on settings approach as deprecated to try and get add-ons to adopt to the new approach or not.

Some (basically the first 25) of the commits could go into a separate PR (as I've already done with a few fixes before) but since they didn't bring any real benefit to the existing code I didn't do that yet. If someone would like me to do that to make reviewing easier I'm fine with that. Just let me know.

Part of this work also overlaps with my media importing work (CSettingsBase et al.) and should make things a bit easier there as well code-wise.

How Has This Been Tested?

Locally by installing some add-ons, opening their add-on settings and comparing the result to before.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Just awesome :)
I'd say mark old settings as deprecated and get v18 Addons on the new system.

return true;
}

void CAddonSettings::FileEnumSetingOptionsFiller(std::shared_ptr<const CSetting> setting, std::vector< std::pair<std::string, std::string> > &list, std::string &current, void *data)

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch May 18, 2017

Member

Maybe a small typo :) "tt"

This comment has been minimized.

Copy link
@Montellese

Montellese May 18, 2017

Author Member

Thanks and fixed.

@Montellese Montellese force-pushed the Montellese:addon_settings branch from 5f4aad1 to 177d032 May 18, 2017

@AlwinEsch

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Has looked over them, is really big :) From my side looks it good.

Just a question of understanding. Are the shared pointers present all the time or are them released after usage?

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 18, 2017

Well as long as the CSettingsManager instance which created the settings is alive the (shared) pointers will be alive as well. Nothing changes in that matter.

But to be honest I've noticed that add-on settings are created and destroyed waaaaaay too often because add-on instances are created and destroyed waaaaaay too often. But I didn't wanna go down that rabbit hole :-/

@AlwinEsch

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I know :-D that's also part of my rework to reduce them.

Addon becomes loaded and stays loaded as long it is used if now a new instance becomes used is still the loaded present and not need to do all the work.

@Montellese Montellese force-pushed the Montellese:addon_settings branch from 177d032 to a6871fa May 19, 2017

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

I forgot to mention that there are two known backwards incompatibilities (both marked with a TODO in the code):

  • it's not possible to specify that the add-on settings dialog should close after having edited a setting. I don't really know what the intention / purpose behind this is / was.
  • it's not possible to specify multiple add-on types for an add-on setting. but then again I couldn't find any add-on using this.

@Montellese Montellese force-pushed the Montellese:addon_settings branch from a6871fa to c025eb2 May 19, 2017

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Doesn't sound like a problem

@@ -684,6 +695,26 @@ bool CSettingsOperations::SerializeSettingAddon(const CSettingAddon* setting, CV
return true;
}

bool CSettingsOperations::SerializeSettingDate(const CSettingDate* setting, CVariant &obj)

This comment has been minimized.

Copy link
@notspiff

notspiff May 19, 2017

Contributor

just because it piqued my curiosity; why isn't this done as CSettingDate::Serialize ?

This comment has been minimized.

Copy link
@Montellese

Montellese May 19, 2017

Author Member

You mean add the JSON serialization to the CSettingFoo classes? Right now those classes only have deserialization logic to read the setting definition from XML. This would be serializing the setting definition to JSON. So it might be a bit confusing.

Furthermore any changes to the JSON serialization implementation could require changes to the JSON-RPC API schema. That is more obvious if the serialization code is located with the rest of the JSON-RPC API code.

But I agree that it's not the cleanest solution.

This comment has been minimized.

Copy link
@notspiff

notspiff May 19, 2017

Contributor

makes perfect sense, thanks for explaining.

This comment has been minimized.

Copy link
@notspiff

notspiff May 19, 2017

Contributor

follow up question though: is the xml really of any value here or could you use json on disk as well? i doubt many ever edit these by hand.

This comment has been minimized.

Copy link
@Montellese

Montellese May 19, 2017

Author Member

It shouldn't be too hard to write the setting definitions in JSON instead of XML and write a parser for JSON. Now (after all the GUI integration rewriting) it would probably even be possible to write such a parser outside of the whole settings library. That's basically what I did for the backwards compatibility layer. Maybe it would be worth extracting all the deserialization logic from the settings library and leave that up to the "integrator". There might be some tricky business around being able to overwrite setting definitions e.g. per platform but it should all be doable.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 21, 2017

Needs a rebase.

  • I never knew we could close the dialog after editing a setting so i doubt any one used that.
  • Same for the multiple addon types for settings. Maybe it never got finished?

Edit:
Also fails on android http://jenkins.kodi.tv/job/Android-ARM/4059/console
and R-Pi http://jenkins.kodi.tv/job/LINUX-RBPI/3663/console

@Montellese Montellese force-pushed the Montellese:addon_settings branch from c025eb2 to 33405b1 May 21, 2017

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 22, 2017

jenkins build this please

@AlwinEsch AlwinEsch referenced this pull request May 23, 2017
0 of 9 tasks complete

@Montellese Montellese force-pushed the Montellese:addon_settings branch 4 times, most recently from 3021828 to ccd0d4c May 24, 2017

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

Should now also build on Linux and on win32 with tests.

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

jenkins build this please

@Montellese Montellese force-pushed the Montellese:addon_settings branch from ccd0d4c to 256f3e8 May 25, 2017

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

jenkins build this please

@Montellese Montellese force-pushed the Montellese:addon_settings branch from 256f3e8 to da36461 May 25, 2017

@Montellese

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 25, 2017

All green :)
Go for it

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone May 25, 2017

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

@Montellese One more thing, the Default Category Button (id=10) texture isn't fading when the Categories Area grouplist loses focus.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2017

@Montellese The above problem and scrollbar problem started with this commit 12245 - previous to this they worked correctly.

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

@HitcherUK: that's odd because that PR doesn't touch the GUI presentation at all.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2017

I'll try to find the exact day it changed but it's easy enough to test using Estuary by going to any system settings window and noting the category button and scrollbars behaviour before and after.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2017

Working fine - KodiSetup-20170604-49ad3bb2a6-master-x86.exe 79M 2017-Jun-05 04:35
Broken category button and scrollbars - KodiSetup-20170606-b196817-master-x86.exe 79M 2017-Jun-07 04:50

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

@HitcherUK: it could also be due to #12213 which has been merged between those two builds.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jun 25, 2017

@Montellese any thoughts on how the music scraper settings functionality can be restored? Have you been able to reproduce the issue, or do you need more from me?

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

@DaveTBlake I'm looking into it now (though with video scrapers because I don't use the music library).

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jun 25, 2017

Great. Not using the video lib I wasn't sure if it had the same issue or not, but suspected that it might (if scraper settings are content dependant and settings get fetched from the db). If I can help let me know.

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

@DaveTBlake can you check whether the two commits in https://github.com/Montellese/xbmc/tree/addon_settings_fix_scrapers solve the problem for you as well?

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jun 25, 2017

@Montellese yes, that fixes the music scraper settings issue I was seeing, thanks.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

@Montellese if I understand the introduction correctly "old" settings are transformed to "new" settings and after that the new settings should be used for further kodi read ops.

11:17:10.727 T:2452   DEBUG: CAddonSettings[plugin.video.netflix]: failed to find definition for setting setting. Creating a setting on-the-fly...
11:17:10.729 T:2452   DEBUG: CAddonSettings[plugin.video.netflix]: loading setting definitions
11:17:10.729 T:2452   DEBUG: CAddonSettings[plugin.video.netflix]: trying to load setting definitions from old format...
11:17:10.732 T:2452   DEBUG: CAddonSettings[plugin.video.netflix]: loading setting values
11:17:10.732 T:2452   DEBUG: CSettingsManager: requested setting (setting) was not found.

Tose lines are flooding my log file and it does not seem that the new settings file is written.
Just a misunderstanding? Do I have to tell the addon-dev to change the settings.xml file?

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2017

@peak3d the add-on settings system has a legacy parser which reads the old settings XML and turns it into new settings object in the code. But it does not serialize that into a new settings XML file (because we don't have a serializer for that). It also reads the old settings XML values but stores those in the new settings XML value format. So as long as add-on authors don't update their add-on with a new settings.xml you will see those debug messages.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented on xbmc/addons/settings/AddonSettings.cpp in 5cfbfe0 Aug 3, 2017

this produces annoying log spam: example: http://paste.ubuntu.com/25228685/

@FernetMenta

This comment has been minimized.

Copy link
Member

commented on xbmc/addons/settings/AddonSettings.cpp in 5cfbfe0 Aug 3, 2017

log spam, please remove or move to some kind of component logging

manuelm added a commit to manuelm/pvr.dvbviewer that referenced this pull request Aug 15, 2017
@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

@Montellese I noticed that there are still many settings in guisettings.xml in the old format. Is there any plan for these to be updated to v2? Does CMediaSettings also need some kind of settings migration?

@basrieter

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2017

I still get a lot of warnings on the latest nightly for my add-on and as a result a lot of missing visibility settings: https://pastebin.com/H6cDWW7q

The settings.xml can be found here: https://pastebin.com/dymVsvUt

Any thing I can do to help out with that?

@Montellese

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2017

Hey guys, sorry I'm pretty slammed with real life right now and probably for a while still so I can't make any promises when I'll get around to look into this.

@DaveTBlake: Those are settings that are not controlled by the settings system but are manually written by different classes derived from ISubSettings. We simply provide them with the XML document / tree and they are free to fill it with whatever they want.

@Montellese Montellese deleted the Montellese:addon_settings branch Oct 22, 2017

@garbear

This comment has been minimized.

Copy link
Member

commented on xbmc/settings/Settings.cpp in a2b4efe Jan 25, 2018

@Montellese did you swap UnregisterSettingsHandler for UnregisterCallback here?

This comment has been minimized.

Copy link
Member Author

replied Apr 5, 2018

That looks wrong :-/

@reddit-reaper

This comment has been minimized.

Copy link

commented Feb 6, 2019

@Montellese The above problem and scrollbar problem started with this commit 12245 - previous to this they worked correctly.

just letting you know, looks like this is back. i heard somewhere that if the category has more than 100 elements it will cause this issue. is this true? because after a certain point in this addons settings it just stops and you have to go in reverse to get past it or use the mouse wheel to scroll painfully slow through it

@basrieter

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

This is true. 100 settings per category is the current limit.

There is a PR that pushes the limit to 512: #14154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.