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

Fix persistent cache incorrectly having higher priority then auto fetch feed option #5743

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Sep 24, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #5724

Description

There is a bug introduced in #5185
The cache when available is showing cached entries instead of consulting auto fetch feed option first
This PR fixes it

Screenshots

Testing

(A) (from #5185)

  • Disable Fetch Feed Automatically
  • Switch/mess around with profiles if wanted
  • Load videos, live, shorts, community posts
  • Open new window(s) to see effects (should NOT auto fetch but shows cached entries)
  • Reboot app
  • Switch to different profiles to see effects (should NOT auto fetch but shows cached entries)

(B) (missing test case in previous PR)

  • Enable Fetch Feed Automatically
  • Switch/mess around with profiles if wanted
  • Open new window(s) to see effects (should auto fetch)
  • Reboot app
  • Switch to different profiles to see effects (should auto fetch)

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 24, 2024 01:01
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 24, 2024
@absidue
Copy link
Member

absidue commented Sep 24, 2024

I haven't tested it but looking at the code this pull request is wrong too, as it always bypasses the cache, but it is only supposed refresh automatically the first time you visit the subscription page in each window.

E.g.

  1. Open window to subscription page -> Should refresh
  2. Open a video
  3. Go back to subscription page -> Should use cache

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 24, 2024
@PikachuEXE
Copy link
Collaborator Author

(A) Pre-persistent cache behaviour:

  • Open new window, no cache anyway
  • Either auto fetch or show nothing

(B) Post-persistent cache behaviour, without this PR:

  • Open new window
  • If not cached (not enough cache entries for the selected profile, same as A)
  • If cached, it shows cached entries, ignore auto fetch settings

(C) Post-persistent cache behaviour, with this PR:

  • Open new window
  • If not cached (not enough cache entries for the selected profile, same as A)
  • (C1) If cached & auto fetch enabled (on) - ???
  • (C2) If cached & auto fetch disabled (off) - shows cached entries

I think we probably need to agree on what to do on C1 before I can make further changes
Specifically on what events would refresh be auto-started (persistent cache is now across all windows remember)

@absidue
Copy link
Member

absidue commented Sep 24, 2024

I think C1 should behave the same as it did before the persistent cache, as that's also what the description of the setting says. One way to implement it would be to add a boolean to the store e.g. hasAutoRefreshed, which isn't synced across windows, that way it would refresh every time a new window is opened but not when you visit the subscriptions page afterwards.

We can of course agree on a different behaviour and change the description of the setting e.g. auto-refresh when the first window is opened/app started and after that just use the cache, even when new windows are opened

@PikachuEXE
Copy link
Collaborator Author

What about profile switch?
Coz that's included in option description as well

@absidue
Copy link
Member

absidue commented Sep 24, 2024

Before the persistent cache, it only refreshed when the profile changed if there were missing cache entries (it calls loadVideosFromCacheSometimes). So if you launched with the all channels profile, switching to any other profile wouldn't refresh, as all cache entries already existed.

@PikachuEXE
Copy link
Collaborator Author

So what's expected is:
(A) New window - auto fetch once per window
(B) Switch profile - auto fetch only on cache miss case

Let me know if anything incorrect

@PikachuEXE PikachuEXE force-pushed the fix/persistent-cache-vs-auto-fetch branch from 73eb790 to 18b191d Compare September 25, 2024 08:19
@PikachuEXE
Copy link
Collaborator Author

Updated

@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Sep 26, 2024
@PikachuEXE
Copy link
Collaborator Author

@ChunkyProgrammer 👋

@FreeTubeBot FreeTubeBot merged commit 30f95e5 into FreeTubeApp:development Sep 28, 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 Sep 28, 2024
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]: v0.21.3-nightly-4778 Beta No longer refreshes subscriptions upon launch.
5 participants