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

Refactor custom filters #1813

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Refactor custom filters #1813

merged 5 commits into from
Aug 22, 2024

Conversation

smalluban
Copy link
Collaborator

@smalluban smalluban commented Aug 13, 2024

Moves the generation logic and fixes some minor bugs of the custom filters feature.

  • Adds options.customFilters.enabled property and on/off feature of the custom filters without the need to remove the whole textarea content to temporarily disable custom filters
  • Fixes minor issues, where for example DNR rules were not set at all if only one rule did not convert - which was not consistent with the behavior for network filters in the engine (Firefox)
  • Simplifies the components/custom-filters.js file - as this should be a component, not a file with that much logic, which is not related to the UI
  • The only "drawback" is removed instant feedback with counters of filters below the textarea. This code moved to the BG. However, I think it was misleading, as the DNR rule might fail when it's converted, not when you pass them there. The feedback stays, but after you make an update to the custom filters.

I tried to not change the logic, but in some places, it looked to me like obvious bugs - please review this code not as an update, but as a whole - if this works as you would expected it to work.

@smalluban smalluban requested a review from chrmod August 13, 2024 07:52
@smalluban smalluban marked this pull request as ready for review August 13, 2024 07:52
@smalluban smalluban added the package CI: create extension packages label Aug 13, 2024
@ghostery ghostery deleted a comment from github-actions bot Aug 14, 2024
@smalluban smalluban removed the package CI: create extension packages label Aug 14, 2024
@ghostery ghostery deleted a comment from github-actions bot Aug 14, 2024
@smalluban
Copy link
Collaborator Author

@philipp-classen Updated and fixed what I could change. The parseInput must stay, but I renamed it to normalizeFilters, as it better describes what actually the function does - it does not produce filters, just split the input into buckets, and fixes scriptlets if needed.

@smalluban smalluban added the package CI: create extension packages label Aug 14, 2024
@smalluban
Copy link
Collaborator Author

smalluban commented Aug 14, 2024

@GRadziejewski Testing highlights:

  • The feature logic has not changed
  • Added on/off toggle for the whole feature, so we can disable custom filters without removing the input
  • Fixed issue with updated adblocker engine - to proof the bug update 3.18 -> 4.0, to proof the fix 3.18 -> 4.0 -> this PR - The bug cleans out the engine, so the cosmetic filters and network filters (only Firefox) are removed. The best way to test - add custom filters before updating to 4.0, then check out that custom filters don't work anymore until the next update.

@smalluban smalluban added the QA: Tests The test round specified in the title or description label Aug 14, 2024
@GRadziejewski
Copy link
Contributor

Test results: #1822 (comment)

Issue: #1823

@smalluban
Copy link
Collaborator Author

After testing the feature, I added a small change to not confuse users - the "Update" (actually "Save" now - it's more what it is, and it's already translated phrase) is disabled only when filters update.
Also, it fixes the state when the global pause is turned on.

@GRadziejewski
Copy link
Contributor

Test results: #1822

LGTM.

@smalluban smalluban merged commit 01c3ecf into main Aug 22, 2024
2 checks passed
@smalluban smalluban deleted the feat-custom-filters branch August 22, 2024 07:57
@smalluban smalluban mentioned this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package CI: create extension packages QA: Tests The test round specified in the title or description
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants