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

Add 'ask to skip' to media segments #6196

Merged
merged 12 commits into from
Oct 25, 2024
Merged

Conversation

viown
Copy link
Member

@viown viown commented Oct 14, 2024

(Does not support TV OSes just yet)

The button appears for 6 seconds, and can be shown again by moving the mouse to unhide the OSD panel.

Testing appreciated.

Changes

  • Add a "Prompt To Skip" option to media segments
  • Display "Next Episode" if it's an outro segment, and the segment exceeds or is equal to the runtime, and if there is a next track.

Screenshots
Untitled
Screenshot_20241014_105324

Issues

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 14, 2024

Cloudflare Pages deployment

Latest commit 8cc23f2
Status ✅ Deployed!
Preview URL https://f68267e6.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

Would be nice if we kept this implementation consistent with ATV (jellyfin/jellyfin-androidtv#4068):

  • Name the action ÄskToSkip"
  • Only show for segments with duration of 3 seconds or more
  • Auto-hide after 8 seconds
  • Skipping does nothing if there is only 1 second left in the segment (just like the normal skip action)

@viown
Copy link
Member Author

viown commented Oct 15, 2024

Good point. And it already checks for segments <3s.

@viown viown changed the title Add 'prompt to skip' to media segments Add 'ask to skip' to media segments Oct 15, 2024
@dmitrylyzo
Copy link
Contributor

We have UpNextDialog:

function showComingUpNext(player) {
import('../../../components/upnextdialog/upnextdialog').then(({ default: UpNextDialog }) => {
if (!(currentVisibleMenu || currentUpNextDialog)) {
currentVisibleMenu = 'upnext';
comingUpNextDisplayed = true;
playbackManager.nextItem(player).then(function (nextItem) {
currentUpNextDialog = new UpNextDialog({
parent: view.querySelector('.upNextContainer'),
player: player,
nextItem: nextItem
});
Events.on(currentUpNextDialog, 'hide', onUpNextHidden);
}, onUpNextHidden);
}
});
}

I thought we could extract it as PlaybackSubscriber, but it could be merged to MediaSegmentManager.

@viown
Copy link
Member Author

viown commented Oct 15, 2024

We have UpNextDialog:

function showComingUpNext(player) {
import('../../../components/upnextdialog/upnextdialog').then(({ default: UpNextDialog }) => {
if (!(currentVisibleMenu || currentUpNextDialog)) {
currentVisibleMenu = 'upnext';
comingUpNextDisplayed = true;
playbackManager.nextItem(player).then(function (nextItem) {
currentUpNextDialog = new UpNextDialog({
parent: view.querySelector('.upNextContainer'),
player: player,
nextItem: nextItem
});
Events.on(currentUpNextDialog, 'hide', onUpNextHidden);
}, onUpNextHidden);
}
});
}

I thought we could extract it as PlaybackSubscriber, but it could be merged to MediaSegmentManager.

Do you suggest we use the UpNextDialog for all segments or just the "Next Episode" part?

@dmitrylyzo
Copy link
Contributor

Do you suggest we use the UpNextDialog for all segments or just the "Next Episode" part?

🤔 Not for all segments, for sure. I like simple buttons for general case. UpNextDialog should be able to work without segments (I'm leaning more towards a dedicated PlaybackSubscriber), and we need to make sure it doesn't conflict with the segment action buttons.

@viown
Copy link
Member Author

viown commented Oct 15, 2024

I do like the idea of using the up next dialog over the current "Next Episode" button for outro segments as there is a possibility these could clash with each other currently.

Since I believe this dialog shows up 30s before the runtime, we can keep that behaviour as is, but also show the dialog on outro segments that result in the next track. This won't eliminate the clashing possibility completely, but it's acceptable.

@Shadowghost
Copy link
Contributor

If skipping a segment leads to playing the next episode, I think it's sensible to re-use the dialogue - but with a different text to properly indicate the skipping

@thornbill thornbill added the feature New feature or request label Oct 15, 2024
@thornbill thornbill added this to the v10.10.0 milestone Oct 15, 2024
@pimw1
Copy link

pimw1 commented Oct 17, 2024

Cloudflare Pages deployment

Latest commit 6ca8311
Status ✅ Deployed!
Preview URL https://3e8577ef.jellyfin-web.pages.dev
Type 🔀 Preview
View build logs

The into skipper works as expected. I don't have media with outro segments, so i have not tested that.

src/components/playback/playbackmanager.js Show resolved Hide resolved
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
src/components/playback/playbackmanager.js Outdated Show resolved Hide resolved
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/components/playback/skipsegment.ts Outdated Show resolved Hide resolved
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Oct 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
2 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@solidsnake1298
Copy link
Member

Skip prompts work for all segment types. As does the prompt to start the next episode for outro segments.

@thornbill thornbill merged commit fa1934a into jellyfin:master Oct 25, 2024
14 checks passed
@thornbill thornbill added the release-highlight A major new feature for the next release label Oct 25, 2024
@viown viown deleted the prompt-to-skip branch November 1, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request release-highlight A major new feature for the next release
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants