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

Support for youtube playlist imports in cvs #5498

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

Conversation

Sp3rick
Copy link

@Sp3rick Sp3rick commented Jul 29, 2024

Main Change:
Support for youtube playlist imports in cvs

Others:
moved some code to parseCsv
added updatePlaylistVideo for update single playlist video on database added preference.js to get unique dataset results using the preferred api

Title

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

Screenshots

Testing

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

Main Change:
Support for youtube playlist imports in cvs

Others:
moved some code to parseCsv
added updatePlaylistVideo for update single playlist video on database
added preference.js to get unique dataset results using the preferred api
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 29, 2024 19:11
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 29, 2024
@absidue
Copy link
Member

absidue commented Jul 29, 2024

I have not reviewed the code or tested this pull request yet, however I have noticed that you haven't filled in the pull request template properly including the rather important testing section which for something like this should also include example files that reviewers can use to test the pull request.

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

This could potentially close #5057

@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 Aug 1, 2024
Copy link
Contributor

github-actions bot commented Aug 1, 2024

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

auto-merge was automatically disabled August 13, 2024 10:49

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 13, 2024 10:49
Copy link
Contributor

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

auto-merge was automatically disabled August 13, 2024 10:52

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 13, 2024 10:52
auto-merge was automatically disabled August 13, 2024 10:55

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 13, 2024 10:55
@absidue
Copy link
Member

absidue commented Aug 21, 2024

I have not tested this but looking at the code alone this pull request looks like it'll require an overhaul before it could be merged.

  • The playlists.csv file contains all the metadata about the playlists, from what I can tell this pull request doesn't have any logic for that. It would probably be better to create a separate button for the YouTube playlists import, then prompt the user to selected the metadata file first and then afterwards prompt them to select the csv files for the playlists they want to import.
  • You should avoid making requests to YouTube as much as possible, during an import like this you'll likely be sending hundreds if not thousands of requests in rapid succession, which will most certainly get you ratelimited or worse (the subscriptions import already has that problem and that only makes one request per channel, which is why in the long term we want to refactor the subscriptions import to not make any requests at all). Before doing any requests you should be checking every possible cache and store (e.g. watch history, existing playlists, subscriptions cache, search cache, trending cache, popular cache etc), making requests to YouTube or Invidious should be the last resort.
  • While your preferences.js idea sounds great on paper, in practice it means that with the local API for example you'll be making about 6 requests per video (more in the future once we add support for YouTube's poToken stuff). For the import you only need one request per video because you only need a few metadata fields, whereas the watch page needs to create a real session on YouTube, make extra requests to get playable streaming URLs, fetch the full description and recommended videos, as well as attempt to bypass age-restrictions. For the local API you should be creating a dedicated function that calls YouTube.js' getBasicInfo function).

Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants