Respect playlist sort order in watch-video-playlist

[kommunarr]

Respect playlist sort order in watch-video-playlist

Pull Request Type

• Feature Implementation

Related issue

#5008 (comment)

Description

• Implements sort order for a given user playlist when opening it with watch-video-playlist. This ensures sort order is respected in the order it was presented on the Playlists page.
• Also sets the sorted user playlist on the Playlist route to be cached when opening that playlist in the Watch route.

Screenshots

Testing

• Test sort order is reflected in watch-video-playlist for arbitrary sort
• Ensure inserted elements are shown in the proper place as per the sort order
• (REG) Test reverse/shuffle work as intended

Desktop

• OS: OpenSUSE
• OS Version: TW

Additional context

Note: Below performance concerns will be addressed in future PRs.

Playlist sort is computed when the user playlist in watch-video-playlist is first loaded and when it is updated with additions / removals during that session. While this is not necessarily having an apparent performance effect in my testing, I would imagine it's at least a potential concern w.r.t. sum power draw across our user base.

As it stands, the O(nlogn) sorting operation is executed for each playlist load, or upon each addition/removal to a playlist while it is currently being watched. In the future, 1) we may want to consider adding an ordering property to playlistItems for the custom sort order instead of relying on its default order, 2) thus allowing the "sort" action to be one-and-done per sort preference change by storing that playlist in its sort preference, and 3) using binary search for each subsequent insertion into a playlist. This would remove the O(nlogn) operation upon each playlist load, and it would reduce the addition cost to O(klogn), where k is the number of elements being added.

• Alternatively, instead of implementing 1, we can 1b) just remove "Custom". although we'll still have to implement 2.
• Note that 2 means we'll have to update all playlists upon each sort, unless we implement it in a way that delays the sort change on a given playlist until that playlist is actually accessed (e.g., having a property on the DB object called "sortOrder" that we check if it lines up with the userPlaylistSortOrder on DB retrieval).
• We can make "Added First" the new default logical stored ordering with 1 or 1b, thus improving performance and minimizing processing power even more.
• By modifying the base ordering, the playlist thumbnail will update as per the user's preferences instead of the currently disparate logical order, which is a more preferable state of affairs.
• Having a separate user playlist update function on the Watch route specifically for incrementing/decrementing the number (and thus making an individual insert/delete) would be a noticeable improvement for large playlists, where I notice quick bookmarking or removing the quick bookmark takes a noticeable amount of time.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[kommunarr]

I'm still feeling shaky about the energy expenditure aspect of it, so I'm leaning towards shelving it this release and working on the performance suggestions.

[kommunarr]

Update 5/23:

• Nedb doesn't let you insert at a specific place. So then I thought that maybe you can do the sorting in grabAllPlaylists, but you'd also have to do it every time the locale changed as well if it's set to one of the alphabetical sorting options. I suppose that both changing your language frequently and using an alphabetical sorting option are pretty rare, so I think this is still a pretty clear net benefit.
  • Delaying applying the sort until the playlist is accessed is weird too, as the thumbnails on the UserPlaylists page would be misrepresentative, unless we want to also just load all of the sorts there as well.
• Has anyone ever looked at how YT handles moving playlist videos? It's very weird, I'm surprised it is the way it is. I'm still trying to wrap my head around it. It seems like whenever you change the sort order of the playlist, all of your changes are wiped. When you change the sort order and click "Play All," it opens & plays it in your suggested sort order. But when you move the videos around while the playlist is being watched in a different sort order, the next time you open the playlist page, it maintains the position of those manual moved video entries, but with the default Date added (newest) as the sort!

[kommunarr]

I'm seeing merit to what you're saying, although:

It will also make adding a video to a playlist more expensive, as we'll have to sort the playlist every time you add a video to the playlist, regardless of what page you are on, instead of just if you are on the playlist or watch page.

This isn't exactly true, to be pedantic. If you have the sorting applied in grabAllPlaylists, on sort change, and (for specifically the alphabetical sort options) on locale change, then you can do a binary insertion or deletion in the mutation logic at O(logn) cost (Well, O(n) counting splicing and resizing respectively), which is different from sorting the playlist. The main struggle I'm having is that when watching a playlist and adding or removing videos to it, it will require the playlist to be re-sorted in full each time, which is the full O(nlogn).

We can skip the first sort on the watch page by using the already sorted playlist from the playlists page, by using the cached playlist trick that we use for remote playlists (currently not done for the user playlists as we fetch them from the playlists store every time)

This is definitely the right choice if we're to go this path. Thanks for this suggestion.

[kommunarr]

Update for those not privy to the private discussion: We will be addressing these performance concerns separately. Only additional requested change for this PR is that we send the cached playlist with sort order enabled whenever available to avoid an unnecessary re-sort whenever pulling up the cached user playlist.

[kommunarr]

Gonna wait till #5158 is merged first.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[kommunarr]

One other unrelated note (concerning future work) with doing sorting on the fly is that we won't be able to implement placing the proper playlist thumbnail corresponding to the video at the top of the sort order on the User Playlist page as easily. I don't think we've discussed that side of things before, but it is a minor thorn in the side I was hoping we could get around to. It shouldn't be an issue on the Playlist page at least, though, and we could carve out obvious O(1) exceptions for Earliest / Latest / Custom cases (even though it's not ideal to have that be inconsistent, those are the majority of sort types in use, I'd imagine). Maybe we could even make those non-3 sort options session-specific in their duration as a workaround for this, but that's probably a bit too far out of left field until we get more feedback on how users actually use them. It's perhaps me imposing my way of using FT, but I find it likely that people would want videos to be sorted alphabetically temporarily, but they would very rarely want it as a permanent thing.

[efb4f5ff-1298-471a-8973-3d47447115dc]

Insert inserted elements are shown in the proper place

What do you mean with this? User is on the watch page and adds video to the playlist they are currently watching?

[kommunarr]

What do you mean with this? User is on the watch page and adds video to the playlist they are currently watching?

Yes, sorry, I meant to say

Ensure* inserted elements are shown in the proper place

As in, adding a video to the playlist while watching it, and ensuring it's placed in the proper place in the sort order

[PikachuEXE]

Seems to be working well

[PikachuEXE]

(Optional) I know nothing about Intl.Collator
But I would like to know if using a constant object be possible (map of locale to Collator objects lazily created)
(No idea how much it helps, if too complicated might not worth it

[kommunarr]

I'll mark it as a chore, although I'm skeptical if the access time would be considerably less than the creation time of this