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] Recordings window: Fix recording groups not working (regression introduced by #10333) #10342

Merged
merged 1 commit into from Aug 28, 2016

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Aug 26, 2016

Fix recording groups not working (regression introduced by #10333).

@MilhoseVH fyi
@Jalle19 for review?

@ksooo ksooo added Type: Fix non-breaking change which fixes an issue Component: PVR v17 Krypton labels Aug 26, 2016
@ksooo ksooo added this to the Krypton 17.0-beta2 milestone Aug 26, 2016
@MilhouseVH
Copy link
Contributor

Ah missed this for tonight's build, will include it tomorrow assuming @Jalle19 finds no issues. Thanks.

@Jalle19
Copy link
Member

Jalle19 commented Aug 28, 2016

Too bad we didn't catch this last time. +1 if it fixes the regression.

@ksooo
Copy link
Member Author

ksooo commented Aug 28, 2016

Yeah I've learned now that PlayFile returns false if it is called with a folder item and this is the condition to call the base class which handles folders correctly. Imo bad design, should be much more clear now.

@ksooo
Copy link
Member Author

ksooo commented Aug 28, 2016

jenkins build this please

@ksooo
Copy link
Member Author

ksooo commented Aug 28, 2016

Travis error is not related to this PR

@ksooo ksooo merged commit 4c18c6b into xbmc:master Aug 28, 2016
@ksooo ksooo deleted the pvr-fix-recordings-folders-regression branch August 28, 2016 08:44
@bas-t
Copy link

bas-t commented Aug 28, 2016

I see you fixed the resume option too. But wasn't it supposed to ask wether to resume or start from the beginning?

@ksooo
Copy link
Member Author

ksooo commented Aug 28, 2016

I did not change resume behavior. What line of code in this PR you are referring to?

@ksooo
Copy link
Member Author

ksooo commented Aug 28, 2016

In case you mean "case SELECT_ACTION_RESUME:", there is no change in behavior, I just added a comment explaining what the two bool parameters are doing.

The abovementioned case is called when a user has the setting "default action when selecting video files" set to "resume". Then, no popup menu shall appear. If resume is not possible, the recording will automatically be played. Makes sense?

@bas-t
Copy link

bas-t commented Aug 28, 2016

Resume was broken, I guessed that lines 280, 287, and 301 in xbmc/pvr/windows/GUIWindowPVRRecordings.cpp restored the function, as it is working again. Then again, I might be wrong.
Only thing: while it is working again, I'd like to have the option, as it was previously, to either resume or start from the beginning. That was a pop up dialog. That dialog does not show right now.

I see you beat me to it, while typing I receive your reply.
The use case you refer to is indeed what I mean

@bas-t
Copy link

bas-t commented Aug 29, 2016

The context menue still offers me the choise I need (play from the beginning or play from the moment I quited), I wasn't aware of that.
While I still think it is a good choise to implement the previous behaviour: show that popup, I can do without it.
Kind regards,
Tycho.

@ksooo
Copy link
Member Author

ksooo commented Aug 29, 2016

Sorry, but i'm not aware that i have changed the behavior.

Media settings -> Videos -> Default select action = "Play"

  • select a recording with resume info => popup with two items "play from beginning" and "resume from xx:xx:xx" appears
  • select a recording without resume info => it will automatically be played from the beginning

=> this is exactly how select on a recording worked before #10333, no change in behavior

Media settings -> Videos -> Default select action = "Resume"

  • select a recording with resume info => it will be automatically started from the saved resume point
  • select a recording without resume info => it will automatically be played from the beginning

@bas-t
Copy link

bas-t commented Aug 29, 2016

"select a recording with resume info => popup with two items "play from beginning" and "resume from xx:xx:xx" appears"

That does not happen atm, prreviously it did. Bottomline: what do I have to do to make this popup show? In a previous reply you stated that "Then, no popup menu shall appear.".

I guess we have a communication problem.
Nevertheless, let's try to get on the same page.
Kind regards,
Tyucho.

@ksooo
Copy link
Member Author

ksooo commented Aug 30, 2016

That does not happen atm, prreviously it did. Bottomline: what do I have to do to make this popup show?

Okay, this is my last attempt to explain it. There is a setting that controls the behavior. Open Kodi settings, Goto "Media settings" -> "Videos". Set the setting "Default select action" to "Play".

screenshot003

@bas-t
Copy link

bas-t commented Aug 30, 2016

OK thanks, this was previously the behaviour when default action was set to resume. I obviously missed that.

@ksooo
Copy link
Member Author

ksooo commented Aug 30, 2016

"Previously" this setting was not respected by the recordings window. You could have set it to whatever you wanted, it would not have changed behavior of the recordings window. So, your statement that "this was behavior when set to 'resume' is just plain wrong, because "previously" recordings window and this setting were 100% unrelated.

It is important to me that you get that, because everything you are writing here implies that now something with 'play' action on recordings is broken. It's not! Okay?

@bas-t
Copy link

bas-t commented Aug 30, 2016

No, not OK. Everything I've written here implies only that I had it working (the resume or start from the beginning popup) if, and only if I set the default action to "resume" previously.
Where "previously" means like in Isengard, Jarvis and as I recall Krypton when it was in early alfa state.
It might be so that it never worked for you, regardless the setting of whatever default action, but that does not mean that it never worked for anyone else.
But let's not fight over this, we can simply conclude that my experience regarding the matter is not matching yours. Okay?

@bas-t
Copy link

bas-t commented Aug 30, 2016

Moreover, I thank you very much for fixing the matter, it is very usefull to me in it's present state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Fix non-breaking change which fixes an issue v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants