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

Python API cleanup #17456

Merged
merged 6 commits into from
Mar 23, 2020
Merged

Python API cleanup #17456

merged 6 commits into from
Mar 23, 2020

Conversation

ronie
Copy link
Member

@ronie ronie commented Mar 9, 2020

as discussed internally, we'll use the python 3 migration as an opportunity to clean up our python api.
this pr will remove deprecated classes / functions / parameters from our python api.

note:
dialogs that previously used the line1, line2, line3 parameters to specify the text will now use the message parameter.

@jimfcarroll could you please (at least) review the onAbortRequested() removal from the Monitor class?

@ronie ronie added API change: Python Component: Python v19 Matrix Type: Breaking change fix or feature that will cause existing functionality to change labels Mar 9, 2020
@ronie ronie added this to the Matrix 19.0-alpha 1 milestone Mar 9, 2020
@ronie ronie requested a review from jimfcarroll March 9, 2020 00:48
@fuzzard
Copy link
Contributor

fuzzard commented Mar 9, 2020

Looks good ronie. Was about what i saw when i looked at it yesterday. The only change i picked up that may be worth doing was in

Player::Player(int _playerCore)

I removed the int _playerCore completely, and also removed it in definition of Player in Player.h header, as it no longer has any use.

explicit Player(int playerCore = 0);

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

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

We should never remove history. If the methods get changed we need to know when a certain argument got deprecated (reviews). If we remove methods, we need to add that to history too.
Thanks for tackling this one

It would be nice to have someone looking into the abortRequested stuff

xbmc/interfaces/legacy/Control.h Show resolved Hide resolved
xbmc/interfaces/legacy/Control.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Dialog.cpp Show resolved Hide resolved
xbmc/interfaces/legacy/Dialog.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/Dialog.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/ListItem.h Show resolved Hide resolved
xbmc/interfaces/legacy/ListItem.h Show resolved Hide resolved
xbmc/interfaces/legacy/Monitor.h Show resolved Hide resolved
xbmc/interfaces/legacy/Window.h Show resolved Hide resolved
xbmc/interfaces/legacy/Window.h Show resolved Hide resolved
@ronie
Copy link
Member Author

ronie commented Mar 9, 2020

thanx a bunch for the reviews @fuzzard / @enen92 !
all issues should be resolved now

"",
<b>xbmc.monitor().onAbortRequested()</b> function was removed completely.
}
\python_removed_class{
Copy link
Member

Choose a reason for hiding this comment

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

python_removed_class doesn't exist. Those are doxy ALIAS that translate to html:

"python_removed_function{3}=\htmlonly <dl class=\"reflist\"><dt>Member <a class=\"el\" href=\"\2\">\1</a> (...)</dt><dd>\3</dd></dl>\endhtmlonly" \

Copy link
Member Author

Choose a reason for hiding this comment

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

would it be as simple as adding a new python_removed_class line there?
i'm hardly familiar with doxygen code...

Copy link
Member

Choose a reason for hiding this comment

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

Probably add the 3 functions instead of the class, result is pretty much the same. And python classes are not deleted everyday :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, updated

@phunkyfish phunkyfish added the PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. label Mar 10, 2020
@zag2me
Copy link
Contributor

zag2me commented Mar 13, 2020

Just a question about the line1,line2 thing:

https://forum.kodi.tv/showthread.php?tid=209948&pid=2931138#pid2931138

EDIT: Thx for the forum reply, solution is to use:

line1 = "Hello World! We can write anything we want here Using Python" xbmcgui.Dialog().ok(addonname, line1)

@fuzzard
Copy link
Contributor

fuzzard commented Mar 19, 2020

@jimfcarroll any insights into if the onabortrequested change is ok? any problems you can think of if we removed that from xbmc/interfaces/legacy/Monitor.*?

@fuzzard
Copy link
Contributor

fuzzard commented Mar 19, 2020

jenkins build this please

@fuzzard fuzzard merged commit a884e88 into xbmc:master Mar 23, 2020
@MilhouseVH
Copy link
Contributor

MilhouseVH commented Mar 24, 2020

After this PR, Monitor::abortRequested() is not being set to True when kodi starts to shut down.

Consequently, Monitor::waitForAbort() is not aborting on shutdown.

I'm guessing because abortEvent.Set() is no longer called?

The following sample code:

def dbg_log(source, text, level=4):
    xbmc.log('## LibreELEC Addon ## ' + source + ' ## ' + text, level)

class cxbmcm(xbmc.Monitor):
    def __init__(self, *args, **kwargs):
        xbmc.Monitor.__init__(self)

xbmcm = cxbmcm()

while not xbmcm.abortRequested():
    dbg_log('service', 'entering xbmcm.waitForAbort, abort is %s' % xbmcm.abortRequested())
    if xbmcm.waitForAbort(1):
        dbg_log('service', 'aborted xbmcm.waitForAbort, abort is %s' % xbmcm.abortRequested())
        break
    dbg_log('service', 'exiting xbmcm.waitForAbort, abort is %s' % xbmcm.abortRequested())

never reaches the aborted xbmcm.waitForAbort log statement once kodi starts to shut down, and xbmcm.abortRequested() is also permanently False so the loop never terminates.

Consequently, kodi kills the service (service.py) and logs the following:

2020-03-24 00:27:56.706 T:1381   ERROR: CPythonInvoker(0, /usr/share/kodi/addons/service.libreelec.settings/service.py): script didn't stop in 5 seconds - let's kill it
2020-03-24 00:27:56.706 T:1386   DEBUG: Thread ActiveAE 139992142837504 terminating
2020-03-24 00:27:56.713 T:1387   DEBUG: Thread AESink 139992134444800 terminating
2020-03-24 00:27:56.906 T:1381  NOTICE: Application stopped
2020-03-24 00:27:57.095 T:1405    INFO: CPythonInvoker(0, /usr/share/kodi/addons/service.libreelec.settings/service.py): script aborted
2020-03-24 00:27:57.095 T:1405   ERROR: CPythonInvoker(0, /usr/share/kodi/addons/service.libreelec.settings/service.py): failed to set abortRequested
2020-03-24 00:27:57.095 T:1405    INFO: CPythonInvoker(0, /usr/share/kodi/addons/service.libreelec.settings/service.py): waiting on thread 139990323410688
2020-03-24 00:27:57.106 T:1381  NOTICE: XBApplicationEx: destroying...

Full log: http://ix.io/2f6d - kodi.bin starts shutting down at 00:27:51.266

Compare with the same Python code, but using a build with this PR reverted: http://ix.io/2f6j - note the aborted log item at 00:47:22.015 (please ignore the let's kill it message in this log - that's a separate issue which we think has something to do with dbus).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change: Python Component: Python PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. Type: Breaking change fix or feature that will cause existing functionality to change v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants