8000 Bypass list feature by ameerhossein · Pull Request #60 · bepass-org/oblivion · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Conversation

ameerhossein
Copy link
Contributor
@ameerhossein ameerhossein commented Feb 21, 2024

#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.

@markpash
Copy link
Member

The Use OblivionVpnService static methods... commit is already merged I think, or maybe I'm reading it wrong.

Copy link
Member
@markpash markpash left a 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!

8000
Comment on lines 454 to 479
if (splitTunnelMode != SplitTunnelMode.DISABLED) {
for (String packageName : getSplitTunnelApps(fileManager)) {
if (splitTunnelMode == SplitTunnelMode.BLACKLIST)
builder.addDisallowedApplication(packageName);
}
Copy link
Member

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);
  }
}

Copy link
Contributor Author

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.

@markpash
Copy link
Member

I think you deleted the wrong commit :P
I've enabled the button in github that suggests that to update the branch. Choose the "Update with rebase" option. Hopefully it helps.

@ameerhossein
Copy link
Contributor Author
ameerhossein commented Feb 21, 2024

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.

@markpash
Copy link
Member

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.
Let me know if you're ok with that.

@ameerhossein
Copy link
Contributor Author
ameerhossein commented Feb 21, 2024

Squashed the font commit you mentioned.

The Use OblivionVpnService static methods... commit is already merged I think, or maybe I'm reading it wrong.

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 as the maintainer get permissions to push to your PR branch, so I can fix some of this for you right now.
Let me know if you're ok with that.

I squashed the commit related to your suggestions. I believe you already have permission to edit the PR if you want to change anything.

@ameerhossein
Copy link
Contributor Author

Don't merge it yet. Android manifest needs an update again after the rebase

@ameerhossein
Copy link
Contributor Author

Ok I squashed the android manifest commit too. Everything seems ok now in my tests.

@markpash markpash merged commit 66790a6 into bepass-org:main Feb 21, 2024
@ameerhossein ameerhossein deleted the bypass-list-feat branch February 22, 2024 09:31
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
0