feat(Sensors): Add search filter to sensor setting allow list dialog#6567
feat(Sensors): Add search filter to sensor setting allow list dialog#6567danielgomezrico wants to merge 30 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an in-dialog search field to SensorDetailSettingDialog so users can locally filter long list-type sensor setting entries (apps, Bluetooth devices, zones, etc.) without changing ViewModel/state handling.
Changes:
- Added a search
TextFieldwith clear button to filter list-type setting dialog entries. - Introduced
filterSettingEntries(...)helper to apply case-insensitive filtering. - Added unit tests covering filtering behavior and edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/src/main/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailView.kt | Adds search UI + filtering logic for list dialogs; introduces filterSettingEntries helper |
| app/src/test/kotlin/io/homeassistant/companion/android/settings/sensor/views/SensorDetailSettingDialogFilterTest.kt | Adds JUnit tests validating filtering behavior |
You can also share your feedback on Copilot code review. Take the survey.
|
Can you provide screenshots for the change? Did you consider only showing search when there is a long list? Showing search when there are only 3 items is a bit weird. This dialog isn't really suited for a large list anyway, I wonder if it should be changed to a different picker like the bottom sheet/dialog we have implemented for entities recently. Both comments generated by Copilot are valid, please address them (try doing it yourself without AI to understand the change you're making). |
Add a search TextField to `SensorDetailSettingDialog` so users can quickly find apps in long lists (e.g., Last Notification Allow List). Filtering is local to the composable; selections persist across searches.
danielgomezrico
force-pushed
the
feature/search-sensor-allow-list
branch
from
41b5f66 to
2baef1b
Compare
danielgomezrico
changed the title
Sure! updated.
Oh that Idea is cool I will adapt it for that case
It makes total sense, but in the mean while it would help, the list of apps could be pretty long and having to search in that dialog is pretty hard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… lists Show search only when entries > 10. Extract SettingSearchField composable for readability.
Got it, Ill check if I can make it for this case. One thing I've noticed is that these types of dialogs are used in other places; should the bottom sheet be limited to this case? |
Just this specific dialog, with a long list of apps that you may want to search. Replacing all other dialogs makes this PR too big and also doesn't make sense as a pattern when there are only 2-3 options. |
|
@danielgomezrico could you address the lint issue and when you are ready put the PR in ready for review. |
Ktlint function-signature rule requires single-line when signature fits within max_line_length (120 chars).
|
@TimoPtr can you help me aproving the CI 🙏🏻 ? |
| Checkbox( | ||
| checked = checked, | ||
| onCheckedChange = null, | ||
| modifier = Modifier.size(width = HADimens.SPACE12, height = HADimens.SPACE12), | ||
| ) |
There was a problem hiding this comment.
The color of the checkbox are wrong and should use the color from HAColorScheme, we could wrap this into a HACheckbox like we did for the other composable (it means adding it to the Compose Catalog and update the screenshots).
There was a problem hiding this comment.
You can check HASwitch for inspiration and the color could be something like
CheckboxDefaults.colors(
checkedColor = colorScheme.colorFillPrimaryLoudResting,
uncheckedColor = colorScheme.colorBorderNeutralNormal,
checkmarkColor = colorScheme.colorSurfaceDefault,
disabledCheckedColor = colorScheme.colorFillDisabledLoudResting,
disabledUncheckedColor = colorScheme.colorBorderNeutralQuiet,
)
To be verified within the screenshot tests
There was a problem hiding this comment.
Tracking as follow-up. Introducing HACheckbox means a new :common component, a Compose Catalog entry and a Compose Catalog screenshot refresh — too broad for this PR's review pass. I've kept the suggested CheckboxDefaults palette captured here so the follow-up PR can lift it verbatim.
There was a problem hiding this comment.
Applied in 83b88f3 — the checkbox now uses CheckboxDefaults.colors(...) wired to HAColorScheme (the palette you suggested: colorFillPrimaryLoudResting, colorBorderNeutralNormal, colorSurfaceDefault, colorFillDisabledLoudResting, colorBorderNeutralQuiet). The dedicated HACheckbox + Compose Catalog entry remains a follow-up.
- Animate sheet collapse: invoke bottomSheetState.hide() inside a rememberCoroutineScope before propagating cancel/save so the dismissal is reflected visually before the dialog state mutates. - Drop the @immutable SettingEntryList wrapper (did not actually freeze the underlying list); pass List<SettingEntry> directly and baseline the resulting ComposeUnstableCollections warning, matching how other list-of-data-class composables in this module already handle it. - Hide the SettingEntry data class behind @VisibleForTesting. - Extract parseSettingLabel into ParsedSettingLabel.kt and return a named (primary, secondary) data class instead of Pair, with an explicit parts.size == 2 guard. Shared by the new sheet and the legacy Material 2 row. - Inline SheetHeader onto its ColumnScope and rely on the parent Column's Arrangement.spacedBy for inter-section spacing; remove the per-child vertical padding. - Set role = Role.Checkbox on the row's .clickable so screen readers announce the toggle correctly. - Render the primary line of both rows with HATextStyle.Body so the legacy dialog inherits the same hierarchy as the new sheet. - Pass viewModel::cancelSettingWithDialog and onDialogSettingSubmitted as method references instead of wrapping lambdas, and point the HATheme TODO at the live tracking issue (home-assistant#6839). - Drop SettingDialogStateImmutabilityTest now that the invariant is obvious from the call site. - Parameterize ParseSettingLabelTest cases via @MethodSource.
- Promote DEFAULT_DEBOUNCE to @VisibleForTesting/internal so test code can reference the same source of truth instead of duplicating 300ms. - Document that the LocalInspectionMode branch in rememberHAModalBottomSheetState produces a no-op state with no animation or hide/expand semantics — it only exists so previews and screenshot tests can render the sheet content.
… Modifier EntityPicker and SensorDetailSettingSheet were each declaring an inline NestedScrollConnection plus a pointerInput(detectVerticalDragGestures) block — same code, same intent: stop the modal bottom sheet from interpreting list scrolls or flings as collapse gestures. Extract that into `Modifier.consumeSheetScrollFling()` next to HAModalBottomSheet so future sheets reuse it instead of copying the pattern.
Replace the per-call `composed { remember { ... } }` wrapper with a
stateless singleton NestedScrollConnection. The object had no per-call
state, so hoisting it to a top-level `val` is semantically identical
and clears the slack ComposeModifierComposed lint failure introduced
when this Modifier was extracted into :common.
…tive The two `List<SettingEntry>` ComposeUnstableCollections warnings were already baselined in app/lint-baseline.xml but missing in automotive, which reuses the same source set and so re-runs the same lint checks.
- Rename SettingDialogState.loading to isLoading - Use HALoading instead of raw CircularProgressIndicator - Hoist row onClick; SheetEntryList takes selectedIds + onToggle instead of a SnapshotStateList - Use HAColorScheme palette for the checkbox - Extract SensorDetailSettingSheetContent for previewability
Robolectric Compose test covering clear-icon show/hide, clear behavior, and debounce timing
Cover with/without search, loading, and empty-filter states across device specs
…or-allow-list # Conflicts: # app/lint-baseline.xml
Compose cannot infer stability of Set/List parameters, failing lint. Pass an isSelected lambda instead of selectedIds Set, and inline the filtered-entries helper so no Composable takes an unstable collection.
…sheet The lambda refactor deleted rememberFilteredSettingEntries, leaving a baseline entry pointing at a removed function. Replace it with the two SensorDetailSettingSheetContent/SheetEntryList entries: List<SettingEntry> params that are actually flagged, so the baseline stays honest.
Summary
TextFieldtoSensorDetailSettingDialogso users can quickly filter long app lists (e.g., Last Notification Allow List)Checklist
Screenshots
Before
No search bar, hard to read and search:
After