Hiding Watched Videos on Channel View

[palharesf]

Issue 4497 - Hiding Watched Videos on Channel View

Pull Request Type

• [ ] Bugfix
• [x] Feature Implementation
• [ ] Documentation
• [ ] Other

Related issue

Closes #4497

Description

The 'Hide Videos on Watch' setting has been moved from the 'Subscription' section to the 'Distraction Free' section. Now, it not only hides watched videos (and shorts and lives) from the subscription page, but also from the channel page (from the videos, shorts and lives tab, but not from the home tab).

Screenshots

Current implementation:

Proposed change (settings page):

Channel pages (untoggled):

Channel pages (toggled):

Tooltip:

Testing

(A)

• Navigate to a random channel
• Navigate to Videos tab
• Watch a video or manually mark it as watched -- it should only gray out
• Go to Settings --> Distraction Free --> General --> toggle 'Hide Videos on Watch'
• Return to the previously channel page
• Watched videos should not load anymore
• Untoggling the 'Hide Videos on Watch' and returning to the page should reload the videos

(B)

• Repeat all steps from the test (A), but choose a channel with few videos
• Leave two unwatched videos
• Ensure the toggle is on
• Return to the channel page
• Upon marking one of the last two videos as watched, the 'Sort By' dropdown will disappear (since there's no need to sort a single video, shorts or live)
• Screenshots:

Desktop

Windows 10 Pro OS Build: 19045.5796 Freetube v0.23.4 Beta

Additional context

[efb4f5ff-1298-471a-8973-3d47447115dc]

hi @palharesf in the testing section you mainly are mentioning how you tested it. It would be nice if you could provide some testcases (ofcourse we will be testing outside of those cases too but would be nice to have some kind of baseline)

[palharesf]

Oh, of course @efb4f5ff-1298-471a-8973-3d47447115dc - I thought the request was for information on how I tested it hahaha

For a specific Channels page, the feature is differentiating between four kinds of states:

• No videos watched, toggle off: all videos should be visible
• No videos watched, toggle on: all videos should be visible
• One or more videos watched, toggle: all videos should be visible
• One or more videos watched, toggle on: one or few videos should not be visible

If I were to write an automated test for this, I would implement these four test cases.

Is that what you were alluding to, @efb4f5ff-1298-471a-8973-3d47447115dc ?

[palharesf]

I didn't originally touch Shorts and Live as I felt those were outside the scope of the feature, but I'm happy to incorporate those changes soon to harmonize the experience. I'll work on that tomorrow unless told otherwise

[efb4f5ff-1298-471a-8973-3d47447115dc]

Oh, of course @efb4f5ff-1298-471a-8973-3d47447115dc - I thought the request was for information on how I tested it hahaha

Maybe that section has to be revised so its more clear that those need to be provided, ill look into it.

For a specific Channels page, the feature is differentiating between four kinds of states:

• No videos watched, toggle off: all videos should be visible
• No videos watched, toggle on: all videos should be visible
• One or more videos watched, toggle: all videos should be visible
• One or more videos watched, toggle on: one or few videos should not be visible If I were to write an automated test for this, I would implement these four test cases.

Is that what you were alluding to, @efb4f5ff-1298-471a-8973-3d47447115dc ?

Sorry i had to provide examples. From looking at your cases they look good. Everyone in our team does it a bit different depending on what type of PR they're sending in but these examples should give you a good view with what i meant with testcases.

• https://github.com/FreeTubeApp/FreeTube/pull/5743
• https://github.com/FreeTubeApp/FreeTube/pull/7349
• https://github.com/FreeTubeApp/FreeTube/pull/5125
• https://github.com/FreeTubeApp/FreeTube/pull/7338

[efb4f5ff-1298-471a-8973-3d47447115dc]

Also just a tip make sure your branch is up to date with the latest changes from the development branch before opening your PR.

[palharesf]

Ok, I updated the 'Testing' section of the PR to reflect the examples provided, thank you for those.

I did run git fetch origin and git merge origin/development before running git push origin feature/4497-hide-watched-videos -- all the console messages I got indicated my branch was up-to-date with the dev branch, so I'm sorry if that was not the case.

And the case you mentioned (i.e. marking videos as watched and having them only hidden in the next refresh) was mentioned in my PR as something intentional - I decided against refreshing the video Panel every time one video was marked as watched because I considered it excessive. That said, I can work on an event watcher that tracks videos being marked as 'Watched' manually and forcing a refresh of the Video Panel if that's the preferred way moving forward. I'll include that in my next batch of changes along with the Live and Shorts pages. Thank you!

[efb4f5ff-1298-471a-8973-3d47447115dc]

And the case you mentioned (i.e. marking videos as watched and having them only hidden in the next refresh) was mentioned in my PR as something intentional - I decided against refreshing the video Panel every time one video was marked as watched because I considered it excessive. That said, I can work on an event watcher that tracks videos being marked as 'Watched' manually and forcing a refresh of the Video Panel if that's the preferred way moving forward.

I think you interpreted my comment wrong. I pointed out a bug. Look at the screen recording again and try the steps to reproduce for yourself. It hides the videos when the toggle is OFF and doesnt hide them when the toggle is ON

[palharesf]

And the case you mentioned (i.e. marking videos as watched and having them only hidden in the next refresh) was mentioned in my PR as something intentional - I decided against refreshing the video Panel every time one video was marked as watched because I considered it excessive. That said, I can work on an event watcher that tracks videos being marked as 'Watched' manually and forcing a refresh of the Video Panel if that's the preferred way moving forward.

I think you interpreted my comment wrong. I pointed out a bug. Look at the screen recording again and try the steps to reproduce for yourself. It hides the videos when the toggle is OFF and doesnt hide them when the toggle is ON

You're right, I had misunderstood your request. I apologize. I believe forcing the video panel to refresh once a video is marked as watched would fix this issue.

I've been trying to implement that, but the code for marking as watched is in 'ft-list-video.js' and the code for refreshing the video panel on the Channel page is, naturally, on Channel.vue. I don't want to tightly couple the two pages, so what's the preferred approach here? Using the Vuex store to manage state?

I don't have a lot of familiarity with Vuex, but I know how to create an event emitter and listener more generally and I assume that's the preferred route here. Any pointers would be welcome.

I'll implement the toggle on the Live and Shorts pages when this is working

[absidue]

There are a few issues here but they all stem from the fact that you are overwriting the latestVideos array, which is why the setting is not working properly and also why you have to do the anti-pattern of fetching the data again from YouTube each time the switch is toggled.

So instead of overwriting the array and causing various problems, could instead create a computed which takes that array, checks the current value of the switch and if it is enabled returns a filtered copy of it, otherwise the original array. You can then reference that computed in the template instead of the latestVideos array. That way you don't have to do the unnecessary extra API requests and it will also automatically update when videos have been watched instead of only when you toggle the switch, it will also work properly with loading more data.

[palharesf]

@absidue this is gold. I was not super familiar with the computed fields, but I implemented your suggestion and besides being simpler and cleaner, it works perfectly and is very fast.

I was also able to remove the code that forces a refresh of the videoPanel when the toggle is pressed, so everything looks much cleaner and responsive. Thanks a lot for this massive tip.

@efb4f5ff-1298-471a-8973-3d47447115dc I believe this also fixes the bug you encountered.

I'll implement the toggle for Live and Shorts before updating the PR, thank you all.

[efb4f5ff-1298-471a-8973-3d47447115dc]

I tried to hide every video to see what would happen. The user must click on load more videos when all are currently loaded videos are marked as watched. Maybe its best to recognize when this is the case and auto load more videos. So if current window contains no videos because all are watched automatically load more.

https://github.com/user-attachments/assets/5d217c62-bc1a-447e-90d8-e3fbbbcd6d8e

[palharesf]

Really good tests, I appreciate it @efb4f5ff-1298-471a-8973-3d47447115dc . I have a decent idea on how to fix both, working on them now

[palharesf]

Ok, I think my current changes fixes both of those issues. I recorded a screen cap that should show it:

The one thing I noticed is that, when I mark all the videos on a page as 'Watched', I get the following error as a toast:

TypeError: Cannot read properties of null (reading 'getContinuation')

It looks like the error is being caught in the getChannelVideosLocalMore() function but I can't seem to follow what's happening. I'll push the current changes but would appreciate some guidance on what's happening in this case and how to prevent it.

[absidue]

You are likely calling the get more functions when there are no more continuations/you got to the end. The get more functions were not designed to handle being called when there is no continuation left, as previously they were only called in response to a user clicking the button and the button was hidden when there was nothing left.

Personally I would have just left it up to the user to manually press load more, but then again I'm not the target audience for this, so it won't be spamming requests to YouTube on my computer...

[palharesf]

I updated the PR, and now fetching more videos won't happen automatically. I think this is a good compromise and avoids unnecessary issues that were being brought up by the automatic fetching of additional videos.

I'll take a look later at hiding the 'Sort By' selector once there's only one video available in the video panel. I think I know how to implement it. Here hoping that after this, we're ready to put a lid on it. I'm sure there are several additional edge cases we could account for, but I'm starting to worry we're over-complicating something that should be simpler. We can always improve in the future but what's already coded more than resolve the original issue I came to support with.

[palharesf]

Ok, I updated the logic so the Sort By dropdown is hidden if the hideWatchedToggle is true and there's only one visible video in the videoPanel. If the user fetchs more videos manually, it automatically reappears.

[palharesf]

Thanks @palharesf for creating this PR and making the necessary changes so this feature will have the same standards as other features within the application which in turn creates a coherent UX. I hope we'll see more of your contributions in the future!

My pleasure! I'd be happy to keep an eye out for features involving the Channel page, as this is the page I've got somewhat familiar with. If there are any issues any of you think I could get involved, feel free to @ me :)

[efb4f5ff-1298-471a-8973-3d47447115dc]

Hi @palharesf

After reviewing the PR, I think we made the wrong call in approving the direction for the "Hide Watched Videos" toggle on the Channel page. I’ve had some time to think it over, and I feel it would be better to avoid adding new settings outside of the Settings page.

Settings like this should really be centralized in the Settings page to prevent any confusion or the potential of other settings being added to random pages. This is something we've been consistent about, so I believe it makes more sense to maintain that approach.

What we should do instead is reuse the existing setting that’s listed in the Subscription settings and move it into the General section on the Distraction Free page. This will help keep the interface clean and consistent with our usual approach.

Let me know your thoughts on this and if you need help with the change.

[palharesf]

Hi @palharesf

After reviewing the PR, I think we made the wrong call in approving the direction for the "Hide Watched Videos" toggle on the Channel page. I’ve had some time to think it over, and I feel it would be better to avoid adding new settings outside of the Settings page.

Settings like this should really be centralized in the Settings page to prevent any confusion or the potential of other settings being added to random pages. This is something we've been consistent about, so I believe it makes more sense to maintain that approach.

What we should do instead is reuse the existing setting that’s listed in the Subscription settings and move it into the General section on the Distraction Free page. This will help keep the interface clean and consistent with our usual approach.

Let me know your thoughts on this and if you need help with the change.

Hi @efb4f5ff-1298-471a-8973-3d47447115dc, Conceptually, I'm aligned with the direction of centralizing all settings on the Settings page instead of having custom settings applied into / managed by different pages.

I just want to re-hash what I see as the proposed implementation in your vision then (assuming we're starting from my edited branch):

1. Remove my toggle from the Channels Page
2. Add a toggle in the Settings page (under "Distraction Free", "General", titled 'Hide Watched Videos')
3. Update localization settings (to reflect the changes above)
4. Link all other functions and logic from the current Channels page toggle to the new Settings toggle

In summary, is that it? I think the hardest part for me will be familiarizing myself with Vuex, but it's a good time as any to become familiar with it. I just want to make sure the direction is good before moving ahead.

Also, should I open a new PR or simply implement the changes over this PR / branch? I can also change the main post to reflect the updated approach. Let me know

[efb4f5ff-1298-471a-8973-3d47447115dc]

1. Remove my toggle from the Channels Page

2. Add a toggle in the Settings page (under "Distraction Free", "General", titled 'Hide Watched Videos')

3. Update localization settings (to reflect the changes above)

4. Link all other functions and logic from the current Channels page toggle to the new Settings toggle

In summary, is that it? I think the hardest part for me will be familiarizing myself with Vuex, but it's a good time as any to become familiar with it. I just want to make sure the direction is good before moving ahead.

1. Correct
2. Correct but we can use the title of the already existing toggle that resides withing the Subscription Settings Hide Videos on Watch
3. See 2, so that would mean you can remove your localization and use the existing one. The placement does need to be changed so from https://github.com/FreeTubeApp/FreeTube/blob/development/static/locales/en-US.yaml#L531 to reside within the distraction free settings keys https://github.com/FreeTubeApp/FreeTube/blob/development/static/locales/en-US.yaml#L537
4. Correct, in addition to that remove the Subscription Settings Hide Videos on Watch toggle and add that functionality to the toggle you will add within the Distraction Free Settings. So that toggle will Hide Videos from the subscription page and the channel page.
5. Now that it will reside within the General section of the Distraction Free Settings I think it would also be good to add a tooltip with the description that this only applies to those two pages.

Also, should I open a new PR or simply implement the changes over this PR / branch? I can also change the main post to reflect the updated approach. Let me know

Feel free to take the approach you feel comfortable with

[absidue]

Just to clarify you do not need to add a new setting, you would just need to move the existing distraction free setting around in the same file and then change your channel page logic to use that existing setting instead of its own switch.

[palharesf]

Ok, good clarifications. Thank you, I'll get started on it now

[palharesf]

OK a few questions to ensure I'm on the right track:

1. It looks like settings.js is where all components get assembled together, but Distraction Free Settings and Subscription Settings are actually two different components. So I would move the 'Hide Videos on Watch' toggle from subscription-settings.vue to distraction-settings.vue according to what we previously discussed right?
2. I assume I don't need to make any changes to the store if I just move the toggle from one settings component to the other, correct?
3. From there I can just access the settings value from the store within the Channels page and update all the logic to reference it right?

I'll look into the tooltip as soon as I got the basic logic working properly. Thank you all!

[github-actions[bot]]

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

[palharesf]

I implemented the base functionality (still missing the tooltip, working on it now) -- but it's not evident to me where are the conflicts that the bot is warning me of. I'd appreciate some guidance on that while I get the tooltip up and running

[palharesf]

Ok I think we're in a good spot. Tooltip implemented (and wording reviewed), functionality seems to be working for all the cases we expected (in the subscriptions page it's unchanged, for the Channels page it works for videos, lives and shorts), and the functionality to hide the Sorting button on the Channels page when there's only one video being displayed is still working. I still didn't understand the conflicts generated by GitHub Actions, but happy to resolve them with some guidance.

Please let me know if there's any additional work anyone notices that I missed. Otherwise, thank you all for the help and I'll start looking at related issues to tackle once we close this :)

[efb4f5ff-1298-471a-8973-3d47447115dc]

We're all a bit busy at the moment. We will get back to you as soon as possible. I cant put my finger on why you are receiving the conflicts. What you can try is updating your branch so your changes are being made on top of the newest version of the dev branch

[palharesf]

Ok here's what I tried so far.

When I ran git fetch origin while on the development branch, I updated my local copy of the development branch, but that didn't automatically update my feature branch. When I switched to my feature branch and ran git fetch origin or git merge origin/development, Git didn't apply the updates to my feature branch because it was not tracking the development branch. My feature branch was likely still based on an older version of the development branch.

I have since rebased my feature branch onto the updated development branch, but that alone hasn't solved the conflicts. I checked git status and git diff and both are clean. git merge origin/development also said I was up to date. I then decided to git reset --hard and pushed it again. It says 'Everything is up-to-date' on my terminal, but seems the conflicts are still there.

I'm happy to try other approaches if anyone has suggestions.

[github-actions[bot]]

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

[PikachuEXE]

The conflict is caused by migration to composition API https://github.com/FreeTubeApp/FreeTube/pull/7335 Some files edited in this PR (before merge) no longer exist I did the merge for you since it's rare