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 batch selection mode #4198

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

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 21, 2023

Add video batch selection mode

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #2517
#1053 (go to history -> select videos you want to delete / press Ctrl+A / enter search query and press Ctrl+A -> Remove from Watched)
#413 (when playlist PR is merged: go to subscriptions -> press Ctrl+A -> Add to Playlist -> watch the playlist)
#629 (when playlist PR is merged: go to channel -> press Ctrl+A -> Add to Playlist -> watch the playlist)
#1088 (when playlist PR is merged: select videos you want to add to Watched -> Mark as Watched)
closes PikachuEXE#67

Description

Implements a Selection Mode toggle for selecting multiple videos at once and performing all the batch actions that you would on a singular video.

  • Select button on the top nav bar that changes hover/click/(multi-)select/(multi-)touch behavior to "select" a video with visible styling
    • Mouse drag selection to more easily select multiple videos while dragging mouse
  • When "Select" mode is enabled, a "three dots" button appears on the top nav right next to it (disabled if no selection is made yet)
    • The dropdown labels become plural, if applicable, when 2 or more videos are selected (use i18n interpolation)
      • With exact # of affected videos per action, e.g., save 6 videos, download 8 videos.
  • Only applies to videos (if other types of results appear, e.g., playlists, hashtags, or channels in search results, they cannot be selected in Select mode)
  • Each video's three-dot, favorite, etc. behavior only affects that video (no change)
  • Copying multiple links: by default, has a newline delimiter between each link
  • Select Mode dropdown has a Save/Unsave option
  • Select Mode dropdown has a "Clear selections" option
  • When Select mode is enabled, Ctrl+A selects all videos in the Selection mode manner
  • Selections are automatically cleared when Select Mode is toggled off
  • Select Mode is toggled off on route change
  • Users cannot open more than 15 links at one time

Incidental:

  • Adds & implements previously missing "More Options" label in localization

Video

This video does everything that needs to be tested, more or less. Only two things I forgot to include in the demo were the "mouse drag select" to drag to select multiple videos more easily in selection mode and mobile interaction.

Demo-Selection-Mode.mp4

Testing

  • Confirm Select Mode is switched to disabled & all videos are unselected when switching routes or toggling the button
  • Confirm that selection styling is never missing on any selected video, & counts always match up
  • Confirm that videos can be selected and manipulated properly in all places that videos can appear (search results, playlists, history, channel pages, recommended videos, community pages)
  • Try copying multiple types of links
  • Try doing every kind of batch action with multiple videos selected, and ensure pluralized toasts are shown
  • Try mouse drag selection
  • Try selecting videos in "mobile" using devtools
  • Confirm all visible videos are selected on pressing Ctrl+A in select mode, and ensure that the whole page is not selected when select mode is disabled
  • Select 15 videos and confirm that link-opening options are present and functional
  • Select 16 or more videos and confirm that link-opening options are no longer present
  • Examine selection styling in all/different themes
  • Select an incredibly large number of videos in selection mode; then try favoriting/unfavoriting, adding to/removing from history, etc., and ensure that nothing is breaking (e.g., counts being accurate, whether all videos are actually being affected etc.)

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.0

Additional context

  • Opening 50 videos in YouTube with a single button click can be a pretty miserable experience for some browser + device combinations. With enough videos selected, it also enables a baby DoS attack on Invidious servers with a very low barrier to entry. This is clearly a childish and subpar attack vector which most/all Invidious servers have existing protections against, but making bad stuff easily accessible is a recipe for disaster. Instituting an arbitrary cap for this goes against the spirit of letting our most vocal [power]users do whatever they want, but 99% of users will probably not want to do that 99% of the time. After considering these concerns, I instituted an arbitrary count of 15 max videos in the selection that can be opened in one go, which is a fine place for now.
  • Note that the icon title currently clearly indicates that this is a Video Selection Mode. This will not always be the case, but I made the label this way so as to best communicate its current limitations (i.e., only video entries, not playlists or channels).
  • Saving and unsaving are notably the slowest operations when done to a large quantity of videos. Their performance could be improved by refactoring the save/unsave logic and updating it to use the batch addVideos call, but this is a lower priority with the Playlists PR on the horizon. I'd prefer that we wait until the Playlists PR is merged before making more sizable alterations in this section of the codebase.
  • Note that we're replacing a good deal of labels to make them pluralized, which means that the other languages with these labels will lose them.

Let's wait on the playlist PR to be merged first.

We will need to think about what to do for lazy loading. Probably add a click/etc handler that checks if highlight-interrupting event was made at any point.
This prevents titles/thumbnails/etc. being selected when Ctrl+A is pressed in Select Mode.
…deos when 'Clear selections' button is pressed with many videos
While the more conventional 'highlighting' behavior could be argued for, this would be more awkward while we don't actually have a visible selection rectangle. This dragging behavior is a decent compromise.
Copy link
Contributor

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

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Copy link
Contributor

github-actions bot commented Jan 2, 2024

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

Copy link
Contributor

github-actions bot commented Jan 2, 2024

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

Copy link
Contributor

github-actions bot commented Jan 3, 2024

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

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

Boosting this now that playlists PR is merged

@kommunarr
Copy link
Collaborator Author

I still have some work left here (resolving conflicts, adding "Add to Playlist" to the Select Mode dropdown options). I intend to get on this next month or later. If there are any concerns about the way the code itself is currently structured, please speak up now, so it doesn't come up late in the process.

@kommunarr
Copy link
Collaborator Author

Looking into working on this soon. To reiterate my comment from February, please share any concerns with the code structure before I move forward and it's too late to attend to them. CC: @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @PikachuEXE @ChunkyProgrammer @MarmadileManteater

@absidue
Copy link
Member

absidue commented Apr 11, 2024

Other than the obvious ones of it requiring a bunch of changes due to it being abandoned for so long (make sure you check everything not just the merge conflicts), the performance problems this will cause and realising that there is probably no good way to structure the code regardless of what you do (you are trying to plug patch processing into an app that has been designed to do the opposite), I currently don't have any additional concerns no.

@kommunarr
Copy link
Collaborator Author

@absidue Thanks for providing your input! Do you have any recommendations on how you would prefer this to be implemented, or do you just personally not think there is an optimal way to do it?

@absidue
Copy link
Member

absidue commented Apr 11, 2024

I think there isn't an optimal way to do it, all the functionality that you want to do in batches, is currently handled directly in video element itself. So you'll either have to duplicate a load of stuff into the various ancestors or find some hacky way to call stuff on the video elements that are nested multiple layers deep.

@kommunarr
Copy link
Collaborator Author

From that perspective, then, is the way that things are being done in the PR the best we can do for the situation? Would you think that there are any broader cons to adding this as an optional feature?

@kommunarr
Copy link
Collaborator Author

@absidue and other teammates, still awaiting response on the above:

From that perspective, then, is the way that things are being done in the PR the best we can do for the situation? Would you think that there are any broader cons to adding this as an optional feature?

I personally would quite like this feature, but I don't want to continue work on adding it in if it's too divisive amongst the team, and/or only have any blocking concerns actually be fully expressed until after the work is actually complete. I would be disappointed to both lose this feature and have this work go to waste, but arguing takes more years off of my life than developing.

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Apr 25, 2024

I like how this feature is currently implemented and would like this to be merged bc there are way too much moments when i thought i wish i could select them all to do X

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. label Oct 21, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 22, 2024

@absidue and other teammates, still awaiting response on the above:

From that perspective, then, is the way that things are being done in the PR the best we can do for the situation? Would you think that there are any broader cons to adding this as an optional feature?

I personally would quite like this feature, but I don't want to continue work on adding it in if it's too divisive amongst the team, and/or only have any blocking concerns actually be fully expressed until after the work is actually complete. I would be disappointed to both lose this feature and have this work go to waste, but arguing takes more years off of my life than developing.

Coming back to this PR and every actions feels pretty slow when you have allot of items. Not sure if thats just because this needs to be rebased or that its related to the concerns laid out earlier. Personally i like the PR but if this PR brings more cons with it for us than pro's for the users then we should maybe close this PR and all the related issues. I would also like to hear from the others if we should continue this or just kill it.

Edit:

#413 (when playlist PR is merged: go to subscriptions -> press Ctrl+A -> Add to Playlist -> watch the playlist)
#629 (when playlist PR is merged: go to channel -> press Ctrl+A -> Add to Playlist -> watch the playlist)

I also dont this PR addresses these issues as this is a way to over engineered workaround for it

@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 22, 2024

Coming back to this PR and every actions feels pretty slow when you have allot of items. Not sure if thats just because this needs to be rebased or that its related to the concerns laid out earlier.

I would like to make two major logical changes to this PR if we are okay with the feature in the abstract as a team:

  1. Using the proper batch functions for things rather than individually calling doX x times, the latter of which is much more inefficient
  2. Handling the way that methods are accessed. Currently, we pass in the Vue component itself and puppeteer it. That is not good practice and has led to problem #1. Instead, we should be getting the data for these videos, bundling it, and using those aforementioned batch functions.

With the above two changes, we should have much better performance and handle a large number of elements better. Plus, like we discussed with our playlist performance issues, reactivity is a non-negligible part of the performance issues that should hopefully be noticeably improved post-Vue 3 migration.

#413 (when playlist PR is merged: go to subscriptions -> press Ctrl+A -> Add to Playlist -> watch the playlist)
#629 (when playlist PR is merged: go to channel -> press Ctrl+A -> Add to Playlist -> watch the playlist)

I also dont this PR addresses these issues as this is a way to over engineered workaround for it

I agree, I don't think I marked this PR as closing them, just more remarking for these users' use cases in case we don't come around to building those specific solutions.

@absidue
Copy link
Member

absidue commented Oct 22, 2024

@kommunarr is right that implementing proper batch actions would help with the performance problems. However as we have no one-size-fits-all list implementation in FreeTube, the batch actions code would likely have to be similarly specialised for all the different parts of FreeTube. The advantage of the current solution is that it doesn't have to care too much about handling all those different situations as it simulates the user manually doing the action lots of times so can rely on all of the existing code and logic.

Batching code would definitely by the right solution, I'm just not sure if the feature is worth the extra maintenance burden of having to maintain what is essentially two versions of everything, one to do the action once on user interaction and a second version to do stuff in batches.

To be completely transparent it's a feature that I have no use for, so please take my opinions with a grain of salt, as the only times I would use it would be during the testing of this pull request and any pull request in the future that goes anywhere near the affected parts of the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. PR: merge conflicts / rebase needed PR: WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Batch selection with actions [Feature Request]: Edit playlists with multi select
3 participants