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

[settings][video][PVR][listproviders][favourites] Add default play action setting. #23863

Merged
merged 5 commits into from Oct 4, 2023

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Oct 3, 2023

Adds a new setting for defining what happens when "play" action is executed, similar to what we already have for the "select" action.

Currently, despite from behaving differently at different places throughout Kodi, you cannot control whether playback will automatically resume if possible or if user shall be asked whether to start over or resume. We have complaints in the Forum, that users find it annoying to always being asked, they want automatic resume, others stated exactly the opposite. This feels like we need a setting. :-)

With this PR, behavior will be consistent throughout video windows, pvr recordings window, favourites window and listprovider lists (aka home screen widgets) ... and user-customizable via the new setting.

screenshot00003
screenshot00004

Runtime-tested on macOS and Android, latest Kodi master.

@enen92 be my guest for a review, best done commit-by-commit.

@ksooo ksooo added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality Component: PVR Component: Video Component: Settings v21 Omega Component: Favourites labels Oct 3, 2023
@ksooo ksooo added this to the Omega 21.0 Beta 1 milestone Oct 3, 2023
@ksooo ksooo requested a review from enen92 October 3, 2023 14:46
@enen92 enen92 changed the title [settings][video][PVR][lictproviders][favourites] Add default play action setting. [settings][video][PVR][listproviders][favourites] Add default play action setting. Oct 3, 2023
@ksooo
Copy link
Member Author

ksooo commented Oct 4, 2023

Fixes #23868

@the-black-eagle
Copy link
Contributor

Might just be me but I would like an option to "choose" whether to resume or re-start. However, if you decide not to implement that, this is still much better than what we have currently 👍

@ksooo
Copy link
Member Author

ksooo commented Oct 4, 2023

Might just be me but I would like an option to "choose" whether to resume or re-start.

If the setting is set to "play" and you select something with a resume point, it will present a small context menu asking whether to resume or start from the beginning. If no resume point, there is nothing to "choose", thus no dialog, straight playback.

If the setting is set to "resume", then the playback will automatically resumed if possible or playback automatically starts from beginning, no user choice here, as no options.

The help text for the setting actually explains the behavior.

@the-black-eagle
Copy link
Contributor

it will present a small context menu asking whether to resume or start from the beginning

Brilliant! Thanks.

@@ -117,6 +118,25 @@ class CVideoSelectActionProcessor : public VIDEO::GUILIB::CVideoSelectActionProc
return true;
}
};

class CVideoPlayActionProcessor : public VIDEO::GUILIB::CVideoPlayActionProcessorBase
Copy link
Member

Choose a reason for hiding this comment

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

I'll have to implement something like this for discs too

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I was already of the opinion that I may have missed some places where we also kick off video playback action. :-) Maybe we can do this in a follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it's a bit involved when coming to discs since it's tangled together in builtins

Copy link
Member Author

Choose a reason for hiding this comment

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

CAutorun::RunDisc it is maybe.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but the autorun itself also doesn't make sense (it's a poll approach from the CApp itself)...
Long story short, don't worry about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Long story short, don't worry about this.

With pleasure. Had a quick look at the code and it felt like another can of worms. I think for Omega I already have refactored enough old code.

@Hitcher
Copy link
Contributor

Hitcher commented Oct 4, 2023

Personally I think it should be 3 options -

  • Ask
  • Play
  • Resume

This way everything is covered.

@jjd-uk
Copy link
Member

jjd-uk commented Oct 4, 2023

Personally I think it should be 3 options -

  • Ask
  • Play
  • Resume

This way everything is covered.

To me that would be the ideal except I'd have

  • Choose (default - have the pop up only if this is selected)
  • Play (if you always want to auto-playback from start)
  • Resume

However if that means having to touch far more code than you wanted I could live with:

  • Play or Choose
  • Resume

@ksooo
Copy link
Member Author

ksooo commented Oct 4, 2023

@jjd-uk @HitcherUK thanks for the feedback. We will go with

msgid "Default play action"
msgid "Ask if resumable"
msgid "Resume"

... and explain in the help text the fallback to play from beginning.

@ksooo
Copy link
Member Author

ksooo commented Oct 4, 2023

@enen92 requested/discussed code and string changes are in now. Good to go in?

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks
I added a small nitpick which I think reads better on the help text.

addons/resource.language.en_gb/resources/strings.po Outdated Show resolved Hide resolved
@ksooo ksooo merged commit 0f28947 into xbmc:master Oct 4, 2023
2 checks passed
@ksooo ksooo deleted the video-play-actions branch October 4, 2023 19:43
@Hitcher
Copy link
Contributor

Hitcher commented Oct 7, 2023

Is this still a WIP because there's no 'Always play' option?

@ksooo
Copy link
Member Author

ksooo commented Oct 7, 2023

Is this still a WIP because there's no 'Always play' option?

No WIP. I do not plan to add "Always play" because I cannot see the need for this. Who would want to always ignore any resume points for all videos?

@the-black-eagle
Copy link
Contributor

Who would want to always ignore any resume points for all videos?

All I would say is, I think its preferable to have options of

  • Always do A
  • Always do B
  • Ask should I do A or B

Otherwise users either always get asked (resume point exists) or video just resumes. I guess there is plenty of stuff I once started watching where I wouldn't want to resume as I no longer remember what happened. Getting asked each time could become tedious I guess, especially if I don't remember what might have a resume point.

Just my POV. I'm not pushing for changes, just pointing out what I think users might want/think.

@ksooo
Copy link
Member Author

ksooo commented Oct 7, 2023

I guess there is plenty of stuff I once started watching where I wouldn't want to resume as I no longer remember what happened.

"Always play" means you forget always everything, thus you never want to resume.

From formal stand point I see what you mean with 3 options being best to have, looking at what users really can make use of, I doubt that "Always play" will actually used by anybody.

@Hitcher
Copy link
Contributor

Hitcher commented Oct 7, 2023

Sorry, I thought from your initial post you meant some users always want to resume and others always want to play.

@Hitcher
Copy link
Contributor

Hitcher commented Oct 8, 2023

If there's no actual play option for a Default Play Action setting, maybe it should be renamed Default Resume Action, as this setting only affects what happens when there's a resume point.

@ksooo
Copy link
Member Author

ksooo commented Oct 8, 2023

We discussed this, and decided to go with default play. You can read this up on the discussion in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Favourites Component: PVR Component: Settings Component: Video Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants