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 autostart on TV and remove auto connect feature on android #6761

Merged
merged 7 commits into from
Sep 26, 2024

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Sep 9, 2024

This PR does three things:

  1. Add a connect on boot feature for devices that does not vpn settings (mostly TV)
  2. Disables the auo-connect feauture for all devices.
  3. Remove the auto-connect feature from the ui for all devices.

This change is Reviewable

@Pururun Pururun added the Android Issues related to Android label Sep 9, 2024
@Rawa Rawa added the On hold Means the PR is paused for some reason. No need to review it for now label Sep 16, 2024
@albin-mullvad albin-mullvad removed the On hold Means the PR is paused for some reason. No need to review it for now label Sep 24, 2024
@Pururun Pururun marked this pull request as ready for review September 25, 2024 08:59
Copy link
Contributor

@kl kl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 17 at r1 (raw file):

    private val bootCompletedComponentName: ComponentName,
) {
    val autoStartAndConnectOnBoot = MutableStateFlow(isAutoStartAndConnectOnBoot())

The public type could be a StateFlow here instead of a MutableStateFlow

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks ready, I just have a few minor comments and I also believe we need another quick round of copy discussion

Reviewed 19 of 21 files at r1, all commit messages.
Reviewable status: 19 of 21 files reviewed, 8 unresolved discussions (waiting on @Pururun)


android/CHANGELOG.md line 30 at r1 (raw file):

- Add WireGuard over Shadowsocks.
- Add feature indicators to the main view along with redesigning the connection details.
- Add auto connect and start setting for devices without system vpn settings.

I suggest rephrasing a bit:

Add new "Auto-connect on boot" setting for devices without system VPN settings.

Code quote:

Add auto connect and start setting for devices without system vpn settings.

android/CHANGELOG.md line 33 at r1 (raw file):

### Removed
- Legacy auto connect feature.

nit: auto-connect

Code quote:

auto connect

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r1 (raw file):

    private fun startAndConnectTunnel(context: Context) {
        val hasVpnPermission = VpnService.prepare(context) == null
        if (hasVpnPermission) {

Maybe we should add a info log here? Could be useful if users report that it doesn't work


android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 19 at r1 (raw file):

    val autoStartAndConnectOnBoot = MutableStateFlow(isAutoStartAndConnectOnBoot())

    fun setAutoStartAndConnectOnBoot(connect: Boolean) {

nit: change to something like enabled?

Code quote:

connect

gui/locales/messages.pot line 2291 at r1 (raw file):

msgstr ""

msgid "Automatically connect on device start up. Please make sure the app has VPN permission."

I believe this is usually written as "startup" or "start-up"

Code quote:

start up

gui/locales/messages.pot line 2291 at r1 (raw file):
I believe this is better phrased as:

Only works if the system VPN permission has been granted.

Code quote:

Please make sure the app has VPN permission.

gui/locales/messages.pot line 2303 at r1 (raw file):
"boot" sounds very technical. Something like the following makes more sense IMO:

Connect on device start-up

Code quote:

boot

Copy link
Contributor

@kl kl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Pururun)

Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 7 unresolved discussions


android/CHANGELOG.md line 33 at r1 (raw file):

Previously, albin-mullvad wrote…

nit: auto-connect

Done

Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 20 of 21 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @kl)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r1 (raw file):

Previously, albin-mullvad wrote…

Maybe we should add a info log here? Could be useful if users report that it doesn't work

Added info log


android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 17 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

The public type could be a StateFlow here instead of a MutableStateFlow

Done


android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 19 at r1 (raw file):

Previously, albin-mullvad wrote…

nit: change to something like enabled?

Done

@Pururun Pururun force-pushed the android-auto-start-migration branch 2 times, most recently from a3bf4d0 to b2699f1 Compare September 25, 2024 13:43
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 16 of 21 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @kl)


android/CHANGELOG.md line 30 at r1 (raw file):

Previously, albin-mullvad wrote…

I suggest rephrasing a bit:

Add new "Auto-connect on boot" setting for devices without system VPN settings.

Added but changed the text in the quotation marks to be the same as the name of the setting, "Connect on device start-up"


gui/locales/messages.pot line 2291 at r1 (raw file):

Previously, albin-mullvad wrote…

I believe this is usually written as "startup" or "start-up"

Done


gui/locales/messages.pot line 2291 at r1 (raw file):

Previously, albin-mullvad wrote…

I believe this is better phrased as:

Only works if the system VPN permission has been granted.

Changed to "Only works if the app has been granted the VPN permission." after discussion.


gui/locales/messages.pot line 2303 at r1 (raw file):

Previously, albin-mullvad wrote…

"boot" sounds very technical. Something like the following makes more sense IMO:

Connect on device start-up

Done

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Added info log

Is it clear enough that it's a automatic start on boot? 🤔

albin-mullvad
albin-mullvad previously approved these changes Sep 26, 2024
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 21 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)

Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 19 of 21 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @kl)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r1 (raw file):

Previously, albin-mullvad wrote…

Is it clear enough that it's a automatic start on boot? 🤔

Clarified

Copy link
Contributor

@kl kl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 19 of 21 files reviewed, all discussions resolved (waiting on @albin-mullvad)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa merged commit c61a054 into main Sep 26, 2024
62 checks passed
@Rawa Rawa deleted the android-auto-start-migration branch September 26, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants