-
Notifications
You must be signed in to change notification settings - Fork 563
Bypass list feature #60
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
Conversation
The |
fcd8346
to
a8dd5b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know Java so my code review skills will be poor. Let me know if my suggestions are stupid. But otherwise, this looks good. Thanks!
if (splitTunnelMode != SplitTunnelMode.DISABLED) { | ||
for (String packageName : getSplitTunnelApps(fileManager)) { | ||
if (splitTunnelMode == SplitTunnelMode.BLACKLIST) | ||
builder.addDisallowedApplication(packageName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (splitTunnelMode == SplitTunnelMode.BLACKLIST) {
for (String packageName : getSplitTunnelApps(fileManager)) {
builder.addDisallowedApplication(packageName);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. It's like this because we had two modes at first (BLACKLIST and WHITELIST) but then I realized the whitelist mode required more work from the Go core lib side so I just removed the WHITELIST branch from the conditions. I will simplify the leftovers.
I think you deleted the wrong commit :P |
I used the update branch button but it defaulted to merge so I had to drop it. |
I as the maintainer get permissions to push to your PR branch, so I can fix some of this for you right now. |
6776ee2
to
f8dd561
Compare
Squashed the font commit you mentioned.
It's a different commit. Actually it should have been committed with the previous PR. Don't know why did it end up here. probably my mistake.
I squashed the commit related to your suggestions. I believe you already have permission to edit the PR if you want to change anything. |
f8dd561
to
f31d7db
Compare
… ConnectionState.java
f31d7db
to
d61b30b
Compare
Don't merge it yet. Android manifest needs an update again after the rebase |
d61b30b
to
f7dfddf
Compare
Ok I squashed the android manifest commit too. Everything seems ok now in my tests. |
#59
I rebased the previous PR onto my own outdated main branch accidentally and caused even more conflicts. I believe this new PR is fine and can be merged correctly.