-
Notifications
You must be signed in to change notification settings - Fork 900
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
Play next/prev video after removing current video from the playlist #5158
Play next/prev video after removing current video from the playlist #5158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is going wrong here: When loop is enabled it will skip a video
VirtualBoxVM_QRsYYT01CW.mp4
…al is zeroth & loop is enabled
Excellent catch John, we are very lucky to have someone with such great testing skills & discipline! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found another one
VirtualBoxVM_XhUkrcovrp.mp4
@efb4f5ff-1298-471a-8973-3d47447115dc Could you share what you did before this? From the logs, it seems like somehow a |
This is all i did VirtualBoxVM_pzRc1vEla3.mp4 |
Thanks @efb4f5ff-1298-471a-8973-3d47447115dc, fixed now. Here's one thing I added:
Here's one thing I gave up on adding:
|
cd9d91b
to
9d71186
Compare
9d71186
to
d00bc83
Compare
|
Thank you and great catches, I admittedly would never have caught these last two. I think it's all good now knock on wood |
Removed video spawns back in? VirtualBoxVM_U5A7sv2Y4J.mp4 |
Still looking into this one. I think I'm going to be fixing bugs with this PR until I die Edit: thankfully this was an obvious error from my part, now things seem good |
379e62e
to
4f37130
Compare
src/renderer/components/watch-video-playlist/watch-video-playlist.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comments only so I only wish I spot them earlier next time
@@ -232,6 +216,23 @@ export default defineComponent({ | |||
} | |||
}, | |||
methods: { | |||
findIndexOfCurrentVideoInPlaylist: function (playlist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this should be videos
/ playlistItems
findIndexOfCurrentVideoInPlaylist
is called with various source of video lists (with different sorting orders applied)
const isCurrentVideoInParsedPlaylist = this.findIndexOfCurrentVideoInPlaylist(playlist.videos) !== -1 | ||
if (!isCurrentVideoInParsedPlaylist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use double negative here... (and below)
Play next/prev video after removing current video from the playlist
Pull Request Type
Related issue
closes #4918
Description
When you delete the video that is shown as being currently watched in
watch-video-playlist
, the video index will now visually appear to be the video prior to that one. if you delete that one, it carries on the video prior, and so forth. When the video autoplays or you clickPlay Next Video
in this state, it plays the video that is shown as the one after the current video. When you clickPlay Previous Video
in this state, it plays the video that is shown as the "current" video. (Edit 2024-05-31) If the current video cannot be found in the user playlist at all, e.g., removing it while watching it & reloading the page, the index becomes the 1st video in the playlist, preventing the glitchy "0/x videos" state from ever occurring. These are all existing behaviors of YT.Note that if you were to navigate back to the previous route for a deleted playlist entry through the
Back
button, the current video index will be set to zero, as this feature is reset when you navigate to a new route. This pre-existing behavior is fine, and making this more "durable" is not worth the effort. Believe me, I've tried.Screenshots
Testing
watch-video-playlist
update to the prior videoA. Test
Play Previous Video
plays the video with the same index as the video with the play iconB. Test
Play Next Video
and autoplay plays the video with index one higher than the video with the play iconwatch-video-playlist
update to the prior video, then delete that one. See that the play icon then goes the video prior to that.A. (Optional) Repeat validations 1A and 1B in this state.
Desktop
Additional context