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

Show when Subscriptions / Trending / Most Popular were last updated #4380

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Nov 25, 2023

Show when Subscriptions / Trending / Most Popular were last updated

Pull Request Type

  • 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

Screenshot_20231124_190539

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.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 25, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2023 01:23
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Nov 25, 2023

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
Copy link
Collaborator

Unexpected refresh button position initially
image

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

@kommunarr
Copy link
Collaborator Author

kommunarr commented Nov 25, 2023

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
Copy link
Collaborator

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
Copy link
Member

absidue commented Nov 25, 2023

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.

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

In addition to the other comment, it looks like you've added class="ref" in a couple of places, but can't see where it is referenced.

Comment on lines 58 to 61
currentLocale: function () {
return this.$i18n.locale.replace('_', '-')
},

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is unused? I'm guessing it's leftover from testing?

Suggested change
currentLocale: function () {
return this.$i18n.locale.replace('_', '-')
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thank you - as for the first one, that was a relic from before that also wasn't being used as far as I could see, but yeah, definitely need to interrogate those more rather than taking an overly cautious stance

@PikachuEXE
Copy link
Collaborator

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

Copy link
Member

Choose a reason for hiding this comment

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

Every subscriptions tab got its own separate Feed last updated. To me it makes sense but maybe to a normal user not. Maybe we should specify it a bit more Video feed last updated, Shorts feed last updated, Live feed last updated and Community feed last updated

@kommunarr
Copy link
Collaborator Author

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
Copy link
Member

absidue commented Nov 25, 2023

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
Copy link
Member

absidue commented Nov 25, 2023

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
Copy link
Collaborator Author

kommunarr commented Nov 25, 2023

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
Copy link
Member

absidue commented Nov 25, 2023

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
  1. As you have automatic fetching disabled the cache is currently empty
  2. Switch to Profile 1
  3. Fetch subscriptions
  4. Wait 5 minutes
  5. Switch to Profile 2
  6. Fetch subscriptions
  7. Switch to Profile 1
  8. 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
Copy link
Collaborator Author

kommunarr commented Nov 25, 2023

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.

@kommunarr kommunarr force-pushed the feat/refresh-button-timestamp branch from 30e5813 to 366ecec Compare November 25, 2023 20:00
@kommunarr kommunarr requested a review from absidue November 25, 2023 23:49
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.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 16, 2024
PikachuEXE
PikachuEXE previously approved these changes Apr 16, 2024
Copy link
Contributor

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

@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 Apr 17, 2024
Copy link
Contributor

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

@FreeTubeBot FreeTubeBot merged commit ab3c1b9 into FreeTubeApp:development Apr 17, 2024
5 checks passed
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Apr 18, 2024

@jasonhenriquez Uh oh found a bug: progress bar and community images will overlap on the statusbar

FreeTube_kZ6ZHyToD2.mp4

@manifest-9
Copy link

Is there an option to disable or hide the toolbar after it refreshes?

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

@manifest-9 No and IMO it shouldn't be hidden especially when something like #4803 is going to be added at some point

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.

Show when subscriptions were last updated
7 participants