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

Video title filter / blacklist #4202

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 21, 2023

Video & playlist title filter / blacklist

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #1070
closes #3086
closes #95

Description

Enter a fragment, word, or phrase (case insensitive) to hide all videos with titles containing it throughout all of FreeTube, except History and videos inside of playlists. Special exclusion text appears for community posts. Nothing is hidden on the Channel route. Now those videos do not appear on any view (reasoning: you don't want to see videos or playlists with blacklisted text in any part of your FreeTube browsing experience). Incidentally: to set a minimum character length of 3 for blacklisted inputs, this PR implements a minInputLength prop in ft-input and ft-input-tags.

Other notes:

  • Fixes "Play Next Video" playing hidden recommended videos (hidden either by this or Hide Channels)
  • Fixes hidden Video Recommendations taking up height

Screenshots

Screenshot_20231021_044819
Screenshot_20231021_045130
Screenshot_20231021_045248

Testing

  • Ensure videos whose titles contain blacklisted tags are never visible on any route
  • Ensure tags are case-insensitive and can be substrings of words
  • Try inputting tags of length < 3
  • Enable autoplay with a hidden video at the top of the recommended videos data, and ensure that only the next visible recommendation is autoplayed, not any hidden ones

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 21, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 21, 2023 10:04
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 21, 2023

A bunch of issues regarding shorts/live videos were closed as a duplicate #1070

So we should also take that into account somehow

  • A way to block a shorts on all pages
  • A way to block a live streams on all pages
  • A way to block upcoming live streams on all pages

U could also just remove Closes behind issue number

@github-actions github-actions bot added the U: duplicate This issue or pull request already exists label Oct 21, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc removed the U: duplicate This issue or pull request already exists label Oct 21, 2023
@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 21, 2023

Whoops, I'll remove the closing. Although to be fair, those other aspects aren't made clear in the original post.

@absidue
Copy link
Member

absidue commented Oct 21, 2023

@efb4f5ff-1298-471a-8973-3d47447115dc With the current information provided by YouTube, it's impossible to hide shorts on all pages, the only way we know something is a short, is if it's on a shorts specific page.

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

Would it be possible to hide live, premiere and upcoming streams

@absidue
Copy link
Member

absidue commented Oct 21, 2023

We can probably hide currently live stuff and upcoming stuff, we won't be able to differentiate between live stream or premiere and we also can't do any of those for RSS.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 21, 2023

Would it be possible to hide live, premiere and upcoming streams

From what I can discern, given that they have a title property on their data, I think they are being hidden appropriately if they contain a banned title, which I believe is what is being asked. Do you know of any videos I can use to test / how I can find videos matching these conditions easily?

@absidue
Copy link
Member

absidue commented Oct 21, 2023

I think having this pull request just do the title filtering is fine.

@kommunarr
Copy link
Collaborator Author

Yeah, that sounds good to me. If #1070 is serving a secondary purpose of improving the expansiveness of the existing Hide Upcoming Premieres & Hide Live Streams features, it certainly doesn't seem clear to me why that is the case, but it's not in scope for this PR IMO.

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 21, 2023

I agree it doesnt have to be added in this PR

but

Should we even implement that if we cant differentiate them when RSS is enabled?

Feels like a half baked feature

If the answer to this is no then i guess we can still put closes behind issue nr

@kommunarr
Copy link
Collaborator Author

Seems like a separate issue should be created for it. If we have sub-issues that are not referenced in main issue titles or descriptions, it's hard for people to search for those & inevitably leads to dupes.

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Nov 5, 2023

Doesnt this close #3086 ?

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

Why we have so many dupes

Closes #95

@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 Nov 9, 2023
Copy link
Contributor

github-actions bot commented Nov 9, 2023

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

Copy link
Contributor

github-actions bot commented Nov 9, 2023

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 Nov 9, 2023
@gelatinbomb
Copy link

Why we have so many dupes

Closes #95

It seems a few users might want this feature. I mean I was just about to open an issue asking for this exact thing until I found this PR so it's nice to see someone implemented the thing :D @jasonhenriquez Nice job!

There is only one thing I would like to ask: It is possible to add a way to export the "blocklist" as a text file? You know as a backup file. That would be nice in the case you had a lot of blocked words.

@kommunarr
Copy link
Collaborator Author

@gelatinbomb That will be possible once #1018 is implemented - a user-friendly means of sharing settings.db files. Until then, you can make a backup of the settings.db file on your device and grab it from there.

Copy link
Member

Choose a reason for hiding this comment

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

  • Copy original video title from subscriptions -> Put it in the blacklist -> check subscription page -> video gone -> Enable DeArrow -> video still gone even though DeArrow title is different
  • Enable DeArrow -> Go to subscription page -> copy a word/title of a vid that has a title change -> insert into blacklist -> go to subscription page -> See video not gone
  • Should we even do word fragments? What if i really hate ores for some reason so i insert ore but i really love cores, now i dont get to see all the beautiful vids about cores :(
  • Why not hide playlist titles and video titles within a playlist?

@kommunarr
Copy link
Collaborator Author

kommunarr commented Nov 17, 2023

  • Copy original video title from subscriptions -> Put it in the blacklist -> check subscription page -> video gone -> Enable DeArrow -> video still gone even though DeArrow title is different

    • Enable DeArrow -> Go to subscription page -> copy a word/title of a vid that has a title change -> insert into blacklist -> go to subscription page -> See video not gone

Implementation-wise, the irksome part of that is that the DeArrow logic is in the ft-list-video, but our video hiding logic bits have been in the video list container classes a level or two above that, which I believe is done that way for performance & logical reasons, correct me if I'm wrong (if you can't show the child, you can't show the parent wrapper, so that introduces problems that can't be solved easily by performance).

Behavior-wise, the main question is what would we want the expected behavior to be in both of these cases. Would you really want to see a video if either the DeArrow title or the original contained a banned word? I would think not, so that would imply that the most preferred behavior would be to filter if either matched. But then again, that does put a lot of power in DeArrow contributors, so maybe we shouldn't care about it to that extent. So I'm currently leaning on just ignoring the DeArrow title consistently, which is what the code is currently doing. Let me know what you think.

* Should we even do `word fragments`? What if i really hate `ores` for some reason so i insert `ore` but i really love `cores`, now i dont get to see all the beautiful vids about `cores` :(

I do think this is a worthwhile hypothetical. It's a trail that often ends in regex, which I think is probably a bit of overkill for the initial feature & isn't always good for usability (How do you indicate that this is a regex tag versus just a regular tag? If all tags are regex tags, will non - power users like it if they can't use special characters like they want? How do you handle errors? How does it affect performance?). So I think we should let it simmer and see how it goes for people.

On if you want to be able to differentiate it without regex, it becomes a question of "is the banning of partial words banning too much stuff for me" versus "is the banning of only full words banning too little". It's a good question to ask. I think the second one is closer to where people are, but I can't really know that for sure. Is it okay if my answer to this is "let's see how many people complain" and leave it there for now?

* Why not hide playlist titles and video titles within a playlist?

Good question! I was going off of what we were doing in our components, as discussed in the Matrix chat. I figure that the same push to add it to playlists will also be adding the channel logic as well. Unsure of the considerations there, and this PR is a little big, so I left it be. But I can add this and/or channel filtering logic to there as well if desired.

Edit: following the discussion in the Matrix chat, I'll address this.

@kommunarr kommunarr removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 17, 2023
@PikachuEXE
Copy link
Collaborator

I am not sure if clearing input when input already exists is a good UX
Maybe user wants to correct the string (make it more specific)
It's easier to delete the whole input text then to retype the whole input text

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@verdantburrito
Copy link

bump

@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 Jan 3, 2024
Copy link
Contributor

github-actions bot commented Jan 3, 2024

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

Copy link
Contributor

github-actions bot commented Jan 4, 2024

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

@kommunarr kommunarr force-pushed the feat/hide-videos-containing-text branch from c985430 to 1bbc75e Compare January 4, 2024 00:28
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

  • Video list (subscription)
  • Play Next in watch page (with some video hidden) to ensure only visible video played next
  • Tested community post (with video) hidden

@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 Jan 5, 2024
@FreeTubeBot FreeTubeBot merged commit 64e3f32 into FreeTubeApp:development Jan 22, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 22, 2024
@BarbzYHOOL

This comment was marked as abuse.

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