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.

Sign up for GitHub

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

Jump to bottom

Add Playlist Sort By Video Duration #5627

Merged
merged 16 commits into from
Oct 11, 2024
Merged

Add Playlist Sort By Video Duration #5627

merged 16 commits into from
Oct 11, 2024

Conversation

Hoverth
Copy link
Contributor

@Hoverth Hoverth commented Aug 31, 2024 •
edited
Loading

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
PikachuEXE
PikachuEXE reviewed Sep 16, 2024
View reviewed changes
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
PikachuEXE
PikachuEXE reviewed Sep 18, 2024
View reviewed changes
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
Contributor 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
Contributor Author

Hoverth commented Sep 18, 2024

Alright that should fix the issue.

PikachuEXE
PikachuEXE requested changes Sep 19, 2024
View reviewed changes
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
Contributor Author

Hoverth commented Sep 21, 2024 •
edited
Loading

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
Contributor 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
PikachuEXE
PikachuEXE reviewed Oct 2, 2024
View reviewed changes
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.

Maybe just loop it once?
Test locally before committing that change~

src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
@Hoverth @PikachuEXE
feat: apply suggestions from code review (use a single loop instead)
38ba927 
Co-authored-by: PikachuEXE <git@pikachuexe.net>
auto-merge was automatically disabled October 2, 2024 01:40

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 2, 2024 01:41
auto-merge was automatically disabled October 2, 2024 01:43

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 2, 2024 01:43
@Hoverth
Copy link
Contributor Author

Hoverth commented Oct 2, 2024

Maybe just loop it once? Test locally before committing that change~

It does feel a little bit faster, but that could just be placebo. Either way, it should be more efficient now.

PikachuEXE
PikachuEXE previously approved these changes Oct 2, 2024
View reviewed changes
absidue
absidue reviewed Oct 3, 2024
View reviewed changes
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 October 4, 2024 09:26

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 4, 2024 09:26
auto-merge was automatically disabled October 4, 2024 09:31

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 4, 2024 09:31
@Hoverth Hoverth requested a review from absidue October 6, 2024 02:33
absidue
absidue approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Not a fan of cloning the entire playlist, but as I can't come up with a better solution at the moment and everything else about this pull request is good, I'll approve.

@FreeTubeBot FreeTubeBot merged commit 89b32de into FreeTubeApp:development Oct 11, 2024
6 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 11, 2024
Closed
1 task
efb4f5ff-1298-471a-8973-3d47447115dc added a commit that referenced this pull request Oct 11, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Add Playlist Sort By Video Duration (#5627)
f03f4b6
Closed
1 task
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Oct 12, 2024
@PikachuEXE
Merge branch 'development' into feature/history/remember-search-query
1be051e 
* development: (35 commits)
  Translated using Weblate (Hungarian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified Han script))
  Translated using Weblate (Serbian)
  Translated using Weblate (Italian)
  Translated using Weblate (English (United Kingdom))
  Translated using Weblate (French)
  Translated using Weblate (Czech)
  Translated using Weblate (Bulgarian)
  Translated using Weblate (Italian)
  Translated using Weblate (Turkish)
  Translated using Weblate (German)
  Translated using Weblate (Spanish)
  Add Playlist Sort By Video Duration (FreeTubeApp#5627)
  Translated using Weblate (Estonian)
  Translated using Weblate (Italian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified Han script))
  Translated using Weblate (Serbian)
  Translated using Weblate (Serbian)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@absidue absidue absidue approved these changes

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc Awaiting requested review from efb4f5ff-1298-471a-8973-3d47447115dc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Sort Playlist Videos by Length / Duration
6 participants
@Hoverth @PikachuEXE @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer @FreeTubeBot