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

[addons/settings] Fix fallout #3 #12314

Merged
merged 4 commits into from Jun 18, 2017

Conversation

Projects
None yet
4 participants
@Montellese
Member

Montellese commented Jun 17, 2017

These commits should fix the following problems caused by #12125:

  • with my first set of fallout fixes I thought I fixed settings with hidden values (passwords etc) but I actually made it worse. This should fix it for real.
  • I noticed that localized strings in spinners weren't properly localized with add-on strings
  • I re-added the Addon.ID window property to CGUIDialogAddonSettings which was broken and reported at https://forum.kodi.tv/showthread.php?tid=316560
  • I'm sneaking in a commit which will remove code that still stored skin settings in guisettings.xml even though we migrated them to skin-specific add-on settings quite a while ago

There are still other issues that need fixing which I will cover in separate PRs because they are more complex.

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

@Montellese Montellese added this to the L 18.0-alpha1 milestone Jun 17, 2017

@Montellese Montellese changed the title from [addons/settings] Fix fallout #2 to [addons/settings] Fix fallout #3 Jun 18, 2017

@Montellese Montellese merged commit e8e4be7 into xbmc:master Jun 18, 2017

1 check passed

default You're awesome. Have a cookie
Details

@Montellese Montellese deleted the Montellese:addon_settings_fallout_3 branch Jun 18, 2017

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Jun 19, 2017

Member

@Montellese Still experiencing a reproducible crash when clicking on the icon of the popular addon "Amazon VOD" (https://github.com/Sandmann79/xbmc/tree/master/plugin.video.amazon-test):

Crash stack trace:

* thread #56, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010068a93d kodi.bin`ADDON::CAddonSettings::Load(this=0x000000012ede74b8, doc=0x000070000e733a20) at AddonSettings.cpp:307
    frame #1: 0x000000010045e736 kodi.bin`ADDON::CAddon::SettingsFromXML(this=0x000000012edb6b30, doc=0x000070000e733a20, loadDefaults=false) at Addon.cpp:344
    frame #2: 0x000000010045a4c1 kodi.bin`ADDON::CAddon::LoadUserSettings(this=0x000000012edb6b30) at Addon.cpp:171
    frame #3: 0x0000000100459ee2 kodi.bin`ADDON::CAddon::LoadSettings(this=0x000000012edb6b30, bForce=false) at Addon.cpp:131
    frame #4: 0x000000010045a98f kodi.bin`ADDON::CAddon::GetSetting(this=0x000000012edb6b30, key="playmethod") at Addon.cpp:212
    frame #5: 0x00000001011ae24b kodi.bin`XBMCAddon::xbmcaddon::Addon::getSetting(this=0x000000012edd79d0, id="playmethod") at Addon.cpp:91
    frame #6: 0x000000010013611e kodi.bin`PythonBindings::xbmcaddon_XBMCAddon_xbmcaddon_Addon_getSetting(self=0x000000012edd71a0, args=0x000000013c5ee210, kwds=0x0000000000000000) at AddonModuleXbmcaddon.i.cpp:200
    frame #7: 0x00000001035ab751 kodi.bin`PyCFunction_Call(func=0x000000013a1c45e0, arg=0x000000013c5ee210, kw=0x0000000000000000) at methodobject.c:85
    frame #8: 0x000000010364ac8c kodi.bin`call_function(pp_stack=0x000070000e734848, oparg=1) at ceval.c:4350
    frame #9: 0x0000000103646e04 kodi.bin`PyEval_EvalFrameEx(f=0x000000012d566620, throwflag=0) at ceval.c:2987
    frame #10: 0x0000000103640f15 kodi.bin`PyEval_EvalCodeEx(co=0x000000012d54c820, globals=0x000000012a007ea0, locals=0x000000012a007ea0, args=0x0000000000000000, argcount=0, kws=0x0000000000000000, kwcount=0, defs=0x0000000000000000, defcount=0, closure=0x0000000000000000) at ceval.c:3582
    frame #11: 0x000000010363fe25 kodi.bin`PyEval_EvalCode(co=0x000000012d54c820, globals=0x000000012a007ea0, locals=0x000000012a007ea0) at ceval.c:669
    frame #12: 0x000000010367c972 kodi.bin`run_mod(mod=0x000000010f5cc7c0, filename="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", globals=0x000000012a007ea0, locals=0x000000012a007ea0, flags=0x0000000000000000, arena=0x000000013c510920) at pythonrun.c:1376
    frame #13: 0x000000010367cd4f kodi.bin`PyRun_FileExFlags(fp=0x000000010a8f62b0, filename="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", start=257, globals=0x000000012a007ea0, locals=0x000000012a007ea0, closeit=1, flags=0x0000000000000000) at pythonrun.c:1362
    frame #14: 0x0000000101242629 kodi.bin`CPythonInvoker::executeScript(this=0x00000001312607f0, fp=0x000000010a8f62b0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", module=0x000000012c9a2af0, moduleDict=0x000000012a007ea0) at PythonInvoker.cpp:442
    frame #15: 0x000000010123dc17 kodi.bin`CPythonInvoker::execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at PythonInvoker.cpp:300
    frame #16: 0x0000000101059e03 kodi.bin`ILanguageInvoker::Execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at ILanguageInvoker.cpp:41
    frame #17: 0x000000010123b16d kodi.bin`CPythonInvoker::Execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at PythonInvoker.cpp:154
    frame #18: 0x000000010105aacc kodi.bin`CLanguageInvokerThread::Process(this=0x000000012d4ba770) at LanguageInvokerThread.cpp:91
    frame #19: 0x000000010105aaf9 kodi.bin`non-virtual thunk to CLanguageInvokerThread::Process(this=0x000000012d4ba770) at LanguageInvokerThread.cpp:0
    frame #20: 0x0000000101c400b0 kodi.bin`CThread::Action(this=0x000000012d4ba770) at Thread.cpp:221
    frame #21: 0x0000000101c3e729 kodi.bin`CThread::staticThread(data=0x000000012d4ba770) at Thread.cpp:131
    frame #22: 0x0000000107b90b90 libsystem_pthread.dylib`_pthread_body + 180
    frame #23: 0x0000000107b90adc libsystem_pthread.dylib`_pthread_start + 286
    frame #24: 0x0000000107b902c5 libsystem_pthread.dylib`thread_start + 13

Crashing line (newSetting is nullptr):

if (!newSetting->FromString(setting.second))

Log excerpt:

2017-06-11 20:37:48.906988+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: cannot reference setting (relative index: -100; absolute index: -95) in another category in old setting condition "eq(-100,true)" for "login_acc"
2017-06-11 20:37:48.907020+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: failed to parse enable condition "eq(-100,true)" of old setting definition for "login_acc"
2017-06-11 20:37:48.907066+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: cannot reference setting (relative index: -100; absolute index: -94) in another category in old setting condition "eq(-100,true)" for "use_mfa"
2017-06-11 20:37:48.907097+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: failed to parse visible condition "eq(-100,true)" of old setting definition for "use_mfa"
Member

ksooo commented Jun 19, 2017

@Montellese Still experiencing a reproducible crash when clicking on the icon of the popular addon "Amazon VOD" (https://github.com/Sandmann79/xbmc/tree/master/plugin.video.amazon-test):

Crash stack trace:

* thread #56, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010068a93d kodi.bin`ADDON::CAddonSettings::Load(this=0x000000012ede74b8, doc=0x000070000e733a20) at AddonSettings.cpp:307
    frame #1: 0x000000010045e736 kodi.bin`ADDON::CAddon::SettingsFromXML(this=0x000000012edb6b30, doc=0x000070000e733a20, loadDefaults=false) at Addon.cpp:344
    frame #2: 0x000000010045a4c1 kodi.bin`ADDON::CAddon::LoadUserSettings(this=0x000000012edb6b30) at Addon.cpp:171
    frame #3: 0x0000000100459ee2 kodi.bin`ADDON::CAddon::LoadSettings(this=0x000000012edb6b30, bForce=false) at Addon.cpp:131
    frame #4: 0x000000010045a98f kodi.bin`ADDON::CAddon::GetSetting(this=0x000000012edb6b30, key="playmethod") at Addon.cpp:212
    frame #5: 0x00000001011ae24b kodi.bin`XBMCAddon::xbmcaddon::Addon::getSetting(this=0x000000012edd79d0, id="playmethod") at Addon.cpp:91
    frame #6: 0x000000010013611e kodi.bin`PythonBindings::xbmcaddon_XBMCAddon_xbmcaddon_Addon_getSetting(self=0x000000012edd71a0, args=0x000000013c5ee210, kwds=0x0000000000000000) at AddonModuleXbmcaddon.i.cpp:200
    frame #7: 0x00000001035ab751 kodi.bin`PyCFunction_Call(func=0x000000013a1c45e0, arg=0x000000013c5ee210, kw=0x0000000000000000) at methodobject.c:85
    frame #8: 0x000000010364ac8c kodi.bin`call_function(pp_stack=0x000070000e734848, oparg=1) at ceval.c:4350
    frame #9: 0x0000000103646e04 kodi.bin`PyEval_EvalFrameEx(f=0x000000012d566620, throwflag=0) at ceval.c:2987
    frame #10: 0x0000000103640f15 kodi.bin`PyEval_EvalCodeEx(co=0x000000012d54c820, globals=0x000000012a007ea0, locals=0x000000012a007ea0, args=0x0000000000000000, argcount=0, kws=0x0000000000000000, kwcount=0, defs=0x0000000000000000, defcount=0, closure=0x0000000000000000) at ceval.c:3582
    frame #11: 0x000000010363fe25 kodi.bin`PyEval_EvalCode(co=0x000000012d54c820, globals=0x000000012a007ea0, locals=0x000000012a007ea0) at ceval.c:669
    frame #12: 0x000000010367c972 kodi.bin`run_mod(mod=0x000000010f5cc7c0, filename="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", globals=0x000000012a007ea0, locals=0x000000012a007ea0, flags=0x0000000000000000, arena=0x000000013c510920) at pythonrun.c:1376
    frame #13: 0x000000010367cd4f kodi.bin`PyRun_FileExFlags(fp=0x000000010a8f62b0, filename="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", start=257, globals=0x000000012a007ea0, locals=0x000000012a007ea0, closeit=1, flags=0x0000000000000000) at pythonrun.c:1362
    frame #14: 0x0000000101242629 kodi.bin`CPythonInvoker::executeScript(this=0x00000001312607f0, fp=0x000000010a8f62b0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", module=0x000000012c9a2af0, moduleDict=0x000000012a007ea0) at PythonInvoker.cpp:442
    frame #15: 0x000000010123dc17 kodi.bin`CPythonInvoker::execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at PythonInvoker.cpp:300
    frame #16: 0x0000000101059e03 kodi.bin`ILanguageInvoker::Execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at ILanguageInvoker.cpp:41
    frame #17: 0x000000010123b16d kodi.bin`CPythonInvoker::Execute(this=0x00000001312607f0, script="/Users/kai/Library/Application Support/Kodi/addons/plugin.video.amazon-test/default.py", arguments=size=3) at PythonInvoker.cpp:154
    frame #18: 0x000000010105aacc kodi.bin`CLanguageInvokerThread::Process(this=0x000000012d4ba770) at LanguageInvokerThread.cpp:91
    frame #19: 0x000000010105aaf9 kodi.bin`non-virtual thunk to CLanguageInvokerThread::Process(this=0x000000012d4ba770) at LanguageInvokerThread.cpp:0
    frame #20: 0x0000000101c400b0 kodi.bin`CThread::Action(this=0x000000012d4ba770) at Thread.cpp:221
    frame #21: 0x0000000101c3e729 kodi.bin`CThread::staticThread(data=0x000000012d4ba770) at Thread.cpp:131
    frame #22: 0x0000000107b90b90 libsystem_pthread.dylib`_pthread_body + 180
    frame #23: 0x0000000107b90adc libsystem_pthread.dylib`_pthread_start + 286
    frame #24: 0x0000000107b902c5 libsystem_pthread.dylib`thread_start + 13

Crashing line (newSetting is nullptr):

if (!newSetting->FromString(setting.second))

Log excerpt:

2017-06-11 20:37:48.906988+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: cannot reference setting (relative index: -100; absolute index: -95) in another category in old setting condition "eq(-100,true)" for "login_acc"
2017-06-11 20:37:48.907020+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: failed to parse enable condition "eq(-100,true)" of old setting definition for "login_acc"
2017-06-11 20:37:48.907066+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: cannot reference setting (relative index: -100; absolute index: -94) in another category in old setting condition "eq(-100,true)" for "use_mfa"
2017-06-11 20:37:48.907097+0200 kodi.bin[94610:2717489] Debug Print: CAddonSettings[plugin.video.amazon-test]: failed to parse visible condition "eq(-100,true)" of old setting definition for "use_mfa"
@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jun 19, 2017

Member

@ksooo I tried and wasn't able to reproduce the problem. I see the same log messages you posted about not being able to reference settings in another category but the add-on then opens perfectly fine.
Reading the code around the crash-line you posted I also don't see how newSetting can be nullptr without there being a log message about it.

I'm also not sure if I should hack in some support for these non-referenceable setting conditions because the docs on the wiki clearly state "Comparisons CANNOT be made across category/page boundaries." but I've seen a few add-ons which (try to) do it nonetheless.

Member

Montellese commented Jun 19, 2017

@ksooo I tried and wasn't able to reproduce the problem. I see the same log messages you posted about not being able to reference settings in another category but the add-on then opens perfectly fine.
Reading the code around the crash-line you posted I also don't see how newSetting can be nullptr without there being a log message about it.

I'm also not sure if I should hack in some support for these non-referenceable setting conditions because the docs on the wiki clearly state "Comparisons CANNOT be made across category/page boundaries." but I've seen a few add-ons which (try to) do it nonetheless.

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Jun 19, 2017

Member

Reading the code around the crash-line you posted I also don't see how newSetting can be nullptr without there being a log message about it.

@Montellese AddSettingWithoutDefinition gets called with an empty settingId. In this case, this member function simply returns nullptr without a log message.

Member

ksooo commented Jun 19, 2017

Reading the code around the crash-line you posted I also don't see how newSetting can be nullptr without there being a log message about it.

@Montellese AddSettingWithoutDefinition gets called with an empty settingId. In this case, this member function simply returns nullptr without a log message.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jun 19, 2017

Member

@ksooo Ah right I forget that with the old add-on settings system any (unthinkable) scenario is a possibility :-/ Could you try with 8bec277? Since we can't assign setting values without identifiers to any setting, we should simply ignore them.

Member

Montellese commented Jun 19, 2017

@ksooo Ah right I forget that with the old add-on settings system any (unthinkable) scenario is a possibility :-/ Could you try with 8bec277? Since we can't assign setting values without identifiers to any setting, we should simply ignore them.

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Jun 19, 2017

Member

@Montellese this fixes the crash, but hard to say for me which functional side effects ignoring the setting might have. What do you think is the root cause of the issue ending in a setting without an id?

Member

ksooo commented Jun 19, 2017

@Montellese this fixes the crash, but hard to say for me which functional side effects ignoring the setting might have. What do you think is the root cause of the issue ending in a setting without an id?

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Jun 19, 2017

Member

@Montellese fyi. The settings.xml causing trouble contains a line <setting id="" value="" />. No idea how this can happen, but it's actually in the file.

EDIT: And there is another one: inputstream.adaptive. Could it be that the settings migration code has/had a bug producing these faulty entries? Just a wild guess, sorry.

Member

ksooo commented Jun 19, 2017

@Montellese fyi. The settings.xml causing trouble contains a line <setting id="" value="" />. No idea how this can happen, but it's actually in the file.

EDIT: And there is another one: inputstream.adaptive. Could it be that the settings migration code has/had a bug producing these faulty entries? Just a wild guess, sorry.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jun 19, 2017

Member

Thanks for testing and looking into it. It's hard to say if the bug was introduced by the migration code but if the XML looks like <setting id="" value="" /> it's still from before the migration because afte the migration it would be <somesettingid>somevalue</somesettingid> and the existing code already rejects empty settings.
I've also checked that all code paths leading from python to the add-on settings contain early returns for empty keys/setting identifiers so it should be save to drop them.

Member

Montellese commented Jun 19, 2017

Thanks for testing and looking into it. It's hard to say if the bug was introduced by the migration code but if the XML looks like <setting id="" value="" /> it's still from before the migration because afte the migration it would be <somesettingid>somevalue</somesettingid> and the existing code already rejects empty settings.
I've also checked that all code paths leading from python to the add-on settings contain early returns for empty keys/setting identifiers so it should be save to drop them.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jun 19, 2017

Member

@ksooo btw I looked into those warnings concerning cannot reference setting for the Amazon VOD plugin and those are errors in the add-on's settings.xml. The settings.xml only specifies around 30 settings but it tries to reference a setting with a relative index of -100...

Member

Montellese commented Jun 19, 2017

@ksooo btw I looked into those warnings concerning cannot reference setting for the Amazon VOD plugin and those are errors in the add-on's settings.xml. The settings.xml only specifies around 30 settings but it tries to reference a setting with a relative index of -100...

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Jun 19, 2017

Member

The settings.xml only specifies around 30 settings but it tries to reference a setting with a relative index of -100...

Ah, okay, so nothing we should really be concerned about. This should be fixed by the addon dev, then.

Member

ksooo commented Jun 19, 2017

The settings.xml only specifies around 30 settings but it tries to reference a setting with a relative index of -100...

Ah, okay, so nothing we should really be concerned about. This should be fixed by the addon dev, then.

@MilhouseVH

This comment has been minimized.

Show comment
Hide comment
@MilhouseVH

MilhouseVH Jun 19, 2017

Contributor

Ah, okay, so nothing we should really be concerned about. This should be fixed by the addon dev, then.

But Kodi shouldn't crash because of a duff setting. I've got another example here: forum post - maybe they've all copied from each other and this issue could be repeated elsewhere.

Edit: The user is testing with a build that included this PR in build #0618. Is e31b9a8 a fix for this cannot reference setting issue? If so this is in #0619 and I'll get the user to test with that (plus contact his addon maintainer).

Contributor

MilhouseVH commented Jun 19, 2017

Ah, okay, so nothing we should really be concerned about. This should be fixed by the addon dev, then.

But Kodi shouldn't crash because of a duff setting. I've got another example here: forum post - maybe they've all copied from each other and this issue could be repeated elsewhere.

Edit: The user is testing with a build that included this PR in build #0618. Is e31b9a8 a fix for this cannot reference setting issue? If so this is in #0619 and I'll get the user to test with that (plus contact his addon maintainer).

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jun 20, 2017

Member

@MilhouseVH no argument there. It's just that the old code doesn't always do what the old docs say so it's difficult to get all the odd cases right because they either aren't documented at all or the documentation even states the opposite.

e31b9a8 is not a fix for the cannot reference setting but it fixes the crash reported by @ksooo (which has nothing to do with the other problem) but I've realized that at least in the case of Amazon VOD those errors are correct because what the add-on defines in settings.xml simply doesn't make sense. It still continues processing the settings though.

The crash reported in that forum post is known but it's very hard to fix properly. At least until now I haven't figured out how yet :-/

Member

Montellese commented Jun 20, 2017

@MilhouseVH no argument there. It's just that the old code doesn't always do what the old docs say so it's difficult to get all the odd cases right because they either aren't documented at all or the documentation even states the opposite.

e31b9a8 is not a fix for the cannot reference setting but it fixes the crash reported by @ksooo (which has nothing to do with the other problem) but I've realized that at least in the case of Amazon VOD those errors are correct because what the add-on defines in settings.xml simply doesn't make sense. It still continues processing the settings though.

The crash reported in that forum post is known but it's very hard to fix properly. At least until now I haven't figured out how yet :-/

@peak3d

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 20, 2017

Contributor

@Montellese
1.) Wouldn't it be a good idea to create a post in forum.kodi.tv that from now on all settings wich will be used has to be defined in settings.xml? After this PR its not longer allowed to add settings in e.g. python wich are not in settings.xml (this works before)

2.) Is there any reason why for each "python::GetSetting call the complete setting file is reparsed?
Wouldn't be a map for example lot more performant? If you request 20 settings from python, the file will be parsed 20 times. Its IMO far from performance optimal.

Contributor

peak3d commented Jul 20, 2017

@Montellese
1.) Wouldn't it be a good idea to create a post in forum.kodi.tv that from now on all settings wich will be used has to be defined in settings.xml? After this PR its not longer allowed to add settings in e.g. python wich are not in settings.xml (this works before)

2.) Is there any reason why for each "python::GetSetting call the complete setting file is reparsed?
Wouldn't be a map for example lot more performant? If you request 20 settings from python, the file will be parsed 20 times. Its IMO far from performance optimal.

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jul 21, 2017

Member

@peak3d:

  1. adding settings on-the-fly from python should still be possible. It simply leads to a log message so that it's easier for plugin authors to see it and maybe add a definition to the settings.xml.
  2. it should not parse the settings file for every call as long as the plugin / add-on is running. But yes there is excessive parsing of settings (and always has been but you just didn't see it in the log files) because add-on instances are constantly created and destroyed in the code. If you e.g. want to open an add-on's settings through the context menu it first creates an add-on instance for the list of add-ons, then it creates a new add-on instance for opening the context menu and then after having clicked on "Add-on settings" it creates yet another add-on instance for the add-on settings dialog. Same happens when opening the add-on info dialog. There's no way of caching the settings because new add-on instances are created all the time and from all kinds of places. This is not a flaw in the add-on settings implementation.
Member

Montellese commented Jul 21, 2017

@peak3d:

  1. adding settings on-the-fly from python should still be possible. It simply leads to a log message so that it's easier for plugin authors to see it and maybe add a definition to the settings.xml.
  2. it should not parse the settings file for every call as long as the plugin / add-on is running. But yes there is excessive parsing of settings (and always has been but you just didn't see it in the log files) because add-on instances are constantly created and destroyed in the code. If you e.g. want to open an add-on's settings through the context menu it first creates an add-on instance for the list of add-ons, then it creates a new add-on instance for opening the context menu and then after having clicked on "Add-on settings" it creates yet another add-on instance for the add-on settings dialog. Same happens when opening the add-on info dialog. There's no way of caching the settings because new add-on instances are created all the time and from all kinds of places. This is not a flaw in the add-on settings implementation.
@peak3d

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 21, 2017

Contributor

adding settings on-the-fly from python should still be possible. It simply leads to a log message so that it's easier for plugin authors to see it and maybe add a definition to the settings.xml.

Unfortunately its not only the log message, the settings are not stored any longer persistantly.
If you exit kodi and start again, these settings are not longer available. This issue confuses many addon devs in the past few weeks, it affects as well major addons like youtube. @Paxxi took already a closer look into it and can maybe give a more technical explanation.

2.) This is a back trace from a python "GetSetting" call:

kodi.exe!CSettingsManager::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & id) Zeile 556  C++
    kodi.exe!CSettingsBase::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & id) Zeile 179 C++
    kodi.exe!ADDON::CAddonSettings::Load(const CXBMCTinyXML & doc) Zeile 303    C++
    kodi.exe!ADDON::CAddon::SettingsFromXML(const CXBMCTinyXML & doc, bool loadDefaults) Zeile 346  C++
    kodi.exe!ADDON::CAddon::LoadUserSettings() Zeile 173    C++
    kodi.exe!ADDON::CAddon::LoadSettings(bool bForce, bool loadUserSettings) Zeile 133  C++
    kodi.exe!ADDON::CAddon::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & key) Zeile 214    C++
    kodi.exe!XBMCAddon::xbmcaddon::Addon::getSetting(const char * id) Zeile 92  C++
    kodi.exe!PythonBindings::xbmcaddon_XBMCAddon_xbmcaddon_Addon_getSetting(PythonBindings::PyHolder * self, _object * args, _object * kwds) Zeile 200  C++
    python27.dll!0ff0ccc7() Unbekannt
    [Unten angegebene Rahmen sind möglicherweise nicht korrekt und/oder fehlen, keine Symbole geladen für python27.dll]   
    python27.dll!0ff61ac2() Unbekannt
    python27.dll!0ff5f420() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0fefd079() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0fee07aa() Unbekannt
    [Externer Code] 
    python27.dll!0feeb973() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff5c6aa() Unbekannt
    python27.dll!0fee1836() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff6209e() Unbekannt
    python27.dll!0ff61b2d() Unbekannt
    python27.dll!0ff5f420() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0ff5c70f() Unbekannt
    python27.dll!0ff7820c() Unbekannt
    python27.dll!0ff7be48() Unbekannt
    python27.dll!0ff7b6ec() Unbekannt
    python27.dll!0ff7b1b2() Unbekannt
    python27.dll!0ff7b972() Unbekannt
    python27.dll!0ff7aec0() Unbekannt
    python27.dll!0ff7886d() Unbekannt
    python27.dll!0ff5783a() Unbekannt
    python27.dll!0ff0ccc7() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff5c6aa() Unbekannt
    python27.dll!0ff5efcf() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0ff5c70f() Unbekannt
    python27.dll!0ff962ce() Unbekannt
    python27.dll!0ff940f8() Unbekannt
    kodi.exe!CPythonInvoker::executeScript(void * fp, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, void * module, void * moduleDict) Zeile 443    C++
    kodi.exe!CPythonInvoker::execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 301   C++
    kodi.exe!ILanguageInvoker::Execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 40  C++
    kodi.exe!CPythonInvoker::Execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 156   C++
    kodi.exe!CLanguageInvokerThread::Process() Zeile 91 C++
    kodi.exe!CThread::Action() Zeile 221    C++
    kodi.exe!CThread::staticThread(void * data) Zeile 134   C++
    [Externer Code] 

Isn't this: kodi.exe!ADDON::CAddonSettings::Load(const CXBMCTinyXML & doc) Zeile 303 C++
Loadng settings again?

Contributor

peak3d commented Jul 21, 2017

adding settings on-the-fly from python should still be possible. It simply leads to a log message so that it's easier for plugin authors to see it and maybe add a definition to the settings.xml.

Unfortunately its not only the log message, the settings are not stored any longer persistantly.
If you exit kodi and start again, these settings are not longer available. This issue confuses many addon devs in the past few weeks, it affects as well major addons like youtube. @Paxxi took already a closer look into it and can maybe give a more technical explanation.

2.) This is a back trace from a python "GetSetting" call:

kodi.exe!CSettingsManager::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & id) Zeile 556  C++
    kodi.exe!CSettingsBase::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & id) Zeile 179 C++
    kodi.exe!ADDON::CAddonSettings::Load(const CXBMCTinyXML & doc) Zeile 303    C++
    kodi.exe!ADDON::CAddon::SettingsFromXML(const CXBMCTinyXML & doc, bool loadDefaults) Zeile 346  C++
    kodi.exe!ADDON::CAddon::LoadUserSettings() Zeile 173    C++
    kodi.exe!ADDON::CAddon::LoadSettings(bool bForce, bool loadUserSettings) Zeile 133  C++
    kodi.exe!ADDON::CAddon::GetSetting(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & key) Zeile 214    C++
    kodi.exe!XBMCAddon::xbmcaddon::Addon::getSetting(const char * id) Zeile 92  C++
    kodi.exe!PythonBindings::xbmcaddon_XBMCAddon_xbmcaddon_Addon_getSetting(PythonBindings::PyHolder * self, _object * args, _object * kwds) Zeile 200  C++
    python27.dll!0ff0ccc7() Unbekannt
    [Unten angegebene Rahmen sind möglicherweise nicht korrekt und/oder fehlen, keine Symbole geladen für python27.dll]   
    python27.dll!0ff61ac2() Unbekannt
    python27.dll!0ff5f420() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0fefd079() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0fee07aa() Unbekannt
    [Externer Code] 
    python27.dll!0feeb973() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff5c6aa() Unbekannt
    python27.dll!0fee1836() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff6209e() Unbekannt
    python27.dll!0ff61b2d() Unbekannt
    python27.dll!0ff5f420() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0ff5c70f() Unbekannt
    python27.dll!0ff7820c() Unbekannt
    python27.dll!0ff7be48() Unbekannt
    python27.dll!0ff7b6ec() Unbekannt
    python27.dll!0ff7b1b2() Unbekannt
    python27.dll!0ff7b972() Unbekannt
    python27.dll!0ff7aec0() Unbekannt
    python27.dll!0ff7886d() Unbekannt
    python27.dll!0ff5783a() Unbekannt
    python27.dll!0ff0ccc7() Unbekannt
    python27.dll!0fed1ac6() Unbekannt
    python27.dll!0ff5c6aa() Unbekannt
    python27.dll!0ff5efcf() Unbekannt
    python27.dll!0ff5cded() Unbekannt
    python27.dll!0ff5c70f() Unbekannt
    python27.dll!0ff962ce() Unbekannt
    python27.dll!0ff940f8() Unbekannt
    kodi.exe!CPythonInvoker::executeScript(void * fp, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, void * module, void * moduleDict) Zeile 443    C++
    kodi.exe!CPythonInvoker::execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 301   C++
    kodi.exe!ILanguageInvoker::Execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 40  C++
    kodi.exe!CPythonInvoker::Execute(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & script, const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & arguments) Zeile 156   C++
    kodi.exe!CLanguageInvokerThread::Process() Zeile 91 C++
    kodi.exe!CThread::Action() Zeile 221    C++
    kodi.exe!CThread::staticThread(void * data) Zeile 134   C++
    [Externer Code] 

Isn't this: kodi.exe!ADDON::CAddonSettings::Load(const CXBMCTinyXML & doc) Zeile 303 C++
Loadng settings again?

@Montellese

This comment has been minimized.

Show comment
Hide comment
@Montellese

Montellese Jul 21, 2017

Member
  1. ok I didn't know that it isn't working properly yet. I'll have a look when I get back from vacation in August.
  2. CAddon::GetSetting() always calls CAddon::LoadSettings() to make sure that the settings are loaded. But CAddon::LoadSettings() should take care of not reading the settings definition again if it has already been read. Maybe something isn't working properly there. I'll have a look as well.
Member

Montellese commented Jul 21, 2017

  1. ok I didn't know that it isn't working properly yet. I'll have a look when I get back from vacation in August.
  2. CAddon::GetSetting() always calls CAddon::LoadSettings() to make sure that the settings are loaded. But CAddon::LoadSettings() should take care of not reading the settings definition again if it has already been read. Maybe something isn't working properly there. I'll have a look as well.
@peak3d

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 21, 2017

Contributor

Ok, thanx, enjoy your vacation!

Contributor

peak3d commented Jul 21, 2017

Ok, thanx, enjoy your vacation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment