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] activate PVR windows only if PVR is enabled #8232

Merged
merged 1 commit into from Oct 16, 2015

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Oct 15, 2015

If PVR is disabled, prevent activation of PVR windows using shortcut keys (e.g. ctrl+o for "Recordings" window). Simply ignore the key mapping in this case.

Today, if PVR is disabled in settings, an empty window with a dialog "PVR is not yet started up... waiting..." pops up. This dialog must be cancelled then manually by the user.

Not sure whether it is a good approach to put the logic into builtins, though feedback is very welcome @xhaggi, ...

@MartijnKaijser this is the fix we talked about at DevCon.

@ksooo ksooo added Type: Fix non-breaking change which fixes an issue Component: PVR labels Oct 15, 2015
@xhaggi
Copy link
Member

xhaggi commented Oct 16, 2015

makes totally sense

@mkortstiege
Copy link
Member

@ksooo, @xhaggi wouldn't make more sense to move IsPVRWindow() to the actual WindowManager? +1 other than that.

@ksooo
Copy link
Member Author

ksooo commented Oct 16, 2015

Yeah, doing the check in CGUIWindowManager::ActivateWindow_Internal is IMHO better. Any objections to change it that way?

@xhaggi
Copy link
Member

xhaggi commented Oct 16, 2015

sure, makes sense too 😄

@Montellese
Copy link
Member

Doing it in CGUIWindowManager will cover all cases and not just builtins but it doesn't feel quite right there either as the window manager should be agnostic to the state and settings of PVR.

@ksooo
Copy link
Member Author

ksooo commented Oct 16, 2015

@Montellese that's exactly why I did not put it there initially. So, any ideas?

Stick with the initial approach or head for something different?

Maybe a new virtual member function like bool CGUIWindow::CanBeActivated() that would be used by CGUIWindowManager::ActivateWindow_Internal and overridden by the PVR windows?

@xhaggi
Copy link
Member

xhaggi commented Oct 16, 2015

makes sense IMO

@Jalle19
Copy link
Member

Jalle19 commented Oct 16, 2015

@ksooo that sounds like a good idea

@zag2me
Copy link
Contributor

zag2me commented Oct 16, 2015

No brainer this one, it always bugged me when I accidently hit a pvr button on the remote before I had this installed.

@da-anda
Copy link
Member

da-anda commented Oct 16, 2015

I was also thinking about some way to hook into the process and interfere the action. So basically firing some event/message/signal and when some listener sets some flag (like "preventDefaultAction", "process = false", ...) then the sender ignores doing anyting. A little like JavaScript events. Might be a bit over the top for this particular case, but I think we should in general use more signals/messages for other parts to hook in and decouple stuff.

@ksooo
Copy link
Member Author

ksooo commented Oct 16, 2015

Now I implemented the CGUIWindow::CanBeActivated approach. What do the devs think? @xhaggi @negge @Montellese @mkortstiege

@Jalle19
Copy link
Member

Jalle19 commented Oct 16, 2015

👍

@mkortstiege
Copy link
Member

Jep, looks much cleaner now. Thanks.

@xhaggi
Copy link
Member

xhaggi commented Oct 16, 2015

+1 this could perfectly fits also to other scenarios where we want to block window activation

@Jalle19
Copy link
Member

Jalle19 commented Oct 16, 2015

jenkins build this please

@ksooo ksooo added this to the Jarvis 16.0-alpha4 milestone Oct 16, 2015
ksooo added a commit that referenced this pull request Oct 16, 2015
[PVR] activate PVR windows only if PVR is enabled
@ksooo ksooo merged commit 86c6a15 into xbmc:master Oct 16, 2015
@ksooo ksooo deleted the pvr-keymapping branch October 16, 2015 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Fix non-breaking change which fixes an issue v16 Jarvis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants