[PVR][Estuary] Guide window: Channel groups selector #13364

ksooo · 2018-01-14T18:05:28Z

Requested many times and for a long time in the forum, here it comes... channel groups selector for the Guide window:

Channel groups are displayed as a "band" on top of the epg grid. Channel groups are switched live when selecting a group in the "band". Thus, a user can switch very quickly between channel groups.

The channel group selector can be turned off. There are two new view types for the Guide window "horizontal channels, no group selector" and "vertical channels, no group selector", in addition to the two existing viewtypes, which now contain the group selector. The views without the selector look exactly the same as the views we had before this PR.

Changes are runtime-tested on macOS and Linux, latest kodi master.

@xhaggi mind taking a look at the code changes changes.
@ronie are the skin changes okay?

ronie

very nice!

xhaggi · 2018-01-14T18:23:02Z

I'm not sure if i like it. Does this really improve the group selection? If you stay on channel 15 of 30, somewhere in the middle, how do you easily reach this group selection control? We put the channel group selection into the options menu which can be easily reached with the menu key and this is consistent between all windows where you can switch the group.

xhaggi · 2018-01-14T18:29:27Z

Updated my first comment and sorry it was the wrong button. Never write comments on a phone ;)

ksooo · 2018-01-14T18:36:29Z

If you stay on channel 15 of 30, somewhere in the middle, how do you easily reach this group selection control?

Hit "1" on your remote, that selects first channel row, than hit "up" and here we go. Just two key presses max, no matter were you are in the grid.

ksooo · 2018-01-14T18:37:55Z

I'm not sure if i like it.

If you don't like it switch to one of the views without the selector.

ksooo · 2018-01-14T18:39:10Z

We put the channel group selection into the options menu which can be easily reached with the menu key

I have no menu key on my remote. Thus I have to scroll left for "ages" to get to the side blade. That sucks.

EDIT: And this is, what users are telling us for a long time.

xhaggi · 2018-01-14T19:00:18Z

Hit "1" on your remote, that selects first channel row, than hit "up" and here we go. Just two key presses max, no matter were you are in the grid.

Why not focus the options menu if you leave the grip on top and down? That would be nearly the same. My concern is the the difference in behavior. Next some users request the same for the channel list.

ksooo · 2018-01-14T19:05:29Z

Why not adding this also to the channel list? Seriously.

ksooo · 2018-01-14T19:13:38Z

I really do not like that user experience when changing channel groups using the side blade. First, as I explained, without a menu key it is pain to get there. Then, you have to select the current group and to press select, then, a dialog pops up, where you have to select the new group, then press select again. Then, the new group populated in the guide window, which is half hidden behind the side blade. Finally, I have to hit back to close the side blade and to get beck to the guide window. So ugly, so many clicks...

xhaggi · 2018-01-14T19:30:51Z

I don't have anything against it in general, but it would be nice if we could unify it in the corresponding windows. Now we have different ways in different windows. Therefore it would be nice if the control could be used on the other windows as well. We could replace the channel group button with this blade control and if a skin want to place it in the options menu it should be functional the same. What do you think of that?

jjd-uk · 2018-01-14T20:35:56Z

Just for info, on my current Sky box (and other pvr’s I’ve used) the method to get to the channel group selection bar no matter where you are in epg grid is via the back key. As someone who doesn’t use Kodi pvr much I’ve no idea if that would fit in with current Kodi navigation methods within the guide.

xhaggi · 2018-01-14T20:38:26Z

We have a dedicated key "menu" to reach the options menus in windows. On a full featured remote this is no issue. For me it's easy to switch the channel group, not used very often but it's really no issue.

zag2me · 2018-01-14T21:10:24Z

Big.... awesome.... thanks for this, I literally was going to request this on the forums today. I hate the sideblade group selection and the PR makes it SO MUCH easier. Brilliant! Looks very similar to the sky EPG now which has great usability. Example here

ksooo · 2018-01-15T06:19:32Z

On a full featured remote this is no issue.

All people not having a "fully featured" remote are having an issue with it. It's almost unusable, then.

Therefore it would be nice if the control could be used on the other windows as well.

I moved the implementation to CGUIWindowPVRBase now. :-)

da-anda · 2018-01-15T12:31:51Z

ok, so my SHIELD remote does neither have a menu button nur numeric input. If I now bug long enough on the forum, will I also get my custom channel group switching method implementation? I have a back button, so jjd's solution would work 😜

What about the focus "overflow" of the EPG grid? Up until now I was able to press up on channel 1 to get to the end of the list. Will pressing "up" on the channel selector also end up at the end of the channel list, or is the overflow feature gone in this view mode?

Has it been tested what happens when there are more channel groups than screen width? It might be an idea to change this control into one that has the currently selected channel group always at the same spot and the options just slide left and right. Something like a coverflow view. And only the selected channel group would be clearly visible, while the others would be dimmed (or entirely hidden until the control is focused). Just an idea.

Btw, it might be an idea to instantly focus this group selector when pressing "0" on your remote.

xbmc/pvr/windows/GUIWindowPVRBase.cpp

+void CGUIWindowPVRBase::InitChannelGroupsControl()
+{
+  if (HasChannelGroupsControl())
+  {


xbmc/pvr/windows/GUIWindowPVRBase.cpp

+
+bool CGUIWindowPVRBase::SelectActiveChannelGroup()
+{
+  if (HasChannelGroupsControl())


xbmc/pvr/windows/GUIWindowPVRBase.cpp

+  if (HasChannelGroupsControl())
+  {
+    const CPVRChannelGroupPtr activeGroup = GetChannelGroup();
+    if (activeGroup)


xbmc/pvr/windows/GUIWindowPVRBase.cpp

@@ -151,6 +162,9 @@ void CGUIWindowPVRBase::OnInitWindow(void)
    CGUIWindow::OnInitWindow(); // do not call CGUIMediaWindow as it will do a Refresh which in no case works in this state (no channelgroup!)
    ShowProgressDialog(g_localizeStrings.Get(19235), 0); // PVR manager is starting up
  }
+
+  // This has to be done after base class OnInitWindow to restore correct selection
+  InitChannelGroupsControl();


xbmc/pvr/windows/GUIWindowPVRBase.cpp

+    case ACTION_MOVE_DOWN:
+    {
+      if (IsChannelGroupsControlFocused())
+        return CGUIMediaWindow::OnAction(action) && ActivateSelectedChannelGroup();


xbmc/pvr/windows/GUIWindowPVRBase.h

@@ -112,6 +115,12 @@ namespace PVR
  private:
    bool OpenChannelGroupSelectionDialog(void);

+    bool HasChannelGroupsControl();
+    bool IsChannelGroupsControlFocused();


xbmc/pvr/windows/GUIWindowPVRBase.h

+    bool HasChannelGroupsControl();
+    bool IsChannelGroupsControlFocused();
+    void InitChannelGroupsControl();
+    bool SelectActiveChannelGroup();


xbmc/pvr/windows/GUIWindowPVRBase.h

+    bool IsChannelGroupsControlFocused();
+    void InitChannelGroupsControl();
+    bool SelectActiveChannelGroup();
+    bool ActivateSelectedChannelGroup();


xbmc/pvr/windows/GUIWindowPVRBase.cpp

+    case ACTION_MOVE_RIGHT:
+    case ACTION_MOVE_LEFT:
+    case ACTION_MOVE_UP:
+    case ACTION_MOVE_DOWN:


ksooo · 2018-01-15T13:06:49Z

Sorry, I'm sick about this.

xhaggi · 2018-01-15T14:11:25Z

Sick about discussing a feature you want to merge? Or sick about my review? Could you please explain it a bit more for us.

ksooo · 2018-01-15T14:34:34Z

Your review is okay.

ksooo · 2018-01-15T22:48:49Z

I have a final offer to make (code- and function-wise):

ksooo · 2018-01-15T23:02:25Z

If I now bug long enough on the forum, will I also get my custom channel group switching method implementation?

Please stop this kind of comments. I'm not open to this kind of "humor". Even, if you're using emojis.

Will pressing "up" on the channel selector also end up at the end of the channel list, or is the overflow feature gone in this view mode?

The latter. Technically not possible atm. "Up" in the group selector goes back to where you were when setting focus to the selector (keyboard: row 1, mouse: the last row that had the focus.)

Has it been tested what happens when there are more channel groups than screen width?

Ofc. Was working fine with initial implementation. Does work fine with my "final offer".

It might be an idea to change this control into one that has the currently selected channel group always at the same spot and the options just slide left and right.

Done in the "final offer".

Btw, it might be an idea to instantly focus this group selector when pressing "0" on your remote.

'0' is already taken. It positions the grid to 'now'. Otherwise, a good idea.

ksooo · 2018-01-16T09:15:44Z

Now we have different ways in different windows.

Not really. The old way still works, exactly like it was before. There is only another new way added. I don't see this as a problem. Nobody asked (so far) for a group selector bar for the channel window, but if users want it it could be added easily.

Therefore it would be nice if the control could be used on the other windows as well.

Done. Implementation is now in the base class of all PVR windows. Doing the nessacery skin changes for Estuary for the channel window is out of scope of this PR, though.

We could replace the channel group button with this blade control and if a skin want to place it in the options menu it should be functional the same. What do you think of that?

Sounds good. This is for another day (PR).

ksooo · 2018-01-16T09:18:07Z

Heads up please. I'm going to merge this if I don't get a resaonable veto during the next 12 hours.

xhaggi · 2018-01-16T11:29:34Z

I know that most of the comments are minor stuff, but in most of these cases you are not willing to compromise and want to push through your preferences. We both work in this area, but if you only allow your preferences, you don't need my review next time. Just push and merge, much easier. You can count yourself lucky to have some other dev working in the same area, we don't have much areas where 2 or more devs are working together.

da-anda · 2018-01-16T11:49:49Z

@ksooo yeah, sorry for the "joke" - I realized later that I should have deleted this paragraph as well. I initially also had a comment that I'm more on xhaggi's side and would prefer a one-fits-all approach instead of adding more code that needs maitenance, but I dropped it to not piss you off (which I still did in the end due to the first paragraph that I didn't remove). Oh well, lesson learned. I'd still be in favor of less settings options and a one-fits-all approach though, but I don't want to block this PR in any way.

Also, you don't have to instantly implement every suggestion I make. I'm just giving ideas that are open for discussion. I'm not demanding anything.

I'm not a skinner and have no idea if it's possible, but it might look better if the "focused" channel group is always on the left side (above the channel name column) and not in the center of the screen, along with showing the background only when focused. But this could maybe be fixed/adjusted later by a skinner? Or if one of our skinners would like to give it a spin right away in order to merge it into this PR, that would ofc be even nicer. @ronie @phil65 or anyone else?

ksooo · 2018-01-16T12:00:20Z

I know that most of the comments are minor stuff, but in most of these cases you are not willing to compromise

And that is not a bad thing.

Come on, you know me, I'm always willing to change my code if the reviewer requests more substantial things than "put a return statement where I want it to be" or "name something like I want it to be named". If something is not a bug/design flaw and purely cosmetical I tend to be not happy if I have to spend time on it, yeah. If there is no true and false both "parties" have equal rights. That's important.

Just push and merge, much easier.

I appreciate every code review, but it is a question of the right balance. Purely cosmetical stuff is imo waste of time - as long as it does not violates our coding guidelines.

ronie · 2018-01-16T12:04:13Z

@da-anda yeah, i can look into that, but it shouldn't block this PR from getting merged.

i'd say get it in as-is. if others feel the need to fine tune, they can do so in a follow-up PR.

ksooo · 2018-01-16T12:08:31Z

Oh well, lesson learned

Same here. I will most probably not touch anything UI-related for the next months. Seriously.

ksooo · 2018-01-16T12:09:46Z

And finally, I will leave it up to somebody else to hit the button for this PR...

ksooo · 2018-01-16T16:35:30Z

yeah, i can look into

@ronie I would really much appreciate if you could beautify this a bit. My skinning experiences are ... well .. limited. ;-)

ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR Component: Skin v18 Leia labels Jan 14, 2018

ksooo added this to the L 18.0-alpha1 milestone Jan 14, 2018

ksooo requested review from ronie and xhaggi January 14, 2018 18:05

ronie approved these changes Jan 14, 2018

View reviewed changes

xhaggi closed this Jan 14, 2018

xhaggi reopened this Jan 14, 2018

ksooo force-pushed the pvr-guide-channelgroups branch from db3f95d to ccde1b5 Compare January 15, 2018 06:17

xhaggi requested changes Jan 15, 2018

View reviewed changes

xhaggi reviewed Jan 15, 2018

View reviewed changes

xbmc/pvr/windows/GUIWindowPVRBase.cpp Outdated

case ACTION_MOVE_RIGHT:

case ACTION_MOVE_LEFT:

case ACTION_MOVE_UP:

case ACTION_MOVE_DOWN:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ksooo closed this Jan 15, 2018

ksooo added 2 commits January 15, 2018 23:42

[PVR][Estuary] Guide window: Add channel group selector.

89ea0b5

[PVR] Refactor channel group's path handling.

2250dcc

ksooo reopened this Jan 15, 2018

ksooo force-pushed the pvr-guide-channelgroups branch from ccde1b5 to 2250dcc Compare January 15, 2018 22:49

notspiff merged commit 7249e1b into xbmc:master Jan 16, 2018

ksooo deleted the pvr-guide-channelgroups branch January 16, 2018 12:14

[PVR][Estuary] Guide window: Channel groups selector #13364

[PVR][Estuary] Guide window: Channel groups selector #13364

Conversation

ksooo commented Jan 14, 2018 • edited

ronie left a comment

Choose a reason for hiding this comment

xhaggi commented Jan 14, 2018 • edited

xhaggi commented Jan 14, 2018

ksooo commented Jan 14, 2018 • edited

ksooo commented Jan 14, 2018

ksooo commented Jan 14, 2018 • edited

xhaggi commented Jan 14, 2018

ksooo commented Jan 14, 2018

ksooo commented Jan 14, 2018

xhaggi commented Jan 14, 2018

jjd-uk commented Jan 14, 2018

xhaggi commented Jan 14, 2018 • edited

zag2me commented Jan 14, 2018

ksooo commented Jan 15, 2018

da-anda commented Jan 15, 2018 • edited

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ksooo commented Jan 15, 2018

xhaggi commented Jan 15, 2018

ksooo commented Jan 15, 2018

ksooo commented Jan 15, 2018

ksooo commented Jan 15, 2018 • edited

ksooo commented Jan 16, 2018 • edited

ksooo commented Jan 16, 2018

xhaggi commented Jan 16, 2018

da-anda commented Jan 16, 2018

ksooo commented Jan 16, 2018 • edited

ronie commented Jan 16, 2018 • edited

ksooo commented Jan 16, 2018 • edited

ksooo commented Jan 16, 2018

ksooo commented Jan 16, 2018

ksooo commented Jan 14, 2018 •

edited

xhaggi commented Jan 14, 2018 •

edited

ksooo commented Jan 14, 2018 •

edited

ksooo commented Jan 14, 2018 •

edited

xhaggi commented Jan 14, 2018 •

edited

da-anda commented Jan 15, 2018 •

edited

ksooo commented Jan 15, 2018 •

edited

ksooo commented Jan 16, 2018 •

edited

ksooo commented Jan 16, 2018 •

edited

ronie commented Jan 16, 2018 •

edited

ksooo commented Jan 16, 2018 •

edited