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: Go to date #13282

Merged
merged 3 commits into from
Jan 2, 2018
Merged

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Jan 1, 2018

This feature was several times requested in the forum. It adds a new menu item 'Go to date' to PVR Guide window's items:

screenshot000
screen shot 2017-12-31 at 14 03 17

@xhaggi mind taking a look?

@ksooo ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR v18 Leia labels Jan 1, 2018
@ksooo ksooo added this to the L 18.0-alpha1 milestone Jan 1, 2018
@ksooo ksooo requested a review from xhaggi January 1, 2018 14:36
@@ -452,17 +454,46 @@ bool CGUIWindowPVRGuideBase::OnMessage(CGUIMessage& message)
return bReturn || CGUIWindowPVRBase::OnMessage(message);
}

bool CGUIWindowPVRGuideBase::OnActionContextMenu()

This comment was marked as spam.

This comment was marked as spam.

@ksooo
Copy link
Member Author

ksooo commented Jan 1, 2018

@xhaggi I removed the method with the questionable name and moved the code to CGUIWindowPVRGuideBase::OnMessage. It's not called from different places, so no need for a separate method, imo. Okay now?

Copy link
Member

@xhaggi xhaggi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better now, thanks.

@da-anda
Copy link
Member

da-anda commented Jan 2, 2018

since there are a lot of "go to..." items now - what would you guys think about grouping them into a secondary context menu level? Like on main level we only show "go to..." and then replace the context menu items with the 4 "go to" ones? So basically the same thing we do with the "Manage ..." context menu item on f.e. movie library items. Just a thought, no demand.

@ksooo
Copy link
Member Author

ksooo commented Jan 2, 2018

since there are a lot of "go to..." items now - what would you guys think about grouping them into a secondary context menu level?

Hehe, I actually had that implemented and played around with it. But due to the additional click necessary to access the actual "go to" entry of interest it somehow felt wrong to me, because the "go to" actions are shortcuts for people who want to save time. Otherwise they would just scroll to the epg grid. Adding another step just for cosmetically purposes is imo not the right approach here.

@ksooo ksooo merged commit 0be7da3 into xbmc:master Jan 2, 2018
@ksooo ksooo deleted the pvr-guide-goto-actions branch January 2, 2018 08:19
@da-anda
Copy link
Member

da-anda commented Jan 2, 2018

the one more click argument doesn't really hold up, because when grouped you can reach the "go to beginning" item actually way faster by clicking "up + select + select" - so just 3 clicks instead of 5. Also, it'll always be faster than manually navigating the EGP, regardless if it's one more click or not. But whatever.

@ksooo
Copy link
Member Author

ksooo commented Jan 2, 2018

That implies rust to go to menu is on top ... and this is technically currently not possible - at least to my knowledge.

@da-anda
Copy link
Member

da-anda commented Jan 2, 2018

you can press up in a context menu to get to the last item. And when the grouped "go to..." item is the last item, all it takes is up + select + select to select to the top most "go to" function. Just saying

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants