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

Refine auto connecting #2196

Merged
merged 6 commits into from May 24, 2019

Conversation

@Mygod
Copy link
Contributor

commented Apr 26, 2019

Here's some further possible enhancements for this feature:

  • Auto connect only if the service was not shut down manually by user, i.e. via stop broadcast or other VPN app replacing, but not package upgrades/force stops/system shutdowns.
  • If we did this, maybe we can remove auto connect switch altogether?

@madeye Thoughts?

@Mygod Mygod added the enhancement label Apr 26, 2019
@Mygod Mygod requested a review from madeye Apr 26, 2019
@madeye

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I think auto connect is a legacy option, as we didn't have always-on VPN and tasker integration years ago.

So, I'm fine to remove it in the future.

@madeye
madeye approved these changes Apr 26, 2019
Copy link
Contributor

left a comment

LGTM

@Mygod

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Here's my experience with always-on VPN and the motivations for my proposed change:

  1. You need to go to the settings to switch to another VPN app. (I need to do this occasionally as I need to test VPN Hotspot with a variety of different VPN apps)
  2. You also need to go to the settings to turn off always-on VPN if you are travelling and you no longer need to use VPN.
  3. Always-on VPN tends to confuse user as it forbids other VPN apps from establishing a VPN connection altogether (VpnService.establish simply returns null) and the error message is often unclear.

EDIT: However, I'm not sure if we need a switch "Auto restart service".

@madeye

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Yeah, always-on VPN is a little bit confusing, and very few users understand how it works.

But if we remove the "auto connect" option and enable it by default, I think it may also introduce some confusion, especially if we cannot handle different scenarios well.

If we just "auto connect" on boot, maybe we can simplify the logic and remove the option safely.

Mygod added a commit that referenced this pull request May 8, 2019
Fix #2182, #2196.
@Mygod

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

What? How come it has already been 25 days? 😅

@Mygod Mygod requested a review from madeye May 22, 2019
@Mygod

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@madeye Ready for (final) review. Please see if this new behavior makes sense.

@madeye
madeye approved these changes May 24, 2019
Copy link
Contributor

left a comment

LGTM

@Mygod Mygod merged commit f9ff9f6 into master May 24, 2019
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
License Compliance All checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details
@Mygod Mygod deleted the autoconnect+ branch May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.