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 Playlist Sort By Video Duration #5627

Open
wants to merge 12 commits into
base: development
Choose a base branch
from

Conversation

Hoverth
Copy link

@Hoverth Hoverth commented Aug 31, 2024

Add Playlist Sort By Video Duration

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Closes: #5268

Description

Closes issue #5268 and PR #5423. It works off what @creotove put in their PR, but also includes a small notification if there is a video in the playlist that does not have a time attached to it.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 31, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 31, 2024 10:09
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 18, 2024 09:51

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 18, 2024 09:51
package.json Outdated Show resolved Hide resolved
src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 18, 2024 11:50

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 18, 2024 11:51
auto-merge was automatically disabled September 18, 2024 11:53

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 18, 2024 11:54
auto-merge was automatically disabled September 18, 2024 12:01

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 18, 2024 12:01
@Hoverth
Copy link
Author

Hoverth commented Sep 18, 2024

Odd. Now it is not displaying the toast message at all

auto-merge was automatically disabled September 18, 2024 12:13

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 18, 2024 12:13
@Hoverth
Copy link
Author

Hoverth commented Sep 18, 2024

Alright that should fix the issue.

src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 19, 2024 03:31

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 19, 2024 03:32
@PikachuEXE
Copy link
Collaborator

Works fine so far except I think the notice should be shown also when changing sort order in single playlist view (unless already shown
This is what I have in mind
(A) (already handled)

  • Enter any single playlist
  • Show notice if last used sorting based on length

(B)

  • Enter any single playlist, last used sorting NOT based on length
  • Switch sort order to based on length
  • Show notice (just once within the same view)

auto-merge was automatically disabled September 21, 2024 03:56

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 21, 2024 03:56
@Hoverth
Copy link
Author

Hoverth commented Sep 21, 2024

7143e62 moves the call to when the playlist items are sorted, so if you load a playlist and then change the sorting, it'll still show.

This means (A) and most of (B) are handled.

I'm not terribly sure how to make it only fire once though. Thoughts?

auto-merge was automatically disabled September 21, 2024 04:12

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 21, 2024 04:12
@Hoverth
Copy link
Author

Hoverth commented Sep 21, 2024

Alright, I've given single-firing only a shot, and also tweaked the time to be only 5 seconds, as 10 seemed a little long to me

@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 Sep 23, 2024
auto-merge was automatically disabled September 30, 2024 05:19

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 05:19
auto-merge was automatically disabled September 30, 2024 05:32

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 05:32
@Hoverth
Copy link
Author

Hoverth commented Sep 30, 2024

@absidue it still has to iterate through all the playlist items, but this way it'll only do it if it's needed and seems to fix all the issues raised. I'm not terribly sure how performant it is as of yet, as I'm not sure of a way to test large playlists.

@PikachuEXE
Copy link
Collaborator

I think When the timestamp of an short is known it doesnt sort them properly should be solved by updating the entry on playlist when no valid duration saved instead of reading from history every time

Out of scope for this PR anyway

@Hoverth
Copy link
Author

Hoverth commented Sep 30, 2024

I think When the timestamp of an short is known it doesnt sort them properly should be solved by updating the entry on playlist when no valid duration saved instead of reading from history every time

Also, it would probably be good to do the same for videos fetched via RSS, but yeah that's out of scope for this PR.

@absidue
Copy link
Member

absidue commented Sep 30, 2024

Is there any functional reason for doing it with the subscriptions or just a nice to have?

We previously never had a need to modify the entries because the duration was a purely visual thing, so overlaying it from the watch history was fine. It's only become a problem now that you are building functionality on top of it, which as far as I can tell has nothing to do with the subscriptions...

The reason I'm asking is that if we do have to modify all the subscription cache entries, that would prevent us from switching the videos and live caches to shallowReactive once we switch to Vue 3 and Pinia, because your suggested change would require them to stay deeply reactive. Same with the playlists really and it's even worse there because they are known to have serious performance problems.

@Hoverth
Copy link
Author

Hoverth commented Sep 30, 2024

If you add a video from the subscriptions page, and it has been fetched via RSS, it would be sorted with the value '0:00', which is incorrect (impacting the functionality). You probably wouldn't have to modify the subscriptions cache, most likely just the playlists one, as they get updated when new metadata is available, whereas I don't think the same would be needed for the subscriptions page.

Basically, I foresee two approaches: update the metadata when watching the video, which would require checking each playlist every time you watch a video, or updating the metadata from history when interacting with a playlist.

@absidue
Copy link
Member

absidue commented Sep 30, 2024

The main problem I have with having to update the durations in the playlist is that we'll have to do it every time a video is watched and there is no way to do it quickly, because it will require going through every single video in every single playlist every time and then also doing the database updates. And a large part of the reason it will be slow is because the playlists are currently deeply reactive and they will have to stay that way if we want to do those modifications to the individual playlist items.

Although as long as we make sure that that doesn't impact the actual watch history updates, so doing it in a two step process, updating the watch history first and the metadata updates afterwards, it should be okay.

src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 30, 2024 08:19

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 08:19
…ation/use a foreach loop to check the videos in a playlist
auto-merge was automatically disabled September 30, 2024 08:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 08:24
@Hoverth
Copy link
Author

Hoverth commented Sep 30, 2024

Although as long as we make sure that that doesn't impact the actual watch history updates, so doing it in a two step process, updating the watch history first and the metadata updates afterwards, it should be okay.

As that's what I've got working now, that's all good with me. One thing I would suggest, is maybe a button that reloads the metadata for all videos in a playlist? That way you don't care how long it takes, as it's an action invoked by the user themselves.

@absidue
Copy link
Member

absidue commented Sep 30, 2024

Although as long as we make sure that that doesn't impact the actual watch history updates, so doing it in a two step process, updating the watch history first and the metadata updates afterwards, it should be okay.

As that's what I've got working now, that's all good with me. One thing I would suggest, is maybe a button that reloads the metadata for all videos in a playlist? That way you don't care how long it takes, as it's an action invoked by the user themselves.

@Hoverth That might be misleading for the user as they might be expecting it to fetch everything from YouTube, when it would actually be updating based on the watch history. Fetching loads of data unnecessarily from YouTube is the last thing we would want to do.

@efb4f5ff-1298-471a-8973-3d47447115dc

Weird idea and not sure if feasible but what about using the toast message for this purpose. Message would need to be slightly modified but if we add something like this to the toast Timestamp information could be fetched from other places of the application. Click here to try to fetch the missing timestamps. This can take a while.

@Hoverth
Copy link
Author

Hoverth commented Sep 30, 2024

@absidue That's fair enough. Is there any other issues with this PR?

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Only method with side effects should be named after the action

src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
src/renderer/helpers/playlists.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
Co-authored-by: PikachuEXE <git@pikachuexe.net>
auto-merge was automatically disabled September 30, 2024 22:50

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 22:51
auto-merge was automatically disabled September 30, 2024 22:53

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 30, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Sort Playlist Videos by Length / Duration
4 participants