Show when Subscriptions / Trending / Most Popular were last updated

[kommunarr]

Show when Subscriptions / Trending / Most Popular were last updated

Pull Request Type

• [x] Feature Implementation

Related issue

closes #1010

Description

• Updates refresh button to be ft-refresh-widget component with new full-width bar styling and the timestamp of when the data was fetched from (re-calculated each time page is loaded or data is updated)
• Adds ft-refresh-widget to Most Popular and Trending (as well as new titles - none existed)
• Separate timestamps being stored for Subscription Videos, Live Videos, Shorts, & Community Posts, Trending, and Most Popular
• Different timestamp is stored for each profile for each type of subscription timestamp
• Refresh button is now disabled instead of removed from the DOM when loading
• Updates banner location to accommodate this change
• Minor chore handling: increases banner "close" icon size on smaller display sizes for easier pressing

Screenshots

Testing

• Test on desktop, tablet, and mobile views
• Test that buttons work properly on Subscription Videos, Live Videos, Shorts, & Community Posts, Trending, and Most Popular

Desktop

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

Additional context

Incidentally, "Fetch Feed Automatically" being disabled has no bearing on "Most Popular" and "Trending," which doesn't sound right. But that sounds like a separate issue.

[PikachuEXE]

Not tested yet, but I think the data entries for caching should be inside src/renderer/store/modules/utils.js instead of src/renderer/store/modules/settings.js

[PikachuEXE]

Unexpected refresh button position initially

Also is not reasonable to show the last feed update time when nothing is shown (after opening a new window / reopening the app)

[kommunarr]

Not tested yet, but I think the data entries for caching should be inside src/renderer/store/modules/utils.js instead of src/renderer/store/modules/settings.js

Also is not reasonable to show the last feed update time when nothing is shown (after opening a new window / reopening the app)

See discussion in Additional Context: to reiterate, I'm thinking it might be extra useful to have these in long-term state if we decide we want to have long-term state caching for, say, subscriptions. That, and I do think it's still decently neat to have that info as a tidbit for users with "Fetch Feed Automatically" ~enabled~ disabled. But am fine to change it if there is reasoned rebuttal given and/or agreement from others

[PikachuEXE]

But only useful for those with Fetch Feed Automatically enabled (and in that case it would be generated anyway instead of from local db). But in my case I have that setting disabled and nothing is shown. So it's unreasonable to see last refresh time without the actual data.

Will re-test soon (I am on Windows now)

[absidue]

Incidentally, "Fetch Feed Automatically" being disabled has no bearing on "Most Popular" and "Trending," which doesn't sound right. But that sounds like a separate issue.

Considering that setting is in the "Subscription Settings", as a user I would be very surprised if it applied to Trending and Popular. Additionally it's not necessary to have that setting apply to anything but subscriptions, because you'll only visit trending and popular when you actually want to see content. The subscriptions page is the default page at startup (unless configured otherwise), so firstly people might want to change profile before fetching subscriptions and secondly they might want to select a tab other than videos first. Also don't forget the subscriptions page makes loads of requests, whereas the Trending and popular pages only make a few.

TL;DR, subscriptions is a special page so it makes sense that that is the only page with an option to disable automatically fetching content.

[PikachuEXE]

Also now I remember https://github.com/FreeTubeApp/FreeTube/pull/3668 The "last data loading time" is much more complicated Saving a single entry won't do the job in different profiles

[kommunarr]

Also now I remember #3668 The "last data loading time" is much more complicated Saving a single entry won't do the job in different profiles

Can you take a screenshot of an existing case where the current logic is returning an inaccuracy? Had trouble grokking the proper invariant in the code to check if things had been updated from reading that thread

[absidue]

Haven't tested this pull request yet but based on the code, you only save one last refreshed timestamp, so if you have multiple profiles that were refreshed at different times, this will only keep the latest across all profiles, which makes the timestamp useless because it doesn't match what is visible on screen when it uses the cached data.

[absidue]

Also to make it even more complicated, we cache each channel separately, so if you have profiles with overlapping channels, you could be showing data that was cached at different times on the same page.

[kommunarr]

So I can see that the first suggestion is to store these data on a per-profile basis. For the second problem, there are cases where the first load will potentially be returning only cached data, in which case the label is inaccurate. And we need to be able to discern this case so that we do not update the timestamp when it occurs. Am I understanding you correctly?

Edit: Plus the question of what you want to display for the timestamp if there is only cached data, which in my opinion should either be the latest cache update or nothing at all (in the worst case scenario that this case is too effortful to implement).

Edit 2: Oh, and I see, if automatic fetching is disabled, and the cache was even partially updated since, you also need to show the timestamp of the latest cache that includes these videos.

Before I start implementing this, would it be more or less convenient/understandable to know whether a given profile Y obtained the latest of the currently visible subscriptions on X date (as is being proposed), or just to clarify in the label that this is the last timestamp of an update from any profile?

[absidue]

Here is an theoretical situation where you could end up with different refresh times in the current profile (untested as I haven't downloaded this pull request yet):

Assuming you have automatic subscription fetching disabled and this profile setup:

• All Channels (not relevant but including it for the sake of completeness)
  • Channel A
  • Channel B
  • Channel C
• Profile 1
  • Channel A
  • Channel B
• Profile 2
  • Channel B
  • Channel C

0. As you have automatic fetching disabled the cache is currently empty
1. Switch to Profile 1
2. Fetch subscriptions
3. Wait 5 minutes
4. Switch to Profile 2
5. Fetch subscriptions
6. Switch to Profile 1
7. We are now in a situation where the refresh times are different for the channels in the currently active profile, for A it is 5 minutes ago, for B it is just now

[kommunarr]

Gotcha, I think that's encapsulated in my Edit 2. As a note, before I start implementing this, would it be more or less convenient/understandable to know whether a given profile Y obtained the latest of the currently visible subscriptions on X date (as is being proposed), or just to clarify in the label that this is the last timestamp of an update from any profile? Just trying to assess the relative loss in utility versus the time investment & potential risk of being more confusing, kinda, if you just wanted to know the last time you requested new subscription data from any profile, or the value of seeing when one video was updated from the cache when all others are 30 minutes older, to give an example.

[PikachuEXE]

Already found an issue

• Switch to profile A with 1 sub only
• Refresh
• Switch to profile with all sub & no videos shown (coz not all videos cached for all subs in profile)
• Still see last updated time

I don't expect to see any last updated time when there is no video shown

[kommunarr]

* Switch to profile with all sub & no videos shown (coz not all videos cached for all subs in profile)

Sorry, not sure if I understand. The intended behavior is to show no videos if it's not all of the subscriptions included? Why is that?

[PikachuEXE]

See test case A2b in https://github.com/FreeTubeApp/FreeTube/pull/3668

[kommunarr]

Why is that?

[kommunarr]

I notice as a result of FreeTube having that behavior, there's a weird case where subscribing to a new profile with "Fetch Feed Automatically" disabled hides the existing feed (which happens to All Channels as well, naturally). (Testing on development branch; this is pre-existing behavior separate from this PR, to clarify.) Surely that can't be the desired behavior? Still accommodated it in the latest commit because I'm not here to break practices, but it brings some odd behaviors if we want to have that as one.

[PikachuEXE]

Can the bar not disappear and re-appear on refresh? Feels weird

[kommunarr]

@PikachuEXE Done now! Any thoughts on the above suggestion?

[PikachuEXE]

Which one are you referring to?

[kommunarr]

Sorry, not sure if I understand. The intended behavior is to show no videos if it's not all of the subscriptions included? Why is that?

See test case A2b in https://github.com/FreeTubeApp/FreeTube/pull/3668

I notice as a result of FreeTube having that behavior, there's a weird case where subscribing to a new profile with "Fetch Feed Automatically" disabled hides the existing feed (which happens to All Channels as well, naturally). (Testing on development branch; this is pre-existing behavior separate from this PR, to clarify.) Surely that can't be the desired behavior? Still accommodated it in the latest commit because I'm not here to break practices, but it brings some odd behaviors if we want to have that as one.

Edit: To more explicitly clarify what I would believe to be the logical behavior here, I think we should have this if statement condition be modified to be !this.updateFetchSubscriptionsAutomatically || this.postCacheForAllActiveProfileChannelsPresent. And that I revert my last commit to move the minTimestamp logic outside of this if-statement as was before, as this case would no longer exist as is.

[PikachuEXE]

subscribing to a new profile with "Fetch Feed Automatically" disabled hides the existing feed is expected since the subscription cache (video/live/shorts/etc.) no longer contain stuff for all subscribed channels in active profile If we choose to keep showing cached entries when new channel subscribed, there no way to know some entries are missing

[kommunarr]

If we choose to keep showing cached entries when new channel subscribed, there no way to know some entries are missing

Well, now this timestamp pretty much solves that problem, right? Cases under my proposal:

Case 1:

• Fetch Feed Automatically is disabled
• Profile Alice has subscription to channel A
• Switch to Profile Alice
• Load Videos from Remote. Timestamp is updated
• Profile Alice subscribes to channel B
• Alice returns to Subscriptions tab. Lack of refresh is evident by both basic reasoning on Alice's part (if automatic feed fetching is enabled, why would I see a channel B's new videos?), basic visual cues (no refresh animation), and timestamp (unless all of this was done in same exact minute).

Case 2: (same behavior as before)

• Fetch Feed Automatically is enabled
• Profile Alice has subscription to channel A
• Switch to Profile Alice
• Load Videos from Remote occurs automatically. Timestamp is updated
• Profile Alice subscribes to channel B
• Alice returns to Subscriptions tab. Lack of data refresh is evident by basic visual cues (no refresh animation) and timestamp (unless refresh and subscription were in same exact minute).

Case 3:

• Fetch Feed Automatically is disabled
• Profile Alice has subscription to channel A
• Profile Bob has subscription to channel B and C
• Switch to Profile Bob
• Load Videos from Remote. Timestamp is updated to value X
• Switch to Profile Alice
• Load Videos from Remote. Timestamp is updated to value Y
• Profile Alice subscribes to channel B
• Alice returns to Subscriptions tab. Timestamp is updated to value X. Lack of data refresh is evident by basic visual cues (no refresh animation) and timestamp (unless lots of things happened in under one minute).

Thus, manually having to make additional requests for your feed that you were just seeing a moment ago is now much more of a serious UX problem than any possible confusion of when the feed was last refreshed.

[PikachuEXE]

I would love to continue the discussion on behaviour change for subscription view but is it blocking this PR from being merged? (It's not merged yet) I don't want the discussion here distracting other reviewers from reviewing it as is (i.e. no subscription behaviour change)

Maybe @jasonhenriquez you can make a draft PR in your own repo so we can test and discuss without disturbing others

(Unless this is a PR blocking issue)

[kommunarr]

Since we have so much else in the pipeline, I'll mark this as a WIP for now, and I can reopen and/or add new changes after we discuss in a separate PR.

[github-actions[bot]]

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

[github-actions[bot]]

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

[kommunarr]

I'm fine with this PR as is now.

[github-actions[bot]]

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

[github-actions[bot]]

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