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

Fix Hide Channels and Hide Premier settings #3673

Merged

Conversation

petaded
Copy link
Contributor

@petaded petaded commented Jun 18, 2023

Fix Hide Channels and Hide Premier settings

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

PR #3091 broke the hide channels and hide upcoming premiers option.
fixes #3419

Description

This PR fixes the showResult function to actually be run after PR #3091 stopped it working.
Having a v-if="method" means that it doesn't actually run the method instead just evaluates it (which always evaluates to true).

You can easily test this yourself by adding a console log in showResult and it will currently never be run.

To fix this and keep the wanted fix from 3091. I have changed showResult to a computed property from a method which means it does get run and will be re-evaluated on change (which is what 3091 wanted).

Also this fixes the hide premiere setting which was broken both because of the above issue and because youtube has changed since it was originally implemented. Now instead of checking durationText === 'PREMIERE' which it no longer is. We can instead just check the premiereDate which if its not null is a premiere video.
Note: when you are subscribed to a channel and using RSS premiereDate is not included hence we still need the data.isRSS && data.viewCount === '0' check.

Screenshots

Testing

Tested that this PR keeps the fix that #3091 wanted by doing the same test as them
yarn dev:web and going to channel https://youtube.com/channel/UC1BWMtZbNLVMSFgwSukjqCw
image

Also tested that the hide channels option is working again. by entering "Linus Tech Tips" into it and searching for LTT. Videos and channel are hidden.
image
No LTT videos are shown now if searching LTT
image

Testing the hide premiere option.

with hide premiere disabled with hide premiere enabled:
image image

Testing hide premiere on the subscription page with isRSS enabled.

with hide premiere disabled with hide premiere enabled:
image image

Desktop

  • OS: Manjaro Linux
  • OS Version: 5.15.94-1-MANJARO
  • FreeTube version: development branch (0.18.0)

Additional context

Note: the file preview shown by git doesn't seem to show the correct moved section. If you check the real difference you can see I just moved the showResult from the methods: {} to the computed: {} section.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 18, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 18, 2023 10:37
auto-merge was automatically disabled June 18, 2023 11:01

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 18, 2023 11:01
auto-merge was automatically disabled June 18, 2023 11:12

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 18, 2023 11:13
@PikachuEXE
Copy link
Collaborator

There is another durationText in src/renderer/views/Subscriptions/Subscriptions.js
Can you also fix that?
I search with durationText

return item.durationText !== 'PREMIERE'

auto-merge was automatically disabled June 18, 2023 12:45

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 18, 2023 12:45
@petaded
Copy link
Contributor Author

petaded commented Jun 18, 2023

There is another durationText in src/renderer/views/Subscriptions/Subscriptions.js Can you also fix that? I search with durationText

return item.durationText !== 'PREMIERE'

have updated :)

@PikachuEXE
Copy link
Collaborator

I am using https://www.youtube.com/channel/UCUKPG1-r4WFyxpyhIuCs86Q for testing premiere

PikachuEXE
PikachuEXE previously approved these changes Jun 19, 2023
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested https://youtube.com/channel/UC1BWMtZbNLVMSFgwSukjqCw on yarn dev:web
Tested hide channels option (Using Linux Tech Tips)
Tested with hide premiere (Local API, RSS on & off)

PikachuEXE pushed a commit to PikachuEXE/FreeTube that referenced this pull request Jun 20, 2023
@PikachuEXE
Copy link
Collaborator

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 26, 2023

Hi @petaded thanks for opening this PR. If i search Linus Tech Tips and apply the playlist filter i get playlists created by LTT

@PikachuEXE
Copy link
Collaborator

For local API playlist channelId and channelName should be checked
image

For local API channel id and name should be checked
image

PikachuEXE pushed a commit to PikachuEXE/FreeTube that referenced this pull request Jun 28, 2023
PikachuEXE pushed a commit to PikachuEXE/FreeTube that referenced this pull request Jun 28, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Filter out playlists

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 28, 2023
auto-merge was automatically disabled June 28, 2023 08:18

Head branch was pushed to by a user without write access

@petaded petaded dismissed stale reviews from ChunkyProgrammer and PikachuEXE via 65932b4 June 28, 2023 08:18
if (this.hideUpcomingPremieres &&
// Observed for premieres in Local API Channels.
(data.durationText === 'PREMIERE' ||
(data.premiereDate != null ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Invidious API is mentioned might as well
https://docs.invidious.io/api/common_types/#videoobject

Suggested change
(data.premiereDate != null ||
(data.premiereDate != null ||
// Invidious API
// `premiereTimestamp` only available on premiered videos
// https://docs.invidious.io/api/common_types/#videoobject
data.premiereTimestamp != null ||

Comment on lines 82 to 83
if (this.channelsHidden.includes(data.id) || this.channelsHidden.includes(data.name) ||
this.channelsHidden.includes(data.authorId) || this.channelsHidden.includes(data.author)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better doc

Suggested change
if (this.channelsHidden.includes(data.id) || this.channelsHidden.includes(data.name) ||
this.channelsHidden.includes(data.authorId) || this.channelsHidden.includes(data.author)) {
const attrsToCheck = [
// Local API
data.id,
data.name,
// Invidious API
// https://docs.invidious.io/api/common_types/#channelobject
data.author,
data.authorId,
]
if (attrsToCheck.some(a => a != null && this.channelsHidden.includes(a))) {

Comment on lines 88 to 89
if (this.channelsHidden.includes(data.channelId) || this.channelsHidden.includes(data.channelName) ||
this.channelsHidden.includes(data.authorId) || this.channelsHidden.includes(data.author)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better doc

Suggested change
if (this.channelsHidden.includes(data.channelId) || this.channelsHidden.includes(data.channelName) ||
this.channelsHidden.includes(data.authorId) || this.channelsHidden.includes(data.author)) {
const attrsToCheck = [
// Local API
data.channelId,
data.channelName,
// Invidious API
// https://docs.invidious.io/api/common_types/#playlistobject
data.author,
data.authorId,
]
if (attrsToCheck.some(a => a != null && this.channelsHidden.includes(a))) {

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

Test results after latest commit IV fix commit:

  • Hide Premiere + IV API = Not hidden
  • Hide Premiere + IV API + RSS = Hidden
  • Hide Premiere + Local API + RSS = Hidden
  • Hide Premiere + Local API = Hidden

@PikachuEXE
Copy link
Collaborator

Hide Premiere + IV API = Not hidden

Which is why I request change :)

FreeTubeBot pushed a commit that referenced this pull request Jul 2, 2023
…er channel (#3668)

* Changes from PR #3673

* * Update subscription view to be able to load videos from video cache again

* * Update subscription view to be able to load videos from video cache per channel

* * Remove meaningless argument `allowUseOfChannelCache`

* $ Remove unused imports

---------

Co-authored-by: petaded <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2023

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

@PikachuEXE
Copy link
Collaborator

@petaded
If you wish I can rebase this on development since the conflict is caused by merging #3668 which is made by me
But do let me know since I assume you might want to do it yourself

@petaded
Copy link
Contributor Author

petaded commented Jul 2, 2023

@petaded If you wish I can rebase this on development since the conflict is caused by merging #3668 which is made by me But do let me know since I assume you might want to do it yourself

That would be helpful thank you. I currently don't have access to any computer I can develop on atm.

I have seen your recent replies about updates to the PR but same as above, until I have somewhere I can make changes I'm stuck.

petaded and others added 3 commits July 3, 2023 11:05
Squashed commits:
[65932b4] update playlist and channel filtering
[952a721] update subscriptions js to use premiereDate over durationText
[530dea9] Add back isRSS and viewcount check to fix when in subscriptions page
[93ebb76] Fix hide premiere
[de7a8b4] ft-list-lazy-wrapper put whitespace back to what it was
[8dadb59] move showResult from a method to a computed to work with v-if
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

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

@PikachuEXE
Copy link
Collaborator

@petaded
I have just rebased
You will see some code changes are gone since they were merged with #3668 (I rebased it on this branch earlier to solve conflict early) and it was supposed to be merged after this is merged

#3668 (comment)
image

So now the remaining changes are for Invidious API

@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jul 5, 2023
@PikachuEXE
Copy link
Collaborator

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@FreeTubeBot FreeTubeBot merged commit 3eb657c into FreeTubeApp:development Jul 9, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 9, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

Created discussion #3744!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants