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

Add separate whitelist and blocklist #14

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

Conversation

blickly
Copy link

@blickly blickly commented Jul 14, 2012

Currently, there is only one "site list" that is used for both the blacklist and whitelist. This means that users who want to switch between these two types of blocking must clobber one list every time.

This patch fixes this problem by adding two separate lists for the whitelist and blacklist.

This avoids the problem of the whitelist clobbering
the blacklist or vice versa.
@matchu
Copy link
Owner

matchu commented Jul 14, 2012

Interesting idea! Good call for usability :)

I feel like, before deploying, we'd need to think a bit more about a default whitelist that would work for many users — clearly it's harder than a default blacklist, and a truly perfect solution is impossible, but we can definitely arrive at a good starting point. The default blacklist was based on a list of the 50 (I think) most popular sites on the web and deciding which are almost never work-related. The default whitelist oughta be based on that same pattern rather than what we as developers just so happen to consider our work-related sites.

Also, I'd like to keep the new naming convention of "site" instead of "domain" for pref keys, since we now support path-based entries like "google.com/calendar", which is a site but isn't a domain.

After those changes, I'll give the merge a final look for style consistency and run a bunch of paranoid checks on the new prefs-update function (unless maybe you've already done 'em?), and we'll move from there :) Thanks!

@blickly
Copy link
Author

blickly commented Jul 14, 2012

I agree that we should change the default whitelist, as currently it's just my whitelist unchanged.
It seems like its harder to say for certain that a site is universally work-related than that it universally isn't, so I would err on the side of having too few sites in the whitelist over too many. Also, since the default is to blacklist sites, anyone who is using the whitelist mode must have at least looked at the options page.

I changed the names according to your convention, as well as the name of "whitelistEl" to "whitelistSelectEl", since I think that's clearer.

I haven't tested the prefs-update, so I'd be glad if you did that. Thanks!

Users should probably explicitly add sites that they want to be
whitelisted. Since the default is to use a blacklist, users
who "go with the defaults" will not be affacted.
@blickly
Copy link
Author

blickly commented Dec 6, 2012

On more thought, I think it probably makes most sense for the default whitelist to be empty. Since users must go into the options to switch away from the blacklist, I think it's reasonable to expect that they have some idea of which sites to whitelist. Users who don't change the default options will never interact with the default whitelist anyway.

@matchu
Copy link
Owner

matchu commented Dec 6, 2012

That makes good sense.

Sorry this pull request has been so thoroughly back-burnered. I'm hoping to be able to get a new release out this month once exams are done, so we'll see how that goes :) Thanks for your patience, bro.

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

Successfully merging this pull request may close these issues.

2 participants