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 15 commits intohome-assistant:mainfrom
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 🙏🏻 ? |
TimoPtr
left a comment
TimoPtr
left a comment
There was a problem hiding this comment.
The UX needs to be adjust the rest of the comments are mostly to adjust with the project guidelines.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
- Extract HASearchField to :common, replacing duplicated search fields in EntityPicker and SensorDetailSettingSheet - Replace rememberStandardBottomSheetState (intended for BottomSheetScaffold) with rememberModalBottomSheetState(skipPartiallyExpanded = true) so the sheet no longer blinks before fully expanding; drop the LaunchedEffect.expand() workaround - Extract parseSettingLabel helper used by both BottomSheetSettingRow and SensorDetailSettingRow - Rename consumeFlingNestedScrollConnection to nestedScrollFlingGuard, demote rememberFilteredSettingEntries to private, and simplify the row toggle logic
…ate immutability - joinSelectedValues: pin behavior on empty/single/multi values, bracket stripping (bug-for-bug parity with legacy code), and unicode pass-through - parseSettingLabel: split semantics, parens stripping, multi-newline limit, and edge cases - SettingDialogState immutability: prove state.copy(setting = setting.copy(...)) produces a new SensorSetting and leaves the original instance untouched
Verifies that empty queries propagate immediately while non-empty queries are delayed by the configured debounce, that rapid updates coalesce to the latest value, and that cancellation drops pending emissions.
…-allow-list # Conflicts: # app/src/main/kotlin/io/homeassistant/companion/android/util/compose/entity/EntityPicker.kt
ktlint function-signature rule prefers single-line when within max line length.
| */ | ||
| @Composable | ||
| private fun rememberFilteredSettingEntries( | ||
| entries: List<Pair<String, String>>, |
There was a problem hiding this comment.
Fixed in fee247f — wrapped entries in a file-local @Immutable internal data class SettingEntries(val items: List<Pair<String, String>>) at the composable boundary. rememberFilteredSettingEntries now takes SettingEntries; SettingDialogState and filterSettingEntries keep their plain types. Avoided pulling in kotlinx-collections-immutable since it isn't used anywhere else in the repo. Pinned by new SettingEntriesTest.
| */ | ||
| @Composable | ||
| private fun rememberFilteredSettingEntries( | ||
| entries: List<Pair<String, String>>, |
There was a problem hiding this comment.
Fixed in fee247f — wrapped entries in a file-local @Immutable internal data class SettingEntries(val items: List<Pair<String, String>>) at the composable boundary. rememberFilteredSettingEntries now takes SettingEntries; SettingDialogState and filterSettingEntries keep their plain types. Avoided pulling in kotlinx-collections-immutable since it isn't used anywhere else in the repo. Pinned by new SettingEntriesTest.
| onClick: (Boolean) -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val (primaryText, secondaryText) = parseSettingLabel(label) |
There was a problem hiding this comment.
Fixed in fee247f — parseSettingLabel is genuinely an internal production helper shared between SensorDetailSettingSheet and SensorDetailSettingRow (in SensorDetailView.kt), so the @VisibleForTesting annotation was the wrong contract. Dropped the annotation, kept it internal. Existing ParseSettingLabelTest still exercises it without modification.
| onClick: (Boolean) -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val (primaryText, secondaryText) = parseSettingLabel(label) |
There was a problem hiding this comment.
Fixed in fee247f — parseSettingLabel is genuinely an internal production helper shared between SensorDetailSettingSheet and SensorDetailSettingRow (in SensorDetailView.kt), so the @VisibleForTesting annotation was the wrong contract. Dropped the annotation, kept it internal. Existing ParseSettingLabelTest still exercises it without modification.
…bleForTesting - Wrap setting entries in `@Immutable SettingEntries` data class at the composable boundary so the Compose Compiler can infer parameter stability for `rememberFilteredSettingEntries` (lint id 6307/6309). - Drop `@VisibleForTesting` from `parseSettingLabel`; it is genuinely shared production helper consumed by `SensorDetailView.kt` (lint id 6306/6308). Function stays `internal`. - Add `SettingEntriesTest` exercising the new wrapper so a future regression of the stability fix fails loudly.
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