health: only display system notifications for high severity warnings,… (

#436) 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 <[email protected]>
tailscale · Jul 2, 2024 · b3a7498 · b3a7498
1 parent 840a31d
commit b3a7498
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 19 deletions.
diff --git 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
@@ -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<String, String>? = null,
       var DependsOn: List<String>? = null, // an array of WarnableCodes this depends on
-  ) {
+  ) : Comparable<UnhealthyState> {
     fun hiddenByDependencies(currentWarnableCodes: Set<String>): 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<Severity> {
+    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
@@ -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<String> = mutableSetOf()
+  val currentWarnings: StateFlow<Set<UnhealthyState>> = MutableStateFlow(setOf())
 
   private fun notifyHealthUpdated(warnings: Array<UnhealthyState>) {
-    val warningsBeforeAdd = currentWarnings
+    val warningsBeforeAdd = currentWarnings.value
     val currentWarnableCodes = warnings.map { it.WarnableCode }.toSet()
+    val addedWarnings: MutableSet<UnhealthyState> = mutableSetOf()
+    val isWarmingUp = warnings.any { it.WarnableCode == "warming-up" }
 
-    val addedWarnings: MutableSet<String> = 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<String>) {
-    Log.d(TAG, "Removing notifications for $codes")
-    for (code in codes) {
-      notificationManager.cancel(code.hashCode())
+  private fun removeNotifications(warnings: Set<UnhealthyState>) {
+    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
@@ -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
@@ -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<List<Health.UnhealthyState>> = 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() {