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

[PVR] CDirectoryProvider now supports async PVR startup. Fixes favori… #10248

Merged
merged 8 commits into from Aug 10, 2016

Conversation

@ksooo
Copy link
Member

commented Aug 7, 2016

…te tv and radio channels not appearing on Home screen, sort method and order, item re-selection after update, start and end time when no epg.

Currently, Estuary's favorite tv and radio channels widget gets not filled on initial Kodi startup. One needs to open any other window and to go back to home screen to get the widgets populated. This is due to the related directoty list providers not respecting asynchronous pvr startup.

The problem is similar to and the solution is logically the same as for #10203

Important: This also needs a skin fix to work properly.

@da-anda fyi
@Jalle19 for review?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2016

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2016

jenkins build this please

{
if (URIUtils::IsProtocol(m_currentUrl, "pvr"))
{
if (msg == ObservableMessageManagerStateChanged && g_PVRManager.IsStarted())

This comment has been minimized.

Copy link
@tamland

tamland Aug 7, 2016

Member

Don't call g_PVRManager.IsStarted here please. The directory provider should not need to know about things like this; instead pvr should sent appropriate events.

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 7, 2016

Author Member

I don't get your point. What exactly is wrong with calling g_PVRManager? Need to know this object and thus pvr details anyway in order to register as observer at the pvr manager singleton. Maybe it is not the nicest api, but this is how pvr manager works today - sending one state changed event and offering several methods to obtain the current state. I would like to keep this code as it is now and put pvr messaging rework on my list for v18.

This comment has been minimized.

Copy link
@tamland

tamland Aug 7, 2016

Member

I'm just trying to keep this clean from any dependency that isn't strictly registering and not leaking in details that doesn't have anything to do with the vfs from those singletons. Why does the directory provider need to know, or care, about which state pvr manager is in? Addons have similar state change events (InstalledChanged, MetadataChanged) but it doesn't call back into addonmgr.

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 7, 2016

Author Member

I understand what you mean and your intention for sure is okay. But changing pvr messaging is high effort and i do not want to do this now as we are near beta 1 and this is highly sensitive pvr code (I never touched/read before). As i wrote i will be happy to (completely) rework this during alpha phase for v18.

}

void CDirectoryProvider::Notify(const Observable &obs, const ObservableMessage msg)
{

This comment has been minimized.

Copy link
@tamland

tamland Aug 7, 2016

Member

missing lock

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 7, 2016

Author Member

ooops.

m_currentUrl = value;

if (!m_isAnnounced)
{
m_isAnnounced = true;
CAnnouncementManager::GetInstance().AddAnnouncer(this);
ADDON::CAddonMgr::GetInstance().Events().Subscribe(this, &CDirectoryProvider::OnEvent);

if (URIUtils::IsProtocol(m_currentUrl, "pvr"))

This comment has been minimized.

Copy link
@tamland

tamland Aug 7, 2016

Member

This wont work. m_currentUrl can change, and if it does, it wouldn't register correctly (just do the same as the other two above)

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 7, 2016

Author Member

Not sure what you mean. I do unregister with pvr manager in line 407 in case old m_currentUrl is a pvr url, before m_currentUrl gets assigned a new value and i do register with pvr manager after m_currentUrl got the new value assigned. What is wrong with this?

This comment has been minimized.

Copy link
@tamland

tamland Aug 7, 2016

Member

Right. Though, it would do an unnecessary unregister/register if url changes from pvr to a different pvr path.

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 7, 2016

Author Member

I'm sure I can optimize this. ;-)

@ksooo ksooo force-pushed the ksooo:pvr-fix-directorylistprovider branch from 1661518 to f56ea3a Aug 8, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

@tamland take 2. should be much better now. any objections?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

jenkins build this please

@da-anda

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

only to have this future proof, can this event handling in future be extended to also update PVR channels f.e. on EPG updates, playcount updates of channels etc, or would this require some other man in the middel event handler?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

@BigNoid are you fine with the Estuary fix?

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

are you fine with the Estuary fix?

Yeah looks good. I'll rebase #10253 after this is in then.


namespace PVRManagerEvents
{
struct Starting : PVRManagerEvent {};

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 8, 2016

Member

I am wondering if polymorphism is the right concept here.

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 8, 2016

Author Member

@FernetMenta ask @tamland. he wanted me to do it like he does with addons (at least this is what i understood from his review).

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 8, 2016

Author Member

i think the idea is that if an event is a struct it can carry aditional data. i do not use this for pvr manager, but some of the addon events do.

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 8, 2016

Author Member

but yeah, i guess i will simplify this. if we need to carry data along with the events, we easily can change that later.

@ksooo ksooo force-pushed the ksooo:pvr-fix-directorylistprovider branch from adbd9fd to 964e06e Aug 8, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

@FernetMenta addressed your review comment. thx.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

thanks, imo much better. as you said, in case some future requirements need a change, it still can be changed.

@Jalle19

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

Looks good to me

@ksooo ksooo force-pushed the ksooo:pvr-fix-directorylistprovider branch from 02caeb1 to 699e3bf Aug 8, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

heads-up: if nobody objects i will merge this tomorrow. :-)

@da-anda

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

I've seen @MilhouseVH included it in his nightlies, so maybe wait one or two days if regressions are popping up in these build and merge then?

@ksooo ksooo force-pushed the ksooo:pvr-fix-directorylistprovider branch from 10a8f32 to 1bda2c9 Aug 9, 2016
@ksooo ksooo force-pushed the ksooo:pvr-fix-directorylistprovider branch from 1bda2c9 to 9a51615 Aug 10, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

last rebase before merge
jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

no complaints from Milhouse build users...

@ksooo ksooo merged commit d66b47f into xbmc:master Aug 10, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@ksooo ksooo deleted the ksooo:pvr-fix-directorylistprovider branch Aug 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.