Hiding Watched Videos on Channel View #7366
Conversation
…, and helper variable/function created
… function; considering moving the check logic to outside the function and call it without checks inside of it
…mmented out filtering implementation for now as I debug the code further
…an event watched to update the video list if the toggle is pressed; separated variable check from inside the filter function (still not working)
…ryCache from the store, and iterate through the videos arrray filtering out entries that exist in the History
github-actions
bot
added
the
PR: waiting for review
|
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) |
|
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:
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 ? |
|
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 |
Maybe that section has to be revised so its more clear that those need to be provided, ill look into it.
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. |
|
Also just a tip make sure your branch is up to date with the latest changes from the development branch before opening your PR. |
efb4f5ff-1298-471a-8973-3d47447115dc
added
PR: changes requested
and removed
PR: waiting for review
|
Ok, I updated the 'Testing' section of the PR to reflect the examples provided, thank you for those. I did run 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! |
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 |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
uhhh i think your force push just bring back the issue from before |
|
Ok, I updated the main post with the current implementation, current screenshots and explanations While doing some testing I noticed the logic for hiding the 'Sort By' dropdown when there was only one live left in the |
|
Always a good idea to pull before doing any new changes, especially when you know that someone else has made changes to the branch. What happened here is you were working with an older copy of the branch, so when you tried to push your changes, git rejected the push because the branches didn't match anymore, so then instead of fixing the issue, you force pushed telling git to ignore everything on GitHub overwrite it with your broken local copy of the branch. |
|
@palharesf Before anyone else attempts to fix the conflicts again for you, can we please agree that you won't force push anymore? When git tells you something is wrong it is accurate 99.99% of the time, so before your override it just to make the alert go away, think about why git is telling you that. |
|
Hi @absidue, Sounds good, and sorry again. I had force-pushed yesterday without apparent negative consequences, and wrongly assumed I could do the same again without realizing it would undo the fixes, but I recognize my mistake. Are we able to revert my last push, so we return to the previous state (without conflicts)? There is only one line changed between that state and the current and might be an easier fix than addressing the conflicts again. Since we're already at it, was there anything I could have done besides what I outlined here (#7366 (comment)) to fix my branch? I just tried running |
PikachuEXE
force-pushed
the
feature/4497-hide-watched-videos
branch
from
5211fe6 to
72768e7
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
I forced push my changes (same one as yesterday) again |
Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
Head branch was pushed to by a user without write access
|
One change that is still required (but that I've been unable to push) is on 'Channels.vue', line 64, inside the This was the fix that I had force-pushed previously and that broke the fix for the conflicts |
Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
Head branch was pushed to by a user without write access
| @@ -41,7 +41,7 @@ | |||
| <div class="select-container"> | |||
| <FtSelect | |||
| v-if="showVideoSortBy" | |||
| v-show="currentTab === 'videos' && latestVideos.length > 0" | |||
| v-show="currentTab === 'videos' && (hideWatchedSubs ? filteredVideos.length > 1 : filteredVideos.length > 0)" | |||
There was a problem hiding this comment.
No idea why we need different logic based on Hide Videos on Watch enabled or not
Probably just need filteredVideos.length > 1 here
Or do you mean the opposite?
When watched entries are hidden, allow changing the sorting due to having 1 entry does not mean 1 entry only in other orders (e.g. all except 1 watched, but all oldest still not watched)
However if this is the case the criteria should be on NOT filtered entries = latestVideos.length > 1 (i.e. more than 1 entries from remote = possibility to have unwatched entries in other sorting orders
I might get some of this wrong let me know
There was a problem hiding this comment.
Current behavior in dev branch is that the sort by dropdown gets hidden when there is only one video available in the tabs. With the addition of this feature it should basically do the same. So when there is one video available in the tab because all others are marked as watched, hide sort by dropdown
Edit: oh wait a sec...idk why im just noticing now or maybe it was bad guidance of me prior to this but it hides the sort by dropdown when fetch more results is available, which means sorting is still possible. Sort by dropdown should only be hidden when there is one video listed in the tab AND when there fetch more results button isnt there. So if im reading the code correctly Pika is right, the logic we had in place is sufficient
There was a problem hiding this comment.
I agree, there's no need to differentiate the behavior depending on the state of hideWatchedSubs -- as long as filteredVideos.lenght >1 is met, the dropdown should appear.
I believe when I started working on the feature, the dropdown still appeared even when there was only one video in the page, and I didn't want to touch the original condition, so that would explain why I added another condition looking at hideWatchedSubs, but there's no good reason to keep it that way
I will try to push this fix (without forcing it) and let you know if I have trouble doing so
There was a problem hiding this comment.
The original code checks YouTube's response for the dropdown, which only hides it when there is only 1 or 0 videos and there are no continuations. You need to ensure that the original behaviour still exists when the hide watched setting is disabled.
So the important thing for this pull requests is that you cannot hide it just based on filteredVideos.length < 2, because if there is a continuation available, you don't actually know that all videos are hidden, because there may be some visible ones that were not fetched yet.
There was a problem hiding this comment.
My proposal was to only show the dropdown if we are in one of the tabs it would appear (currentTab === 'videos' and the same for shorts and lives) and if the array containing the videos to be displayed, after computing for hiding watched videos, has more than one video (`filteredVideos.length >1)
So basically just switching
v-show="currentTab === 'videos' && (hideWatchedSubs ? filteredVideos.length > 1 : filteredVideos.length > 0)"
for
` v-show="currentTab === 'videos' && filteredVideos.length > 1"
It makes sense to me unless I'm missing something, and I just tested and it works. Let me know if you see any issues with that approach
There was a problem hiding this comment.
Ohhh interesting. Thank you for the guidance, I was not sure if I could bring a new branch 'up-to-speed' with this PR. I'll try it later today. I don't think I have gh installed but I'll check it out. Thank you very much
There was a problem hiding this comment.
I don't know your needs, but if you prefer a GUI over the command line, I recommend using GitHub desktop: https://github.com/apps/desktop
There was a problem hiding this comment.
The original code checks YouTube's response for the dropdown, which only hides it when there is only 1 or 0 videos and there are no continuations. You need to ensure that the original behaviour still exists when the hide watched setting is disabled.
So the important thing for this pull requests is that you cannot hide it just based on filteredVideos.length < 2, because if there is a continuation available, you don't actually know that all videos are hidden, because there may be some visible ones that were not fetched yet.
@absidue I am reviewing the code now, and since filteredVideos is computed from latestVideos, I would assume it preserves its properties i.e. if latestVideos had no continuations, filteredVideos will have no continuations. Is that reasonable?
As a way of testing that, I logged the length of both filteredVideos and latestVideos for a channel where I have hidden two videos from the first page. You can see that the offset is always two, i.e. even initially latestVideos does not hold data from the whole playlist, just from the first page. While I can try to incorporate data from the full playlist (including continuations) in the current check, it doesn't look like this is how it works currently with latestVideos unless I'm missing something here.
TL;DR - if the current implementation was taking continuations into account, I believe latestVideos should be initialized with 117 instead of 30. Let me know if I'm not understanding something.
There was a problem hiding this comment.
Assuming we agree on our point regarding continuations (i.e. my changes do not modify the base behavior), I wanted to push the last remaining commit but I'm getting an eslint error.
Changes I'm trying to commit:
Error message I'm getting (even after creating a new branch and checking out this PR):
I believe these changes would resolve the outstanding comments on this PR while addressing the concerns raised on the discussion here, but I'm unsure on how to push them with this new error. If it's easier, someone could suggest those changes and I could simple approve them on this PR, otherwise help on how to solve this issue locally would be welcome. Thanks
There was a problem hiding this comment.
You can try do request changes yourself on the lines you want to change. After the change is submitted a button should appear to commit the change.
firefox_EpnLv4l8wR.mp4
| @@ -51,7 +51,7 @@ | |||
| /> | |||
| <FtSelect | |||
| v-if="!hideChannelShorts && showShortSortBy" | |||
| v-show="currentTab === 'shorts' && latestShorts.length > 0" | |||
| v-show="currentTab === 'shorts' && (hideWatchedSubs ? filteredShorts.length > 1 : filteredShorts.length > 0)" | |||
| @@ -61,7 +61,7 @@ | |||
| /> | |||
| <FtSelect | |||
| v-if="!hideLiveStreams && showLiveSortBy" | |||
| v-show="currentTab === 'live' && latestLive.length > 0" | |||
| v-show="currentTab === 'live' && (hideWatchedSubs ? filteredLive.length > 1 : filteredLive.length > 0)" | |||
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
* development: (50 commits) add rules to issue templates (FreeTubeApp#7516) Translated using Weblate (Romanian) Translated using Weblate (Serbian) Translated using Weblate (Russian) Move settings to `more` menu on smaller mobile devices (FreeTubeApp#7506) Translated using Weblate (Russian) Migrate WatchVideoLiveChat to the composition API (FreeTubeApp#7494) Translated using Weblate (Basque) Translated using Weblate (Russian) Bump shaka-player from 4.14.14 to 4.14.15 (FreeTubeApp#7501) Bump electron from 36.3.1 to 36.3.2 (FreeTubeApp#7500) Bump sass from 1.89.0 to 1.89.1 (FreeTubeApp#7499) Bump the eslint group with 3 updates (FreeTubeApp#7498) Bump the stylelint group with 2 updates (FreeTubeApp#7497) Bump @babel/core from 7.27.1 to 7.27.4 in the babel group (FreeTubeApp#7496) Translated using Weblate (Japanese) Translated using Weblate (German) Implement context menu item "search X in new window" (FreeTubeApp#7477) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (French) ... # Conflicts: # static/locales/da.yaml
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Issue 4497 - Hiding Watched Videos on Channel View
Pull Request Type
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)
(B)
Desktop
Windows 10 Pro
OS Build: 19045.5796
Freetube v0.23.4 Beta
Additional context