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

Sort videos within playlist #4921

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Apr 10, 2024

Sort videos within playlist

Pull Request Type

  • Feature Implementation

Related issue

closes #4870

Description

  • Implements sorting types:
  • A-Z / Z-A (Video title)
  • A-Z / Z-A (Channel name)
  • Custom (Custom sorting will be set when user moves a video up/down in the list)
  • Sort by Recently added / Earliest added

Implementation details:

  • Only for user playlists, not remote playlists.
  • I didn't implement Sort By Views or Video Length because those seem quite unimportant. I can implement these if someone actually wants these features.
  • The default sort preference is "Latest added first". If the user sets their sort preference in a user playlist, it applies to all of their user playlists by default.
  • The order adjustment buttons (i.e., move up/down) are not present for sort orders of Custom. Rationale: we want "Custom" orderings to persist even when the sorting preference is changed to something else.

Unrelated mini-fix:

  • Adds locale property that was being referenced (but was actually missing) in src/renderer/components/ft-playlist-add-video-prompt/ft-playlist-add-video-prompt.js

Screenshots

Screenshot_20240410_213458

Testing

  • Test all orders work properly on a remote playlist (tested with this one)
  • Test all orders work properly on a local playlist
  • Test "move/up down" buttons show only for Custom sort orders, and that they continue to work properly

Desktop

  • OS: OpenSUSE
  • OS Version: TW

More info

I have a PR coming out after this is merged for handling sorting in watch-video-playlist. I split this up to ease the testing criteria and the relative unimportance of the other sorting types compared to the big "Added" and "Custom" ones.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 10, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 10, 2024 15:15
@kommunarr kommunarr force-pushed the feat/add-playlist-sort-order branch from b1b1df7 to c07dfb9 Compare April 10, 2024 15:20
@absidue
Copy link
Member

absidue commented Apr 10, 2024

Technically, only each individual "batch" of data is sorted for remote playlists, due to obvious technical limitations. I think this is fine.

I would prefer to not have sorting for remote playlists at all, if we can't do it properly. We don't allow searching in remote playlists for the same reason.

@absidue
Copy link
Member

absidue commented Apr 10, 2024

Also personally I think that there are too many sorting options, it feels a lot like a "because we can" and not because anyone would ever actually use it (all the people that complained just want a way to reverse playlists).

I think we just need 2:

  • Default: respects the order in the playlist, move buttons are shown
  • Reversed: just reverses the playlist, otherwise respects the order in the playlist, move buttons shown but move in the correct direction in the UI, so opposite in code

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 10, 2024

I would prefer to not have sorting for remote playlists at all, if we can't do it properly. We don't allow searching in remote playlists for the same reason.

On it!

I think we just need 2: [...]

Let's talk about that some more in the Dev channel. That conception seems pretty different from what was being recommended by @efb4f5ff-1298-471a-8973-3d47447115dc and @PikachuEXE in the #4870 discussion.

@kommunarr kommunarr force-pushed the feat/add-playlist-sort-order branch from fe20a20 to 5c03d60 Compare April 10, 2024 16:46
@ChunkyProgrammer
Copy link
Member

I think these are good options but the select box definitely seems to have a lot of options for a sort by. I wonder if there should be a separate control (ex: checkbox) for ascending/descending? That way the amount of items in the select box will be halved and reversing the playlist would take one click.

@kommunarr
Copy link
Collaborator Author

I'm not opposed to that. Thoughts? @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @PikachuEXE

@PikachuEXE
Copy link
Collaborator

Disagree
Not every sort order must have a "reverse" one (weak argument I know
Having 2 UI components just to get a sort order seems unnecessary, non-standard UI/UX
You can reorder the options if you want

@PikachuEXE
Copy link
Collaborator

Can sort by component be placed on the right like most other places?
image
image
image

static/locales/en-US.yaml Outdated Show resolved Hide resolved
static/locales/en-US.yaml Outdated Show resolved Hide resolved
Comment on lines 875 to 876
Custom: Custom
CustomDescending: Custom (descending)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Custom already sound a bit weird to me
Custom (descending) makes it even more weird

I guess this means "user specified" order
User-defined? (Suggest a better if you can

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 11, 2024

I'm seeing some different perspectives on this from everyone. I think the option I'm leaning to the most is removing "Custom (Descending)", implementing @PikachuEXE's position (& most likely label) changes, and making "added descending" the default. With "Custom (Descending)" gone we're at 7 playlist sort preferences, compared to YT's 5, which isn't what I would call excessive. I think it's more that the current ordering & wording of the labels could stand to be adjusted to reduce cognitive complexity. Let me know what y'all think.

@kommunarr
Copy link
Collaborator Author

Here's how it looks like now:

Screenshot_20240410_213458

@PikachuEXE
Copy link
Collaborator

Why isn't custom the default sort order?

@kommunarr
Copy link
Collaborator Author

Rationale being that "Latest added first" is the previous behavior before the Playlist PR, and seems to be the most requested ordering from people. Custom ordering seems to be relatively less leveraged on average and thus less suited to be the default behavior.

@kommunarr kommunarr force-pushed the feat/add-playlist-sort-order branch from 54e4ba7 to 9db7e48 Compare April 11, 2024 02:59
@PikachuEXE
Copy link
Collaborator

I am fine having default option changed with the last used sort order is saved

But we should disable order changing UI unless the sort order is "custom"
image

(YT just persists the sort order when a sort order option picked, it sucks)

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 11, 2024

But we should disable order changing UI unless the sort order is "custom"

One step ahead of you:

:can-move-video-up="index > 0 && !playlistInVideoSearchMode && isSortOrderCustom"
:can-move-video-down="index < playlistItems.length - 1 && !playlistInVideoSearchMode && isSortOrderCustom"

Update: Updated the PR description with changes discussed.

PikachuEXE
PikachuEXE previously approved these changes Apr 11, 2024
Comment on lines 20 to 22
// TODO: store video.published for user playlists
// DatePublishedNewest: 'date_published_newest',
// DatePublishedOldest: 'date_published_oldest',
Copy link
Member

@absidue absidue Apr 11, 2024

Choose a reason for hiding this comment

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

If you want to implement this, you'll have to keep in mind that shorts don't have a published date on the shorts channel tab or in shorts only playlists (not returned in youtube's response, thats's why we always use RSS on the shorts tab on the subscriptions page), so if someone were to add a video to a playlist from there, it would make it impossible to sort by published. Also the Invidious API never returns published dates inside playlists, so if someone were to copy a playlist while using the Invidious API, the published dates would be missing for all videos.

For the watch history we just don't store the published date if it isn't available, but we also don't have any sorting functionality that would depend on it.

Also unlike the watch history where marking as watched from a list is probably quite uncommon (most people probably let the watch page do it automatically), adding to a playlist from a list is probably a lot more common (especially the quick bookmark).

TL;DR: I don't think we should even attempt to support it, due to all of the situations where it wouldn't be possible.

Copy link
Collaborator Author

@kommunarr kommunarr Apr 11, 2024

Choose a reason for hiding this comment

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

Thanks for this context absidue. Just removed the code for these.

Context from absidue: 'I don't think we should even attempt to support it, due to all of the situations where it wouldn't be possible.'
Copy link
Contributor

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

@kommunarr
Copy link
Collaborator Author

Didn't mean to push that to this branch, intended to put that in a second PR. My bad! Reset now

PikachuEXE
PikachuEXE previously approved these changes Apr 13, 2024
@kommunarr kommunarr mentioned this pull request Apr 14, 2024
1 task
@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 14, 2024
Copy link
Contributor

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

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 16, 2024
Copy link
Contributor

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

@FreeTubeBot FreeTubeBot merged commit 9815ed3 into FreeTubeApp:development Apr 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Sort videos within playlist
6 participants