[videoplayer] Go to Chapter Start for "Previous Chapter" Past Grace Period

[CrystalP]

Description

Modify VP actions when a backwards chapter skip is requested ("previous chapter", "previous chapter or big step back" actions)

Skip requested within 5 seconds of chapter start: go to the beginning of the previous chapter (same as current behavior)
Skip requested past 5 seconds from chapter start: go to the beginning of the current chapter.

(5 seconds taken from similar code used for scenes, could be adjusted)

Motivation and context

I find the current behavior of going to the previous chapter regardless of the playing position in the current chapter unintuitive, even though it's technically correct.

For example in the situation below, skipping to previous chapter goes to the very beginning position 00:00, even though playback is almost at the end of chapter 2.

In addition, I didn't find a way to go back to the start of the currently playing chapter.

I think it's a nice improvement to extend the previous chapter action, and to make it go back to the start of the current chapter after a certain time has passed in the current chapter. It avoids the need to map another button on a remote or on keyboard for a "back to chapter start" function.

How has this been tested?

Windows, but platform is not relevant.
Files with / without chapters, skip backwards before 5 seconds of playback

What is the effect on users?

More intuitive behavior for chapter backwards skip / Down key for large step or chapter back.

Screenshots (if appropriate):

Initial situation:

Current master: "previous chapter"goes to the beginning of the file

With PR, "previous chapter" goes to start of the current chapter:

An additional "previous chapter" within 5 seconds of chapter start goes to the previous chapter (=beginning of the file here).

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• Student submission (PR was done for educational purposes and will be treated as such)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[thexai]

I like the change and I think improves usability as users surely expect skip to previous chapter allows skip to previous chapter mark. Is not intuitive skip two chapters mark back (although is technically correct).

But current change in PR only changes this for keyboard actions up / down and not if is used OSD playback menu. To maintain coherence same changes should be applied here:

xbmc/xbmc/cores/VideoPlayer/VideoPlayer.cpp

Line 4516 in 22a988a

m_messenger.Put(std::make_shared<CDVDMsgPlayerSeekChapter>(GetChapter() - 1));

and here

xbmc/xbmc/cores/VideoPlayer/VideoPlayer.cpp

Line 4389 in 22a988a

m_messenger.Put(std::make_shared<CDVDMsgPlayerSeekChapter>(GetChapter() - 1));

This allows OSD buttons has same behavior.

[CrystalP]

Good catch. Reimplemented as function GetPreviousChapter() used by the 3 callers.
Takes care of keyboard, OSD and remotes.

Tried testing the if (pMenus->IsInMenu()) branch of OnAction, but didn't find a menu that allows the condition if (pMenus->CanSeek() && GetChapterCount() > 0 && GetChapter() > 0)
A DVD returns false in CanSeek and the BR I had on hand returns CanSeek=true, GetChapterCount=1 but GetChapter=0. I think the change of the PR still makes sense for that case though.

[thexai]

Maybe move to line 372 after void SetUpdateStreamDetails(); as all others are override and usually overrides are enumerated first (anyway is only personal preference).

[CrystalP]

I wasn't sure. makes sense though, Also made it protected, we tend to make too much stuff public for no reason.