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

fix PluginDirectory #9778

Merged
merged 4 commits into from May 10, 2016

Conversation

@FernetMenta
Copy link
Member

commented May 8, 2016

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

@MartijnKaijser this pr has a slight rework for #9748
maybe you are lucky and it fixes the Android issue.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

http://trac.kodi.tv/ticket/16718 appears to be fixed by this PR - many thanks!

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

jenkins build this please

// we have python scripts hooked in everywhere :(
// those scripts may open windows and we can't open a window
// while opening this one.
// for pligin sources delay call to Refresh

This comment has been minimized.

Copy link
@ksooo

ksooo May 8, 2016

Member

typo here. pligin -> plugin

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 9, 2016

Author Member

thanks, corrected

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 8, 2016

No change

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

Although the Search YouTube hang appeared to be resolved, it's possible this PR is causing other odd behaviour when starting addons (or it's another hangover from the original problem, #9664).

With PR9778, sometimes the user interface for the addon will "flash" (appear briefly then disappear), other times the addon will simply show a blank background (but the GUI hasn't hung - "Back" will eventually return to the Estuary Home menu).

I can reproduce this with latest master + PR9778 and YouTube addon in Ubuntu when running Estuary Home > Addons > YouTube.

Another addon showing similar problems is the Amazon Prime Instant addon.

In fact when you run the Amazon Prime Instant addon you see only a blank background. Click "Back" and you see a constantly spinning "Busy" icon. Click "Back" again and you're returned to the Estuary Home menu. Now click on Search > Search YouTube and you'll be shown the Amazon Prime Instant addon screen overlaid with a "Busy" dialog...

@FernetMenta FernetMenta force-pushed the FernetMenta:crappyscripts branch from e4c4f87 to 5220b49 May 9, 2016

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 9, 2016

@MilhouseVH updated. could you test again please

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

Sorry for the delay, updates look good @FernetMenta have tested all the add-ons on my RPi3 system and all are working as expected. Thanks again!

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 9, 2016

thanks for feedback. there is one minor fix I am going to add to this PR before merge. currently you have to cancel twice when the keyboard dialog is displayed.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

currently you have to cancel twice when the keyboard dialog is displayed.

For example the YouTube addon? Yes I see that, first "Back" (or Cancel) shows a busy dialog. Strangely I don't get this issue with Global Search, so I assumed it was specific to the YouTube addon (ie. searching for "").

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 9, 2016

For example the YouTube addon? Yes I see that, first "Back" (or Cancel) shows a busy dialog

Exactly this. The old code did observe script termination. I will add this back but slightly different.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 10, 2016

jenkins build this please

@FernetMenta FernetMenta merged commit f5b8f51 into xbmc:master May 10, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Merged build finished.
Details

@FernetMenta FernetMenta deleted the FernetMenta:crappyscripts branch May 10, 2016

@sualfred

This comment has been minimized.

Copy link

commented on 95ffdcc May 15, 2016

@FernetMenta

I dunno if it's caused by this commit, but the busy dialog could break skin based mechanism on startup.
In my case for example I have a bunch of invisible controls to check if widgets are loaded before they will become visible. This "loop" is broken now because of the busy dialog. It's also a little bit annoying that the busy dialog will swtich to visible-hidden-visible-hidden if several plugin based widgets are used.

Edit:
Got it working again with an workaround, but it's still annoying. If this behaviour is important, I suggest to use the info dialog instead (KaiToast) since the busy dialog is used to be a fullscreen dialog.

Edit2:
Yep, more than annoying with a few dynamic content lists, which are filled by python scripts.

Edit3:
This also breaks addons such as the autocompletion module of @phil65. After pressing one character it will always trigger the busy dialog for new suggestions. And using one suggestion for the input is completely broken, because the busy dialog steals the activate window.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

Same problem here with script filled dynamic lists triggering the busy dialog.

@ronie @phil can you confirm the same?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 18, 2016

scripts are not supposed to hijack the main thread for a long time. if you see the busy dialog, you are doing something wrong

@ghost

This comment has been minimized.

Copy link

commented May 18, 2016

I don't believe this is a case of plugins 'hijacking' the main thread, but rather when a list is filled directly from a plugin (<content>plugin://plugin.name/</content>) this now triggers the busy dialog where it didn't previously. It causes major usability issues with skins which directly fill lists from plugins as widgets, and definite usability issues with all skins which use @phil65's autocompletion plugin with the virtual keyboard, including Estuary.

If plugins are now returning their contents on the main thread, then this would appear to be a change of behaviour leading to a regression in terms or usability, and not the result of any change in the plugin api - and affects all plugins I have tested.

(And if it is otherwise, then the team need to inform most every plugin author of the changes to the api that allow them to do the work they need to do in a background thread, as has automatically happened up to this point when directly filling the contents of a list, or the previous posters statement needs to be redacted)

@ronie

This comment has been minimized.

Copy link
Member

commented May 18, 2016

@ronie @phil can you confirm the same?

yup, can confirm.

perhaps we can grant plugins a little more time to provide a listing before popping up the busy dialog?
if i read the changes correctly, it used to be 1500 msecs before a progressbar was shown.

@phil65

This comment has been minimized.

Copy link
Member

commented May 19, 2016

I think for dynamic content included via <content>plugin://plugin.name/</content> there never was a progress/busydialog. Those dialogs should only appear if the list which is loading content is our main media list (Video/Audio plug-ins for example)

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

the old design was wrong and caused issues. that was the reason why I fixed it.

GetDirectory must not silently hijack the main thread.

if the current behaviour does not fit your requirements, write those down in the dev section of the forum

@phil65

This comment has been minimized.

Copy link
Member

commented May 22, 2016

just fyi, i had a talk with @FernetMenta today and explained the "problem" in more detail. He is aware of all the details now and will come up with a sane fix for this issue soon I think. :)

@Memphiz

This comment has been minimized.

Copy link
Member

commented on xbmc/filesystem/PluginDirectory.cpp in e6ede27 Jan 3, 2019

@FernetMenta - any idea how this could lead to this crash in thread 24?:

https://paste.kodi.tv/fukabizume.kodi

This comment has been minimized.

Copy link
Member Author

replied Jan 4, 2019

yes, I think Abort() should wait until the thread has finished.

This comment has been minimized.

Copy link
Member

replied Jan 4, 2019

Is a thread join without timeout ok in this case or could the thread block forever?

This comment has been minimized.

Copy link
Member

replied Jan 4, 2019

@FernetMenta i will try that - but as far as I can tell the scriptobs object goes out of scope right after the abort is called which means the DTOR of the Thread is called which in turn already does a "StopThread" which waits for the threads exit. So if I got it right - between calling abort and continuation in the upper code layer there is already an implicit wait for the threads exit in the CThread DTOR.

This comment has been minimized.

Copy link
Member Author

replied Jan 5, 2019

At the time the base class dtor calls StopThread, the object has already morphed and m_event is gone.

@Memphiz

This comment has been minimized.

Copy link
Member

commented on xbmc/filesystem/PluginDirectory.cpp in e6ede27 Jan 5, 2019

The second param of the CTOR here is what will be „m_event“ in the scriptobserver object. And it is passed as reference here.

@Memphiz

This comment has been minimized.

Copy link
Member

commented on xbmc/filesystem/PluginDirectory.cpp in e6ede27 Jan 5, 2019

This is the line where the scriptObs is going out of scope leading to the DTOR calling StopThread.
At this time I don‘t see why m_event (which is a reference to m_fetchComplete from above) is invalid already.
Where am I wrong here? Just so I can correct my judgement in the future...

This comment has been minimized.

Copy link
Member

replied Jan 5, 2019

Mhhh Reading my own statement I guess the reference inside the scriptobs object is not pointing to the m_fetchComplete anymore because memory might have been reused for something else already. Is that it?

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