Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[PVR] add possibility to start liveTV playback via cAction and keymaps. #2325

Merged
merged 2 commits into from Mar 9, 2013

Conversation

Projects
None yet
4 participants
Member

da-anda commented Mar 1, 2013

When called, it first tries to play the last watched channel - if this fails/is not available it uses the first TV channel of the current channel group. If PVR is disabled or PVRmanager is not yet ready, it'll throw a kaitoast.

@ghost ghost assigned opdenkamp Mar 1, 2013

Member

da-anda commented Mar 1, 2013

@Montellese, could you add the according code to have that feature available via JsonRPC?

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Mar 1, 2013

xbmc/Application.cpp
@@ -2733,6 +2733,16 @@ bool CApplication::OnAction(const CAction &action)
return true;
}
+ // turn on liveTV
+ if (action.GetID() == ACTION_PVR_PLAYLIVETV)
+ {
+ if (!g_PVRManager.IsStarted())
+ CGUIDialogKaiToast::QueueNotification(CGUIDialogKaiToast::Warning, g_localizeStrings.Get(19045), g_localizeStrings.Get(19044));
@jmarshallnz

jmarshallnz Mar 1, 2013

Member

does it make sense to start the pvr manager here?

@opdenkamp

opdenkamp Mar 1, 2013

Member

nope it doesn't. you need to configure a pvr add-on first, have a backend set up, etc (and i'll review the rest of the patch asap as promised @da-anda)

Member

da-anda commented Mar 5, 2013

question - does it make sense to have the logic where I put it or would cApplicationMessenger or something else be better suited? But keymap shouldn't change to some ugly "Builtins(...)" construct.

Also, would you guys be fine with changing Confluence to start LiveTV when pressing the according home menu button? Would at least make more sense to me, as channel list etc can be accessed via submenu anyways (maybe make it configurable in skin settings?)

And a third point that @JezzX brought up is the name of the action. He suggested to have PVR in it - so probably "playpvrtv" so that we in future could also have a "playpvrradio" or something (I could add playpvrradio as well if you want)

Member

da-anda commented Mar 6, 2013

ok, I rewrote the whole thing to be a bit more flexible and it now also allows to specify which kind of playback to start (any, tv, radio). So there are 3 actions now that can be used in keymaps and skins. The current update is more of a RFC, needs some more tests before it can be merged.

@opdenkamp opdenkamp and 1 other commented on an outdated diff Mar 7, 2013

xbmc/Application.cpp
@@ -2733,6 +2733,35 @@ bool CApplication::OnAction(const CAction &action)
return true;
}
+ // turn on liveTV
+ if (action.GetID() == ACTION_PVR_PLAY || action.GetID() == ACTION_PVR_PLAY_TV || action.GetID() == ACTION_PVR_PLAY_RADIO)
+ {
+ if (!g_PVRManager.IsStarted())
@opdenkamp

opdenkamp Mar 7, 2013

Member

could you move all of this to a method in CPVRManager

@da-anda

da-anda Mar 7, 2013

Member

why or how? Add an action to cPVRManager that deals with those cActions? IMO handlingof cActions is out of it's scope.

@opdenkamp

opdenkamp Mar 7, 2013

Member

handling of pvr related actions can be done in CPVRManager, within the pvr namespace.

@opdenkamp opdenkamp commented on an outdated diff Mar 7, 2013

xbmc/Application.cpp
+ bool isPlayingPvr(IsPlaying() && CurrentFileItem().HasPVRChannelInfoTag());
+ switch (action.GetID())
+ {
+ case ACTION_PVR_PLAY:
+ if (!isPlayingPvr)
+ g_PVRManager.StartPlayback(PlaybackTypeAny);
+ break;
+ case ACTION_PVR_PLAY_TV:
+ if (!isPlayingPvr || CurrentFileItem().GetPVRChannelInfoTag()->IsRadio())
+ g_PVRManager.StartPlayback(PlaybackTypeTv);
+ break;
+ case ACTION_PVR_PLAY_RADIO:
+ if (!isPlayingPvr || !CurrentFileItem().GetPVRChannelInfoTag()->IsRadio())
+ g_PVRManager.StartPlayback(PlaybackTypeRadio);
+ break;
+ }
@opdenkamp

opdenkamp Mar 7, 2013

Member

add a default: break; please

@opdenkamp

opdenkamp Mar 7, 2013

Member

oh it's not an enum, nvm. compilers can complain about unhandled enum values in a switch

@opdenkamp opdenkamp commented on the diff Mar 7, 2013

xbmc/pvr/PVRManager.cpp
+ if (channel && channel->HasPVRChannelInfoTag())
+ {
+ bReturn = StartPlayback(channel->GetPVRChannelInfoTag(), (g_guiSettings.GetInt("pvrplayback.startlast") == START_LAST_CHANNEL_MIN));
+ }
+ else
+ {
+ CPVRChannelGroupPtr channelGroup = GetPlayingGroup(bRadio);
+ if (channelGroup)
+ {
+ CFileItemPtr channel = channelGroup->GetByIndex(0);
+ if (channel && channel->HasPVRChannelInfoTag())
+ bReturn = StartPlayback(channel->GetPVRChannelInfoTag(), false);
+ }
+ }
+
+ if (!bReturn)
@opdenkamp

opdenkamp Mar 7, 2013

Member

might want to show something in the ui too when this happens

@opdenkamp opdenkamp and 1 other commented on an outdated diff Mar 7, 2013

xbmc/pvr/PVRManager.cpp
@@ -1023,6 +1023,55 @@ bool CPVRManager::StartPlayback(const CPVRChannel *channel, bool bPreview /* = f
return true;
}
+bool CPVRManager::StartPlayback(PlaybackType type /* = PlaybackTypeAny */)
+{
+ bool bRadio(false);
+ bool bReturn(false);
+ CFileItemPtr channel;
+
+ switch (type)
@opdenkamp

opdenkamp Mar 7, 2013

Member

i know this all pretty straight forward, but please add some short documentation in the code like did with (most of) the other methods.

@da-anda

da-anda Mar 7, 2013

Member

The method itself (like duplicating the comment from the header file) or are you referring to document the switch construct?

@opdenkamp

opdenkamp Mar 7, 2013

Member

the method itself, just describe in short code documentation what you're doing

Member

da-anda commented Mar 7, 2013

updated. Moved the cAction handling completely to the PVRmanager now, added feedback on playback errors and added some inline documentation as requested.
Edit: also changed the reused error messages slightly to be a bit more generic and make a bit more sense in the new usecase ("This channel" was missleading as user didn't specify a channel)

@opdenkamp opdenkamp commented on an outdated diff Mar 7, 2013

language/English/strings.po
@@ -6815,11 +6815,11 @@ msgid "Already started recording on this channel"
msgstr ""
msgctxt "#19035"
-msgid "This channel cannot be played. Check the log for details."
+msgid "Channel cannot be played. Check the log for details."
@opdenkamp

opdenkamp Mar 7, 2013

Member

let's change "Channel" into "%s", add a new "last channel" entry here, and either set the channel name or "last channel" in the popup

@opdenkamp opdenkamp and 1 other commented on an outdated diff Mar 7, 2013

xbmc/pvr/PVRManager.cpp
@@ -1023,6 +1024,65 @@ bool CPVRManager::StartPlayback(const CPVRChannel *channel, bool bPreview /* = f
return true;
}
+bool CPVRManager::StartPlayback(PlaybackType type /* = PlaybackTypeAny */)
+{
+ bool bRadio(false);
+ bool bReturn(false);
+ CFileItemPtr channel;
+
+ // check if the desired PlaybackType is already playing,
+ // and if not, try to grab the last played channel of this type
+ switch (type)
+ {
+ case PlaybackTypeRadio:
+ if (IsPlayingRadio())
+ return true;
+ bRadio = true;
@opdenkamp

opdenkamp Mar 7, 2013

Member

don't need this bool really, it does the same as type == PlaybackTypeRadio
and i have a general dislike of returning in the middle of methods (but must admit i'm often too lazy to do it cleaner too ;-))

@da-anda

da-anda Mar 7, 2013

Member

the return where not because of lazyness - I just thought early returns are better and the code is cleaner that way (less if/else stuff inside the switch) - can change that ofc.

edit: I really prefer the early return here, because it will prevent the CPU to waste cycles on resolving the channels which is not needed if playback has already started. And nesting the channel fetching in a else condition feels somewhat odd - but can do so if you like.

@pieh pieh commented on the diff Mar 8, 2013

language/English/strings.po
@@ -6815,7 +6815,7 @@ msgid "Already started recording on this channel"
msgstr ""
msgctxt "#19035"
-msgid "This channel cannot be played. Check the log for details."
+msgid "%s could not be played. Check the log for details."
@pieh

pieh Mar 8, 2013

Member

Shouldn't this string be blanked and new one added?

@da-anda

da-anda Mar 8, 2013

Member

As it's not an issue if the old message is displayed or the updated string (newer one is just a bit nicer) it shouldn't be an issue IMO - but I can change that if needed.

@pieh pieh and 1 other commented on an outdated diff Mar 8, 2013

xbmc/pvr/PVRManager.h
@@ -473,6 +488,13 @@
*/
bool WaitUntilInitialised(void);
+ /*!
+ * @brief Handl PVR specific cActions
@pieh

pieh Mar 8, 2013

Member

Handl_e_ :)

@da-anda

da-anda Mar 8, 2013

Member

This PR is going to drive me crazy.

Franz Koch added some commits Mar 8, 2013

[PVR] add possibility to start liveTV or PVR radio playback via cActi…
…on and keymaps.

When called, it first tries to play the last watched channel - if this fails/is not available it uses the first channel of the current channel group.
Member

da-anda commented Mar 8, 2013

updated again - using %s to inject the context for the error messages now. Hope everything is fine now.

Member

opdenkamp commented Mar 9, 2013

nice work, thanks

opdenkamp pushed a commit that referenced this pull request Mar 9, 2013

Merge pull request #2325 from da-anda/instant-livetv
[PVR] add possibility to start liveTV playback via cAction and keymaps.

@opdenkamp opdenkamp merged commit 47e93ba into xbmc:master Mar 9, 2013

@da-anda da-anda deleted the da-anda:instant-livetv branch Mar 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment