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

[addon] support to set binary add-on instance settings by python #23648

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AlwinEsch
Copy link
Member

@AlwinEsch AlwinEsch commented Aug 21, 2023

Description

This allow a python script to edit add-on instance settings where used within PVR add-ons.

To prevent conflicts by user set within Kodi's GUI are the by python created ones starting with id >1000.

Motivation and context

How has this been tested?

By a small python script has it works for tests.

import xbmcaddon
import xbmcgui
 
addon       = xbmcaddon.Addon('pvr.demo')
addonname   = addon.getAddonInfo('name')

newid = addon.getFreeNewInstanceId()
addonsettingnew = addon.getSettings(newid)
addonsettingnew.setString('host', '10.144.12.13')
addon.setInstanceIdState(newid, True, 'By Python added')

# addon.deleteInstanceId(1001);

printstring = "Support Instances: " + str(addon.supportsInstanceSettings()) + "\n" \
              "Used Instances:\n"
for x in addon.getKnownInstanceIds():
    printstring += "ID: " + str(x) + "\n"
    printstring += "Test setting: " + addon.getSettings(x).getString('host') + "\n"

xbmcgui.Dialog().textviewer(addonname, printstring)

If the instance already present is it enough to call:

id = 1001
instancesetting = addon.getSettings(id)
instancesetting.setString('host', '10.144.12.13')

By this the add-on becomes the ADDON_STATUS SetInstanceSetting(const std::string& settingName, const kodi::addon::CSettingValue& settingValue) call.

What is the effect on users?

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

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

throw XBMCAddon::WrongTypeException("Invalid setting type");

if (value)
CServiceBroker::GetAddonMgr().PublishInstanceAdded(pAddon->ID(), instance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this callback to the PVR add-on? It is important that it does as otherwise the add-on cannot handle settings changes. Ideally each changed setting would callback to SetInstanceSetting() for the add-on implementing the interface kodi::addon::IAddonInstance.

IT's also possible this happens for free as part of the regular settings interface.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this callback to the PVR add-on? It is important that it does as otherwise the add-on cannot handle settings changes. Ideally each changed setting would callback to SetInstanceSetting() for the add-on implementing the interface kodi::addon::IAddonInstance.

IT's also possible this happens for free as part of the regular settings interface.

The callback mainly works to the CPVRClients, where then call the PVR add-ons and only needed on add or disable/enable. SetInstanceSetting() now becomes called if instance already know on add-on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, this should mean that the dynamic settings in pvr.iptvsimple should work ok now.

@matthuisman can you build this PR yourself? Or will I build it for you? I'd like you to test this to make sure this functionality is working again in Omega. Thanks in advance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phunkyfish
i dont have the env setup to create builds sorry. If you can get me an x86_64 windows I can test.
We want to make sure that we can force IPTV Simple to reload its playlists right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phunkyfish i dont have the env setup to create builds sorry. If you can get me an x86_64 windows I can test. We want to make sure that we can force IPTV Simple to reload its playlists right?

This mainly to change the use state about the instance, e.g. the disable/enable. Normally should the add-on handle setting changes by self, if he see a change within in his settings. But optionally can the python also do a hacky disable and enable again to force a reload.

@matthuisman
Copy link
Contributor

This looks like it should solve #22779

@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch from 373d36d to 2099ec8 Compare August 23, 2023 10:37
@AlwinEsch
Copy link
Member Author

AlwinEsch commented Aug 23, 2023

This looks like it should solve #22779

Yes this should fix the issue.

@phunkyfish have updated, should be now a bit better.

The kodi_addon_instance_enabled and kodi_addon_instance_name becomes no more set direct on addon and use the function bool Addon::setInstanceIdState(unsigned int instance, bool enabled, const String& name/* = emptyString*/)

The setInstanceIdState must be then called if the instance is added new or enabled/disabled, without becomes more hard to make the works to PVRManager to set it.

Also found here a small bug as the instance id not given to it:

return m_addon->SaveSettings();

xbmc/addons/IAddon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Addon.h Outdated Show resolved Hide resolved
@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch from 2099ec8 to fb1346d Compare August 23, 2023 12:01
@AlwinEsch
Copy link
Member Author

Thanks a lot @enen92! Have updated the text, also removed the spaces on the value text, added they some years ago, but makes not really sense to use.

@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch 2 times, most recently from cea05b4 to 6cef00c Compare August 23, 2023 13:17
@AlwinEsch
Copy link
Member Author

AlwinEsch commented Aug 23, 2023

Have added now also a second commit where allow it within old setting functions, not sure to make, as they are set as deprecated.

Further can the python add-on open the instance settings dialog direct.

}

String Addon::getSetting(const char* id)
String Addon::getSetting(const char* id, unsigned int instance /* = 0*/)
{
return pAddon->GetSetting(id);
Copy link
Member Author

@AlwinEsch AlwinEsch Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: Have forgot to give "instance" here, but not updated now, as the support there maybe becomes removed again.

@emveepee
Copy link
Contributor

The kodi_addon_instance_enabled and kodi_addon_instance_name becomes no more set direct on addon and use the function bool Addon::setInstanceIdState(unsigned int instance, bool enabled, const String& name/* = emptyString*/)

Will this remove the confusing debug message when the resource/instance-setting.xml is read here since it shouldn't have these values. https://github.com/xbmc/xbmc/blob/8de133330cc47e1d4809f271118b8ac2fb3701eb/xbmc/addons/settings/AddonSettings.cpp#L226C55-L226C55

2023-08-22 20:55:54.035 T:22324   debug <CSettingsManager>: requested setting (kodi_addon_instance_name) was not found.
2023-08-22 20:55:54.036 T:22324   debug <CSettingsManager>: requested setting (kodi_addon_instance_enabled) was not found.

@AlwinEsch
Copy link
Member Author

AlwinEsch commented Aug 24, 2023

The kodi_addon_instance_enabled and kodi_addon_instance_name becomes no more set direct on addon and use the function bool Addon::setInstanceIdState(unsigned int instance, bool enabled, const String& name/* = emptyString*/)

Will this remove the confusing debug message when the resource/instance-setting.xml is read here since it shouldn't have these values. https://github.com/xbmc/xbmc/blob/8de133330cc47e1d4809f271118b8ac2fb3701eb/xbmc/addons/settings/AddonSettings.cpp#L226C55-L226C55

2023-08-22 20:55:54.035 T:22324   debug <CSettingsManager>: requested setting (kodi_addon_instance_name) was not found.
2023-08-22 20:55:54.036 T:22324   debug <CSettingsManager>: requested setting (kodi_addon_instance_enabled) was not found.

Ah, thanks. Yes this comes on Debug some times and not needed.
I change the Settings function to this and to ignore on some cases:

  /*!
   \brief Gets the setting with the given identifier.

   \param id Setting identifier
   \param[in] notPresentPossible If is set, error messages are ignored because the given setting
                                 might not actually exist.
                                 Can refer to background settings related to Kodi, which may be
                                 added later, e.g. to an add-on.
   \return Setting object with the given identifier or nullptr if the identifier is unknown
   */
  std::shared_ptr<CSetting> GetSetting(const std::string &id, bool notPresentPossible = false) const;

@emveepee
Copy link
Contributor

  std::shared_ptr<CSetting> GetSetting(const std::string &id, bool notPresentPossible = false) const;

Thanks this has bugged me since I first saw them. Perhaps in a future PR this could lead new to new settings type for removed variables, instead of having to hide them.

@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch from 6cef00c to ad14c91 Compare August 24, 2023 17:37
@AlwinEsch AlwinEsch changed the title [WIP][addon] support to set binary add-on instance settings by python [addon] support to set binary add-on instance settings by python Aug 24, 2023
@AlwinEsch AlwinEsch removed the WIP PR that is still being worked on label Aug 24, 2023
@AlwinEsch
Copy link
Member Author

Updated it, should work now, had recently thrown a bug where name and enable/disable were not saved.

I have removed my changes in the old python API because they are deprecated.

Setting the settings is now also possible in the Python addon class directly via CAddonSettings and no longer via the add-on class (because this way is also used in the new python settings API).

From my side it should be ready, which is why I removed the WIP. Take a look and say what you think.

xbmc/settings/SettingsBase.h Outdated Show resolved Hide resolved
xbmc/settings/lib/SettingsManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/settings/AddonSettings.cpp Outdated Show resolved Hide resolved
/// settings.setInt('m3uPathType', 0)
/// settings.setString('m3uPath', '/SOME_PATH_OF_YOU')
/// ...
/// addon.setInstanceIdState(newinstanceid, True, 'Example by Python added')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this string meaningful for any actual feature or is it just some optional string?
I feel it is just used for some logging or so. If that's the case I'd suggest to drop it. It should be the add-on in charge of logging stuff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above this:

      /// @param name [opt] string or unicode - The instance name used inside Kodi
      ///             @warning Needs on new added instances set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it by the screenshots you posted. Maybe rename the example as "Example instance created from python" (reads a bit better)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @warning Needs on new added instances set. -> "Required when creating a new settings instance" ?

bool CAddon::GetKodiInstanceSetting(AddonInstanceId id,
bool& enabled,
std::string& name,
bool presenceValidated)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the presenceValidated mandatory? Why not return false if the setting does not exist?
I'd say it makes the api a bit cumbersome for no reason

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove as value, thought only for cases if in future this becomes called on place where needs the presence of settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove if possible

/// ~~~~~~~~~~~~~
///
getFreeNewInstanceId();
#else
Copy link
Member

@enen92 enen92 Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Why not simply id = addon.Instance()?
Cleaner

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats as addon.instance() I see really confusing, as I expect then a handling class and not an identifier as return.
Maybe another, but has not found a good name for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, maybe createInstance() ?

@phunkyfish
Copy link
Contributor

Will these new methods work for addons using single settings? Be nice if we can use the same methods regardless of single/multiple instances.

Can you give an example of how this would work?

@matthuisman
Copy link
Contributor

matthuisman commented Sep 6, 2023

getKnownInstanceIds returns a single [ID] that represents the single settings
So could be used for an old or new version of an addon with/without multiple instances.

This may already be the case :)

Not a huge deal to be honest. just makes the calling code a bit easier

Im happy enough to do:

ids = addon.getKnownInstanceIds()
settings = addon.getSettings(id[0] if ids else None)

@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Sep 7, 2023

i cant review because from first implementation in kodi of "multiple instances binary addons" i never understand his purpose and how works and if it works for PVR only or all binary, then with the scripts in the description its not clear to me what are you doing..
would be better having a wiki page (or exists already?) that explain what this is this and how works and for what uses / how implement and/or a sample project

@AlwinEsch
Copy link
Member Author

AlwinEsch commented Sep 7, 2023

i cant review because from first implementation in kodi of "multiple instances binary addons" i never understand his purpose and how works and if it works for PVR only or all binary, then with the scripts in the description its not clear to me what are you doing.. would be better having a wiki page (or exists already?) that explain what this is this and how works and for what uses / how implement and/or a sample project

The instances really only refer to binary add-ons and currently only used in PVR with independent settings of it. The background to Python is that an add-on may be able to change the settings of a PVR add-on. E.g. in pvr.iptvsimple where a python script can then set everything suitable for multiple PVR stream backend servers by adding these necessary instances with the associated settings to the binary add-on.

@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Sep 7, 2023

yes i more or less understand that script above change some settings of an "instance" (you create always a new one?)
but i still don't have a complete background on how "multiple instances binary addons" works,
as for example, who/where create these addon instances, how run/stop them, they are indipendent like a service? and then has nothing to do with media playback? so a wiki would be better, an external user dont understand nothing, unlikely that anyone would use this feature

@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch from c201c98 to 259bb1e Compare September 7, 2023 06:32
@AlwinEsch
Copy link
Member Author

AlwinEsch commented Sep 7, 2023

getKnownInstanceIds returns a single [ID] that represents the single settings So could be used for an old or new version of an addon with/without multiple instances.

This may already be the case :)

Not a huge deal to be honest. just makes the calling code a bit easier

Im happy enough to do:

ids = addon.getKnownInstanceIds()
settings = addon.getSettings(id[0] if ids else None)

Hi @matthuisman is not directly so easy, a add-on can still have both, within id 0 are general settings of add-on where relates to all instances on it. All numbers higher 0 then relates to instance only.

The settings = addon.getSettings(id[0] if ids else None) can be used, only then limited to first available instance and can not directly identify if it is by user set or from python script. Also then works like before, as there no instances supported.

yes i more or less understand that script above change some settings of an "instance" (you create always a new one?) but i still don't have a complete background on how "multiple instances binary addons" works, as for example, who/where create these addon instances, how run/stop them, they are indipendent like a service? and then has nothing to do with media playback? so a wiki would be better, an external user dont understand nothing, unlikely that anyone would use this feature

Hello @CastagnaIT, yes you right, there should be some documentations about to know what exactly is the background about. Will look in next time what would be the best about and to add documentations.
About "(you create always a new one?), not directly. Normally the user add in the settings dialog one or more instances and edit they to match related backends. This are then selectable and editable within a dialog.

Screenshot_20230907_085330

The "Edit add-on settings" there relates as button to global general settings of PVR add-on (only visible if add-on have a standard "settings.xml", they below are the independent instance settings on the add-on, the last added by the python script.
The both buttons above are for the user to add or remove a instance.

Python can then with his new API then add, delete or edit all instances, they from user or they added from him in script.

About "they are indipendent like a service?" not directly, the add-on is like a app, it runs all instances independent from each other in own processes, but everything in same base and can if needed also interact between (the PVR add-on only loaded one time).

#ifdef DOXYGEN_SHOULD_USE_THIS
///
/// \ingroup python_xbmcaddon
/// @brief \python_func{ xbmcaddon.Addon([id]).setInstanceState(id, enabled, [name]) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name set when change the state on the instance and not right when the instance is created? Can it be moved to getFreeNewInstanceId()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a big hen and egg problem, by getFreeNewInstanceId() is and can the instance not created, as the needed settings for it not available and PVR system then start works where always ends in errors, thats why it only give a free id.

The fields about "enabled" and "name" are parts only handled by Kodi itself, about "enabled" is easy to understand why, about name relates to the name used on the GUI in Kodi (see picture above).

The setInstanceState needs be called on new added instances, thats why name needed on it, or can be called too on already present ones to change name and/or enable / disable it, thats why name declared optional.

Maybe the function can become another name. Have you a idea about?


Another solution can be, but then it becomes hacky and needs more functions and much changes to add:

Function Hacky description
settings = addon.createInstanceSettings(enabled, name) It returns the settings class and not an id
- The enabled and name needs temporary stored somewhere
- access to CSettingsBase in Kodi no more enough and needs access to CAddonSettings as this handle the id.
settings.setAddedInstanceReady() or
addon.setAddedInstanceReady(settings)
Something needs always done to report Kodi that new instance is ready to start and thats PVR system does it

Maybe it can optionally also be done by the settings class destructor, but then Python needs to do destruct it and never stores it somewhere.
id = settings.getInstanceId() New function then needed for the cases if python needs the id byself to store something about for him himself or for the future works.
addon.setInstanceState(id, enabled) To allow on already present ones the enable / disable
addon.setInstanceName(id, name) To allow on already present ones the name change
state = settings.getInstanceState() and
state = addon.getInstanceState(id)
For the sake of form, to have the counterpart to the “set”.
name = settings.getInstanceName() and
name = addon.getInstanceName(id)
For the sake of form, to have the counterpart to the “set”.

About this it seems then also more hard to understand what does somewhere what.

@Lunatixz Lunatixz self-requested a review October 5, 2023 19:48
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 6, 2023
…d either

Previously, there were always calls to Kodi's own add-on instance settings in the debug log, since these cannot be known anyway, it makes no sense to set a log message.
This makes it possible to call up an instance setting immediately without first showing the selection dialog.
Before was the instance id not given on "m_addon->SaveSettings(...)" call.
This allow a python script to edit add-on instance settings where used within PVR add-ons.

To prevent conflicts by user set within Kodi's GUI are the by python created ones starting with id >1000.
By a small python script has it works for tests.
@AlwinEsch AlwinEsch force-pushed the add-instance-handling-to-python branch from 259bb1e to 9a590a2 Compare October 24, 2023 19:38
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 24, 2023
@phunkyfish
Copy link
Contributor

phunkyfish commented Nov 16, 2023

What this change does

This allows a python script to edit add-on instance settings to be used within PVR add-ons.

To prevent conflicts from the user creating instances, add-on instances created by python start with id >1000.

The experience for the Python Dev

import xbmcaddon
import xbmcgui
 
addon       = xbmcaddon.Addon('pvr.demo')
addonname   = addon.getAddonInfo('name')

newid = addon.getFreeNewInstanceId()
addonsettingnew = addon.getSettings(newid)
addonsettingnew.setString('host', '10.144.12.13')
addon.setInstanceIdState(newid, True, 'By Python added')

# addon.deleteInstanceId(1001);

printstring = "Support Instances: " + str(addon.supportsInstanceSettings()) + "\n" \
              "Used Instances:\n"
for x in addon.getKnownInstanceIds():
    printstring += "ID: " + str(x) + "\n"
    printstring += "Test setting: " + addon.getSettings(x).getString('host') + "\n"

xbmcgui.Dialog().textviewer(addonname, printstring)

If the instance already present is it enough to call:

id = 1001
instancesetting = addon.getSettings(id)
instancesetting.setString('host', '10.144.12.13')

By this the add-on becomes the ADDON_STATUS SetInstanceSetting(const std::string& settingName, const kodi::addon::CSettingValue& settingValue) call.

@enen92 based on the above example code can you explain how it should be in order to be able to be able to get this code in a mergeable state.

Maybe even re-write the code snippet above and then we can see if that is possible, or if we need to make trade-offs somewhere. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

8 participants