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 video search in user playlist feature #4622

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Jan 31, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Code based on #4597
So maybe reviewing the code after that PR is merged

Description

Add button to single playlist view for user playlists to search for videos
Visible videos changes based on input & video titles

Screenshots

image
image
image

Testing

  • Ensure remote playlist view remains unchanged
  • Ensure new button shown on user playlist view
  • Ensure search mode can be entered and exited (and visible videos reverted to all videos when exited)
  • Ensure pagination from Playlist performance improvements #4597 applied even when displaying matching videos
  • Ensure cannot move video position when search mode entered
  • Ensure can remove video items correctly when search mode entered
  • Ensure a message shown when there are no search results
  • Ensure search results showing only some videos show original indexes
  • Ensure exiting search mode would move focus back to search button
  • Ensure search button hidden in empty user playlists

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 31, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 31, 2024 07:57
@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 Feb 1, 2024
Copy link
Contributor

github-actions bot commented Feb 1, 2024

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

@PikachuEXE PikachuEXE force-pushed the feature/playlist-search-videos-in-one-user-playlist-2 branch from 769626f to 5271a80 Compare February 1, 2024 12:42
Copy link
Contributor

github-actions bot commented Feb 1, 2024

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

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Feb 2, 2024

Shouldnt the focus stay on the search button when i cancel the search?

VirtualBoxVM_wzPZm5yOY6.mp4

Umm maybe discussion is needed here but i dont the numbering in the list should change. 2 reasons against it, 1) when i click on a video to watch that is presented as nr 2 in the search it wont be nr 2 on the player page. This could lead to confusion, users can think that we support playing videos within a playlist with certain words are in it 2) That number can be used as reference point, e.g. i searched for something that is 93 in the list and i know around that video number are videos that im searching for but cant find it with search so i can go to that number in the list and start looking from there

VirtualBoxVM_tNkVsGKHjY.mp4

Copy link
Member

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

Choose a reason for hiding this comment

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

Canceling search drops error bomb

VirtualBoxVM_q6YSlWAkp6.mp4

We need to show a message here when there are no results

VirtualBoxVM_TR3UWr4ilr.mp4

@PikachuEXE
Copy link
Collaborator Author

Canceling search drops error bomb

Cannot reproduce

We need to show a message here when there are no results

image

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

Canceling search drops error bomb

Cannot reproduce

Did some digging around and i think i found reproducible steps

  1. Create playlist with duplicates
  2. search for one of the duplicate vids
  3. cancel search
  4. error
VirtualBoxVM_gvCqGi7hRQ.mp4

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Feb 7, 2024

Cannot reproduce (I did cancel afterward, lazy to make video)
image
image

I did see that error sometimes in watch page (in my custom build) when I watch videos
I think it's related to visibility detection code but I have no way to reproduce it reliably

However it's not an issue created by this PR

Copy link
Contributor

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

…-user-playlist-2

* development: (66 commits)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (Czech)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Estonian)
  Translated using Weblate (Turkish)
  Translated using Weblate (Spanish)
  Translated using Weblate (Arabic)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Chinese (Simplified))
  Fix overflow issue with share playlist icon drop-down (FreeTubeApp#4677)
  Fix the Invidious DASH manifest generation (FreeTubeApp#4672)
  Hide channel sidebar label under `more` when setting is enabled (FreeTubeApp#4678)
  Translated using Weblate (Norwegian Nynorsk)
  Translated using Weblate (Serbian)
  Remove excessive punctuation as part of distraction settings (FreeTubeApp#4673)
  Bump the stylelint group with 1 update (FreeTubeApp#4669)
  Bump swiper from 11.0.5 to 11.0.6 (FreeTubeApp#4670)
  ...

# Conflicts:
#	src/renderer/components/playlist-info/playlist-info.scss
Copy link
Contributor

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

…-user-playlist-2

* development: (37 commits)
  Fix local API search erroring, because the default parameter values were not getting applied (FreeTubeApp#4704)
  Hide date and views separator on the watch page when views are hidden (FreeTubeApp#4697)
  Translated using Weblate (Ukrainian)
  Translated using Weblate (Polish)
  Translated using Weblate (Estonian)
  Bump webpack from 5.90.1 to 5.90.3 (FreeTubeApp#4696)
  Bump sass from 1.70.0 to 1.71.0 (FreeTubeApp#4693)
  Bump electron from 28.2.2 to 28.2.3 (FreeTubeApp#4692)
  Bump the stylelint group with 1 update (FreeTubeApp#4691)
  Remove unused isLoading from ft-community-post (FreeTubeApp#4684)
  Include swiper version in CSS file name, for cache busting (FreeTubeApp#4685)
  Translated using Weblate (Chinese (Traditional))
  Fix local API erroring when the view count is missing on the channel shorts tab (FreeTubeApp#4689)
  Translated using Weblate (Croatian)
  Translated using Weblate (Polish)
  Fix Hungarian Locale Name (FreeTubeApp#4686)
  Translated using Weblate (Dutch)
  Translated using Weblate (French)
  Translated using Weblate (Spanish)
  Bump marked from 11.2.0 to 12.0.0 (FreeTubeApp#4638)
  ...
@PikachuEXE PikachuEXE force-pushed the feature/playlist-search-videos-in-one-user-playlist-2 branch from df1a811 to 0bb522c Compare February 22, 2024 23:02
@PikachuEXE
Copy link
Collaborator Author

Updated!
When will this be reviewed -,-...

absidue
absidue previously approved these changes Feb 23, 2024
@absidue
Copy link
Member

absidue commented Feb 23, 2024

When will this be reviewed -,-...
Now

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

@absidue looking for help on #4622 (comment)

@absidue
Copy link
Member

absidue commented Feb 23, 2024

Shouldnt the focus stay on the search button when i cancel the search?

Yes, it should.

Umm maybe discussion is needed here but i dont the numbering in the list should change.

It's not very efficient, but hopefully if it's only applicable in search mode, it shouldn't be too bad (presumably most people will be using pretty exact queries, so it won't have to do it for too many items).

<ft-list-video-numbered
...
  :playlist-index="!playlistInVideoSearchMode ? index : playlistItems.findIndex(i => i === item)"
  :video-index="!playlistInVideoSearchMode ? index : playlistItems.findIndex(i => i === item)"
...
/>

@PikachuEXE
Copy link
Collaborator Author

Oh this is because I keep missing comments when you have 2 (or more) in a row (It jumps me to latest review when I click the link on email)

@PikachuEXE
Copy link
Collaborator Author

Done these (added to testing)

  • Ensure search results showing only some videos show original indexes
  • Ensure exiting search mode would move focus back to search button

absidue
absidue previously approved these changes Feb 25, 2024
Copy link
Member

Choose a reason for hiding this comment

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

lets hide search when there are no vids in the list

@PikachuEXE
Copy link
Collaborator Author

Done~
image

Copy link
Member

Choose a reason for hiding this comment

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

Edit mode does not hide options on the thumbnail anymore

VirtualBoxVM_Q4pBG375lO.mp4

…-user-playlist-2

* development:
  Translated using Weblate (Basque)
  Prevent layout shifts when thumbnails load in the chapter selector (FreeTubeApp#4713)
  Bump electron-builder from 24.9.1 to 24.12.0 (FreeTubeApp#4718)
  Bump sass-loader from 14.1.0 to 14.1.1 (FreeTubeApp#4719)
  Bump sass from 1.71.0 to 1.71.1 (FreeTubeApp#4716)
  Bump the eslint group with 2 updates (FreeTubeApp#4715)
  Support opening video timestamps in a new window (FreeTubeApp#4687)
  Translated using Weblate (Basque)
  Bump webpack-dev-server from 4.15.1 to 5.0.2 (FreeTubeApp#4695)
  Translated using Weblate (Japanese)
  Translated using Weblate (Czech)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (Japanese)
  Translated using Weblate (Vietnamese)
  Bump youtubei.js to 9.1.0 for watch page fix (FreeTubeApp#4707)
  Translated using Weblate (Vietnamese)
  Translated using Weblate (Vietnamese)
  Translated using Weblate (Portuguese (Brazil))
@PikachuEXE
Copy link
Collaborator Author

  1. I checked latest dev and edit mode does not disable any action on video too
  2. Edit mode only updates title and description, no idea why we disable video actions when edit mode is on (We can but I have no idea why nor it's an issue of this PR)

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Feb 27, 2024

Hmm weird because i definitely saw this do that in this PR. I might be imagining things srry about that

@FreeTubeBot FreeTubeBot merged commit 672803d into FreeTubeApp:development Mar 6, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 6, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Mar 7, 2024
* development: (161 commits)
  Add video search in user playlist feature (FreeTubeApp#4622)
  Add discussion info when opening an issue (FreeTubeApp#4676)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (Serbian)
  Translated using Weblate (Estonian)
  Bump mini-css-extract-plugin from 2.8.0 to 2.8.1 (FreeTubeApp#4742)
  Bump electron from 28.2.3 to 29.1.0 (FreeTubeApp#4743)
  Bump lefthook from 1.6.1 to 1.6.4 (FreeTubeApp#4740)
  Bump the babel group with 2 updates (FreeTubeApp#4739)
  Bump swiper from 11.0.6 to 11.0.7 (FreeTubeApp#4741)
  Bump electron-builder from 24.12.0 to 24.13.3 (FreeTubeApp#4744)
  Split view count and published date into two lines on small displays (FreeTubeApp#4736)
  Translated using Weblate (Chinese (Traditional))
  Fix URL copied via right click menu (FreeTubeApp#4690)
  Translated using Weblate (Croatian)
  Wrap ft-icon buttons below before they go fully vertical (FreeTubeApp#4735)
  * Make activating a chapter selector makes window scroll to top like clicking on timestamp links (FreeTubeApp#4722)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (Portuguese (Brazil))
  Make video thumbnails have certain height before image loading starts to avoid layout shifts (FreeTubeApp#4723)
  ...

# Conflicts:
#	src/renderer/views/Playlist/Playlist.vue
@PikachuEXE PikachuEXE deleted the feature/playlist-search-videos-in-one-user-playlist-2 branch April 2, 2024 03:12
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.

5 participants