diff --git a/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheet.kt b/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheet.kt new file mode 100644 index 00000000000..f1cb5f1bcd7 --- /dev/null +++ b/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheet.kt @@ -0,0 +1,332 @@ +package io.homeassistant.companion.android.settings.sensor.views + +import androidx.annotation.VisibleForTesting +import androidx.compose.foundation.clickable +import androidx.compose.foundation.gestures.detectVerticalDragGestures +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.heightIn +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.items +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Search +import androidx.compose.material3.Checkbox +import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.Immutable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateListOf +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.input.nestedscroll.NestedScrollConnection +import androidx.compose.ui.input.nestedscroll.NestedScrollSource +import androidx.compose.ui.input.nestedscroll.nestedScroll +import androidx.compose.ui.input.pointer.pointerInput +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.Velocity +import io.homeassistant.companion.android.common.R as commonR +import io.homeassistant.companion.android.common.compose.composable.HAFilledButton +import io.homeassistant.companion.android.common.compose.composable.HAModalBottomSheet +import io.homeassistant.companion.android.common.compose.composable.HAPlainButton +import io.homeassistant.companion.android.common.compose.composable.HASearchField +import io.homeassistant.companion.android.common.compose.composable.rememberHAModalBottomSheetState +import io.homeassistant.companion.android.common.compose.theme.HADimens +import io.homeassistant.companion.android.common.compose.theme.HATextStyle +import io.homeassistant.companion.android.common.compose.theme.LocalHAColorScheme +import io.homeassistant.companion.android.settings.sensor.SensorDetailViewModel +import io.homeassistant.companion.android.util.compose.safeScreenHeight +import kotlin.coroutines.CoroutineContext +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext + +/** Threshold above which the search field becomes visible to help users navigate long lists. */ +private const val SEARCH_VISIBILITY_THRESHOLD = 10 + +/** + * Bottom sheet for multi-select allow list sensor settings (apps, bluetooth, zones, beacons). + * + * Renders a search field (when the entry list exceeds [SEARCH_VISIBILITY_THRESHOLD]), a scrollable + * list of selectable rows, and a fixed footer with cancel and save actions. While the selection + * state is loading, a centered progress indicator is shown instead of the list. + * + * Filtering is performed off the UI thread on [dispatcher] to keep the sheet responsive on long + * lists. The search field debounces the query so the list does not re-filter on every keystroke. + * + * @param title Heading displayed at the top of the sheet. + * @param state Current dialog state holding the entries, selection and loading flag. + * @param onDismiss Invoked when the sheet is dismissed without saving. + * @param onSave Invoked with the updated state when the user confirms the selection. + * @param modifier Optional [Modifier] applied to the sheet container. + * @param dispatcher Coroutine context used to compute the filtered entries; injectable for tests. + */ +@OptIn(ExperimentalMaterial3Api::class) +@Composable +internal fun SensorDetailSettingSheet( + title: String, + state: SensorDetailViewModel.Companion.SettingDialogState, + onDismiss: () -> Unit, + onSave: (SensorDetailViewModel.Companion.SettingDialogState) -> Unit, + modifier: Modifier = Modifier, + dispatcher: CoroutineContext = Dispatchers.Default, +) { + val checkedValue = remember { + mutableStateListOf().also { it.addAll(state.entriesSelected) } + } + var searchQuery by remember { mutableStateOf("") } + // Wrap the entries once at the composable boundary so the Compose Compiler can infer + // stability for the `rememberFilteredSettingEntries` parameter (a raw + // `List>` is treated as unstable). + val settingEntries = remember(state.entries) { SettingEntries(state.entries) } + val filteredEntries = rememberFilteredSettingEntries( + entries = settingEntries, + searchQuery = searchQuery, + dispatcher = dispatcher, + ) + val showSearch = state.entries.size > SEARCH_VISIBILITY_THRESHOLD + + val bottomSheetState = rememberHAModalBottomSheetState(skipPartiallyExpanded = true) + val screenHeight = safeScreenHeight() - HADimens.SPACE16 + + val nestedScrollFlingGuard = remember { nestedScrollFlingGuard() } + + HAModalBottomSheet( + bottomSheetState = bottomSheetState, + modifier = modifier, + onDismissRequest = onDismiss, + ) { + Column( + modifier = Modifier + .height(screenHeight) + .nestedScroll(nestedScrollFlingGuard) + .pointerInput(Unit) { + // Consume vertical drag gestures to prevent BottomSheet from interpreting them + // as collapse gestures while the user scrolls the entry list. + detectVerticalDragGestures { _, _ -> } + }, + ) { + Text( + text = title, + style = HATextStyle.HeadlineMedium.copy(textAlign = TextAlign.Start), + modifier = Modifier.padding( + horizontal = HADimens.SPACE4, + vertical = HADimens.SPACE3, + ), + ) + if (showSearch) { + HASearchField( + query = searchQuery, + onQueryChange = { searchQuery = it }, + leadingIcon = { + Icon( + imageVector = Icons.Default.Search, + contentDescription = null, + tint = LocalHAColorScheme.current.colorOnNeutralNormal, + ) + }, + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = HADimens.SPACE4), + ) + } + Box( + modifier = Modifier + .fillMaxWidth() + .weight(1f), + contentAlignment = Alignment.Center, + ) { + if (state.loading) { + CircularProgressIndicator() + } else { + LazyColumn(modifier = Modifier.fillMaxWidth()) { + items(filteredEntries, key = { (id) -> id }) { (id, entry) -> + BottomSheetSettingRow( + label = entry, + checked = checkedValue.contains(id), + onClick = { isChecked -> + if (isChecked) { + if (id !in checkedValue) checkedValue.add(id) + } else { + checkedValue.remove(id) + } + }, + ) + } + } + } + } + Row( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = HADimens.SPACE4, vertical = HADimens.SPACE3), + horizontalArrangement = Arrangement.End, + ) { + HAPlainButton( + text = stringResource(commonR.string.cancel), + onClick = onDismiss, + ) + Spacer(modifier = Modifier.width(HADimens.SPACE2)) + HAFilledButton( + text = stringResource(commonR.string.save), + enabled = !state.loading, + onClick = { + val joinedValue = joinSelectedValues(checkedValue) + onSave(state.copy(setting = state.setting.copy(value = joinedValue))) + }, + ) + } + } + } +} + +/** + * Immutable wrapper around the raw `List>` setting entries so that + * Composable parameters can be marked stable by the Compose Compiler. The Compose Compiler + * cannot infer stability for `List>` directly, which forces unnecessary + * recompositions. + */ +@Immutable +internal data class SettingEntries(val items: List>) + +/** + * Computes the filtered entries off the UI thread. + * + * Re-runs whenever [entries] or [searchQuery] change, dispatching the filter work onto + * [dispatcher] so long lists do not freeze the sheet. + */ +@Composable +private fun rememberFilteredSettingEntries( + entries: SettingEntries, + searchQuery: String, + dispatcher: CoroutineContext = Dispatchers.Default, +): List> { + var filtered by remember(entries) { mutableStateOf(entries.items) } + + LaunchedEffect(entries, searchQuery) { + filtered = withContext(dispatcher) { + filterSettingEntries(entries.items, searchQuery) + } + } + + return filtered +} + +/** + * Filters setting entries by matching the query against entry labels (case-insensitive). + * Returns all entries when the query is blank. + */ +@VisibleForTesting +internal fun filterSettingEntries(entries: List>, query: String): List> { + val trimmed = query.trim() + return if (trimmed.isBlank()) { + entries + } else { + entries.filter { (_, label) -> label.contains(trimmed, ignoreCase = true) } + } +} + +/** + * Joins selected entry IDs into the comma-separated string format expected by [SensorDetailViewModel]. + * + * Mirrors the legacy formatting used elsewhere in the sensor settings code, removing list bracket + * artifacts that originate from older Java-style toString conversions. + */ +internal fun joinSelectedValues(values: List): String { + return values.joinToString().replace("[", "").replace("]", "") +} + +/** + * Splits a sensor setting label into a primary line and an optional secondary line. + * + * Sensor allow-list entries are formatted as `"\n()"` where the secondary + * line is wrapped in parentheses (for example `"Chrome\n(com.google.chrome)"`). Only the first + * newline is used to separate the two parts; any further newlines remain inside the secondary + * value. Surrounding parentheses are stripped from the secondary value when, and only when, both + * the leading `(` and trailing `)` are present, matching [String.removeSurrounding] semantics. + * + * Shared between [SensorDetailSettingSheet] (the new bottom sheet) and the legacy Material 2 + * dialog row in `SensorDetailView` so both render identical primary/secondary text. + * + * @return a pair of `(primary, secondary)` where `secondary` is `null` for single-line labels. + */ +internal fun parseSettingLabel(label: String): Pair { + val parts = label.split("\n", limit = 2) + val primaryText = parts[0] + val secondaryText = parts.getOrNull(1)?.removeSurrounding("(", ")") + return primaryText to secondaryText +} + +/** + * Returns a [NestedScrollConnection] that absorbs fling velocity and post-scroll offsets to prevent + * the bottom sheet from collapsing while the user scrolls or flings the entry list. + */ +private fun nestedScrollFlingGuard(): NestedScrollConnection { + return object : NestedScrollConnection { + override fun onPostScroll(consumed: Offset, available: Offset, source: NestedScrollSource): Offset = available + + override suspend fun onPostFling(consumed: Velocity, available: Velocity): Velocity = available + } +} + +/** + * Single selectable row used inside [SensorDetailSettingSheet]. Built with Material 3 components and + * the project's design tokens; intentionally separate from [SensorDetailSettingRow] used by the + * legacy Material 2 dialog. + */ +@Composable +private fun BottomSheetSettingRow( + label: String, + checked: Boolean, + onClick: (Boolean) -> Unit, + modifier: Modifier = Modifier, +) { + val (primaryText, secondaryText) = parseSettingLabel(label) + val colorScheme = LocalHAColorScheme.current + + Row( + modifier = modifier + .fillMaxWidth() + .clickable { onClick(!checked) } + .padding(horizontal = HADimens.SPACE3) + .heightIn(min = HADimens.SPACE16), + verticalAlignment = Alignment.CenterVertically, + ) { + Checkbox( + checked = checked, + onCheckedChange = null, + modifier = Modifier.size(width = HADimens.SPACE12, height = HADimens.SPACE12), + ) + Column(modifier = Modifier.weight(1f)) { + Text( + text = primaryText, + style = HATextStyle.Body.copy( + textAlign = TextAlign.Start, + color = colorScheme.colorTextPrimary, + ), + ) + if (secondaryText != null) { + Spacer(Modifier.height(HADimens.SPACE1)) + Text( + text = secondaryText, + style = HATextStyle.BodyMedium.copy(textAlign = TextAlign.Start), + ) + } + } + } +} diff --git a/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailView.kt b/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailView.kt index 22a6aabcae4..946755fe87a 100644 --- a/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailView.kt +++ b/app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailView.kt @@ -14,6 +14,7 @@ import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxHeight import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size @@ -73,6 +74,8 @@ import com.mikepenz.iconics.compose.Image import com.mikepenz.iconics.typeface.library.community.material.CommunityMaterial import io.homeassistant.companion.android.common.R as commonR import io.homeassistant.companion.android.common.compose.composable.HAHint +import io.homeassistant.companion.android.common.compose.theme.HADimens +import io.homeassistant.companion.android.common.compose.theme.HATextStyle import io.homeassistant.companion.android.common.sensors.SensorManager import io.homeassistant.companion.android.common.util.kotlinJsonMapper import io.homeassistant.companion.android.database.sensor.SensorSetting @@ -160,13 +163,28 @@ fun SensorDetailView( onDismiss = { sensorUpdateTypeInfo = false }, ) } else { - viewModel.sensorSettingsDialog?.let { - SensorDetailSettingDialog( - viewModel = viewModel, - state = it, - onDismiss = { viewModel.cancelSettingWithDialog() }, - onSubmit = { state -> onDialogSettingSubmitted(state) }, + viewModel.sensorSettingsDialog?.let { dialogState -> + val isMultiSelectList = dialogState.setting.valueType in listOf( + SensorSettingType.LIST_APPS, + SensorSettingType.LIST_BLUETOOTH, + SensorSettingType.LIST_ZONES, + SensorSettingType.LIST_BEACONS, ) + if (isMultiSelectList) { + SensorDetailSettingSheet( + title = viewModel.getSettingTranslatedTitle(dialogState.setting.name), + state = dialogState, + onDismiss = { viewModel.cancelSettingWithDialog() }, + onSave = { updatedState -> onDialogSettingSubmitted(updatedState) }, + ) + } else { + SensorDetailSettingDialog( + viewModel = viewModel, + state = dialogState, + onDismiss = { viewModel.cancelSettingWithDialog() }, + onSubmit = { state -> onDialogSettingSubmitted(state) }, + ) + } } } LazyColumn( @@ -623,7 +641,7 @@ fun SensorDetailSettingDialog( onClick = { isChecked -> if (state.setting.valueType == SensorSettingType.LIST) { inputValue.value = id - onSubmit(state.copy().apply { setting.value = inputValue.value }) + onSubmit(state.copy(setting = state.setting.copy(value = inputValue.value))) } else { if (checkedValue.contains(id) && !isChecked) { checkedValue.remove(id) @@ -659,9 +677,9 @@ fun SensorDetailSettingDialog( } else if (state.setting.valueType != SensorSettingType.LIST) { { if (listSettingDialog) { - inputValue.value = checkedValue.joinToString().replace("[", "").replace("]", "") + inputValue.value = joinSelectedValues(checkedValue) } - onSubmit(state.copy().apply { setting.value = inputValue.value }) + onSubmit(state.copy(setting = state.setting.copy(value = inputValue.value))) } } else { // list is saved when selecting a value null @@ -742,10 +760,13 @@ fun SensorDetailSettingRow( onClick: (Boolean) -> Unit, modifier: Modifier = Modifier, ) { + val (primaryText, secondaryText) = parseSettingLabel(label) + Row( modifier = modifier .clickable { onClick(!checked) } .padding(horizontal = 12.dp) + .heightIn(min = 64.dp) .fillMaxWidth(), verticalAlignment = Alignment.CenterVertically, ) { @@ -762,6 +783,15 @@ fun SensorDetailSettingRow( modifier = Modifier.size(width = 48.dp, height = 48.dp), ) } - Text(label) + Column(modifier = Modifier.weight(1f)) { + Text(text = primaryText) + if (secondaryText != null) { + Spacer(Modifier.height(HADimens.SPACE1)) + Text( + text = secondaryText, + style = HATextStyle.BodyMedium.copy(textAlign = TextAlign.Start), + ) + } + } } } diff --git a/app/src/main/kotlin/io/homeassistant/companion/android/util/compose/entity/EntityPicker.kt b/app/src/main/kotlin/io/homeassistant/companion/android/util/compose/entity/EntityPicker.kt index 6e38dd85278..189ee0060f6 100644 --- a/app/src/main/kotlin/io/homeassistant/companion/android/util/compose/entity/EntityPicker.kt +++ b/app/src/main/kotlin/io/homeassistant/companion/android/util/compose/entity/EntityPicker.kt @@ -65,7 +65,7 @@ import io.homeassistant.companion.android.common.compose.composable.ButtonSize import io.homeassistant.companion.android.common.compose.composable.HAFilledButton import io.homeassistant.companion.android.common.compose.composable.HAHorizontalDivider import io.homeassistant.companion.android.common.compose.composable.HAModalBottomSheet -import io.homeassistant.companion.android.common.compose.composable.HATextField +import io.homeassistant.companion.android.common.compose.composable.HASearchField import io.homeassistant.companion.android.common.compose.composable.rememberHAModalBottomSheetState import io.homeassistant.companion.android.common.compose.theme.HABorderWidth import io.homeassistant.companion.android.common.compose.theme.HAColorScheme @@ -85,9 +85,7 @@ import io.homeassistant.companion.android.util.RegistriesDataHandler import io.homeassistant.companion.android.util.compose.safeScreenHeight import io.homeassistant.companion.android.util.compose.screenWidth import kotlin.coroutines.CoroutineContext -import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -488,7 +486,13 @@ private fun EntityPickerContent( ) Column(modifier = modifier, verticalArrangement = Arrangement.spacedBy(HADimens.SPACE3)) { - SearchField(searchQuery, onSearchQueryChange) + HASearchField( + query = searchQuery, + onQueryChange = onSearchQueryChange, + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = HADimens.SPACE3), + ) if (filteredEntities.isNotEmpty()) { LazyColumn( @@ -514,50 +518,6 @@ private fun EntityPickerContent( } } -@Composable -private fun SearchField(searchQuery: String, onSearchQueryChange: (String) -> Unit) { - val colorScheme = LocalHAColorScheme.current - var searchQueryRaw by remember { mutableStateOf(searchQuery) } - - // Sync local state when parent state changes (e.g., when cleared externally) - LaunchedEffect(searchQuery) { - if (searchQuery != searchQueryRaw) { - searchQueryRaw = searchQuery - } - } - - // Debounced update to parent - LaunchedEffect(searchQueryRaw) { - // Skip debounce for empty strings to provide instant clear feedback - if (searchQueryRaw.isEmpty()) { - onSearchQueryChange(searchQueryRaw) - } else { - delay(300.milliseconds) - onSearchQueryChange(searchQueryRaw) - } - } - - HATextField( - value = searchQueryRaw, - onValueChange = { searchQueryRaw = it }, - label = { Text(stringResource(commonR.string.search)) }, - trailingIcon = { - if (searchQueryRaw.isNotEmpty()) { - IconButton(onClick = { searchQueryRaw = "" }) { - Icon( - imageVector = Icons.Default.Close, - contentDescription = stringResource(commonR.string.clear_search), - tint = colorScheme.colorOnNeutralNormal, - ) - } - } - }, - modifier = Modifier - .fillMaxWidth() - .padding(horizontal = HADimens.SPACE3), - ) -} - @Composable private fun EmptyResultPlaceholder(searchQuery: String) { Row( diff --git a/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/SettingDialogStateImmutabilityTest.kt b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/SettingDialogStateImmutabilityTest.kt new file mode 100644 index 00000000000..9656e238ca3 --- /dev/null +++ b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/SettingDialogStateImmutabilityTest.kt @@ -0,0 +1,82 @@ +package io.homeassistant.companion.android.settings.sensor + +import io.homeassistant.companion.android.database.sensor.SensorSetting +import io.homeassistant.companion.android.database.sensor.SensorSettingType +import io.homeassistant.companion.android.settings.sensor.SensorDetailViewModel.Companion.SettingDialogState +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNotSame +import org.junit.jupiter.api.Test + +/** + * Regression test for the immutable copy fix in `SensorDetailView`'s setting dialog. + * + * The previous implementation called `state.copy().apply { setting.value = newValue }`, which + * shared the same [SensorSetting] instance across the original and "copied" state because + * `data class` copy is shallow. Mutating the shared `var value` field then leaked back into + * the source-of-truth state held by the view model. + * + * The fix replaces that with `state.copy(setting = state.setting.copy(value = newValue))`, which + * creates a fresh [SensorSetting] for the new state, leaving the original untouched. This test + * pins that invariant so a future regression is intentional. + */ +class SettingDialogStateImmutabilityTest { + + private fun makeSetting(value: String) = SensorSetting( + sensorId = "sensor-1", + name = "allow_list", + value = value, + valueType = SensorSettingType.LIST_APPS, + enabled = true, + entries = listOf("com.google.chrome", "org.mozilla.firefox"), + ) + + private fun makeState(value: String) = SettingDialogState( + setting = makeSetting(value), + loading = false, + entries = listOf( + "com.google.chrome" to "Chrome\n(com.google.chrome)", + "org.mozilla.firefox" to "Firefox\n(org.mozilla.firefox)", + ), + entriesSelected = listOf("com.google.chrome"), + ) + + @Test + fun `Given state when copy with new setting value then new state holds new value`() { + val original = makeState(value = "old") + + val updated = original.copy(setting = original.setting.copy(value = "new")) + + assertEquals("new", updated.setting.value) + } + + @Test + fun `Given state when copy with new setting value then original setting value is unchanged`() { + val original = makeState(value = "old") + + original.copy(setting = original.setting.copy(value = "new")) + + assertEquals("old", original.setting.value) + } + + @Test + fun `Given state when copy with new setting value then setting reference is a fresh instance`() { + val original = makeState(value = "old") + + val updated = original.copy(setting = original.setting.copy(value = "new")) + + // Different SensorSetting instance — proves we did not mutate the shared one. + assertNotSame(original.setting, updated.setting) + } + + @Test + fun `Given state when copy with new setting value then untouched state fields share references`() { + val original = makeState(value = "old") + + val updated = original.copy(setting = original.setting.copy(value = "new")) + + // Other state fields are reused (data class shallow copy), confirming we only swapped setting. + assertEquals(original.entries, updated.entries) + assertEquals(original.entriesSelected, updated.entriesSelected) + assertEquals(original.loading, updated.loading) + } +} diff --git a/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/JoinSelectedValuesTest.kt b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/JoinSelectedValuesTest.kt new file mode 100644 index 00000000000..1156c3c9dcb --- /dev/null +++ b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/JoinSelectedValuesTest.kt @@ -0,0 +1,47 @@ +package io.homeassistant.companion.android.settings.sensor.views + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class JoinSelectedValuesTest { + + @Test + fun `Given empty list when joining then return empty string`() { + val result = joinSelectedValues(emptyList()) + + assertEquals("", result) + } + + @Test + fun `Given single value when joining then return value alone without separators`() { + val result = joinSelectedValues(listOf("com.google.chrome")) + + assertEquals("com.google.chrome", result) + } + + @Test + fun `Given multiple values when joining then comma-space separated without surrounding brackets`() { + val result = joinSelectedValues( + listOf("com.google.chrome", "org.mozilla.firefox", "com.example.app"), + ) + + assertEquals("com.google.chrome, org.mozilla.firefox, com.example.app", result) + } + + @Test + fun `Given values containing brackets when joining then strip all bracket characters (bug-for-bug parity)`() { + // Documents current behaviour: the implementation removes any '[' or ']' that appear + // anywhere in the values (including inside the values themselves), preserving the + // legacy behaviour. A future intentional change should update this test. + val result = joinSelectedValues(listOf("foo[bar]", "baz[qux")) + + assertEquals("foobar, bazqux", result) + } + + @Test + fun `Given values containing commas and unicode when joining then values pass through unchanged`() { + val result = joinSelectedValues(listOf("hello,world", "café-☕", "id-中文")) + + assertEquals("hello,world, café-☕, id-中文", result) + } +} diff --git a/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/ParseSettingLabelTest.kt b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/ParseSettingLabelTest.kt new file mode 100644 index 00000000000..552e7effca9 --- /dev/null +++ b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/ParseSettingLabelTest.kt @@ -0,0 +1,66 @@ +package io.homeassistant.companion.android.settings.sensor.views + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Test + +class ParseSettingLabelTest { + + @Test + fun `Given single-line label when parsing then primary equals label and secondary is null`() { + val (primary, secondary) = parseSettingLabel("Chrome") + + assertEquals("Chrome", primary) + assertNull(secondary) + } + + @Test + fun `Given two-line label with parens when parsing then strip parens from secondary`() { + val (primary, secondary) = parseSettingLabel("Chrome\n(com.google.chrome)") + + assertEquals("Chrome", primary) + assertEquals("com.google.chrome", secondary) + } + + @Test + fun `Given label with multiple newlines when parsing then only first split is used`() { + // limit = 2 means subsequent newlines remain inside the secondary value. + val (primary, secondary) = parseSettingLabel("Chrome\nline1\nline2") + + assertEquals("Chrome", primary) + assertEquals("line1\nline2", secondary) + } + + @Test + fun `Given second line without parens when parsing then secondary is the raw line`() { + val (primary, secondary) = parseSettingLabel("Foo\nbar") + + assertEquals("Foo", primary) + assertEquals("bar", secondary) + } + + @Test + fun `Given mismatched parens when parsing then removeSurrounding is a no-op`() { + // String.removeSurrounding only strips when both prefix and suffix match. + val (primary, secondary) = parseSettingLabel("Foo\n(bar") + + assertEquals("Foo", primary) + assertEquals("(bar", secondary) + } + + @Test + fun `Given empty label when parsing then primary is empty and secondary is null`() { + val (primary, secondary) = parseSettingLabel("") + + assertEquals("", primary) + assertNull(secondary) + } + + @Test + fun `Given empty second line when parsing then secondary is empty string`() { + val (primary, secondary) = parseSettingLabel("Foo\n") + + assertEquals("Foo", primary) + assertEquals("", secondary) + } +} diff --git a/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheetFilterTest.kt b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheetFilterTest.kt new file mode 100644 index 00000000000..95a9e275cd8 --- /dev/null +++ b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingSheetFilterTest.kt @@ -0,0 +1,62 @@ +package io.homeassistant.companion.android.settings.sensor.views + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class SensorDetailSettingSheetFilterTest { + + private val entries = listOf( + "com.google.chrome" to "Chrome\n(com.google.chrome)", + "org.mozilla.firefox" to "Firefox\n(org.mozilla.firefox)", + "com.example.app" to "Example App\n(com.example.app)", + ) + + @Test + fun `Given empty query when filtering then return all entries`() { + val result = filterSettingEntries(entries, query = "") + + assertEquals(entries, result) + } + + @Test + fun `Given blank query when filtering then return all entries`() { + val result = filterSettingEntries(entries, query = " ") + + assertEquals(entries, result) + } + + @Test + fun `Given query matching app name when filtering then return matching entries`() { + val result = filterSettingEntries(entries, query = "Chrome") + + assertEquals(listOf(entries[0]), result) + } + + @Test + fun `Given query matching package name in label when filtering then return matching entries`() { + val result = filterSettingEntries(entries, query = "com.google") + + assertEquals(listOf(entries[0]), result) + } + + @Test + fun `Given case-insensitive query when filtering then return matches`() { + val result = filterSettingEntries(entries, query = "CHROME") + + assertEquals(listOf(entries[0]), result) + } + + @Test + fun `Given query matching no entries when filtering then return empty list`() { + val result = filterSettingEntries(entries, query = "nonexistent") + + assertEquals(emptyList>(), result) + } + + @Test + fun `Given query with leading and trailing spaces when filtering then trim and match`() { + val result = filterSettingEntries(entries, query = " Chrome ") + + assertEquals(listOf(entries[0]), result) + } +} diff --git a/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SettingEntriesTest.kt b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SettingEntriesTest.kt new file mode 100644 index 00000000000..e16d9d0beec --- /dev/null +++ b/app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SettingEntriesTest.kt @@ -0,0 +1,63 @@ +package io.homeassistant.companion.android.settings.sensor.views + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNotEquals +import org.junit.jupiter.api.Assertions.assertSame +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +/** + * Pins the contract of the [SettingEntries] wrapper introduced for Compose stability. + * + * The Compose Compiler cannot infer stability for `List>`, so this wrapper + * is annotated `@Immutable` and used as the parameter type of `rememberFilteredSettingEntries`. + * These tests exercise the wrapper so a future regression (e.g. accidentally removing the + * wrapper or changing its shape) fails loudly. + */ +class SettingEntriesTest { + + private val sampleItems = listOf( + "com.google.chrome" to "Chrome\n(com.google.chrome)", + "org.mozilla.firefox" to "Firefox\n(org.mozilla.firefox)", + ) + + @Test + fun `Given list when wrapping then items property exposes the same list reference`() { + val wrapper = SettingEntries(sampleItems) + + assertSame(sampleItems, wrapper.items) + } + + @Test + fun `Given two wrappers built from equal lists when compared then they are equal`() { + val first = SettingEntries(sampleItems) + val second = SettingEntries(sampleItems.toList()) + + assertEquals(first, second) + assertEquals(first.hashCode(), second.hashCode()) + } + + @Test + fun `Given two wrappers built from different lists when compared then they are not equal`() { + val first = SettingEntries(sampleItems) + val second = SettingEntries(emptyList()) + + assertNotEquals(first, second) + } + + @Test + fun `Given wrapper when filtered through filterSettingEntries then matches plain list filtering`() { + val wrapper = SettingEntries(sampleItems) + + val filtered = filterSettingEntries(wrapper.items, query = "Chrome") + + assertEquals(listOf(sampleItems[0]), filtered) + } + + @Test + fun `Given empty list when wrapping then items is empty`() { + val wrapper = SettingEntries(emptyList()) + + assertTrue(wrapper.items.isEmpty()) + } +} diff --git a/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HAModalBottomSheet.kt b/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HAModalBottomSheet.kt index bf86ec912c2..5a9fdb6cbd5 100644 --- a/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HAModalBottomSheet.kt +++ b/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HAModalBottomSheet.kt @@ -19,14 +19,19 @@ import io.homeassistant.companion.android.common.compose.theme.LocalHAColorSchem * In inspection mode (previews and screenshot tests), this returns a [rememberStandardBottomSheetState] * because [rememberModalBottomSheetState] requires a fully running Compose runtime, and [rememberStandardBottomSheetState] * doesn't animate properly. + * + * @param skipPartiallyExpanded When true, the sheet skips the partially expanded state and goes + * straight to fully expanded. Useful when the sheet has a fixed footer (cancel/save) that must stay + * reachable. */ @OptIn(ExperimentalMaterial3Api::class) @Composable -fun rememberHAModalBottomSheetState(): SheetState = if (LocalInspectionMode.current) { - rememberStandardBottomSheetState(skipHiddenState = false) -} else { - rememberModalBottomSheetState() -} +fun rememberHAModalBottomSheetState(skipPartiallyExpanded: Boolean = false): SheetState = + if (LocalInspectionMode.current) { + rememberStandardBottomSheetState(skipHiddenState = false) + } else { + rememberModalBottomSheetState(skipPartiallyExpanded = skipPartiallyExpanded) + } /** * A modal bottom sheet that uses the Home Assistant theme. diff --git a/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchField.kt b/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchField.kt new file mode 100644 index 00000000000..5869e364ef7 --- /dev/null +++ b/common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchField.kt @@ -0,0 +1,106 @@ +package io.homeassistant.companion.android.common.compose.composable + +import androidx.annotation.VisibleForTesting +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Close +import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import io.homeassistant.companion.android.common.R as commonR +import io.homeassistant.companion.android.common.compose.theme.LocalHAColorScheme +import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds +import kotlinx.coroutines.delay + +/** Default debounce applied before propagating non-empty queries to the parent. */ +private val DEFAULT_DEBOUNCE = 300.milliseconds + +/** + * Forwards [rawQuery] to [emit], debouncing non-empty queries by [debounce]. + * + * Empty strings bypass the delay so clearing the field reflects instantly to the parent. For + * non-empty queries the function suspends for [debounce] before emitting; if the surrounding + * coroutine is cancelled (e.g. because the raw query changed again) the pending emit is dropped, + * which is exactly the coalescing behaviour expected from a search debounce. + * + * Extracted from [HASearchField]'s [LaunchedEffect] so the timing rules can be tested without a + * Compose host. + */ +@VisibleForTesting +internal suspend fun debouncedSearchUpdate(rawQuery: String, debounce: Duration, emit: (String) -> Unit) { + if (rawQuery.isEmpty()) { + emit(rawQuery) + } else { + delay(debounce) + emit(rawQuery) + } +} + +/** + * Reusable search input field for filtering lists. + * + * Holds the raw text locally and propagates changes to [onQueryChange] with a [debounce] delay, + * except for empty queries which are forwarded immediately to provide instant clear feedback when + * the user wipes the field. Local state is also kept in sync with [query] so external changes + * (for example a parent clearing the search) are reflected in the field. + * + * The field is rendered as an [HATextField] with the standard "Search" label and a trailing clear + * [IconButton] that appears only when the current text is non-empty. An optional [leadingIcon] slot + * allows callers to render a search glyph or other adornment without changing the debounce logic. + * + * @param query Current query coming from the parent state. Used to seed and re-sync local state. + * @param onQueryChange Callback invoked with the new query, debounced for non-empty values. + * @param modifier The [Modifier] to be applied to this search field. + * @param leadingIcon Optional leading content rendered inside the text field. + * @param debounce Delay before non-empty queries are propagated to [onQueryChange]. + */ +@Composable +fun HASearchField( + query: String, + onQueryChange: (String) -> Unit, + modifier: Modifier = Modifier, + leadingIcon: @Composable (() -> Unit)? = null, + debounce: Duration = DEFAULT_DEBOUNCE, +) { + val colorScheme = LocalHAColorScheme.current + var searchQueryRaw by remember { mutableStateOf(query) } + + // Sync local state when parent state changes (e.g., when cleared externally). + LaunchedEffect(query) { + if (query != searchQueryRaw) { + searchQueryRaw = query + } + } + + // Debounced update to parent. Empty strings propagate immediately to keep clearing snappy. + LaunchedEffect(searchQueryRaw) { + debouncedSearchUpdate(rawQuery = searchQueryRaw, debounce = debounce, emit = onQueryChange) + } + + HATextField( + value = searchQueryRaw, + onValueChange = { searchQueryRaw = it }, + label = { Text(stringResource(commonR.string.search)) }, + leadingIcon = leadingIcon, + trailingIcon = { + if (searchQueryRaw.isNotEmpty()) { + IconButton(onClick = { searchQueryRaw = "" }) { + Icon( + imageVector = Icons.Default.Close, + contentDescription = stringResource(commonR.string.clear_search), + tint = colorScheme.colorOnNeutralNormal, + ) + } + } + }, + modifier = modifier, + ) +} diff --git a/common/src/test/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchFieldDebounceTest.kt b/common/src/test/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchFieldDebounceTest.kt new file mode 100644 index 00000000000..eee89965cc4 --- /dev/null +++ b/common/src/test/kotlin/io/homeassistant/companion/android/common/compose/composable/HASearchFieldDebounceTest.kt @@ -0,0 +1,113 @@ +package io.homeassistant.companion.android.common.compose.composable + +import kotlin.time.Duration.Companion.milliseconds +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +/** + * Tests for [debouncedSearchUpdate], the timing helper extracted from HASearchField. + * + * Because the helper is a plain suspend function (not a Composable) we can drive it directly with + * the [runTest] virtual time machine to verify debounce, instant-clear, and cancellation rules + * without standing up a Compose host. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class HASearchFieldDebounceTest { + + private val debounce = 300.milliseconds + + @Test + fun `Given empty query when debouncing then emit immediately without delay`() = runTest { + val emitted = mutableListOf() + + debouncedSearchUpdate(rawQuery = "", debounce = debounce, emit = { emitted += it }) + + // No virtual time advanced and yet the empty value is already through. + assertEquals(listOf(""), emitted) + } + + @Test + fun `Given non-empty query when debouncing then suspend until delay elapses before emitting`() = runTest { + val emitted = mutableListOf() + + val job = launch { + debouncedSearchUpdate(rawQuery = "chrome", debounce = debounce, emit = { emitted += it }) + } + + // Before the debounce expires nothing has been emitted yet. + advanceTimeBy(299.milliseconds) + runCurrent() + assertEquals(emptyList(), emitted) + + // Crossing the boundary lets the emit happen. + advanceTimeBy(2.milliseconds) + runCurrent() + assertEquals(listOf("chrome"), emitted) + + job.join() + } + + @Test + fun `Given rapid updates when each is cancelled before debounce then only final value is emitted`() = runTest { + val emitted = mutableListOf() + + // Simulates the LaunchedEffect(searchQueryRaw) restart behaviour: each new raw value + // cancels the previous launch and starts a new one. Only the last one runs to completion. + val first = launch { + debouncedSearchUpdate(rawQuery = "c", debounce = debounce, emit = { emitted += it }) + } + advanceTimeBy(100.milliseconds) + first.cancel() + + val second = launch { + debouncedSearchUpdate(rawQuery = "ch", debounce = debounce, emit = { emitted += it }) + } + advanceTimeBy(100.milliseconds) + second.cancel() + + val third = launch { + debouncedSearchUpdate(rawQuery = "chr", debounce = debounce, emit = { emitted += it }) + } + advanceTimeBy(debounce) + runCurrent() + + assertEquals(listOf("chr"), emitted) + third.join() + } + + @Test + fun `Given pending non-empty emit when cancelled before delay then nothing is emitted`() = runTest { + val emitted = mutableListOf() + + val job = launch { + debouncedSearchUpdate(rawQuery = "chrome", debounce = debounce, emit = { emitted += it }) + } + advanceTimeBy(100.milliseconds) + job.cancel() + runCurrent() + + assertEquals(emptyList(), emitted) + } + + @Test + fun `Given empty query after a non-empty one when both run then empty bypasses debounce`() = runTest { + val emitted = mutableListOf() + + // Non-empty: pending until delay. + val nonEmpty = launch { + debouncedSearchUpdate(rawQuery = "chrome", debounce = debounce, emit = { emitted += it }) + } + advanceTimeBy(50.milliseconds) + nonEmpty.cancel() + + // Replaced by an empty raw value — should be reflected to the parent immediately. + debouncedSearchUpdate(rawQuery = "", debounce = debounce, emit = { emitted += it }) + + assertEquals(listOf(""), emitted) + } +}