From b3a74986ac7195037a18d68aa22ed4ba34cc9fa2 Mon Sep 17 00:00:00 2001 From: Andrea Gottardo Date: Tue, 2 Jul 2024 09:04:12 -0700 Subject: [PATCH] =?UTF-8?q?health:=20only=20display=20system=20notificatio?= =?UTF-8?q?ns=20for=20high=20severity=20warnings,=E2=80=A6=20(#436)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit health: only display system notifications for high severity warnings, show low severity notifications in-app Updates tailscale/tailscale#4136 This PR brings the Android health system in line with recent macOS/iOS changes. Only high severity notifications will now trigger a system notification; meanwhile all notifications are now displayed in the app home screen, like we do on iOS. The "warming-up" Warnable is observed to prevent spurious notifications from appearing while the app has just launched. Signed-off-by: Andrea Gottardo --- .../src/main/java/com/tailscale/ipn/App.kt | 2 +- .../java/com/tailscale/ipn/ui/model/Health.kt | 54 +++++++++++++++++-- .../ipn/ui/notifier/HealthNotifier.kt | 33 +++++++----- .../com/tailscale/ipn/ui/view/MainView.kt | 33 +++++++++++- .../ipn/ui/viewModel/MainViewModel.kt | 10 ++++ 5 files changed, 113 insertions(+), 19 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/App.kt b/android/src/main/java/com/tailscale/ipn/App.kt index 1eb7c31..6af508c 100644 --- a/android/src/main/java/com/tailscale/ipn/App.kt +++ b/android/src/main/java/com/tailscale/ipn/App.kt @@ -72,7 +72,7 @@ class App : UninitializedApp(), libtailscale.AppContext { val dns = DnsConfig() private lateinit var connectivityManager: ConnectivityManager private lateinit var app: libtailscale.Application - private var healthNotifier: HealthNotifier? = null + var healthNotifier: HealthNotifier? = null override fun getPlatformDNSConfig(): String = dns.dnsConfigAsString diff --git a/android/src/main/java/com/tailscale/ipn/ui/model/Health.kt b/android/src/main/java/com/tailscale/ipn/ui/model/Health.kt index 9044058..523d764 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/model/Health.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/model/Health.kt @@ -3,6 +3,11 @@ package com.tailscale.ipn.ui.model +import androidx.compose.material3.ListItemColors +import androidx.compose.material3.ListItemDefaults +import androidx.compose.material3.MaterialTheme +import androidx.compose.runtime.Composable +import com.tailscale.ipn.ui.theme.warning import kotlinx.serialization.Serializable class Health { @@ -21,18 +26,59 @@ class Health { var BrokenSince: String? = null, var Args: Map? = null, var DependsOn: List? = null, // an array of WarnableCodes this depends on - ) { + ) : Comparable { fun hiddenByDependencies(currentWarnableCodes: Set): Boolean { return this.DependsOn?.let { it.any { depWarnableCode -> currentWarnableCodes.contains(depWarnableCode) } } == true } + + override fun compareTo(other: UnhealthyState): Int { + // Compare by severity first + val severityComparison = Severity.compareTo(other.Severity) + if (severityComparison != 0) { + return severityComparison + } + + // If severities are equal, compare by warnableCode + return WarnableCode.compareTo(other.WarnableCode) + } } @Serializable - enum class Severity { - high, + enum class Severity : Comparable { + low, medium, - low + high; + + @Composable + fun listItemColors(): ListItemColors { + val default = ListItemDefaults.colors() + return when (this) { + Severity.low -> + ListItemColors( + containerColor = MaterialTheme.colorScheme.surface, + headlineColor = MaterialTheme.colorScheme.secondary, + leadingIconColor = MaterialTheme.colorScheme.secondary, + overlineColor = MaterialTheme.colorScheme.secondary.copy(alpha = 0.8f), + supportingTextColor = MaterialTheme.colorScheme.secondary, + trailingIconColor = MaterialTheme.colorScheme.secondary, + disabledHeadlineColor = default.disabledHeadlineColor, + disabledLeadingIconColor = default.disabledLeadingIconColor, + disabledTrailingIconColor = default.disabledTrailingIconColor) + Severity.medium, + Severity.high -> + ListItemColors( + containerColor = MaterialTheme.colorScheme.warning, + headlineColor = MaterialTheme.colorScheme.onPrimary, + leadingIconColor = MaterialTheme.colorScheme.onPrimary, + overlineColor = MaterialTheme.colorScheme.onPrimary.copy(alpha = 0.8f), + supportingTextColor = MaterialTheme.colorScheme.onPrimary, + trailingIconColor = MaterialTheme.colorScheme.onPrimary, + disabledHeadlineColor = default.disabledHeadlineColor, + disabledLeadingIconColor = default.disabledLeadingIconColor, + disabledTrailingIconColor = default.disabledTrailingIconColor) + } + } } } diff --git a/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt b/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt index 6fa61c9..f3614eb 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt @@ -13,8 +13,10 @@ import com.tailscale.ipn.R import com.tailscale.ipn.UninitializedApp.Companion.notificationManager import com.tailscale.ipn.ui.model.Health import com.tailscale.ipn.ui.model.Health.UnhealthyState +import com.tailscale.ipn.ui.util.set import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged @@ -53,31 +55,36 @@ class HealthNotifier( } } - private val currentWarnings: MutableSet = mutableSetOf() + val currentWarnings: StateFlow> = MutableStateFlow(setOf()) private fun notifyHealthUpdated(warnings: Array) { - val warningsBeforeAdd = currentWarnings + val warningsBeforeAdd = currentWarnings.value val currentWarnableCodes = warnings.map { it.WarnableCode }.toSet() + val addedWarnings: MutableSet = mutableSetOf() + val isWarmingUp = warnings.any { it.WarnableCode == "warming-up" } - val addedWarnings: MutableSet = mutableSetOf() for (warning in warnings) { if (ignoredWarnableCodes.contains(warning.WarnableCode)) { continue } - addedWarnings.add(warning.WarnableCode) + addedWarnings.add(warning) - if (this.currentWarnings.contains(warning.WarnableCode)) { + if (this.currentWarnings.value.contains(warning)) { // Already notified, skip continue } else if (warning.hiddenByDependencies(currentWarnableCodes)) { // Ignore this warning because a dependency is also unhealthy Log.d(TAG, "Ignoring ${warning.WarnableCode} because of dependency") continue - } else { + } else if (!isWarmingUp) { Log.d(TAG, "Adding health warning: ${warning.WarnableCode}") - this.currentWarnings.add(warning.WarnableCode) - this.sendNotification(warning.Title, warning.Text, warning.WarnableCode) + this.currentWarnings.set(this.currentWarnings.value + warning) + if (warning.Severity == Health.Severity.high) { + this.sendNotification(warning.Title, warning.Text, warning.WarnableCode) + } + } else { + Log.d(TAG, "Ignoring ${warning.WarnableCode} because warming up") } } @@ -86,7 +93,7 @@ class HealthNotifier( Log.d(TAG, "Dropping health warnings with codes $warningsToDrop") this.removeNotifications(warningsToDrop) } - currentWarnings.subtract(warningsToDrop) + currentWarnings.set(this.currentWarnings.value.subtract(warningsToDrop)) } private fun sendNotification(title: String, text: String, code: String) { @@ -108,10 +115,10 @@ class HealthNotifier( notificationManager.notify(code.hashCode(), notification) } - private fun removeNotifications(codes: Set) { - Log.d(TAG, "Removing notifications for $codes") - for (code in codes) { - notificationManager.cancel(code.hashCode()) + private fun removeNotifications(warnings: Set) { + Log.d(TAG, "Removing notifications for $warnings") + for (warning in warnings) { + notificationManager.cancel(warning.WarnableCode.hashCode()) } } } diff --git a/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt b/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt index 2ad3950..e3e9416 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt @@ -73,6 +73,7 @@ import com.google.accompanist.permissions.ExperimentalPermissionsApi import com.tailscale.ipn.R import com.tailscale.ipn.mdm.MDMSettings import com.tailscale.ipn.mdm.ShowHide +import com.tailscale.ipn.ui.model.Health import com.tailscale.ipn.ui.model.Ipn import com.tailscale.ipn.ui.model.IpnLocal import com.tailscale.ipn.ui.model.Netmap @@ -117,6 +118,7 @@ fun MainView( viewModel: MainViewModel ) { val currentPingDevice by viewModel.pingViewModel.peer.collectAsState() + val healthWarnings by viewModel.healthWarnings.collectAsState() LoadingIndicator.Wrap { Scaffold(contentWindowInsets = WindowInsets.Companion.statusBars) { paddingInsets -> @@ -198,6 +200,10 @@ fun MainView( ExpiryNotification(netmap = netmap, action = { viewModel.login() }) } + for (warning in healthWarnings) { + HealthNotification(warning = warning) + } + if (showExitNodePicker == ShowHide.Show) { ExitNodeStatus( navAction = navigation.onNavigateToExitNodes, viewModel = viewModel) @@ -225,7 +231,9 @@ fun MainView( } currentPingDevice?.let { peer -> - ModalBottomSheet(onDismissRequest = { viewModel.onPingDismissal() }) { PingView(model = viewModel.pingViewModel) } + ModalBottomSheet(onDismissRequest = { viewModel.onPingDismissal() }) { + PingView(model = viewModel.pingViewModel) + } } } } @@ -701,6 +709,29 @@ fun ExpiryNotification(netmap: Netmap.NetworkMap?, action: () -> Unit = {}) { } } +@Composable +fun HealthNotification(warning: Health.UnhealthyState) { + Box(modifier = Modifier.background(color = MaterialTheme.colorScheme.surfaceContainerLow)) { + Box( + modifier = + Modifier.padding(start = 16.dp, end = 16.dp, top = 8.dp, bottom = 8.dp) + .clip(shape = RoundedCornerShape(10.dp, 10.dp, 10.dp, 10.dp)) + .fillMaxWidth()) { + ListItem( + colors = warning.Severity.listItemColors(), + headlineContent = { + Text( + warning.Title, + style = MaterialTheme.typography.titleMedium, + ) + }, + supportingContent = { + Text(warning.Text, style = MaterialTheme.typography.bodyMedium) + }) + } + } +} + @OptIn(ExperimentalPermissionsApi::class) @Composable fun PromptPermissionsIfNecessary() { diff --git a/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt b/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt index f949afe..30821b5 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt @@ -12,6 +12,7 @@ import androidx.lifecycle.viewModelScope import com.tailscale.ipn.App import com.tailscale.ipn.R import com.tailscale.ipn.mdm.MDMSettings +import com.tailscale.ipn.ui.model.Health import com.tailscale.ipn.ui.model.Ipn import com.tailscale.ipn.ui.model.Ipn.State import com.tailscale.ipn.ui.model.Tailcfg @@ -56,6 +57,9 @@ class MainViewModel : IpnViewModel() { var pingViewModel: PingViewModel = PingViewModel() + // Health warnings displayed in the UI, if any + val healthWarnings: StateFlow> = MutableStateFlow(listOf()) + fun hidePeerDropdownMenu() { expandedMenuPeer.set(null) } @@ -118,6 +122,12 @@ class MainViewModel : IpnViewModel() { viewModelScope.launch { searchTerm.collect { term -> peers.set(peerCategorizer.groupedAndFilteredPeers(term)) } } + + viewModelScope.launch { + App.get().healthNotifier?.currentWarnings?.collect { warnings -> + healthWarnings.set(warnings.sorted()) + } + } } fun showVPNPermissionLauncherIfUnauthorized() {