Migrate entity widget configuration to Compose#7007
Conversation
There was a problem hiding this comment.
Hi @himu-gupta
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Migrates the Entity widget configuration UI from the legacy XML/ViewBinding implementation to a Jetpack Compose-based screen backed by a new ViewModel, and adds unit tests for key configuration behaviors.
Changes:
- Replaced
EntityWidgetConfigureActivity’s legacy view logic with Compose UI and a dedicatedEntityWidgetConfigureViewModel. - Added a Compose configuration screen with attribute selection UI (including custom attribute entry).
- Added unit tests for restoring/persisting widget configuration and handling server changes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/main/res/values/strings.xml | Adds a new string resource used by the Compose attribute picker UI. |
| app/src/test/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureViewModelTest.kt | Introduces unit tests for the new configuration ViewModel behavior. |
| app/src/main/res/layout/widget_static_configure.xml | Removes the legacy XML layout now replaced by Compose UI. |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureViewModel.kt | Adds a new ViewModel managing widget configuration state, persistence, and pin flow. |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureScreen.kt | Adds the Compose configuration UI (server/entity pickers, attributes, separators, styling options). |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureActivity.kt | Updates the activity to host Compose UI and delegate configuration actions to the ViewModel. |
f9669f5 to
3a51bce
Compare
|
The CLA check is now passing, but the original Home Assistant CLA bot review still shows Changes requested and keeps the PR in a blocked state. Could a maintainer please dismiss the stale review when convenient? All review threads are resolved, the branch is rebased onto the latest |
CLA was signed. |
|
Could you update the PR description to add screenshots, thanks. |
|
Added light and dark screenshots to the PR description. |
TimoPtr
left a comment
There was a problem hiding this comment.
I did a first very quick review of the code to identify the missing bits.
…n system Address review feedback by aligning the Media Player Controls widget configuration screen with the Compose pattern used in home-assistant#7007. - Add MediaPlayerControlsWidgetConfigureViewModel exposing an immutable ViewState, with server/entity/registry flows and a Snackbar message channel; the Activity is now a thin BaseActivity that delegates to it - Add unit tests for the ViewModel (MainDispatcherJUnit5Extension) - Use HATheme + Material 3 throughout to fix the broken dark/light colors - Replace the free-text entity field with EntityPicker, providing the entity/device/area registries for friendlier names - Surface widget create/update errors via Snackbar instead of Toast - Split the screen into smaller composables and use HaDimens for spacing - Proper HACheckbox row click, full-width inputs and action button, and a top-left close button - Regenerate screenshot reference images for the new UI Note: EntityPicker is single-select, so the widget now stores a single media_player entity instead of the previous comma-separated list.
…n system Address review feedback by aligning the Media Player Controls widget configuration screen with the Compose pattern used in home-assistant#7007. - Add MediaPlayerControlsWidgetConfigureViewModel exposing an immutable ViewState, with server/entity/registry flows and a Snackbar message channel; the Activity is now a thin BaseActivity that delegates to it - Add unit tests for the ViewModel (MainDispatcherJUnit5Extension) - Use HATheme + Material 3 throughout to fix the broken dark/light colors - Replace the free-text entity field with EntityPicker, providing the entity/device/area registries for friendlier names - Surface widget create/update errors via Snackbar instead of Toast - Split the screen into smaller composables and use HaDimens for spacing - Proper HACheckbox row click, full-width inputs and action button, and a top-left close button - Regenerate screenshot reference images for the new UI Note: EntityPicker is single-select, so the widget now stores a single media_player entity instead of the previous comma-separated list.
…n system Address review feedback by aligning the Media Player Controls widget configuration screen with the Compose pattern used in home-assistant#7007. - Add MediaPlayerControlsWidgetConfigureViewModel exposing an immutable ViewState, with server/entity/registry flows and a Snackbar message channel; the Activity is now a thin BaseActivity that delegates to it - Add unit tests for the ViewModel (MainDispatcherJUnit5Extension) - Use HATheme + Material 3 throughout to fix the broken dark/light colors - Replace the free-text entity field with EntityPicker, providing the entity/device/area registries for friendlier names - Surface widget create/update errors via Snackbar instead of Toast - Split the screen into smaller composables and use HaDimens for spacing - Proper HACheckbox row click, full-width inputs and action button, and a top-left close button - Regenerate screenshot reference images for the new UI Note: EntityPicker is single-select, so the widget now stores a single media_player entity instead of the previous comma-separated list.
218f3fd to
eca4235
Compare
|
Added a review-feedback screenshot to the PR description showing the updated centered form with the modern server picker and append-attributes section expanded, including the merged Add attribute input/chips and entity-derived label. |
|
Added a dedicated server picker fix screenshot to the PR description. This one matches the dark top-of-screen state from your attached image: the server picker now uses the HA dropdown style and is aligned with the selected entity picker. |
c2c3670 to
e483268
Compare
|
I think we could remove the "Append attribute value" checkbox by only showing the tags to pick. Does it make sense to even allow custom attributes? @jpelgrom wdyt |
TimoPtr
left a comment
There was a problem hiding this comment.
Just to let you know that we don't allow usage of autonomous agent, take the time to look at all my comments.
| get() = intent.extras?.getBoolean(ManageWidgetsViewModel.CONFIGURE_REQUEST_LAUNCHER, false) == true | ||
|
|
||
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| enableEdgeToEdgeCompat() |
There was a problem hiding this comment.
| enableEdgeToEdgeCompat() |
It's already in BaseActivity
| val selectedServerId: Int get() = viewState.selectedServerId | ||
| val selectedEntityId: String? get() = viewState.selectedEntityId | ||
| val appendAttributes: Boolean get() = viewState.appendAttributes | ||
| val selectedAttributeIds: List<String> get() = viewState.selectedAttributeIds | ||
| val label: String get() = viewState.label | ||
| val textSize: String get() = viewState.textSize | ||
| val stateSeparator: String get() = viewState.stateSeparator | ||
| val attributeSeparator: String get() = viewState.attributeSeparator | ||
| val selectedTapAction: WidgetTapAction get() = viewState.selectedTapAction | ||
| val selectedBackgroundType: WidgetBackgroundType get() = viewState.selectedBackgroundType | ||
| internal val selectedTextColor: EntityWidgetTextColor get() = viewState.selectedTextColor | ||
| val isUpdateWidget: Boolean get() = viewState.isUpdateWidget |
There was a problem hiding this comment.
Let's just expose the ViewState.
| internal var widgetId: Int = AppWidgetManager.INVALID_APPWIDGET_ID | ||
| private set | ||
|
|
||
| val servers = serverManager.serversFlow |
There was a problem hiding this comment.
Keep this in the ViewState, and make it already a HADropdownItem so mapping happen in the viewModel scope. Same for the selected server it needs to be in the ViewState.
|
|
||
| val servers = serverManager.serversFlow | ||
|
|
||
| internal var viewState by mutableStateOf(EntityWidgetConfigureViewState(selectedEntityId = preselectedEntityId)) |
| internal data class EntityWidgetTextColors(val white: String, val black: String) | ||
|
|
||
| internal enum class EntityWidgetTextColor { | ||
| WHITE, | ||
| BLACK, | ||
| } | ||
|
|
||
| internal enum class EntityWidgetConfigureError { | ||
| CREATE, | ||
| UPDATE, | ||
| } |
There was a problem hiding this comment.
This is something that is common to all the widget configure screen.
| HADropdownItem( | ||
| key = WidgetTapAction.REFRESH, | ||
| label = stringResource(commonR.string.refresh), | ||
| ), |
There was a problem hiding this comment.
Let's remember the list to avoid recreating it on every recomposition
| onTextColorSelected: (EntityWidgetTextColor) -> Unit, | ||
| ) { | ||
| HADropdownMenu( | ||
| items = buildList { |
|
|
||
| if (selectedBackgroundType == WidgetBackgroundType.TRANSPARENT) { | ||
| HADropdownMenu( | ||
| items = listOf( |
| onClick = onActionClick, | ||
| modifier = Modifier | ||
| .formControlWidth() | ||
| .testTag(ENTITY_WIDGET_ACTION_BUTTON_TAG), |
There was a problem hiding this comment.
Let's not use TAG when you can use the text
| verticalArrangement = Arrangement.spacedBy(HADimens.SPACE2), | ||
| ) { | ||
| unselectedAttributes.forEach { attributeId -> | ||
| InputChip( |
There was a problem hiding this comment.
Since it's a M3 element it has the wrong colors, you need to use the HAColors (like you did for the the selected one ) so it gets the proper colors for light/dark and not the one from m3. The text needs to use a proper style too.
You could open a first PR that makes a HAInputChip for instance.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Also I would like to see a close button in the topbar |
- Move media-player entity filtering and name resolution into the view model (uiState.availableEntities / selectedEntities) so the screen no longer filters or resolves names itself. - Drop the wrapper Column in favour of a formControlWidth() modifier and extract EntityPickerSection, matching the entity widget (home-assistant#7007). - Add a 'Selected entities:' title and a divider to the selection list. - remember() the background-type option list. - Replace the throwing check() in updateWidgetConfiguration() with a Boolean result that surfaces a user message; guard it in the activity. - Share the entity/registry flows eagerly since the uiState combine keeps them subscribed for the view model's lifetime. - Add a setScreen() test helper and a single combined interaction test covering the toggles, remove, action and close callbacks.


Summary
EntityWidgetConfigureActivityfrom XML views to Compose and Home Assistant Material 3 componentsCloses #6307
Screenshots
Server picker fix
Testing
./gradlew --no-daemon --console=plain :build-logic:convention:ktlintFormat ktlintFormat./gradlew --no-daemon --console=plain :app:testFullDebugUnitTest --tests io.homeassistant.companion.android.widgets.entity.EntityWidgetConfigureViewModelTest