android/ui: stop treating settings as data (#294)

* android/ui: reflect latest state of allow LAN access setting Updates tailscale/corp#18983 Signed-off-by: Percy Wegmann <percy@tailscale.com> * android/ui: stop treating settings as data Updates tailscale/corp#18983 Signed-off-by: Percy Wegmann <percy@tailscale.com> --------- Signed-off-by: Percy Wegmann <percy@tailscale.com>
4 weeks ago · 2e237e375e
parent 71f03cf0d2
commit 2e237e375e
8 changed files with 95 additions and 242 deletions
--- a/android/src/main/java/com/tailscale/ipn/ui/view/DNSSettingsView.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/view/DNSSettingsView.kt
@ -21,6 +21,7 @@ import androidx.compose.ui.unit.dp
 import androidx.lifecycle.viewmodel.compose.viewModel
 import com.tailscale.ipn.R
 import com.tailscale.ipn.ui.model.DnsType
+import com.tailscale.ipn.ui.notifier.Notifier
 import com.tailscale.ipn.ui.util.ClipboardValueView
 import com.tailscale.ipn.ui.util.Lists
 import com.tailscale.ipn.ui.util.LoadingIndicator
@ -44,6 +45,7 @@ fun DNSSettingsView(
      model.dnsConfig.collectAsState().value?.Routes?.mapNotNull { entry ->
        entry.value?.let { resolvers -> ViewableRoute(name = entry.key, resolvers) } ?: run { null }
      } ?: emptyList()
+  val useCorpDNS = Notifier.prefs.collectAsState().value?.CorpDNS == true

  Scaffold(topBar = { Header(R.string.dns_settings, onBack = nav.onBack) }) { innerPadding ->
    LoadingIndicator.Wrap {
@ -63,8 +65,13 @@ fun DNSSettingsView(
              supportingContent = { Text(stringResource(state.caption)) })

          Lists.ItemDivider()
-
-          SettingRow(model.useDNSSetting)
+          Setting.Switch(
+              R.string.use_ts_dns,
+              isOn = useCorpDNS,
+              onToggle = {
+                LoadingIndicator.start()
+                model.toggleCorpDNS { LoadingIndicator.stop() }
+              })
        }

        if (resolvers.isNotEmpty()) {
--- a/android/src/main/java/com/tailscale/ipn/ui/view/ExitNodePicker.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/view/ExitNodePicker.kt
@ -21,6 +21,7 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.res.stringResource
 import androidx.lifecycle.viewmodel.compose.viewModel
 import com.tailscale.ipn.R
+import com.tailscale.ipn.ui.notifier.Notifier
 import com.tailscale.ipn.ui.theme.disabledListItem
 import com.tailscale.ipn.ui.theme.listItem
 import com.tailscale.ipn.ui.util.Lists
@ -43,6 +44,7 @@ fun ExitNodePicker(
      val mullvadExitNodesByCountryCode = model.mullvadExitNodesByCountryCode.collectAsState().value
      val mullvadExitNodeCount = model.mullvadExitNodeCount.collectAsState().value
      val anyActive = model.anyActive.collectAsState()
+      val allowLANAccess = Notifier.prefs.collectAsState().value?.ExitNodeAllowLANAccess == true

      LazyColumn(modifier = Modifier.padding(innerPadding)) {
        item(key = "header") {
@ -71,7 +73,11 @@ fun ExitNodePicker(
        // TODO: make sure this actually works, and if not, leave it out for now
        item(key = "allowLANAccess") {
          Lists.SectionDivider()
-          SettingRow(model.allowLANAccessSetting)
+
+          Setting.Switch(R.string.allow_lan_access, isOn = allowLANAccess) {
+            LoadingIndicator.start()
+            model.toggleAllowLANAccess { LoadingIndicator.stop() }
+          }
        }
      }
    }
--- a/android/src/main/java/com/tailscale/ipn/ui/view/SettingsView.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/view/SettingsView.kt
@ -4,7 +4,6 @@
 package com.tailscale.ipn.ui.view

 import androidx.compose.foundation.clickable
-import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.text.ClickableText
@ -29,21 +28,15 @@ import com.tailscale.ipn.ui.Links
 import com.tailscale.ipn.ui.theme.link
 import com.tailscale.ipn.ui.theme.listItem
 import com.tailscale.ipn.ui.util.Lists
-import com.tailscale.ipn.ui.viewModel.Setting
-import com.tailscale.ipn.ui.viewModel.SettingType
 import com.tailscale.ipn.ui.viewModel.SettingsNav
 import com.tailscale.ipn.ui.viewModel.SettingsViewModel
-import com.tailscale.ipn.ui.viewModel.SettingsViewModelFactory

@Composable
-fun SettingsView(
-    settingsNav: SettingsNav,
-    viewModel: SettingsViewModel = viewModel(factory = SettingsViewModelFactory(settingsNav))
-) {
+fun SettingsView(settingsNav: SettingsNav, viewModel: SettingsViewModel = viewModel()) {
  val handler = LocalUriHandler.current
  val user = viewModel.loggedInUser.collectAsState().value
  val isAdmin = viewModel.isAdmin.collectAsState().value
-  val managedBy = viewModel.managedBy.collectAsState().value
+  val managedByOrganization = viewModel.managedByOrganization.collectAsState().value

  Scaffold(
      topBar = {
@ -53,107 +46,88 @@ fun SettingsView(
          UserView(
              profile = user,
              actionState = UserActionState.NAV,
-              onClick = viewModel.navigation.onNavigateToUserSwitcher)
+              onClick = settingsNav.onNavigateToUserSwitcher)

          if (isAdmin) {
            AdminTextView { handler.openUri(Links.ADMIN_URL) }
          }

          Lists.SectionDivider()
-          SettingRow(viewModel.dns)
+          Setting.Text(R.string.dns_settings, onClick = settingsNav.onNavigateToDNSSettings)

          Lists.ItemDivider()
-          SettingRow(viewModel.tailnetLock)
+          Setting.Text(R.string.tailnet_lock, onClick = settingsNav.onNavigateToTailnetLock)

          Lists.ItemDivider()
-          SettingRow(viewModel.permissions)
+          Setting.Text(R.string.permissions, onClick = settingsNav.onNavigateToPermissions)

-          managedBy?.let {
+          managedByOrganization?.let {
            Lists.ItemDivider()
-            SettingRow(it)
+            Setting.Text(
+                title = stringResource(R.string.managed_by_orgName, it),
+                onClick = settingsNav.onNavigateToManagedBy)
          }

          Lists.SectionDivider()
-          SettingRow(viewModel.bugReport)
+          Setting.Text(R.string.bug_report, onClick = settingsNav.onNavigateToBugReport)

          Lists.ItemDivider()
-          SettingRow(viewModel.about)
+          Setting.Text(R.string.about_tailscale, onClick = settingsNav.onNavigateToAbout)

          // TODO: put a heading for the debug section
          if (BuildConfig.DEBUG) {
            Lists.SectionDivider()
-            SettingRow(viewModel.mdmDebug)
+            Setting.Text(R.string.mdm_settings, onClick = settingsNav.onNavigateToMDMSettings)
          }
        }
      }
 }

-@Composable
-fun SettingRow(setting: Setting) {
-  Box {
-    when (setting.type) {
-      SettingType.TEXT -> TextRow(setting)
-      SettingType.SWITCH -> SwitchRow(setting)
-      SettingType.NAV -> {
-        NavRow(setting)
-      }
+object Setting {
+  @Composable
+  fun Text(
+      titleRes: Int = 0,
+      title: String? = null,
+      destructive: Boolean = false,
+      enabled: Boolean = true,
+      onClick: (() -> Unit)? = null
+  ) {
+    var modifier: Modifier = Modifier
+    if (enabled) {
+      onClick?.let { modifier = modifier.clickable(onClick = it) }
    }
+    ListItem(
+        modifier = modifier,
+        colors = MaterialTheme.colorScheme.listItem,
+        headlineContent = {
+          Text(
+              title ?: stringResource(titleRes),
+              style = MaterialTheme.typography.bodyMedium,
+              color = if (destructive) MaterialTheme.colorScheme.error else Color.Unspecified)
+        },
+    )
  }
-}

-@Composable
-private fun TextRow(setting: Setting) {
-  val enabled = setting.enabled.collectAsState().value
-  var modifier: Modifier = Modifier
-  if (enabled) {
-    setting.onClick?.let { modifier = modifier.clickable(onClick = it) }
-  }
-  ListItem(
-      modifier = modifier,
-      colors = MaterialTheme.colorScheme.listItem,
-      headlineContent = {
-        Text(
-            setting.title ?: stringResource(setting.titleRes),
-            style = MaterialTheme.typography.bodyMedium,
-            color = if (setting.destructive) MaterialTheme.colorScheme.error else Color.Unspecified)
-      },
-  )
-}
-
-@Composable
-private fun SwitchRow(setting: Setting) {
-  val enabled = setting.enabled.collectAsState().value
-  val swVal = setting.isOn?.collectAsState()?.value ?: false
-  var modifier: Modifier = Modifier
-  if (enabled) {
-    setting.onClick?.let { modifier = modifier.clickable(onClick = it) }
+  @Composable
+  fun Switch(
+      titleRes: Int = 0,
+      title: String? = null,
+      isOn: Boolean,
+      enabled: Boolean = true,
+      onToggle: (Boolean) -> Unit = {}
+  ) {
+    ListItem(
+        colors = MaterialTheme.colorScheme.listItem,
+        headlineContent = {
+          Text(
+              title ?: stringResource(titleRes),
+              style = MaterialTheme.typography.bodyMedium,
+          )
+        },
+        trailingContent = {
+          TintedSwitch(checked = isOn, onCheckedChange = onToggle, enabled = enabled)
+        })
  }
-  ListItem(
-      modifier = modifier,
-      colors = MaterialTheme.colorScheme.listItem,
-      headlineContent = {
-        Text(
-            setting.title ?: stringResource(setting.titleRes),
-            style = MaterialTheme.typography.bodyMedium,
-        )
-      },
-      trailingContent = {
-        TintedSwitch(checked = swVal, onCheckedChange = setting.onToggle, enabled = enabled)
-      })
-}
-
-@Composable
-private fun NavRow(setting: Setting) {
-  var modifier: Modifier = Modifier
-  setting.onClick?.let { modifier = modifier.clickable(onClick = it) }
-  ListItem(
-      modifier = modifier,
-      colors = MaterialTheme.colorScheme.listItem,
-      headlineContent = {
-        Text(
-            setting.title ?: stringResource(setting.titleRes),
-            style = MaterialTheme.typography.bodyMedium)
-      })
 }

@Composable
--- a/android/src/main/java/com/tailscale/ipn/ui/view/UserSwitcherView.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/view/UserSwitcherView.kt
@ -111,12 +111,29 @@ fun UserSwitcherView(

                item {
                  Lists.SectionDivider()
-                  SettingRow(viewModel.addProfileSetting)
+                  Setting.Text(R.string.add_account) {
+                    viewModel.addProfile {
+                      if (it.isFailure) {
+                        viewModel.errorDialog.set(ErrorDialogType.ADD_PROFILE_FAILED)
+                      }
+                    }
+                  }
+
                  Lists.ItemDivider()
-                  SettingRow(viewModel.loginSetting)
+                  Setting.Text(R.string.reauthenticate) { viewModel.login {} }
+
                  if (currentUser != null) {
                    Lists.ItemDivider()
-                    SettingRow(viewModel.logoutSetting)
+                    Setting.Text(
+                        R.string.log_out,
+                        destructive = true,
+                        onClick = {
+                          viewModel.logout {
+                            if (it.isFailure) {
+                              viewModel.errorDialog.set(ErrorDialogType.LOGOUT_FAILED)
+                            }
+                          }
+                        })
                  }
                }
              }
--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/DNSSettingsViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/DNSSettingsViewModel.kt
@ -36,13 +36,6 @@ class DNSSettingsViewModel() : IpnViewModel() {
      MutableStateFlow(DNSEnablementState.NOT_RUNNING)
  val dnsConfig: StateFlow<Tailcfg.DNSConfig?> = MutableStateFlow(null)

-  val useDNSSetting =
-      Setting(
-          R.string.use_ts_dns,
-          type = SettingType.SWITCH,
-          isOn = MutableStateFlow(Notifier.prefs.value?.CorpDNS),
-          onToggle = { toggleCorpDNS {} })
-
  init {
    viewModelScope.launch {
      Notifier.netmap
@ -51,19 +44,12 @@ class DNSSettingsViewModel() : IpnViewModel() {
          .collect { (netmap, prefs) ->
            Log.d("DNSSettingsViewModel", "prefs: CorpDNS=" + prefs?.CorpDNS.toString())
            prefs?.let {
-              useDNSSetting.isOn?.set(it.CorpDNS)
-              useDNSSetting.enabled.set(true)
-
              if (it.CorpDNS) {
                enablementState.set(DNSEnablementState.ENABLED)
              } else {
                enablementState.set(DNSEnablementState.DISABLED)
              }
-            }
-                ?: run {
-                  enablementState.set(DNSEnablementState.NOT_RUNNING)
-                  useDNSSetting.enabled.set(false)
-                }
+            } ?: run { enablementState.set(DNSEnablementState.NOT_RUNNING) }
            netmap?.let { dnsConfig.set(netmap.DNS) }
          }
    }
--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/ExitNodePickerViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/ExitNodePickerViewModel.kt
@ -6,7 +6,6 @@ package com.tailscale.ipn.ui.viewModel
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.ViewModelProvider
 import androidx.lifecycle.viewModelScope
-import com.tailscale.ipn.R
 import com.tailscale.ipn.ui.localapi.Client
 import com.tailscale.ipn.ui.model.Ipn
 import com.tailscale.ipn.ui.model.StableNodeID
@ -57,17 +56,6 @@ class ExitNodePickerViewModel(private val nav: ExitNodePickerNav) : IpnViewModel
  val anyActive: StateFlow<Boolean> = MutableStateFlow(false)
  val isRunningExitNode: StateFlow<Boolean> = MutableStateFlow(false)

-  val allowLANAccessSetting =
-      Setting(
-          R.string.allow_lan_access,
-          type = SettingType.SWITCH,
-          isOn = MutableStateFlow(Notifier.prefs.value?.ExitNodeAllowLANAccess),
-          enabled = MutableStateFlow(true),
-          onToggle = {
-            LoadingIndicator.start()
-            toggleAllowLANAccess { LoadingIndicator.stop() }
-          })
-
  init {
    viewModelScope.launch {
      Notifier.netmap
@ -155,7 +143,7 @@ class ExitNodePickerViewModel(private val nav: ExitNodePickerNav) : IpnViewModel
    }
  }

-  private fun toggleAllowLANAccess(callback: (Result<Ipn.Prefs>) -> Unit) {
+  fun toggleAllowLANAccess(callback: (Result<Ipn.Prefs>) -> Unit) {
    val prefs =
        Notifier.prefs.value
            ?: run {
--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/SettingsViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/SettingsViewModel.kt
@ -3,10 +3,7 @@

 package com.tailscale.ipn.ui.viewModel

-import androidx.lifecycle.ViewModel
-import androidx.lifecycle.ViewModelProvider
 import androidx.lifecycle.viewModelScope
-import com.tailscale.ipn.R
 import com.tailscale.ipn.mdm.MDMSettings
 import com.tailscale.ipn.ui.notifier.Notifier
 import com.tailscale.ipn.ui.util.set
@ -14,36 +11,6 @@ import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.launch

-enum class SettingType {
-  NAV,
-  SWITCH,
-  TEXT
-}
-
-// Represents a UI setting.
-// title: The title of the setting
-// type: The type of setting
-// enabled: Whether the setting is enabled
-// value: The value of the setting for textual settings
-// isOn: The value of the setting for switch settings
-// onClick: The action to take when the setting is clicked (typically for navigation)
-// onToggle: The action to take when the setting is toggled (typically for switches)
-// icon: An optional Composable that draws a trailing icon to display with nav settings
-//
-// Behavior is undefined if you mix the types here. Switch settings should supply an
-// isOn and onToggle, while navigation settings should supply an onClick and an optional
-// value
-data class Setting(
-    val titleRes: Int = 0,
-    val title: String? = null,
-    val type: SettingType,
-    val destructive: Boolean = false,
-    val enabled: StateFlow<Boolean> = MutableStateFlow(true),
-    val isOn: StateFlow<Boolean?>? = null,
-    val onClick: (() -> Unit)? = null,
-    val onToggle: (Boolean) -> Unit = {}
-)
-
 data class SettingsNav(
    val onNavigateToBugReport: () -> Unit,
    val onNavigateToAbout: () -> Unit,
@ -56,75 +23,12 @@ data class SettingsNav(
    val onBackPressed: () -> Unit,
 )

-class SettingsViewModelFactory(private val navigation: SettingsNav) : ViewModelProvider.Factory {
-  override fun <T : ViewModel> create(modelClass: Class<T>): T {
-    return SettingsViewModel(navigation) as T
-  }
-}
-
-class SettingsViewModel(val navigation: SettingsNav) : IpnViewModel() {
+class SettingsViewModel() : IpnViewModel() {
  // Display name for the logged in user
-  var isAdmin: StateFlow<Boolean> = MutableStateFlow(false)
-
-  val dns =
-      Setting(
-          titleRes = R.string.dns_settings,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToDNSSettings() },
-          enabled = MutableStateFlow(true))
-
-  val tailnetLock =
-      Setting(
-          titleRes = R.string.tailnet_lock,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToTailnetLock() },
-          enabled = MutableStateFlow(true))
-
-  val permissions =
-      Setting(
-          titleRes = R.string.permissions,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToPermissions() },
-          enabled = MutableStateFlow(true))
-
-  val about =
-      Setting(
-          titleRes = R.string.about_tailscale,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToAbout() },
-          enabled = MutableStateFlow(true))
-
-  val bugReport =
-      Setting(
-          titleRes = R.string.bug_report,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToBugReport() },
-          enabled = MutableStateFlow(true))
-
-  val managedBy: StateFlow<Setting?> = MutableStateFlow(null)
-
-  val mdmDebug =
-      Setting(
-          titleRes = R.string.mdm_settings,
-          type = SettingType.NAV,
-          onClick = { navigation.onNavigateToMDMSettings() },
-          enabled = MutableStateFlow(true))
+  val isAdmin: StateFlow<Boolean> = MutableStateFlow(false)
+  val managedByOrganization = MDMSettings.managedByOrganizationName.flow

  init {
-    viewModelScope.launch {
-      MDMSettings.managedByOrganizationName.flow.collect { managedByOrganization ->
-        managedBy.set(
-            managedByOrganization?.let {
-              Setting(
-                  R.string.managed_by_orgName,
-                  it,
-                  SettingType.NAV,
-                  onClick = { navigation.onNavigateToManagedBy() },
-                  enabled = MutableStateFlow(true))
-            })
-      }
-    }
-
    viewModelScope.launch {
      Notifier.netmap.collect { netmap -> isAdmin.set(netmap?.SelfNode?.isAdmin ?: false) }
    }
--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/UserSwitcherViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/UserSwitcherViewModel.kt
@ -4,7 +4,6 @@
 package com.tailscale.ipn.ui.viewModel

 import androidx.lifecycle.viewModelScope
-import com.tailscale.ipn.R
 import com.tailscale.ipn.ui.localapi.Client
 import com.tailscale.ipn.ui.model.Ipn
 import com.tailscale.ipn.ui.notifier.Notifier
@ -21,34 +20,6 @@ class UserSwitcherViewModel : IpnViewModel() {
  // True if we should render the kebab menu
  val showHeaderMenu: StateFlow<Boolean> = MutableStateFlow(false)

-  val loginSetting =
-      Setting(titleRes = R.string.reauthenticate, type = SettingType.NAV, onClick = { login {} })
-
-  val logoutSetting =
-      Setting(
-          titleRes = R.string.log_out,
-          destructive = true,
-          type = SettingType.TEXT,
-          onClick = {
-            logout {
-              if (it.isFailure) {
-                errorDialog.set(ErrorDialogType.LOGOUT_FAILED)
-              }
-            }
-          })
-
-  val addProfileSetting =
-      Setting(
-          titleRes = R.string.add_account,
-          type = SettingType.NAV,
-          onClick = {
-            addProfile {
-              if (it.isFailure) {
-                errorDialog.set(ErrorDialogType.ADD_PROFILE_FAILED)
-              }
-            }
-          })
-
  // Sets the custom control URL and immediatly invokes the login flow
  fun setControlURL(urlStr: String) {
    // Some basic checks that the entered URL is "reasonable".  The underlying