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: gui improvements for live tv section #3387

Closed
wants to merge 18 commits into from
Closed

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Oct 6, 2013

This PR changes the way how to access the live tv channels (tv / radio) and channel groups within the gui. The channels (tv / radio) are populate into the same control list, so for skinners it's easier to integrate the channel view.
It's possible switching between the channel types (tv / radio) via a new control button.
To access the channel groups i integrated a select dialog which contains all groups. This is also accessible via a new control button.
The selected group is now also used for the EPG view, so you can easily switching between groups while you stay in epg view.

The needed skin changes are in a second commit.

screenshot002

@CharlieMarshall
Copy link
Contributor

While I'm not in a position to remark on your code. I just tried this and It works great for me. I have been wanting something like this for a while. My only remark would be I would prefer if it performed like the "Channels" & "EPG" ie not a pop up menu but it displays the group name next to group ie "Group: All" and hitting enter changes to the next one, don't know how it would handle long group names, could the text there scroll? I realise this might not suit everyone so maybe an option on the LiveTV settings page to specify how it works.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 6, 2013

The select dialog has the best usability for all situations, having long group names and if you use many groups it gives you a better overview and you can switch direcly to a bottom group.

{
return OpenGroupDialogSelect();
}
else {

This comment was marked as spam.

This comment was marked as spam.

@jmarshallnz
Copy link
Contributor

Trivials noted. @opdenkamp for full review.

@da-anda: Perhaps you could comment on the UI?

@dhead666
Copy link
Contributor

dhead666 commented Oct 7, 2013

I hope this ok to add here this remark.
Worth to mention this will remove a "hidden" remote mapping feature.

Currently I've got on my remote dedicated TV and Radio buttons which globally set to open MyPVR window with TV or Radio visible.
On MyPVR window there is another mapping in which the TV and Radio buttons are set to toggle the groups with just one button press.

  • For TV is Control.Message(32, click)
  • For Radio is Control.Message(33, click)

So maybe add NextChannelGroup action as the one in PVROSDChannels windows so this feature will not be lost.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 7, 2013

@dhead666 Thank you for your comment, but this is user-specific stuff.
Still it would be useful to integrate the actions, so that skinners can benefit from.

@opdenkamp
Copy link
Member

I'll review this one (and any other PVR related PR) tonight or tomorrow
evening.

On 10/07/2013 09:45 AM, Sascha Woo wrote:

@dhead666 https://github.com/dhead666 Thank you for your comment,
but this is user-specific stuff.
Still it would be useful to integrate the actions, so that skinners
can benefit from.


Reply to this email directly or view it on GitHub
#3387 (comment).

@ghost ghost assigned opdenkamp Oct 7, 2013
@da-anda
Copy link
Member

da-anda commented Oct 7, 2013

btw - what happened to the rework/split of the PVR windows @pieh was working on?

@ronie
Copy link
Member

ronie commented Oct 7, 2013

having to toggle between the tv and radio listings through some select dialog doesn't seem very intuitive to me.
i'd rather keep those sections separate, as they currently are.
i'm fine with the group selection functionality.

would it be possible to handle it like this, with separate tv and radio buttons on the side menu:

  • TV: All
  • Radio: All

clicking either of those buttons would bring up the group selection dialog.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 7, 2013

@ronie no select dialog to change the channel type, just click on the control in the media menu (it toggles between tv and radio).

I decided to implement this, because there is no different between tv and radio channels.

  • now using the epg depends on the selected type (before you can't use radio epg without playing a radio channel)
  • only one channel list for tv and radio, so that's easier to integrate for skinners

And to be honest, the handling is the same.

@da-anda
Copy link
Member

da-anda commented Oct 7, 2013

while you're at it, can you hide radio related options in case there are no radio channels? Can also be done as seperate PR to not hold this one back.

edit: one thing that I also don't like in the screenshot is that the user doens't know/see that "Groups" is supposed to be clickable and change something. Maybe better name it "Group: [Name of group]" or "change channel group" or something like that

@xhaggi
Copy link
Member Author

xhaggi commented Oct 8, 2013

@da-anda sure both is possible. I will check if it's easy to hide the radio stuff.

I don't use "Group: [Name of group]" as label for the control because of long group names, but you're right, so i will change it.

something else that still bothers me (not only me) that the controls (channels, EPG) immediately activate the corresponding window. This creates an unsightly behaviour, for example if you want to switch to timers.
Now i think we can use click to activate the window and if it's active cycle through the view options e.g. on guide window (timeline, channel ..)

@xhaggi
Copy link
Member Author

xhaggi commented Oct 8, 2013

xbmc_pvr

@xhaggi
Copy link
Member Author

xhaggi commented Oct 9, 2013

ahh ... i found some issues .. let you know if it's ready for review

@xhaggi
Copy link
Member Author

xhaggi commented Oct 9, 2013

fixed, reorganized and rebased .. i added an additional commit which change the behaviour for the control buttons.
now you have to click the button to change the corresponding window (channels, guide, recordings etc.) and it cycles through the view types (e.g. timeline, now, next, ..) if the window is active.

@ronie
Copy link
Member

ronie commented Oct 9, 2013

@xhaggi thanx for addressing my unfounded concerns :-)

the confluence changes are ok with me. please squash those.

i'm not too sure about the focus to click change. what's the reason you decided to alter it?
it's quite common in the skin that the content shown changes on the focus of a button,
eg. weather window and all of the settings windows...
to keep a unified experience in the skin, it might be better to leave it like that?

@xhaggi
Copy link
Member Author

xhaggi commented Oct 10, 2013

@ronie I have made these changes partly because all view options in the sidebar (video, music, pictures) are triggered by click and the more important thing for me todo this are the unsightly behaviour while switching from tv to recordings and back.

Let me explain it in detail:
Before these changes, if you stay in TV channels in the sidebar and want to switch to timers you have to move through the radio window, the guide window, the recording window and all windows pops up. if you have 100+ channels in the default group the epg needs 1-2 seconds for loading, so you stuck a bit.

okay this is the first reason. The second is more skin related. Now it is more flexible because you can reorder the button controls without thinking about in which direction the user navigate through (e.g. place the channel group selection button on top and than followed by channels, epg, etc.).
After reordering it's possible if you stay in epg view and want to change the channel group, just open the sidebar go to the channel group button (over the channels button, nothing happens!!), switch the group and close the sidebar and you will already stay in epg view.

and please at least try it ;)

@opdenkamp
Copy link
Member

eeh you removed radio. please put it back. will look at the rest asap and comment, but that just caught my attention when i was clicking through the forum threads :)

@xhaggi
Copy link
Member Author

xhaggi commented Oct 13, 2013

@opdenkamp i didn't removed radio, i only changed the way you access the radio channels (toggle between tv/radio channel types). this relates to the fake that you can't access the radio epg with the current implementation without playing a radio channel.
let me know what i should put back?

@piotrasd
Copy link

@xhaggi please test this skin (LiveTV section) - and maybe you can something port to confluence ;)
https://copy.com/byigdNDQOidBoqyt

@xhaggi PS. sorry i forget when i answer on email i just comment - this commit

@opdenkamp
Copy link
Member

hmm okay, well in my opinion radio should not be "hidden" but be accessible directly from the main window, since it's one of the primary functions of pvr.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 13, 2013

@piotrasd please stop comment on this PR, use the forum instead.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 13, 2013

@opdenkamp if you know you can toggle between tv/radio there is no need for an extra radio button, but if you think that's a must have i can add back the button. I my opinion it's totally enough if you can toggle between with one button ;)

@opdenkamp
Copy link
Member

I've just discussed it with ronie (yay for devcon :)), and we both think that the radio item should be there instead of toggling. thanks

@xhaggi
Copy link
Member Author

xhaggi commented Oct 13, 2013

okay i will re-add the button ;)

@xhaggi
Copy link
Member Author

xhaggi commented Oct 21, 2013

@opdenkamp you wrote:
so with the current code, what you're asking is not possible (again, properly) and TV and radio are primary functions and channel groups are not. So both TV and radio must be visible in the list, and not be a switch (discussed it with ronie, who is now the confluence boss ;-))

could you tell me exactly how you are imagining it, splitting up the various views and separate window for tv and radio?

@opdenkamp
Copy link
Member

well it would have to be split up in code, and be implemented like all the other windows instead of trying to handle it inside the GUIWindowPVR instance, and both TV and radio should be made separate entries in the home screen, instead of having both TV and radio under live tv

@xhaggi
Copy link
Member Author

xhaggi commented Oct 21, 2013

well, that's what I thought almost ;) .. what about recordings, it's the only part which is same for both tv and radio?

@opdenkamp i asked all this, because if this is the desired solution i will change this PR.

@xhaggi
Copy link
Member Author

xhaggi commented Oct 25, 2013

@opdenkamp let me know if I should change this

@opdenkamp
Copy link
Member

we can just send them to the same (new) recordings window, and/or add a field to the api to flag whether a recording is radio or tv

Memphiz and others added 17 commits October 30, 2013 17:30
* tv/radio channels were merged into one list
* channel type switching (tv/radio)
* adds a new selection dialog for channel groups
This adds a new label control for output the guide type (timeline,
channel etc.), so that the group label control holds the channel group
name in epg view.
This adds the ability to use built-in function ActivateWindow()
with addition [dir] parameter to open a specified pvr window.
Valid values: "tv", "radio", "guide", "recordings", "timers", "search"
@xhaggi
Copy link
Member Author

xhaggi commented Nov 1, 2013

okay.. I'll deal with it when I have some more time

@xhaggi
Copy link
Member Author

xhaggi commented Nov 6, 2013

move to #3585

@xhaggi xhaggi closed this Nov 6, 2013
@piotrasd
Copy link

piotrasd commented Nov 9, 2013

@xhaggi Hi please look at my proposition of progress bar of timeshift
http://forum.xbmc.org/showthread.php?tid=177613 Thanks

@t-nelson
Copy link
Contributor

t-nelson commented Nov 9, 2013

@piotrasd, and again...

@xhaggi xhaggi deleted the pvr branch July 17, 2014 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants