Skip to content

Bypass list feature#60

Merged
markpash merged 3 commits into
bepass-org:mainfrom
ameerhossein:bypass-list-feat
Feb 21, 2024
Merged

Bypass list feature#60
markpash merged 3 commits into
bepass-org:mainfrom
ameerhossein:bypass-list-feat

Conversation

@ameerhossein

@ameerhossein ameerhossein commented Feb 21, 2024

Copy link
Copy Markdown
Contributor

#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
Copy Markdown
Member

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

@markpash markpash left a comment

Copy link
Copy Markdown
Member

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!

Comment thread app/src/main/java/org/bepass/oblivion/ConnectionState.java Outdated
Comment thread app/src/main/java/org/bepass/oblivion/MainActivity.java Outdated
Comment on lines +454 to +479
if (splitTunnelMode != SplitTunnelMode.DISABLED) {
for (String packageName : getSplitTunnelApps(fileManager)) {
if (splitTunnelMode == SplitTunnelMode.BLACKLIST)
builder.addDisallowedApplication(packageName);
}

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread app/src/main/res/layout/installed_app_item.xml
@markpash

Copy link
Copy Markdown
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

ameerhossein commented Feb 21, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

ameerhossein commented Feb 21, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

@ameerhossein

Copy link
Copy Markdown
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