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

Don't fetch additional data when importing subscriptions #4340

Conversation

ChunkyProgrammer
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer commented Nov 16, 2023

Don't fetch additional data when importing subscriptions

Pull Request Type

  • Bugfix
  • Feature Implementation

Related issue

Description

This PR will load the subscriptions as is and will load data only when needed (thumbnails will be slowly loaded in the side bar if channels are set to be displayed there). If someone disables the sidebar and doesn't use the Subscribed Channels page then thumbnails will be added when a user navigates to a channel.

Testing

See: #3735 (comment)

Change file extension .txt to .db

(Notice how much faster importing subscriptions is)

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.19.1

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 16, 2023 04:29
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 16, 2023
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.

Also I wonder if the prompt should be auto closed after import is done
image

src/renderer/components/data-settings/data-settings.vue Outdated Show resolved Hide resolved
@kommunarr
Copy link
Collaborator

kommunarr commented Nov 17, 2023

issue (non-blocking): Minor z-index issue on the tooltip.

Screenshot_20231116_223657

@kommunarr
Copy link
Collaborator

issue (blocking): The tooltip text could use a bit more clarity on the implications of the choice & what exactly is at stake (pros and/or cons). As a user, the tooltip opens up more questions for me than it answers.

question: I'm also missing some context on the intention of this change. Is this more of a temporary workaround to that issue, a longer-term workaround because that issue has no clear fix, a way to reduce requests that is worthy as a feature on its own merits, or some mix of the three? A clearer tooltip as mentioned above would clear up my confusion.

@ChunkyProgrammer
Copy link
Member Author

issue (blocking): The tooltip text could use a bit more clarity on the implications of the choice & what exactly is at stake (pros and/or cons). As a user, the tooltip opens up more questions for me than it answers.

question: I'm also missing some context on the intention of this change. Is this more of a temporary workaround to that issue, a longer-term workaround because that issue has no clear fix, a way to reduce requests that is worthy as a feature on its own merits, or some mix of the three? A clearer tooltip as mentioned above would clear up my confusion.

The main purpose is to avoid rate limiting when importing subscriptions from other sources (invidious, newpipe, youtube) but disabling the toggle also fixes other issues for channels that are age restricted, deleted, etc. as we handle that logic on the subscription page already. It also greatly improves the speed of importing subscriptions.

Now that I think about it, I wonder if I should just remove the prompt and not make any extra requests during the import at all.

@ChunkyProgrammer ChunkyProgrammer added PR: WIP and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 18, 2023
@absidue
Copy link
Member

absidue commented Nov 20, 2023

One worry I have with adding the thumbnail fetching to the side bar, is that we might just be moving the ratelimiting problem to the side bar. #4231 will make the problem worse though as it allows you to display up to 5 columns of channels in the sidebar at the same time.

Another option instead of doign nothing is doing a much smaller number of requests e.g. maximum 50 requests, that way it won't get ratelimited but it'll still be able to show thumbnails for a bunch of channels straight away.

Also it should probably be made more robust so it can actually skip deleted/removed channels without aborting the whole import process.

@absidue
Copy link
Member

absidue commented Nov 21, 2023

If we can avoid fetching data that would probably be best.

@ChunkyProgrammer
Copy link
Member Author

If we can avoid fetching data that would probably be best.

Sounds good, I'll most likely update it this weekend to avoid fetching the data

@ChunkyProgrammer ChunkyProgrammer marked this pull request as draft November 21, 2023 17:31
auto-merge was automatically disabled November 21, 2023 17:31

Pull request was converted to draft

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

Could this fix #3211 (comment)?

@absidue
Copy link
Member

absidue commented Nov 25, 2023

Sounds like it would introduce the same issue to the side bar, that said comment mentions on the subscribed channels page, the the logic to fetch the thumbnails from the API when it's missing was copied/inspired from there.

@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: WIP labels Nov 26, 2023
@ChunkyProgrammer ChunkyProgrammer changed the title Make fetching more data optional when importing subscriptions Don't fetch additional data when importing subscriptions Nov 26, 2023
@ChunkyProgrammer ChunkyProgrammer marked this pull request as ready for review November 26, 2023 14:11
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 26, 2023 14:11
@ChunkyProgrammer
Copy link
Member Author

Could this fix #3211 (comment)?

I can't seem to reproduce that error, so it might be fixed or it might be a bit more complex

Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

issue (very minor, non-blocking): When the channel thumbnail fails to load, it still shows something ugly for a few seconds until it corrects itself.

Screenshot_20231126_090927

@ChunkyProgrammer
Copy link
Member Author

issue (very minor, non-blocking): When the channel thumbnail fails to load, it still shows something ugly for a few seconds until it corrects itself.

The thumbnail failing to load is what's used for FreeTube to correct itself, I'm not sure if there's a better way to handle it than by failing to load an image and then making a request to get an image when it fails

@PikachuEXE
Copy link
Collaborator

Right now it seems that the new version relies on invalid url and error handler to update thumbnail URLs
To properly fix this a default image (URL) needs to be displayed and a handler (on show?) to conditionally update the thumbnail
Not sure if it's quick to implement

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 16, 2024
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Mar 8, 2024

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Apr 7, 2024

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

Copy link
Contributor

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

Copy link
Contributor

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

@ChunkyProgrammer
Copy link
Member Author

I feel like this PR is more complicated than it needs to be. I might take a look at redoing this PR again but for now I will close it.

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.

[Bug]: App stuck importing subscriptions
5 participants