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] Guide window: Fix timer icon issues. Fixes #16621 #10036

Merged
merged 3 commits into from Jun 28, 2016

Conversation

Projects
None yet
4 participants
@ksooo
Copy link
Member

commented Jun 27, 2016

Fixes issues with not appearing/disappearing timer state icons in PVR Guide window, like discussed here: #10024 (comment) ff.

Fixes http://trac.kodi.tv/ticket/16621

Code changes are strait forward.

Reported to actually fix the problems by @bas-t and @seo

ksooo added some commits Jun 26, 2016

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

@bas-t

This comment has been minimized.

Copy link

commented Jun 27, 2016

@ksooo thanks for your effort, this works like a charm for me.

Is marking the trac ticket as fixed (referencing this PR) and closing it an automated process? It's not done yet and I don't know how to do it.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2016

Is marking the trac ticket as fixed (referencing this PR) and closing it an automated process?

@bas-t no, no automated process, but cannot login to trac currently. no idea what's broken. if trac is working again i will close the ticket.

@BigNoid

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

If you add Fixes #16621 to the PR title it should automatically close the ticket when merged.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2016

@BigNoid does this work for trac tickets? I thought this is only for github issues, no?

@BigNoid

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

There's some trac integration afaik. Github Janitor will close the ticket.
See #4115 and http://trac.kodi.tv/ticket/14885 for an example.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2016

Cool. Thanks. Will change PR title, then.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2016

Will changing the PR title do the trick or do I have to change one if the commit messages?

@BigNoid

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

I am not sure tbh. Let's see what happens :)
We can always close it manually.

EDIT: I mean changing the title, that should be enough.

@ksooo ksooo changed the title [PVR] Guide window: Fix timer icon issues (trac #16621) [PVR] Guide window: Fix timer icon issues. Fixes #16621 Jun 28, 2016

@ksooo ksooo merged commit c605aa8 into xbmc:master Jun 28, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details

@ksooo ksooo deleted the ksooo:pvr-fix-timer-icon-issues branch Jun 28, 2016

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2016

@BigNoid changing PR title seems not to be enough. Ticket got not closed automatically on merge. One needs to put 'fixes #xxxxx' into one of the involved commit messages.

}

void CGUIWindowPVRGuide::UnregisterObservers(void)
{
CSingleLock lock(m_critSection);
CGUIWindowPVRBase::UnregisterObservers();
if (g_PVRTimers)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 28, 2016

Member

this will cause the system crash when PVRManager is restarted because i.e. another client gets connects. There is no guarantee the the lifetime of Timers is longer than that of this window.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 28, 2016

Member

IMO the entire observes should go away from GUIWindows. This pattern is not suited for this purpose. Why don't you use window messages?

This comment has been minimized.

Copy link
@ksooo

ksooo Jul 1, 2016

Author Member

Just to confirm that we are on the same page: problem here is that g_PVRTimers (which is a CPVRTimers *) can get invalid between call to if (g_PVR_Timers) and g_PVRTimers->..., right? OMG, we have this pattern all over the place...

Why don't you use window messages?

I'm not the original author of this code, but valid point.

Would it be okay to use window messages inside the PVR core classes, like CPVRTimers? I think so.

Would using shared pointers for acquiring CPVRTimers instances (and the other pvr/epg globals) do any good as an interim solution?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 2, 2016

Member

Your first statement is correct. If it does not access an invalid pointer, it may unregister at a different instance which is not correct either. I don't think we should switch to shared pointers as a solution, the problem is the observer pattern that is an anti pattern here.
I would use window messages, and maybe route them through a proxy object.

This comment has been minimized.

Copy link
@ksooo

ksooo Jul 2, 2016

Author Member

ok. what would be the purpose of the proxy object?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 2, 2016

Member

using window messages as replacement for current observer approach relies on the fact that there can only be max one active window at a time, right?

not correct.

regarding non-window observers, I think we should treat this as a separate topic.

This comment has been minimized.

Copy link
@ksooo

ksooo Jul 2, 2016

Author Member

never dealt with kodi window messaging before. How does broasdcasting messages work?

regarding non-window observers, I think we should treat this as a separate topic.

ok

This comment has been minimized.

Copy link
@ksooo

ksooo Jul 2, 2016

Author Member

what do you think about this approach, which does not need window messages:

pvr components will be no longer Observables, instead of calling NotifyObservers() directly like they do now, they will call g_PVRManager.NotifyObservers(), passing the same observable message like they do today. the pvr windows no longer observe different pvr components, but the pvr manager. problem solved.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 2, 2016

Member

sounds reasonable :)

This comment has been minimized.

Copy link
@ksooo

ksooo Jul 2, 2016

Author Member

Okay, I will see what I can make out of this.

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