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] Feature: Separate windows for "Timer Rules" and "Timers" #8561

Merged
merged 5 commits into from Dec 18, 2015

Conversation

@ksooo
Copy link
Member

ksooo commented Dec 11, 2015

Currently, PVR timer window contains both "timer rules" (repeating timers) and "timers" (oneshot timers) in one list. Users complained (and I agree) that this unnecessarily clutters window content with actually different things. Thus, this PR introduces a new window for only the timer rules and leaves the timer window with only the "real" timers (like it was before Jarvis).

screenshot000
screenshot001
screenshot002

@Jalle19 @xhaggi others... What do you think?

@ksooo ksooo added this to the K***** 17.0-alpha1 milestone Dec 11, 2015
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 11, 2015

@ronie ping, for the confluence changes...

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Dec 11, 2015

confluence code looks good to me.

@ksooo ksooo force-pushed the ksooo:pvr-separate-timer-window branch 2 times, most recently from 7468281 to 331b34e Dec 11, 2015
@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 11, 2015

why do we separate the window if this is only a possible type for a timer. would it be better to only have a filter which let you choose which kind of timer type you want to show? (both, timer or rules)

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 11, 2015

btw instead of using a label which indicates the type why not introduce different icons infront of each timer entry. not sure why people complaining about that. i don't want to switch between two windows to check what are my next scheduled timers regardless of which type the timers are.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 12, 2015

why do we separate the window if this is only a possible type for a timer.

Recording rules as timer types is purely technical stuff I "invented" because it was easy to implement with what we had already in our code base. But normal users do not understand and don't care about this, as several postings in the forum prove. For users, there are "recording rules" and "upcoming recordings". They treat this as completely different things. Users tell as that "all other" PVRs have two windows for that.

would it be better to only have a filter which let you choose which kind of timer type you want to show? (both, timer or rules)

IMHO, no. Played with this as well and for me this felt not nearly as "natural" as two windows. Using the side blade to switch the view mode "persistently" is perfectly okay, but not if you just want to do a quick look at "the other" possible items in the window.

btw instead of using a label which indicates the type why not introduce different icons infront of each timer entry.

Nice idea, but does not actually solve the problem the users complain about. Let's discuss/do this in another PR/thread, please.

i don't want to switch between two windows to check what are my next scheduled timers regardless of which type the timers are.

Hehe. Many others want. ;-)

@metaron-uk

This comment has been minimized.

Copy link
Contributor

metaron-uk commented Dec 13, 2015

Definitely a +1 from me. Being able to see at a glance what is going to record on what channel this evening without being swamped by all the outstanding 'rules' I've got set up is a godsend (talking of which, why isn't this evening's episode of Strictly recording...).

Having tested it out with yesterday's Millhouse build, I have some suggestions (maybe in this PR, maybe for subsequent PRs...):

  1. This needs a new context menu in the 'Timers' window - 'Edit Rule' or some such name. It would be good to have this in the 'Search' results and EPG context menus as well.
    In this case 'Edit' should probably become 'Edit Timer'. (Confusing if you read the code, but makes sense on the UI imo)
  2. The Recording rules sideblade needs sort by 'Type' as an option
  3. It would be brilliant if a 'Timer Count' column could be added to Recording rules view to show which rules are 'busy' (have active 'timers') and which are 'stale' (no active timers). Maybe this could be merged with the 'Status' column e.g.: Disabled/Enabled/5 Timers?
  4. If that is added, a sort by 'Status' sideblade option starts to make sense as well....
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 13, 2015

@metaron-uk I like your suggestions. Feel free to come up with PRs. Don't want to extend this PR, though.

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 13, 2015

please wait until next week before merge it in. I'll take a deeper look at it tomorrow or within the next days.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 13, 2015

@xhaggi sure. All fine. Always listening carefully to your comments. :-)

@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Dec 14, 2015

Haven't looked at the code yet but I support the change. tvheadend itself also separates recording rules from actual upcoming recordings.

@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Dec 14, 2015

Would it make sense to split the window into a real separate window (maybe not with this PR but in the future? Remember we used to have all PVR windows as a single pseudo-window and it was a pain in the ass for @xhaggi to split them, but the end result was 100% worth it.

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 14, 2015

@Jalle19 yep that's the first I was thinking about, but did not talked to @ksooo yet. We should not introduce such a hack to re-use window code. We should derive from a common abstract class where we put in all the dupe-code we need.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 14, 2015

No problems with introducing a common (abstract - don't know this detail yet) base class CGUIWindowPVRTimersBase and derive CGUIWindowPVRTimers and CGUIWindowPVRRecordingRules from this. Makes sense, although the derived classes would currently contain VERY little code.

Shall I also introduce MyPVRRecordingRules.xml which (for now) would be a 99% copy of MyPVRTimers.xml?

@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Dec 14, 2015

The skin part I'm not sure abouy, @ronie is it possible to use an include somehow?

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Dec 14, 2015

please don't add new xml files to the skin, unless strictly needed.
we're trying to reduce the number of xml files for skins, not expand it.

both windows should be able to use the same xml i think?
skins can include (or show/hide) certain parts by checking which of the two windows is visible.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 15, 2015

Currently I'm using the same xml file for both Windows. Works just fine. So, I propose to stick with the one xml approach.

@ksooo ksooo force-pushed the ksooo:pvr-separate-timer-window branch from 331b34e to 16f5280 Dec 15, 2015
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 15, 2015

@xhaggi refactored. better now?

@ksooo ksooo force-pushed the ksooo:pvr-separate-timer-window branch from 16f5280 to 4159b26 Dec 16, 2015
@ksooo ksooo changed the title [PVR] Feature: Separate windows for "Recording Rules" and "Timers" [PVR] Feature: Separate windows for "Timer Rules" and "Timers" Dec 16, 2015
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 16, 2015

Changed internal and external (UI) wording from "recording rules" to "timer rules", as requested internally by @xhaggi

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 16, 2015

@ksooo thanks 👍

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 16, 2015

you need an osx sync?

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 16, 2015

you need an osx sync?

Will provide that myself, thanks. But VS solution files update would be very welcome (don't have Windows).

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Dec 16, 2015

me too, i don't have win ;)

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 16, 2015

@Razzeee mind to provide the VS sln file update needed due to introduction of new files in this PR?

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Dec 17, 2015

Pick Razzeee@d7cc978
Compiles fine for me, but I'm having problems running it, likely only my system as this one has a new installation of VS and it might be borked.

@@ -226,8 +228,8 @@
39BD2AD91B845D40004A5A15 /* DialogHelper.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 39BD2AD71B845D40004A5A15 /* DialogHelper.cpp */; };
39C38CCA1BBFF1EE000F59F5 /* InputCodingTableKorean.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 39C38CC81BBFF1EE000F59F5 /* InputCodingTableKorean.cpp */; };
39C38CCB1BBFF1EE000F59F5 /* InputCodingTableKorean.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 39C38CC81BBFF1EE000F59F5 /* InputCodingTableKorean.cpp */; };
39C38CE11BCD600E000F59F5 /* FFmpegImage.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 39C38CDF1BCD600E000F59F5 /* FFmpegImage.cpp */; settings = {ASSET_TAGS = (); }; };

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Dec 17, 2015

Member

Seems unrelated?

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 17, 2015

Author Member

Come on! This gets auto generated by Xcode and is obviously not harmful. ;-)

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Dec 17, 2015

Member

Okay, in that case

@@ -43,6 +43,7 @@ namespace PVR

protected:
void UpdateSelectedItemPath();
virtual std::string GetDirectoryPath(void) { return ""; }

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Dec 17, 2015

Member

Could you add override since the parent is a pure virtual? Same in the other derived class.

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 17, 2015

Author Member

Sure. Will do.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 17, 2015

@Razzeee thanks

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 17, 2015

@Jalle19 now with the override's you suggested.

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 17, 2015

jenkins build this please

@ksooo ksooo force-pushed the ksooo:pvr-separate-timer-window branch from a38abce to 6ebec96 Dec 17, 2015
@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 17, 2015

jenkins build this please

@Jalle19

This comment has been minimized.

Copy link
Member

Jalle19 commented Dec 17, 2015

Seems okay to me

@ksooo

This comment has been minimized.

Copy link
Member Author

ksooo commented Dec 17, 2015

Okay then, if nobody objects I will merge this tomorrow.

ksooo added a commit that referenced this pull request Dec 18, 2015
 [PVR] Feature: Separate windows for "Timer Rules" and "Timers"
@ksooo ksooo merged commit 5cd85cb into xbmc:master Dec 18, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@ksooo ksooo deleted the ksooo:pvr-separate-timer-window branch Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.