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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pvr] refactor guide osd to be channel guide dialog and use it in guide info #11554

Merged
merged 9 commits into from Mar 13, 2017

Conversation

@xhaggi
Copy link
Member

commented Jan 27, 2017

This PR refactor the PVR dialog guide OSD to supports listing of all EPG items for a supplied channel instead of just the current playing channel in fullscreen mode. Therefore the dialog is renamed to GUIDialogPVRChannelGuide.

I also added a new button Channel guide to the guide info dialog from where you can open the full EPG list for this channel.

screenshot003

@ksooo mind taking a look. I hope you remember that we talked about it some times ago 馃槃
@phil65 or @ronie It would be nice if you could adjust the skins.

@xhaggi xhaggi requested review from ksooo and ronie Jan 27, 2017
@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch from 14dd49c to 236c842 Jan 27, 2017
Copy link
Member

left a comment

Nice one, like it. Despite from my minor nitpicking it is fine already.

<animation effect="fade" start="100" end="0" time="200" tween="sine" condition="$EXP[infodialog_active]">Conditional</animation>
-->

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

I suggest to remove the code instead of only commenting it.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

it's part of the skip commit. I don't want to merge it as it is, because it needs a bit more love from @ronie or @phil65

@@ -68,6 +68,13 @@ namespace PVR
bool ShowEPGInfo(const CFileItemPtr &item) const;

/*!
* @brief Open a dialog with the epg list for a given item.
* @param item containing the channel. item must a channel.

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

"must a " -> "must be a"

EDIT: despite from this, item can be either a channel, an epg tag or a timer. CPVRItem::GetChannel() takes care of the conversion. You are actually feeding this method with an epg tag from CGUIDialogPVRGuideInfo.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

will adjust the doc

@@ -46,5 +47,8 @@ namespace PVR

CFileItemList *m_vecItems;

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

While you are at refactoring this, would it make sense to use a std::unique_ptr instead of a raw pointer here?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

sure could be done in a separate commit

g_PVRManager.GetCurrentEpg(*m_vecItems);
// no user-specific channel set use current playing channel
if (!m_channel)
m_channel = g_PVRManager.GetCurrentChannel();

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

I know it was like this before, but it is no good idea to call into g_PVRManager with locked g_graphicsContext. May I suggest to move the two lines above to the very beginning of this method, before locking the global graphics mutex?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

ok, will move it up.

{
CGUIDialog::OnDeinitWindow(nextWindowID);
m_channel = nullptr;

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

m_channel.reset() is more efficient.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

it does the same IIRC, but sure i can use reset() instead

if (!m_progItem || !m_progItem->HasPVRChannel())
{
/* invalid channel */
CGUIDialogOK::ShowAndGetInput(CVariant{19033}, CVariant{19067});

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

Could you please add the text for the two resource identifiers as a comment, like we usually do?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

c/p from another method. will change it.

@@ -324,7 +324,7 @@ static const ActionMapping windows[] =
{ "pvrchannelscan" , WINDOW_DIALOG_PVR_CHANNEL_SCAN },
{ "pvrupdateprogress" , WINDOW_DIALOG_PVR_UPDATE_PROGRESS },
{ "pvrosdchannels" , WINDOW_DIALOG_PVR_OSD_CHANNELS },
{ "pvrosdguide" , WINDOW_DIALOG_PVR_OSD_GUIDE },

This comment has been minimized.

Copy link
@da-anda

da-anda Jan 27, 2017

Member

I think it would be nice if we'd keep this ID in addition to the new one for backwards compat reasons to not kill all custom user keymaps. We could add a comment to remove it in like 2 versions from now.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

sure, will re-add the mapping for backward compat

@phil65

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

Cool. I'm still on holidays, so give me few more days.

@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch from 236c842 to 5cb5d35 Jan 27, 2017
@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

addressed your comments.

@ksooo
ksooo approved these changes Jan 27, 2017
Copy link
Member

left a comment

just found another small glitch...

@@ -11261,7 +11261,11 @@ msgctxt "#19685"
msgid "Confirm shutdown"
msgstr ""

#empty string with id 19686
#: Label for buttons to open channel guide dialog
#: addons/skin.estuary/1080i/DialogPVRInfo.xml

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

鈥/1080i/鈥 => 鈥/xml/鈥

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

There are tons of other places reference 1080i, we should change all at once

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 27, 2017

Member

Right, but what speaks against doing it right in the first place for new stuff we're adding? ;-)

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 27, 2017

Author Member

Nothing

This comment has been minimized.

Copy link
@ksooo
@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch 2 times, most recently from fff8c1a to 79d6711 Jan 30, 2017
@IchabodFletchman

This comment has been minimized.

Copy link

commented Feb 4, 2017

I've made the changes to Estuary including the new icon. @phil65 can cherry pick it once the PR is merged
IchabodFletchman/skin.estuary@58333df

@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2017

@IchabodFletchman thanks for your contribution but we should think about changing the dialog to fit both, being an OSD dialog and a normal dialog laying on top of other dialogs. IMO the dialog looks a bit odd on top of the guide info dialog.

@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

@phil65 do have some spare time to do the necessary skin changes?

@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch from 79d6711 to dbb6d9d Mar 13, 2017
@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch from dbb6d9d to e7cf503 Mar 13, 2017
@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2017

I've dropped the skin changes for now because we can add it later.

@phil65 if you want to adapt something 8cf27ed or IchabodFletchman/skin.estuary@58333df

@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2017

@ksooo I'll merge this after the build is fine.

@xhaggi xhaggi force-pushed the xhaggi:pvr-channel-guide-dialog branch from e7cf503 to 38b5d08 Mar 13, 2017
@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2017

jenkins build and merge

@jenkins4kodi jenkins4kodi merged commit fa6693c into xbmc:master Mar 13, 2017
2 of 3 checks passed
2 of 3 checks passed
jenkins4kodi I've found some spare time so building this now
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xhaggi xhaggi deleted the xhaggi:pvr-channel-guide-dialog branch Mar 13, 2017
@Rechi Rechi added the v18 Leia label Mar 13, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Mar 13, 2017
@Leatherface75

This comment has been minimized.

Copy link

commented Mar 13, 2017

Would be nice with IMDB support too there.

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