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 DAITA support to the android app #6684

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

albin-mullvad
Copy link
Collaborator

@albin-mullvad albin-mullvad commented Aug 27, 2024

This PR aims to add DAITA support to the Android app.

The PR is mostly ready to be reviewed, however there are some minor remaining work TODO:

  • Improve and clean up changes to the wireguard-go-rs build. (done in separate PR)
  • Fix failing clippy and udeps actions. These might be fixed by the item above. (done in separate PR)
  • Add daita filter chip.
  • Add daita confirmation dialog.

This change is Reviewable

Copy link

linear bot commented Aug 27, 2024

@albin-mullvad albin-mullvad added the Android Issues related to Android label Aug 27, 2024
@albin-mullvad albin-mullvad self-assigned this Aug 27, 2024
@albin-mullvad albin-mullvad marked this pull request as ready for review August 27, 2024 13:33
Copy link
Contributor

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

Reviewed 71 of 71 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

                Spacer(modifier = Modifier.height(Dimens.cellLabelVerticalPadding))
                HeaderSwitchComposeCell(
                    title = "DAITA",

Should be an untranslatable string resource I think?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 510 at r1 (raw file):

                    title = "DAITA",
                    isToggled = state.isDaitaEnabled,
                    isEnabled = true,

Is true by default so not needed


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModel.kt line 143 at r1 (raw file):

                    add(FilterChip.Provider(providerCountFilter))
                }
                if (settings?.tunnelOptions?.wireguard?.daita == true) {

Maybe a bit nice with some kind of extension function like Settings.hasDaita(): Boolean ?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 51 of 71 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

                Spacer(modifier = Modifier.height(Dimens.cellLabelVerticalPadding))
                HeaderSwitchComposeCell(
                    title = "DAITA",

Should be resource?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 73 at r1 (raw file):

}

fun RelayItem.Location.Country.applyFilters(

Should we call it just filter? With applyFilters it sounds like we apply a filter to a Country, thus that it would be added on top of the country.


android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/DaitaSettings.kt line 3 at r1 (raw file):

package net.mullvad.mullvadvpn.lib.model

enum class DaitaSettings {

If we only have On/Off, we should make this a Boolean.


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 7 at r1 (raw file):

import java.io.FileOutputStream

class AssetToFilesDirExtractor(val context: Context) {

Do we even need to have this instance? We should be able to just make it a function right? Maybe it can also be and extension function on Context if just plan to use it within the scope of a Context.


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 8 at r1 (raw file):

class AssetToFilesDirExtractor(val context: Context) {
    fun extract(assetName: String, overwriteFileIfAssetMoreRecent: Boolean = false) {

Do we ever user anything else than true? Is the flag needed?

Copy link
Collaborator Author

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

Reviewable status: 58 of 74 files reviewed, 7 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Should be resource?

Fixed! 👍


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should be an untranslatable string resource I think?

Fixed! 👍


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 510 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is true by default so not needed

This was copy pasta from other toggles were we also seem to explicitly set it, but removed it from this one now


android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 73 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Should we call it just filter? With applyFilters it sounds like we apply a filter to a Country, thus that it would be added on top of the country.

Done


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModel.kt line 143 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe a bit nice with some kind of extension function like Settings.hasDaita(): Boolean ?

Done.


android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/DaitaSettings.kt line 3 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

If we only have On/Off, we should make this a Boolean.

Done.


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 7 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we even need to have this instance? We should be able to just make it a function right? Maybe it can also be and extension function on Context if just plan to use it within the scope of a Context.

Done.


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 8 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we ever user anything else than true? Is the flag needed?

Done.

@albin-mullvad albin-mullvad force-pushed the add-android-daita-droid-1192 branch 4 times, most recently from abde4be to 123d561 Compare August 28, 2024 14:08
Pururun
Pururun previously approved these changes Aug 29, 2024
Copy link
Contributor

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

Reviewed 36 of 36 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 182 at r2 (raw file):

    }

    daitaConfirmationDialogResult.OnNavResultValue { doEnableDaita ->

I wonder if we should move this code to the dialog to not increase the complexity of VpnSettings

Copy link
Contributor

@Rawa Rawa 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 36 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DaitaConfirmationDialog.kt line 48 at r2 (raw file):

                modifier = Modifier.fillMaxWidth().height(Dimens.dialogIconHeight),
                painter = painterResource(id = R.drawable.icon_alert),
                contentDescription = "",

contentDescription = null, seems more reasonable than ""


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 521 at r2 (raw file):

                    title = stringResource(id = R.string.daita),
                    isToggled = state.isDaitaEnabled,
                    onCellClicked = { newValueIsEnable ->

Name is pretty weird newValudIsEnable, I think it is fine that it is just enable, it is implicit that it is the new value.

Copy link
Collaborator Author

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

Reviewable status: 73 of 75 files reviewed, 3 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DaitaConfirmationDialog.kt line 48 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

contentDescription = null, seems more reasonable than ""

Done! Copy pasta artifact, so we might want to clean that up in other places as well


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 182 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I wonder if we should move this code to the dialog to not increase the complexity of VpnSettings

Went with the other approach at first, but felt like way too much boiler plate for such a trivial call where the result is always either to cancel or just call a single vm function (always with true as an argument). Me and David discussed it and agreed to go with this approach for now, but we said that we might want to eventually take a step back and restructure some parts of the vpn settings screen. Does that sound OK?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 521 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Name is pretty weird newValudIsEnable, I think it is fine that it is just enable, it is implicit that it is the new value.

Done.

@albin-mullvad
Copy link
Collaborator Author

Rebased on #6701 which we aim to merge first.

@albin-mullvad albin-mullvad force-pushed the add-android-daita-droid-1192 branch 2 times, most recently from d4bb188 to debc6b6 Compare August 29, 2024 14:39
@albin-mullvad albin-mullvad changed the title Add android daita Add DAITA support to the android app Aug 29, 2024
Copy link
Contributor

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

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 67 at r10 (raw file):

        border =
            FilterChipDefaults.filterChipBorder(
                borderColor = borderColor,

Should probably set disabledBorderColor as well since it is transparent.

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

Copy link
Contributor

@Rawa Rawa 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, 2 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 75 at r10 (raw file):

        label = { Text(text = text, style = MaterialTheme.typography.labelMedium) },
        trailingIcon = {
            if (enabled) {

Trailing icon takes nullable composable, so emitting a empty composable count result in some unexpected padding to be added. If we don't have a icon we should not emit any lambda at all.

Copy link
Collaborator Author

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

Reviewable status: 75 of 78 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98, @Pururun, and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 67 at r10 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should probably set disabledBorderColor as well since it is transparent.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 75 at r10 (raw file):

Previously, Rawa (David Göransson) wrote…

Trailing icon takes nullable composable, so emitting a empty composable count result in some unexpected padding to be added. If we don't have a icon we should not emit any lambda at all.

Done.

Rawa
Rawa previously approved these changes Sep 6, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

Pururun
Pururun previously approved these changes Sep 6, 2024
Copy link
Contributor

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

:lgtm:

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

@albin-mullvad albin-mullvad force-pushed the add-android-daita-droid-1192 branch 2 times, most recently from d6276f8 to c7cd012 Compare September 6, 2024 12:26
Temporary fix to address maybenot.h (checked in) sometimes needing to
be re-generated due to how `make` looks at last-modifications while
git neither stores nor consistently sets modification metadata on
file checkout.

NOTE: The version should match the one used in the checked in
maybenot.h file.
Copy link
Contributor

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

:lgtm:

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

@albin-mullvad albin-mullvad merged commit 17788c6 into main Sep 6, 2024
58 of 61 checks passed
@albin-mullvad albin-mullvad deleted the add-android-daita-droid-1192 branch September 6, 2024 13:28
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.

4 participants