Feat/different subscription behavior #2
Conversation
…ubscriptions can be found in cache" This reverts commit efd2498.
|
@PikachuEXE Here is the PR with the different behavior. Also moves repeated functions to helper functions and renames certain functions for clarity to make it a bit cleaner. The idea is that it loads all the videos it can from the cache if automatic fetching is disabled. The debate is whether showing partial videos when switching profiles would confuse any users, who would have One compromise position is that we can show the # of channels updated in the refresh label (e.g., |
kommunarr
force-pushed
the
feat/test-different-subscription-behavior
branch
from
18da33f
to
706855c
Compare
kommunarr
force-pushed
the
feat/test-different-subscription-behavior
branch
from
706855c
to
515597b
Compare
|
When users switch profile, and cached entries are shown and entries from some (1 ~ N-1) channels are absent.
to help determine whether a manual refresh is needed (Not tested yet, working) |
|
So are you thinking something like a |
|
Yup |
|
@PikachuEXE Updated now! |
kommunarr
force-pushed
the
feat/test-different-subscription-behavior
branch
from
475917a
to
d589150
Compare
| @@ -140,3 +140,36 @@ export function addPublishedDatesInvidious(videos) { | |||
| return video | |||
| }) | |||
| } | |||
|
|
|||
| export async function loadSubscriptionVideosFromCacheOrServer(subscriptionComponent) { | |||
| subscriptionComponent.isLoading = true | |||
There was a problem hiding this comment.
I don't think this is a good practice to
- pass the component inside
- assume the properties of component
This seems like a parent component's job (or mixin?)
There was a problem hiding this comment.
Mixins aren't a best practice, and neither is this I imagine. But I'm not sure what's a better practice in Vue 2 for handling code duplication.
There was a problem hiding this comment.
A renderless component as a parent, maybe? Not sure, but I can look into it if this is a serious enough code quality concern. Will wait to hear back first, though.
There was a problem hiding this comment.
I agree with Pikachu here, passing in a component seems like such a bad idea, especially as this looks like it could be restructured to avoid that without too much work.
- Don't set loading in here, that can be done in the component before and after the function call.
- Use function parameters to pass the values you need in, e.g. fetchSubscriptionsAutomatically or just access that directly from the store in here.
- You are allowed to return values from functions, in this case you can do it instead of setting the property on the component.
There was a problem hiding this comment.
I get that that's better practice, but it is annoying that all of the glue code requires almost identical code in each file. It ends up not really reducing the code duplication in these files much. Is what is in this commit closer to what you're getting at? (Ignore that the helper should now just be called something like tryLoadSubscriptionVideosFromCacheOrServer now)
There was a problem hiding this comment.
I guess I could also move the computed properties of cacheEntriesForAllActiveProfileChannels, videoCacheForAllActiveProfileChannelsPresent, and activeProfileId into the helper function since they're not being used elsewhere in these files to reduce the degree of code duplication, but that also feels bad
src/renderer/components/subscriptions-community/subscriptions-community.js
Outdated
Show resolved
Hide resolved
|
It works fine though (with a brief test) |
…community.js Co-authored-by: PikachuEXE <[email protected]>
…m:jasonhenriquez/FreeTube into feat/test-different-subscription-behavior
|
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. |
|
This PR was closed because it has been stalled for 14 days with no activity. |
Title
Pull Request Type
Related issue
Description
Screenshots
Testing
Desktop
Additional context