Migrate the WatchVideoPlaylist component to the composition API#8762
Open
absidue wants to merge 3 commits intoFreeTubeApp:developmentfrom
Open
Migrate the WatchVideoPlaylist component to the composition API#8762absidue wants to merge 3 commits intoFreeTubeApp:developmentfrom
absidue wants to merge 3 commits intoFreeTubeApp:developmentfrom
Conversation
absidue
requested review from
ChunkyProgrammer,
MarmadileManteater,
PikachuEXE,
efb4f5ff-1298-471a-8973-3d47447115dc and
kommunarr
github-actions
bot
added
the
PR: waiting for review
src/renderer/components/WatchVideoPlaylist/WatchVideoPlaylist.vue
Outdated
Show resolved
Hide resolved
absidue
changed the title
Member
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
There was a problem hiding this comment.
On User or Creator playlists when you enable shuffle and skip to next video you receive an error. This error wont occur when shuffle is disbaled
VirtualBoxVM_mZvTGpq7CD.mp4
absidue
force-pushed
the
watch-video-playlist-composition
branch
from
37e999d to
c7f139b
Compare
absidue
requested review from
ChunkyProgrammer and
efb4f5ff-1298-471a-8973-3d47447115dc
Member
|
needs rebase into dev |
efb4f5ff-1298-471a-8973-3d47447115dc
added
PR: changes requested
and removed
PR: waiting for review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Type
Description
This pull request migrates the
WatchVideoPlaylistcomponent to the composition API. While testing the changes I noticed that shuffling was slow so I also improved the performance of it by switching to the modern version of the Fisher-Yates shuffle algorithm, as pushing to an array in a loop requires resizing arrays and splicing an array requires moving items around in the array each time an item is removed, the new in-place shuffling algorithm is a lot faster.Screenshots
Measuremeants with my 16k item test playlist:
Before (notice how it is so slow that after clicking the shuffle button a few times in a row, V8 decides it needs to apply aggressive optimisations to the function, to avoid freezing the UI for 5.5 seconds):
After (the new approach is fast enough that V8 doesn't need to apply such aggressive optimisations):
While yes in the end the old approach ends up being faster because of the aggressive optimisations that V8 applies after clicking the shuffle button a few times, in real world situations users will probably only click the shuffle button once so a shuffle algorithm that is fast from the start is definitely the better choice.
Testing
Test both user playlists and YouTube playlists, as well as the various functions of the watch page playlist component.
Desktop