Android gRPC communication #6251
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 416 files reviewed, 2 unresolved discussions (waiting on @Pururun)
android/app/build.gradle.kts line 342 at r1 (raw file):
ksp(Dependencies.Compose.destinationsKsp) implementation("androidx.activity:activity-ktx:1.9.0")
Should be fixed with and import
android/app/lint-baseline.xml line 5 at r1 (raw file):
<issue id="SdCardPath"
Should we revert/fix?
There was a problem hiding this comment.
Reviewable status: 0 of 416 files reviewed, 3 unresolved discussions (waiting on @Pururun)
android/app/src/main/AndroidManifest.xml line 1 at r1 (raw file):
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
We should run the XML tidy thingy, so there isn't a lot of formatting changes on this file.
There was a problem hiding this comment.
Reviewable status: 0 of 416 files reviewed, 4 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/WireguardCustomPortDialog.kt line 43 at r1 (raw file):
WireguardCustomPortDialog( WireguardCustomPortNavArgs( customPort = null,
We should change this to be of type Port?
There was a problem hiding this comment.
Reviewable status: 0 of 416 files reviewed, 5 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 143 at r1 (raw file):
navigationIcon = { NavigateBackIconButton(onNavigateBack = onBackClick) }, actions = { Actions(
Maybe we should hide the actions until the title has been loaded. It would make more sense then to present a broken button.
There was a problem hiding this comment.
Reviewable status: 0 of 416 files reviewed, 5 unresolved discussions (waiting on @Rawa)
android/app/build.gradle.kts line 342 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Should be fixed with and import
What is it used for?
android/app/lint-baseline.xml line 5 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Should we revert/fix?
If we can use Context.getFilesDir().getPath() and it works all the time we should probably use that instead.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 5 unresolved discussions (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/WireguardCustomPortDialog.kt line 43 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
We should change this to be of type
Port?
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 143 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Maybe we should hide the actions until the title has been loaded. It would make more sense then to present a broken button.
I disabled the dropdown instead.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 5 unresolved discussions (waiting on @Rawa)
android/app/src/main/AndroidManifest.xml line 1 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
We should run the XML tidy thingy, so there isn't a lot of formatting changes on this file.
I ran the tidy thing, but it still moves a lot of stuff around, not sure why. The github action passes at least.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/build.gradle.kts line 342 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
What is it used for?
If I remembered correctly if we don't declare this one we get inherently an older version which makes us miss a specific extension function we used. Not sure if we ended up using that extension function though, so could be worth removing it if possible.
android/app/lint-baseline.xml line 5 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
If we can use
Context.getFilesDir().getPath()and it works all the time we should probably use that instead.
I believe it should be the right way to get it since it is the filesDir of the app, it should be more reliable than a hardcoded path that we don't know if it is correct on all platforms. We just need to make sure the daemon has the same idea of what path it is as well.
android/app/src/main/AndroidManifest.xml line 1 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I ran the tidy thing, but it still moves a lot of stuff around, not sure why. The github action passes at least.
Huh, strange. May @albin-mullvad has some input
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/WireguardCustomPortDialog.kt line 43 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Done
Nice 🙏
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 143 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I disabled the dropdown instead.
👍
There was a problem hiding this comment.
Reviewed 105 of 416 files at r1, 1 of 13 files at r3, 4 of 5 files at r4, 1 of 1 files at r5, 2 of 10 files at r6, all commit messages.
Reviewable status: 0 of 430 files reviewed, 29 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialogTest.kt line 48 at r6 (raw file):
// Arrange val state = UpdateCustomListUiState(error = UpdateCustomListError.NameAlreadyExists("name"))
Can we use string from the state? Or should we avoid doing that?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/MtuDialog.kt line 43 at r7 (raw file):
@Destination(style = DestinationStyle.Dialog::class) @Composable fun MtuDialog(mtuInitial: Int?, navigator: ResultBackNavigator<Boolean>) {
Consider using Mtu type for this one.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DeviceListScreen.kt line 140 at r7 (raw file):
when (sideEffect) { DeviceListSideEffect.FailedToRemoveDevice -> { launch {
Use showSnackbarImmediately extension function if possible
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 511 at r6 (raw file):
selectedItem = selectedItem, onSelectRelay = onSelectRelay, onLongClick = { onShowLocationBottomSheet(it as RelayItem.Location) },
Is this cast dangerous? We should take a look and see if we can avoid it.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 844 at r6 (raw file):
when (this) { is CustomListResult.Created -> context.getString(R.string.location_was_added_to_list, locationNames.first(), name)
Possible dangerous use of first() we should look into alternatives.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ServerIpOverridesScreen.kt line 127 at r6 (raw file):
this, message = if (result) {
Result is very ambiguous, we should clarify it.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 140 at r6 (raw file):
navigator: DestinationsNavigator, dnsDialogResult: ResultRecipient<DnsDialogDestination, DnsDialogResult>, customWgPortResult: ResultRecipient<WireguardCustomPortDialogDestination, Int?>,
Can we get a result of type Port?instead?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 177 at r6 (raw file):
is VpnSettingsSideEffect.ShowToast -> launch { snackbarHostState.currentSnackbarData?.dismiss()
We have an extension function called showSnackbarImmediately that should do this dance.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/PathConstant.kt line 3 at r6 (raw file):
package net.mullvad.mullvadvpn.constant const val GRPC_SOCKET_PATH = "/data/data/net.mullvad.mullvadvpn/rpc-socket"
Don't believe this should be hard-coded, lets review it and maybe us context
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/CustomListsRepository.kt line 57 at r6 (raw file):
customLists .mapNotNull { it?.find { customList -> customList.id == id } } .firstOrNullWithTimeout(GET_CUSTOM_LIST_TIMEOUT_MS) ?: raise(GetCustomListError)
Do we need the timeout? Any reason that this can fail?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/RelayOverridesRepository.kt line 26 at r7 (raw file):
.mapNotNull { it.relayOverrides } .onStart { // Get relay overrides
Unnecessary? Should remove or implement if needed?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SelectedLocationRepository.kt line 14 at r7 (raw file):
import net.mullvad.mullvadvpn.model.RelayItemId class SelectedLocationRepository(
Should this be part of some other repository? RelayListRepository or how do we feel about it?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SettingsRepository.kt line 54 at r7 (raw file):
suspend fun addCustomDns(address: InetAddress) = managementService.addCustomDns(address) suspend fun setWireguardMtu(value: Int?) = managementService.setWireguardMtu(value ?: 0)
This signature looks bad, we should take a Int instead, if 0 is used for clearing the option we should make that behavior more explicit, e.g exposing clearWireguardMtu() in ManagementService.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SplitTunnelingRepository.kt line 12 at r7 (raw file):
import net.mullvad.mullvadvpn.model.AppId class SplitTunnelingRepository(
This just looks so nice an clean 🌟
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 77 at r7 (raw file):
// super.onNewIntent(intent) // intentProvider.setStartIntent(intent) // }
Should this be removed?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 82 at r7 (raw file):
requestNotificationPermissionIfMissing(requestNotificationPermissionLauncher) serviceConnectionManager.bind( // vpnPermissionRequestHandler = ::requestVpnPermission,
Clean up
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/AppVersionInfoCache.kt line 9 at r7 (raw file):
import net.mullvad.mullvadvpn.ui.VersionInfo class AppVersionInfoCache(private val managementService: ManagementService) {
This feels very much one of those legacy names. Feels like this maybe should even be reduced down to a UseCase? AppVersionInfoUseCase or is it needed at all?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/AppVersionInfoCache.kt line 14 at r7 (raw file):
managementService.versionInfo, managementService.settings.map { it.showBetaReleases } ) { appVersionInfo, showBetaReleases ->
showBetaReleases isn't even used right now, do we have some regression?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ConnectionProxy.kt line 15 at r7 (raw file):
val tunnelState = managementService.tunnelState suspend fun connect(ignorePermission: Boolean = false): Either<ConnectError, Unit> = either {
This signature looks a bit weird, can we avoid the ignorePermission ?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ConnectionProxy.kt line 18 at r7 (raw file):
if (ignorePermission || vpnPermissionRepository.hasVpnPermission()) { managementService.connect().map { if (it) {
When does connect return false? Do we know this?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ConnectionProxy.kt line 29 at r7 (raw file):
} suspend fun disconnect() {
We could make use of = for now until we expand these.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 33 at r7 (raw file):
override fun onNullBinding(name: ComponentName?) { error("Received onNullBinding, why u do this to me?")
Hehe, maybe we should change this error a bit 😂
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 39 at r7 (raw file):
fun bind(apiEndpointConfiguration: ApiEndpointConfiguration?) { if (_connectionState.value is ServiceConnectionState.Unbound) { // this.vpnPermissionRequestHandler = vpnPermissionRequestHandler
Remove if not needed.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 50 at r7 (raw file):
intent, serviceConnection, ServiceInfo.FOREGROUND_SERVICE_TYPE_SYSTEM_EXEMPTED or BIND_AUTO_CREATE
Double check we should call it with BIND_AUTO_CREATE
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 87 at r7 (raw file):
// private fun handleVpnPermissionRequest() { // vpnPermissionRequestHandler?.invoke()
Remove all this if it is not needed.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/AccountExpiryNotificationUseCase.kt line 21 at r7 (raw file):
.distinctUntilChanged() private fun accountExpiryNotification(accountData: AccountData?) =
If we don't care about null we should replace with a filterNotNull and then a filter checking if the expiry is close to expiring.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 29 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/build.gradle.kts line 342 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
If I remembered correctly if we don't declare this one we get inherently an older version which makes us miss a specific extension function we used. Not sure if we ended up using that extension function though, so could be worth removing it if possible.
Removed and nothing seems to break.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 29 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/lint-baseline.xml line 5 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
I believe it should be the right way to get it since it is the filesDir of the app, it should be more reliable than a hardcoded path that we don't know if it is correct on all platforms. We just need to make sure the daemon has the same idea of what path it is as well.
Should be somewhat possible, looking into it.
There was a problem hiding this comment.
Reviewed 8 of 416 files at r1.
Reviewable status: 0 of 430 files reviewed, 35 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/AvailableProvidersUseCase.kt line 11 at r7 (raw file):
class AvailableProvidersUseCase(private val relayListRepository: RelayListRepository) { fun availableProviders(): Flow<List<Provider>> =
Just wow, this looks nice. So simple and idiomatic.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/LastKnownLocationUseCase.kt line 24 at r7 (raw file):
.filterIsInstance<TunnelState.Disconnected>() .map { it.location } .filterNotNull()
Combine map and filter and use mapNotNull
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PortRangeUseCase.kt line 11 at r7 (raw file):
class PortRangeUseCase(private val relayListRepository: RelayListRepository) { fun portRanges(): Flow<List<PortRange>> = relayListRepository.wireguardEndpointData.map { it.portRanges }.distinctUntilChanged()
Maybe use move this to the RelayListRepository. It doesn't really do any merging or logic
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/SelectedLocationRelayItemUseCase.kt line 22 at r7 (raw file):
private val selectedLocationRepository: SelectedLocationRepository ) { fun selectedRelayItem() = selectedRelayItemWithTitle().map { it?.first }
At a first glance this class looks really similar to the SelectedLocationRepository, are they roughly the same?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/SelectedLocationRelayItemUseCase.kt line 39 at r7 (raw file):
relayCountries: List<RelayItem.Location.Country>, customLists: List<CustomList> ): Pair<RelayItem, String>? {
This return type looks a bit weird, feel like we should be able to do something more simple.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/CustomListActionUseCase.kt line 20 at r7 (raw file):
private val relayListRepository: RelayListRepository ) { suspend fun performAction(action: CustomListAction): Either<Any, CustomListResult> {
Can we avoid using Any? It is very broad.
There was a problem hiding this comment.
Reviewable status: 0 of 430 files reviewed, 34 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialogTest.kt line 48 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Can we use string from the state? Or should we avoid doing that?
No we can do that.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 140 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Can we get a result of type
Port?instead?
Why would DNS dialog return port?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 177 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
We have an extension function called
showSnackbarImmediatelythat should do this dance.
Hmm I wonder if we should add some konsist test for this.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/CustomListsRepository.kt line 57 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Do we need the timeout? Any reason that this can fail?
So the reason for the timeout is that we first create the list and then we need to get the list after it has been created and this is not instantenious as we are waiting for an event from the daemon. I guess we could move the wait somewhere else if it is clearer.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/RelayOverridesRepository.kt line 26 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Unnecessary? Should remove or implement if needed?
Probably a left over, will remove.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SplitTunnelingRepository.kt line 12 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
This just looks so nice an clean 🌟
🚀
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/AppVersionInfoCache.kt line 14 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
showBetaReleasesisn't even used right now, do we have some regression?
No it has never been used as far as I know
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 50 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Double check we should call it with
BIND_AUTO_CREATE
Otherwise the service will not be started right? Since previously we started it explicitly.
There was a problem hiding this comment.
Reviewed 1 of 416 files at r1.
Reviewable status: 0 of 430 files reviewed, 36 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/CustomListsRelayItemUseCase.kt line 13 at r7 (raw file):
) { fun customListsRelayItems() =
Not easy to understand what this function returns from the name. If I understand it correctly it maps a List of CustomLists's to another representation of RelayItem.CustomList that also has it's locations. Not sure what a a good name would be? relayItemCustomLists?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/CustomListsRelayItemUseCase.kt line 17 at r7 (raw file):
customLists, relayList -> customLists?.toRelayItemLists(relayList) ?: emptyList()
I believe this extension function makes it a bit more confusing, it is only used in this place. Let's remove it and just use map and then invoke the toRelayItemCustomList on each CustomList instead.
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 35 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DeviceListScreen.kt line 140 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Use
showSnackbarImmediatelyextension function if possible
Done
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 34 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 844 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Possible dangerous use of
first()we should look into alternatives.
Made it safer.
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 33 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ServerIpOverridesScreen.kt line 127 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Result is very ambiguous, we should clarify it.
Done
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 33 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SelectedLocationRepository.kt line 14 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Should this be part of some other repository?
RelayListRepositoryor how do we feel about it?
I kinda split it before because it was using things unrelated to RelayList (GeographicLocation I think) but since we are now using RelayItemId instead I guess it is more natural to merge them.
There was a problem hiding this comment.
Reviewed 8 of 416 files at r1, 1 of 5 files at r4.
Reviewable status: 0 of 433 files reviewed, 40 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CreateCustomListDialogViewModel.kt line 48 at r7 (raw file):
) .fold( { _error.emit(it) },
_error::emit should be possible I believe
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 35 at r7 (raw file):
private val customListsRepository: CustomListsRepository ) : ViewModel() { private var customListName: String? = null
I know I've looked at this before, but we should try and remove it if possible. Should be part of some uiState
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 101 at r7 (raw file):
_uiSideEffect.tryEmit( // This is so that we don't show a snackbar after returning to // the select
Fix formatting of this comment.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 201 at r7 (raw file):
private fun List<RelayItem.Location>.selectChildren(): Set<RelayItem.Location> = (this.map { it } + flatMap { it.descendants() }).toSet()
We should refactor this a bit, and I believe this entire function can be removed:
- Change existing extension function
withDecendantsto act onRelayitem.Location - Remove this extension function and directly use
...?.withDecendats()?.toSet()
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 207 at r7 (raw file):
customListRelayItemsUseCase .getRelayItemLocationsForCustomList(customListId) .firstOrNullWithTimeout(GET_CUSTOM_LIST_TIMEOUT_MS)
Can we remove time out?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 214 at r7 (raw file):
private suspend fun fetchCustomListName() { customListName = customListsRepository.getCustomListById(customListId).getOrNull()?.name?.value ?: ""
Should we use !! instead? Or is empty string legit use case? Don't we expect it do exist?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListsViewModel.kt line 23 at r7 (raw file):
customListsRepository.customLists .filterNotNull() .map { CustomListsUiState.Content(it) }
when we are at it .map(CustomListsUiState::Content) 😅
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 42 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceListViewModel.kt line 51 at r7 (raw file):
} .onStart { fetchDevices() } .stateIn(viewModelScope, SharingStarted.Lazily, DeviceListUiState.Loading)
We introduced Lazily here is this dangerous? Isn't it possible that we cache an old deviceList from an old account?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 46 at r7 (raw file):
connectionProxy.disconnect() accountRepository.logout() }
We should just should double check that this can't get cancelled since we navigate away below.
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 41 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SelectedLocationRepository.kt line 14 at r7 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I kinda split it before because it was using things unrelated to RelayList (GeographicLocation I think) but since we are now using
RelayItemIdinstead I guess it is more natural to merge them.
Done
There was a problem hiding this comment.
Reviewable status: 0 of 433 files reviewed, 40 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ConnectionProxy.kt line 29 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
We could make use of
=for now until we expand these.
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 87 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Remove all this if it is not needed.
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 37 at r8 (raw file):
} fun bind(apiEndpointConfiguration: ApiEndpointConfiguration?) {
We are not using apiEndpointConfiguration so we should remove this.
There was a problem hiding this comment.
Reviewed 15 of 416 files at r1.
Reviewable status: 14 of 433 files reviewed, 42 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/AvailableProvidersUseCase.kt line 11 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Just wow, this looks nice. So simple and idiomatic.
It is quite the improvement ⭐
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewDeviceNotificationUseCase.kt line 15 at r8 (raw file):
fun notifications() = combine( deviceRepository.deviceState.map { it?.displayName() }.distinctUntilChanged(),
Could perhaps do mapNotNull here?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeleteCustomListConfirmationViewModel.kt line 39 at r8 (raw file):
fun deleteCustomList() { viewModelScope.launch { _error.tryEmit(null)
This should be just emit since we are in a coroutinescope
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceListViewModel.kt line 51 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
We introduced
Lazilyhere is this dangerous? Isn't it possible that we cache an old deviceList from an old account?
Yeah I think we should just try to use while subscribed
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 46 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
We should just should double check that this can't get cancelled since we navigate away below.
Maybe safer to just do it in the same launch?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModel.kt line 139 at r8 (raw file):
accountRepository.accountData.filterNotNull().map { it.expiryDate.isBeforeNow }.first() } delay(1000)
Do we need this delay? Should at least be a constant.
There was a problem hiding this comment.
Reviewable status: 14 of 433 files reviewed, 41 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SettingsRepository.kt line 54 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
This signature looks bad, we should take a
Intinstead, if 0 is used for clearing the option we should make that behavior more explicit, e.g exposingclearWireguardMtu()in ManagementService.
Done
There was a problem hiding this comment.
Reviewed 6 of 416 files at r1, 1 of 1 files at r9, 5 of 9 files at r13.
Reviewable status: 25 of 433 files reviewed, 40 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 33 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Hehe, maybe we should change this error a bit 😂
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/LastKnownLocationUseCase.kt line 24 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
Combine map and filter and use
mapNotNull
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/SelectedLocationRelayItemUseCase.kt line 22 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
At a first glance this class looks really similar to the
SelectedLocationRepository, are they roughly the same?
Well no longer since that repository is gone :D
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/SelectedLocationRelayItemUseCase.kt line 39 at r7 (raw file):
Previously, Rawa (David Göransson) wrote…
This return type looks a bit weird, feel like we should be able to do something more simple.
What would be more simple a wrapper class?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/NoDaemonViewModel.kt line 84 at r13 (raw file):
GrpcConnectivityState.TransientFailure -> DaemonState.Show GrpcConnectivityState.Idle, GrpcConnectivityState.Ready -> DaemonState.Hidden.Connected
Should we consider idle state as connected, it really is not I guess?
There was a problem hiding this comment.
Reviewed 15 of 416 files at r1, 1 of 1 files at r9, 5 of 8 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 7 of 9 files at r13.
Reviewable status: 24 of 433 files reviewed, 47 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 844 at r6 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Made it safer.
👍
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ServerIpOverridesScreen.kt line 127 at r6 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Done
Nice
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 140 at r6 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Why would DNS dialog return port?
Not DNS:
customWgPortResult: ResultRecipient<WireguardCustomPortDialogDestination, Int?>,
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 177 at r6 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Hmm I wonder if we should add some konsist test for this.
Hmm worth considering.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Snackbar.kt line 25 at r10 (raw file):
SnackbarResult.ActionPerformed -> onAction() SnackbarResult.Dismissed -> onDismiss() }
Can we use showSnackbarImmediately? also if it doesn't already it should return this SnackbarResult
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/ServiceConnectionManager.kt line 50 at r7 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Otherwise the service will not be started right? Since previously we started it explicitly.
I believe that might be correct. Should we add a comment about it?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModel.kt line 139 at r7 (raw file):
accountRepository.accountData.filterNotNull().map { it.expiryDate.isBeforeNow }.first() } delay(1000)
Magic number that should be const.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModel.kt line 140 at r7 (raw file):
} delay(1000) val result = isOutOfTimeDeferred.getOrDefault(false)
No need to introduce result if we just return it. Maybe also add an explicit comment to make it more clear.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 64 at r13 (raw file):
fun onSitePaymentClick() { viewModelScope.launch { accountRepository.getWwwAuthToken()?.let { wwwAuthToken ->
Maybe out of scope for this PR, but this may result in a network request that can take time, we should then show loading.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModel.kt line 137 at r13 (raw file):
fun addLocationToList(item: RelayItem.Location, customList: RelayItem.CustomList) { viewModelScope.launch { // If this is null then something is seriously wrong
What can be null? Is this comment relevant still? Or do we mean empty?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SettingsViewModel.kt line 28 at r13 (raw file):
isLoggedIn = accountState is DeviceState.LoggedIn, appVersion = BuildConfig.VERSION_NAME, isUpdateAvailable = versionInfo.let { it.isSupported.not() || it.isOutdated },
Should we make this logic into the versionInfo directly?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SplashViewModel.kt line 59 at r15 (raw file):
} return if (accountData != null) {
This return statement is a mess, we should use a single if.
There was a problem hiding this comment.
Reviewed 74 of 416 files at r1, 8 of 10 files at r6, 1 of 2 files at r8, 1 of 9 files at r13.
Reviewable status: 108 of 433 files reviewed, 51 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 36 at r13 (raw file):
class AccountViewModelTest { private val mockAccountRepository: net.mullvad.mullvadvpn.lib.account.AccountRepository =
Remove package name
android/buildSrc/src/main/kotlin/Dependencies.kt line 31 at r13 (raw file):
const val lifecycleRuntimeCompose = "androidx.lifecycle:lifecycle-runtime-compose:${Versions.AndroidX.lifecycle}" const val lifecycleService =
This is added to app and talpid, do we know what it is used for?
android/buildSrc/src/main/kotlin/Dependencies.kt line 49 at r13 (raw file):
const val optics = "io.arrow-kt:arrow-optics:${Versions.Arrow.base}" const val opticsKsp = "io.arrow-kt:arrow-optics-ksp-plugin:${Versions.Arrow.base}" const val fxCoroutines = "io.arrow-kt:arrow-fx-coroutines:${Versions.Arrow.base}"
What is this library used for?
android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 570 at r13 (raw file):
} sealed interface ServiceConnectionState {
Is this used? Otherwise I think we can remove it.
|
Closing and opening new PR due to Reviewable not liking these big changes. #6272 |
This PR performs a major overhaul of the communication between the Daemon and the Android Service layer. It migrates over most of the old legacy JNI and IPC Messenger implementation to use the same protocol as the desktop client, gRPC.
Most notable changes:
This change is