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

Better URL Matching #2517

Closed
wants to merge 48 commits into from
Closed

Better URL Matching #2517

wants to merge 48 commits into from

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented May 14, 2020

Resolves #84
Resolves #147
Resolves #373
Resolves #660
Resolves #1689
Resolves #1614
Resolves #2055
Resolves #2103
Resolves #3359
Resolves #3729
Resolves #3802
Resolves #4263
Resolves #4375
Resolves #4675
Resolves #4731
Resolves #6417
Resolves #7206

@duredhel13
Copy link

Any news on getting this merged and released? I'm really looking forward to a fix to #660

@Gusted
Copy link
Contributor Author

Gusted commented Aug 2, 2020

This is a breaking change, changing a main component. A lot of system relies on this.
Also it's still unclear how this should work #373 give a good explanation. I've tried pushing this multiple times to get this merged. But their are more bug/features that is more wanted over this.

@KaKi87
Copy link
Contributor

KaKi87 commented Aug 3, 2020

Do you think it would be possible to publish on Chrome Web Store a beta version of Dark Reader that would include this PR and maybe others ?

@Gusted
Copy link
Contributor Author

Gusted commented Aug 3, 2020

Do you think it would be possible to publish on Chrome Web Store a beta version of Dark Reader that would include this PR and maybe others ?

That will be a hell to manage.

Also seems like conflicts in this PR time to update.

@KaKi87
Copy link
Contributor

KaKi87 commented Aug 4, 2020

Okay, well, I see this PR merged in my dreams. xD

@Gusted
Copy link
Contributor Author

Gusted commented Aug 5, 2020

I hope that dream can be fillfulled this month.
I've seen plans to get V5 by end of the month 👀 released.

@megamorphg
Copy link

What do the latest commits pending review do?
Doing it the way Bitwarden does would be too huge a change most likely (and maybe overkill).
Maybe just a "whitelist" that over-rides the current blacklist? That way, we could whitelist specific subdomains and regexes.

@mmseng
Copy link

mmseng commented Sep 19, 2022

Maybe just a "whitelist" that over-rides the current blacklist? That way, we could whitelist specific subdomains and regexes.

That wouldn't address other issues like the need to exclude subdomains from a "higher-level" allowlist, unless they also implemented the inverse solution. Even then, I don't think that would be flexible enough to allow for custom configurations per subdomain, only inclusion or exclusion from higher-level rules, although it would depend on the implementation I guess. Plus, it might have a scaling problem, like not allowing for customization of sub-sub-domains. To be fair, that's getting pretty in the weeds.

Gusted added a commit that referenced this pull request Oct 24, 2022
- While it's not rock solid, it's better than the current function.
Ultimately should be fixed by #2517
- Resolves #10160
Gusted added a commit that referenced this pull request Oct 24, 2022
- While it's not rock solid, it's better than the current function.
Ultimately should be fixed by #2517
- Resolves #10160
@ghost
Copy link

ghost commented Jan 2, 2023

Pinging @Gusted.

Here is a Pull Request from you that has been sitting open for approximately 2 3/4 years.

@KaKi87
Copy link
Contributor

KaKi87 commented Feb 1, 2023

In the meantime, I created a Dark Reader dynamic blacklist userscript that, among other things, manages this use case, using the darkreader-lock meta tag.

For example, it will disable filtering on github.com, gist.github.com and docs.github.com, while keeping it enabled on education.github.com.

@Elaborendum
Copy link
Contributor

@KaKi87 If you don't mind me asking, is it supposed to work automatically or does it need any input? Because I've been trying with alternativeto.net, and it doesn't detect the available-but-not-default dark theme.

@KaKi87
Copy link
Contributor

KaKi87 commented Feb 3, 2023

is it supposed to work automatically

Yes.

I've been trying with alternativeto.net, and it doesn't detect the available-but-not-default dark theme

Are you using Violentmonkey, like recommended on my userscript homepage ?
By the way, I think we should discuss this at my repository, or on the Discord community linked on that repository, or at Violentmonkey's, but not here.

EDIT regarding @Jackenmen's message below : the blacklist doesn't have to be empty, just not contain sites supported by the userscript.

@Jackenmen
Copy link

It seems universal enough for anyone who saw Kaki's comment that I figured I may as well mention it here. In order for it to work, you need to set "Theme generation mode" to Dynamic as it's the only one that supports darkreader-lock meta tag:
image
Additionally, you may want to make sure that your site list is configured such that it can actually make pages dark. One such configuration is to set it to "Not invert listed" with an empty list:
image

@Francewhoa
Copy link

Francewhoa commented Feb 4, 2023

This is to confirm this bug with Dark Reader V4.9.62. Using Firefox V102.7 or Ungoogle Chromium V109.0.

For users facing this challenge, I published this easy, quick, and temporary workaround.

For developers interested to contribute to resolving this bug, you might be interested in this:

@KaKi87
Copy link
Contributor

KaKi87 commented Feb 10, 2023

@Gusted Will the subdomains rule for the global dark list still be enforced once this PR is merged ?

@sakgoyal
Copy link

is there an update on this?

@iTrooz
Copy link

iTrooz commented Nov 24, 2023

Is there anything we can do to help this get merged ?

If this PR is problematic to merge and is expected to stay open for a long time, I'm going to try to upload a fork of Dark Reader to https://addons.mozilla.org based on this branch (Don't worry, I'll change the name). That way, users can still get around the issue

@KaKi87
Copy link
Contributor

KaKi87 commented Nov 24, 2023

@iTrooz I'd appreciate a CRX build as well if you can. Thanks

@alexanderby
Copy link
Member

Hi @iTrooz, there are now 2 approaches: without regular expressions (like www.google.*.*) and with regular expressions (like /www\.google\..*?\//). Please leave a comment if anything is not covered here #373

@KaKi87
Copy link
Contributor

KaKi87 commented Nov 24, 2023

If google.* still matches maps.google.com then it's still a problem.

@alexanderby
Copy link
Member

Try www.google.* or ^google.*

@iTrooz
Copy link

iTrooz commented Nov 24, 2023

Hi @iTrooz, there are now 2 approaches: without regular expressions (like www.google..) and with regular expressions (like /www.google..*?//). Please leave a comment if anything is not covered here #373

Hey @alexanderby ! I'm sorry, but I don't think I understood your message. Could you reformulate please ?

@alexanderby
Copy link
Member

This week a support for Regular Expressions was added. When a pattern starts with / and ends with / it means it is a RexExp. It would be nice to track feedback in a single place. This is a source issue #373

@iTrooz
Copy link

iTrooz commented Nov 24, 2023

I'm sorry, but I'm not interested in testing that feature if it doesn't help this PR get merged. I am mainly interested in working on the code in this PR in order to it to get merged. My motivation for this is that I'd like to control subdomain dark themes independently from the main DarkReader UI (I'm affected by #1896 (comment) which brought me here)

@iTrooz
Copy link

iTrooz commented Nov 24, 2023

So, is there any way in which I can currently help, to get this PR merged ? (Or maybe you would accept a smaller PR to specifically handle subdomains independently ? But I'm not sure why this would be better, since nobody complained that this PR doesn't work ?)

@alexanderby
Copy link
Member

This PR can't be merged unfortunately, but some features can now be reimplemented in several smaller PRs. Particularly the root domain issue (e.g. GitHub.com and docs.github.com) can be fixed in 1 line and seems like a low risk change that can be done quickly.

The Site List separation into enabled/disabled has been done, and it is unclear if the negative patterns will be still necessary. Anyway it should be also in a separate PR.

I'm sorry that it takes so long, unfortunately my life situation didn't let me look into the problem 3 years ago.

@alexanderby
Copy link
Member

This commit allows www as a subdomain, but not anything else. I know that domains with www and without are technically different, but this will allow us to easily handle site lists, fixes configs etc. @iTrooz Let me know if it solves your problem.

@iTrooz
Copy link

iTrooz commented Nov 25, 2023

This works perfectly, thank you !
As a site note, you may want to make the www subdomain and the main domain equivalent. If I go to https://example.com, disable the dark theme, and go to www.example.com, the "toggle theme" button doesn't do anything. You may want to make it toggle the dark theme for the main domain

I'm sorry that it takes so long, unfortunately my life situation didn't let me look into the problem 3 years ago

Don't worry, it's perfectly okay ! You are doing this in your free time, and at no point did I want to imply that the work you are putting into this was bad. Thank you and the maintainers in general for making such a good extension, and devoting time for it. It's really useful for me on a daily basis, and I'm thankful for Dark Reader to exist

@KaKi87
Copy link
Contributor

KaKi87 commented Nov 25, 2023

In which version is this/will this be published to the stores ?
Thanks

@alexanderby
Copy link
Member

Thanks, I see this www toggling issue and will try to make a fix.

If no other issues are found, it can go live next week (hopefully Mozilla will review the previous release soon).

I'm attaching a build if you want to try:
darkreader-firefox-4.9.70.1.zip

@alexanderby alexanderby closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment