Fix the long-press sheet's dropped actions and mismatched rows

manager/src/main/kotlin/org/matrix/vector/manager/ui/components/PackageActionMenu.kt

@@ -16,32 +16,35 @@ import androidx.compose.foundation.layout.height
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.size
 import androidx.compose.foundation.layout.width
+import androidx.compose.foundation.selection.toggleable
 import androidx.compose.foundation.shape.CircleShape
 import androidx.compose.material.icons.Icons
 import androidx.compose.material.icons.automirrored.rounded.Launch
 import androidx.compose.material.icons.rounded.Bolt
-import androidx.compose.material.icons.rounded.DeleteOutline
+import androidx.compose.material.icons.rounded.Delete
 import androidx.compose.material.icons.rounded.Info
 import androidx.compose.material.icons.rounded.Stop
 import androidx.compose.material3.ExperimentalMaterial3Api
 import androidx.compose.material3.HorizontalDivider
 import androidx.compose.material3.Icon
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.ModalBottomSheet
+import androidx.compose.material3.Switch
 import androidx.compose.material3.Text
 import androidx.compose.material3.rememberModalBottomSheetState
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.remember
-import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.clip
 import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.graphics.vector.ImageVector
 import androidx.compose.ui.res.stringResource
+import androidx.compose.ui.semantics.Role
 import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.unit.dp
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import org.matrix.vector.manager.Constants
 import org.matrix.vector.manager.ui.theme.LocalizedOverlay
@@ -134,7 +137,14 @@ fun PackageActionSheet(
  }
  var confirmSoftReboot by remember { mutableStateOf(false) }
 
- val scope = rememberCoroutineScope()
+ // Deliberately not `rememberCoroutineScope()`. Every action on this sheet dismisses it before
+ // it starts working, and the dismissal takes this composable out of the composition — which
+ // cancels the scope a composition remembered for it. The work launched into that scope then
+ // dies at the first `withContext` hop inside the daemon call, before the transaction is ever
+ // made, and dies quietly: no daemon call, no error branch, no snackbar, a button that did
+ // nothing. Worse, it is a race against the next frame rather than a reliable failure, so it
+ // reads as a flaky button. `appScope` belongs to the process and outlives the sheet.
+ val scope = ServiceLocator.appScope
  val daemon = ServiceLocator.daemon
 
  if (confirmSoftReboot) {
@@ -148,7 +158,7 @@ fun PackageActionSheet(
  onClick = {
  confirmSoftReboot = false
  onDismiss()
- scope.launch {
+ scope.launch(Dispatchers.Main) {
  daemon.softReboot().onFailure {
  Log.e(Constants.TAG, "actions: soft reboot request failed", it)
  }
@@ -177,9 +187,11 @@ fun PackageActionSheet(
  // still open at their own height and nothing gains a useless drag.
  val sheetState = rememberModalBottomSheetState()
 
+ // `Dispatchers.Main` because [onResult] reaches a snackbar on the screen underneath, and
+ // because that is the thread the composition scope this replaces used to resume on.
  fun finish(block: suspend () -> PackageActionResult) {
  onDismiss()
- scope.launch { onResult(block()) }
+ scope.launch(Dispatchers.Main) { onResult(block()) }
  }
 
  ModalBottomSheet(onDismissRequest = onDismiss, sheetState = sheetState) {
@@ -361,7 +373,7 @@ LocalizedOverlay {
  if (isModule) {
  HorizontalDivider(Modifier.padding(horizontal = 24.dp, vertical = 4.dp))
  ActionRow(
- icon = Icons.Rounded.DeleteOutline,
+ icon = Icons.Rounded.Delete,
  title = stringResource(R.string.action_uninstall),
  tint = colors.error,
  ) {
@@ -392,26 +404,39 @@ LocalizedOverlay {
 }
 
 /**
- * One action, with its icon in a tinted disc.
+ * The one shape every row on this sheet takes: a glyph in a tinted disc, the verb, and — when it
+ * needs one — the sentence under it saying what the verb costs.
  *
  * The disc is what lets a destructive action look destructive: an error-red glyph on a bare row is
- * easy to miss, the same glyph on a red disc is not.
+ * easy to miss, the same glyph on a red disc is not. Once one row carries it they all have to, or
+ * the bare one reads as a different kind of thing sitting in the same list — which is what the mute
+ * switch did while it was borrowing the generic [ToggleRow], a Material list item whose leading
+ * icon has no disc and whose text starts ten pixels to the left of every other row here.
+ *
+ * The measurements are chosen so that one column runs down the whole sheet: 24dp of margin, a 40dp
+ * disc and 20dp of gap put every title at 84dp, which is where the header puts the app's name over
+ * its 44dp icon and 16dp gap.
+ *
+ * [trailing] is for a row that carries state as well as an action, and the click behaviour comes in
+ * through [modifier] rather than as a callback: a switch row has to announce itself to a screen
+ * reader as a switch, not as a button, and only the caller knows which it is.
  */
 @Composable
-private fun ActionRow(
+private fun ActionRowLayout(
+ modifier: Modifier,
  icon: ImageVector,
  title: String,
- subtitle: String? = null,
- tint: Color? = null,
-onClick: () -> Unit,
+ subtitle: String?,
+ tint: Color?,
+trailing: (@Composable () -> Unit)? = null,
 ) {
  val colors = MaterialTheme.colorScheme
  val accent = tint ?: colors.onSurfaceVariant
 
  Row(
  modifier =
  Modifier.fillMaxWidth()
- .clickable(onClick = onClick)
+ .then(modifier)
  .padding(horizontal = 24.dp, vertical = 12.dp),
  verticalAlignment = Alignment.CenterVertically,
  ) {
@@ -422,7 +447,7 @@ private fun ActionRow(
  ) {
  Icon(icon, contentDescription = null, tint = accent, modifier = Modifier.size(22.dp))
  }
- Spacer(Modifier.width(18.dp))
+ Spacer(Modifier.width(20.dp))
  Column(Modifier.weight(1f)) {
  Text(
  text = title,
@@ -437,9 +462,63 @@ private fun ActionRow(
  )
  }
  }
+ if (trailing != null) {
+ Spacer(Modifier.width(12.dp))
+ trailing()
+ }
  }
 }
 
+/**
+ * One action.
+ *
+ * [onClick] is nullable because one row on this sheet is a statement rather than an action — "not
+ * in the store" — and a row that ripples under a thumb and then does nothing is a worse answer than
+ * one that visibly cannot be pressed.
+ */
+@Composable
+private fun ActionRow(
+ icon: ImageVector,
+ title: String,
+ subtitle: String? = null,
+ tint: Color? = null,
+ onClick: (() -> Unit)?,
+) {
+ ActionRowLayout(
+ modifier = if (onClick != null) Modifier.clickable(onClick = onClick) else Modifier,
+ icon = icon,
+ title = title,
+ subtitle = subtitle,
+ tint = tint,
+ )
+}
+
+/** One setting, in the same shape as the actions it sits among. */
+@Composable
+private fun ActionToggleRow(
+ icon: ImageVector,
+ title: String,
+ checked: Boolean,
+ onCheckedChange: (Boolean) -> Unit,
+ subtitle: String? = null,
+) {
+ ActionRowLayout(
+ modifier =
+ Modifier.toggleable(
+ value = checked,
+ role = Role.Switch,
+ onValueChange = onCheckedChange,
+ ),
+ icon = icon,
+ title = title,
+ subtitle = subtitle,
+ tint = null,
+ // The whole row is the target, and the switch itself takes no callback, so a tap on it
+ // cannot be counted twice.
+ trailing = { Switch(checked = checked, onCheckedChange = null) },
+ )
+}
+
 /**
  * What this module's update situation is, and the two things to do about it.
  *
@@ -484,7 +563,7 @@ private fun ModuleUpdateSection(
  icon = Icons.Rounded.CloudOff,
  title = stringResource(R.string.action_not_in_store),
  subtitle = stringResource(R.string.action_not_in_store_summary),
- onClick = {},
+ onClick = null,
  )
  HorizontalDivider(Modifier.padding(horizontal = 24.dp))
  Spacer(Modifier.height(4.dp))
@@ -538,7 +617,7 @@ private fun ModuleUpdateSection(
  )
  }
 
-ToggleRow(
+ActionToggleRow(
  title = stringResource(R.string.store_mute_updates),
  icon = Icons.Rounded.NotificationsOff,
  checked = packageName in muted,

manager/src/main/kotlin/org/matrix/vector/manager/ui/components/SheetParts.kt

@@ -14,6 +14,8 @@ import androidx.compose.foundation.selection.toggleable
 import androidx.compose.material3.Icon
 import androidx.compose.material3.LocalContentColor
 import androidx.compose.material3.ListItem
+import androidx.compose.material3.ListItemColors
+import androidx.compose.material3.ListItemDefaults
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Switch
 import androidx.compose.material3.Text
@@ -35,6 +37,19 @@ import androidx.compose.ui.unit.dp
  * wraps at another width. A new sheet inherits the pattern, and changing the pattern changes every
  * sheet at once.
  */
+
+/**
+ * What a [ListItem] needs to be given to sit on a sheet.
+ *
+ * A list item's container defaults to `surface`; a `ModalBottomSheet` is drawn on
+ * `surfaceContainerLow`, which is a shade darker. On a screen those two agree, so the default looks
+ * right in the place a row is usually written and wrong the moment it is put in a sheet — a pale
+ * full-width band across the sheet, ending wherever the row ends. Transparent takes whatever it is
+ * placed on, so it is right in both, and it is what every list item in a sheet should be given.
+ */
+val sheetRowColors: ListItemColors
+ @Composable get() = ListItemDefaults.colors(containerColor = Color.Transparent)
+
 @Composable
 fun SheetHeading(text: String, icon: ImageVector) {
  Row(
@@ -105,6 +120,7 @@ fun ToggleRow(
  supportingContent = subtitle?.let { { Text(it) } },
  leadingContent = { Icon(icon, contentDescription = null) },
  trailingContent = { Switch(checked = checked, onCheckedChange = null) },
+ colors = sheetRowColors,
  )
 }
 
@@ -167,5 +183,6 @@ fun SheetAction(
  leadingContent = {
  Icon(icon, contentDescription = null, tint = tint ?: LocalContentColor.current)
  },
+ colors = sheetRowColors,
  )
 }

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/logs/LogsScreen.kt

@@ -110,6 +110,7 @@ import org.matrix.vector.manager.R
 import org.matrix.vector.manager.ui.theme.VectorLogLine
 import org.matrix.vector.manager.data.log.LogLevel
 import org.matrix.vector.manager.ui.components.PanelHeader
+import org.matrix.vector.manager.ui.components.sheetRowColors
 import org.matrix.vector.manager.ui.components.SearchField
 import org.matrix.vector.manager.ui.theme.VectorMono
 
@@ -721,6 +722,7 @@ LocalizedOverlay {
  trailingContent = {
  Switch(checked = enabled, onCheckedChange = { viewModel.setVerbose(it) })
  },
+ colors = sheetRowColors,
  )
 
  HorizontalDivider(Modifier.padding(vertical = 4.dp))
@@ -730,6 +732,7 @@ LocalizedOverlay {
  headlineContent = { Text(stringResource(R.string.logs_save)) },
  supportingContent = { Text(stringResource(R.string.logs_save_summary)) },
  leadingContent = { Icon(Icons.Rounded.Save, contentDescription = null) },
+ colors = sheetRowColors,
  )
  ListItem(
  modifier = Modifier.clickable(onClick = onRotate),
@@ -742,6 +745,7 @@ LocalizedOverlay {
  tint = MaterialTheme.colorScheme.error,
  )
  },
+ colors = sheetRowColors,
  )
  }
  }

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ModulesScreen.kt

@@ -29,7 +29,7 @@ import androidx.compose.material.icons.rounded.SettingsBackupRestore
 import androidx.compose.material.icons.rounded.Block
 import androidx.compose.material.icons.rounded.Check
 import androidx.compose.material.icons.rounded.CheckCircle
-import androidx.compose.material.icons.rounded.DeleteOutline
+import androidx.compose.material.icons.rounded.Delete
 import androidx.compose.material.icons.rounded.Close
 import androidx.compose.material.icons.rounded.FilterList
 import androidx.compose.material.icons.rounded.Android
@@ -101,6 +101,7 @@ import org.matrix.vector.manager.data.model.ReleaseAsset
 import org.matrix.vector.manager.data.model.StoreEntry
 import org.matrix.vector.manager.data.repository.ModuleUpdateQueue
 import org.matrix.vector.manager.ui.components.SheetHeading
+import org.matrix.vector.manager.ui.components.sheetRowColors
 import org.matrix.vector.manager.ui.screens.repo.StoreChannel
 import org.matrix.vector.manager.ui.screens.repo.releasesOn
 import org.lsposed.lspd.ILSPManagerService
@@ -428,7 +429,7 @@ fun ModulesScreen(
  if (confirmUninstall) {
  VectorAlertDialog(
  onDismissRequest = { confirmUninstall = false },
- icon = { Icon(Icons.Rounded.DeleteOutline, contentDescription = null) },
+ icon = { Icon(Icons.Rounded.Delete, contentDescription = null) },
  title = { Text(stringResource(R.string.modules_uninstall_title)) },
  // Names the consequence rather than asking "are you sure". The backup on this screen
  // holds the enabled flag and the scope; the module's own stored settings go with it
@@ -526,7 +527,7 @@ private fun SelectionBar(
  SelectionAction(Icons.Rounded.Block, R.string.modules_batch_disable, onDisable)
  SelectionAction(Icons.Rounded.SaveAlt, R.string.modules_backup, onBackup)
  SelectionAction(
- Icons.Rounded.DeleteOutline,
+ Icons.Rounded.Delete,
  R.string.action_uninstall,
  onUninstall,
  tint = MaterialTheme.colorScheme.error,
@@ -1402,6 +1403,7 @@ private fun ModuleUpdatesSheet(
  )
  }
  },
+ colors = sheetRowColors,
  )
  }
  }

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsScreen.kt

@@ -4,6 +4,7 @@ import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import org.matrix.vector.manager.ui.components.ConfirmInstall
 import org.matrix.vector.manager.ui.components.ToggleRow
 import org.matrix.vector.manager.ui.components.SheetHeading
+import org.matrix.vector.manager.ui.components.sheetRowColors
 import androidx.compose.material3.rememberModalBottomSheetState
 import androidx.compose.material.icons.rounded.Tune
 import androidx.compose.material.icons.rounded.NotificationsOff
@@ -881,6 +882,7 @@ LocalizedOverlay {
  }
  Text(listOfNotNull(size, downloads).joinToString(" · "))
  },
+ colors = sheetRowColors,
  )
  }
  Spacer(Modifier.navigationBarsPadding().height(16.dp))

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateScreen.kt

@@ -6,6 +6,7 @@ import androidx.compose.foundation.clickable
 import org.matrix.vector.manager.ui.theme.currentLocale
 import org.matrix.vector.manager.ui.theme.LocalizedOverlay
 import org.matrix.vector.manager.ui.components.SheetHeading
+import org.matrix.vector.manager.ui.components.sheetRowColors
 import org.matrix.vector.manager.data.repository.ReleaseDirection
 import org.matrix.vector.manager.data.github.FrameworkRelease
 import java.util.Date
@@ -645,6 +646,7 @@ private fun VersionsSheet(
  )
  }
  },
+ colors = sheetRowColors,
  )
  }
  }