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

Filter posts #1226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Filter posts #1226

wants to merge 4 commits into from

Conversation

eikaramba
Copy link

this fixes #646 and allows to specify a comma separated list of keywords which will be used to filter posts by title.

In the future of course regex or filter based on user flairs can be integrated.

@QuantumBadger
Copy link
Owner

Thanks for this! I've left a few minor comments, although I'm not sure I'd want to merge this in its current form.

Currently it's just doing a title.contains(keyword) to check if it matches, but that's not really sufficient as the keyword could be part of some other word in the title. For example, adding the word "trump" to the filter will filter out the word "trumpet", "trumping", etc. In other words this falls foul of the Scunthorpe problem.

I guess we'd want to split titles by punctuation/whitespace and check for matches that way?

@@ -838,6 +843,17 @@ public void onDataStreamComplete(
&& blockedSubreddits.contains(
new SubredditCanonicalId(post.getSubreddit().getDecoded()));

final String postTitle = post.getTitle()
Copy link
Owner

Choose a reason for hiding this comment

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

PostListingFragment is already quite cluttered -- it would be nice to split out all this new code into a new class.

@thousandsofthem
Copy link

I guess we'd want to split titles by punctuation/whitespace and check for matches that way?

It could be a phrase, like long series/book/etc title

@eikaramba
Copy link
Author

thank you for having a look at the PR. yes you are absolutely right, i changed it so that the keywords are matched against complete words (using \W+, which matches one or more non-word characters (punctuation, whitespace, etc.))

I also resolved the aforementioned reviews apart from the refactoring, as i am unsure how you would like this to be done. i guess it would be good to refactor the whole class then and also make all the other conditions separate methods/classes.

@folkemat
Copy link
Contributor

Very nice. I see you put the option in Settings -> Behaviour -> Posts. I would instead suggest creating a whole new settings item, Settings -> Filters. Here we could then simply add more filters later (by Post domain, keyword, and Comment keyword etc. pp). Makes it easier to find and access in my opinion.

@eikaramba
Copy link
Author

eikaramba commented Sep 1, 2024

no problem i modified the PR to introduce a new settings category

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

Successfully merging this pull request may close these issues.

[Feature request] blacklist post titles
4 participants