-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Hiding Watched Videos on Channel View #7366
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
Open
palharesf
wants to merge
51
commits into
FreeTubeApp:development
Choose a base branch
from
palharesf:feature/4497-hide-watched-videos
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 47 commits
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
6e4fecf
Toggle Switch created with proper label, imported from component list…
palharesf fcaddb8
Make toggle only visible on the Videos tab
palharesf bbddf3c
Adjusted vertical position of the toggle
palharesf b8d85a6
Adjusted styling to separate toggle and dropdowns
palharesf ee82ca2
Refactor variable names for better readability
palharesf f7d5d93
Created the function to filter watched videos with check logic in the…
palharesf dd46933
Fixed hideWatchedToggle variable initialization and updating; also co…
palharesf 6604ab9
Updated the toggle helper function to update the ref values; created …
palharesf 6565d6c
Removed autoRefresh options
palharesf 3bc97bc
Created helper function to check if video exists in history
palharesf 97ec3f0
Updated helper function 'filterWatchedVideos' to be able to get histo…
palharesf d991ed6
Added some comments about limitations found during testing
palharesf d159660
Fixing localization key in the app and adding localization key in the…
palharesf fadc362
Added logic to filter watched videos when getting video list from Inv…
palharesf 1962830
Missing space
palharesf 3af3a23
Changed the template reference from the latestVideos array to a compu…
palharesf fcb0088
Removed all the logic that changed the original latestVideos (replace…
palharesf 464407a
Same as last commit, but I forgot to include this last bit of code
palharesf 20107d1
Added the button for both shorts and live pages, but functionality st…
palharesf 4f7cc1c
Implemented filtering logic through computed field for Shorts
palharesf 69df855
Implemented computed field for Lives, working properly
palharesf 8fe1a72
Since 'Hide Watched' is being used outside of the Videos tab, I incre…
palharesf 1e5b5d3
Since the helper function is not specific to videos, I renamed it to …
palharesf 8a8f855
Added a visibility flag to hide the toggle when the videoPanel is bei…
palharesf 1cd18e6
Adjusted computer variable to force a screen refresh if the filteredA…
palharesf 7a0ec56
Returned filteredVideos to a pure computed function
palharesf c51626b
Update src/renderer/views/Channel/Channel.vue
palharesf ccccfde
Update src/renderer/views/Channel/Channel.vue
palharesf a608731
Update src/renderer/views/Channel/Channel.vue
palharesf 6b53c2c
Hide the 'Sort By' dropdown if there's only one video loaded on the v…
palharesf 6331f0c
Refactor - simplify logic for when to show the toggle on videos page
palharesf feaf4ba
Refactor - simplify logic for when to show the toggle on shorts page
palharesf 083da11
Refactor - simplify logic for when to show the toggle on live page
palharesf 6e695af
Refactor - updated variable name for better readability
palharesf 7fd310c
Refactor, adjusted case for function name
palharesf 2bf29b8
Refactor, adjusted case in function call
palharesf 2eaca07
Removed the switch from the Channel page
palharesf 12b063a
Removed the CSS rules created for proper positioning of the toggle bu…
palharesf e446c06
Removed redundant import (since we're not rendering the toggle anymore)
palharesf db6e48d
Removed unused sentence from the Channel page
palharesf a1275f2
Moved toggle element from subscription to distractionfree component
palharesf 8af16ab
Moved computed functions and methods between components
palharesf 77343ba
Updated localization settings
palharesf 38bcb24
Updated variable declaration to access value from the store (global s…
palharesf 1eb673b
Refactor all variable referencs to the new variable name; removed old…
palharesf 468062c
Implemented tooltip and localization
palharesf 72768e7
Merge branch 'development' into feature/4497-hide-watched-videos
PikachuEXE 135cc6f
Update static/locales/en-US.yaml
palharesf 59c8dd0
Update src/renderer/views/Channel/Channel.vue
palharesf f49bbe9
Move translations for Hide Videos on Watch key
efb4f5ff-1298-471a-8973-3d47447115dc 727b7a9
Merge branch 'development' into feature/4497-hide-watched-videos
PikachuEXE File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)" | ||
:value="videoSortBy" | ||
:select-names="videoLiveShortSelectNames" | ||
:select-values="videoLiveShortSelectValues" | ||
|
@@ -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)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
:value="shortSortBy" | ||
:select-names="videoLiveShortSelectNames" | ||
:select-values="videoLiveShortSelectValues" | ||
|
@@ -61,7 +61,7 @@ | |
/> | ||
<FtSelect | ||
v-if="!hideLiveStreams && showLiveSortBy" | ||
v-show="currentTab === 'live' && latestLive.length > 0" | ||
v-show="currentTab === 'live' && (hideWatchedSubs ? filteredShorts.length > 1 : filteredShorts.length > 0)" | ||
palharesf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
:value="liveSortBy" | ||
:select-names="videoLiveShortSelectNames" | ||
:select-values="videoLiveShortSelectValues" | ||
|
@@ -97,7 +97,7 @@ | |
<FtElementList | ||
v-show="currentTab === 'videos'" | ||
id="videoPanel" | ||
:data="latestVideos" | ||
:data="filteredVideos" | ||
:use-channels-hidden-preference="false" | ||
role="tabpanel" | ||
aria-labelledby="videosTab" | ||
|
@@ -112,7 +112,7 @@ | |
<FtElementList | ||
v-if="!hideChannelShorts && currentTab === 'shorts'" | ||
id="shortPanel" | ||
:data="latestShorts" | ||
:data="filteredShorts" | ||
:use-channels-hidden-preference="false" | ||
role="tabpanel" | ||
aria-labelledby="shortsTab" | ||
|
@@ -128,7 +128,7 @@ | |
v-if="!hideLiveStreams" | ||
v-show="currentTab === 'live'" | ||
id="livePanel" | ||
:data="latestLive" | ||
:data="filteredLive" | ||
:use-channels-hidden-preference="false" | ||
role="tabpanel" | ||
aria-labelledby="liveTab" | ||
|
@@ -469,6 +469,9 @@ const hideChannelPlaylists = computed(() => store.getters.getHideChannelPlaylist | |
/** @type {import('vue').ComputedRef<boolean>} */ | ||
const hideChannelCommunity = computed(() => store.getters.getHideChannelCommunity) | ||
|
||
/** @type {import('vue').ComputedRef<boolean>} */ | ||
const hideWatchedSubs = computed(() => store.getters.getHideWatchedSubs) | ||
|
||
/** | ||
* @template T | ||
* @param {T[]} array | ||
|
@@ -1048,6 +1051,30 @@ const videoContinuationData = shallowRef(null) | |
const showVideoSortBy = ref(true) | ||
const videoSortBy = ref('newest') | ||
|
||
const filteredVideos = computed(() => { | ||
if (hideWatchedSubs.value) { | ||
return filterWatchedArray(latestVideos.value) | ||
} else { | ||
return latestVideos.value | ||
} | ||
}) | ||
|
||
const filteredShorts = computed(() => { | ||
if (hideWatchedSubs.value) { | ||
return filterWatchedArray(latestShorts.value) | ||
} else { | ||
return latestShorts.value | ||
} | ||
}) | ||
|
||
const filteredLive = computed(() => { | ||
if (hideWatchedSubs.value) { | ||
return filterWatchedArray(latestLive.value) | ||
} else { | ||
return latestLive.value | ||
} | ||
}) | ||
|
||
watch(videoSortBy, () => { | ||
if (!autoRefreshOnSortByChangeEnabled) { return } | ||
|
||
|
@@ -2315,6 +2342,11 @@ function handleSubscription() { | |
posts: latestCommunityPosts.value | ||
}) | ||
} | ||
|
||
function filterWatchedArray(videos) { | ||
const historyCache = store.getters.getHistoryCacheById | ||
return videos.filter(video => !Object.hasOwn(historyCache, video.videoId)) | ||
} | ||
</script> | ||
|
||
<style scoped src="./Channel.css" /> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea why we need different logic based on
Hide Videos on Watch
enabled or notProbably just need
filteredVideos.length > 1
hereOr 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 ordersI might get some of this wrong let me know
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, there's no need to differentiate the behavior depending on the state of
hideWatchedSubs
-- as long asfilteredVideos.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 wayI will try to push this fix (without forcing it) and let you know if I have trouble doing so
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also realized why I was not able to push my previous changes cleanly, my forked repo was behind the actual origin repo so that's why simply pulling the changes was not working. I have just synced my repo with the origin one, and when I try to pull the changes into my repo I finally get the conflicts.
I assume that I can accept the remote repository changes by using
git checkout --theirs src/renderer/components/subscription-settings/subscription-settings.js
andgit checkout --theirs src/renderer/components/subscription-settings/subscription-settings.vue
, but I'm afraid of doing that without checking with you all and breaking things. If its easier to just suggest the changes I outlined in my previous comment directly into the code, I'm also happy with that approachThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I re-read both of your comments and understand I was overlooking the case where there was the possibility of fetching more videos. I need to think how to incorporate that into the logic and test the implementation, as it's not coming to me immediately. I'll try to sync my branch to the origin before doing that so I don't rely on you guys to push every little change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @palharesf just checking in. What is the status on this? Do you need any additional help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I still haven't been able to properly merge the changes from the remote branch onto my local copy of the repo, as outlined here.
It's unclear what I can do without breaking things, and as I have already broken stuff here in the past I decided to wait for guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just rename your existing branch (delete it later)
Check out this pull request (you can use
gh pr checkout 7366
if you gotgh
installed) and work from thereIf you got something in the old branch committed but not pushed yet you can see try to apply those on new branch once you have done the above