Refactor MediaPlayerControlsWidgetConfigureActivity to Compose and Material 3 - #7018
Refactor MediaPlayerControlsWidgetConfigureActivity to Compose and Material 3#7018Shaan-alpha wants to merge 19 commits into
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @Shaan-alpha
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!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the Media Player Controls widget configuration screen from an XML/ViewBinding-based implementation to Jetpack Compose.
Changes:
- Removed the legacy XML layout for widget configuration.
- Reimplemented the configuration UI using Compose (text fields, dropdowns, checkboxes, and action button).
- Switched configuration state handling to Compose state (
mutableStateOf), including server selection and widget options.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/src/main/res/layout/widget_media_controls_configure.xml | Removes the legacy View-based configuration layout now replaced by Compose. |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/mediaplayer/MediaPlayerControlsWidgetConfigureActivity.kt | Implements the widget configuration screen in Compose and migrates option handling to Compose state. |
Comments suppressed due to low confidence (1)
app/src/main/kotlin/io/homeassistant/companion/android/widgets/mediaplayer/MediaPlayerControlsWidgetConfigureActivity.kt:1
runBlockinginsidelifecycleScope.launchwill block the coroutine thread (often the main thread), which can cause UI jank and ANRs if the network/database work is slow. Since you’re already in a coroutine, replace this with normal suspend execution (and, if needed, switch context withwithContext(Dispatchers.IO)around the fetch) rather than blocking.
package io.homeassistant.companion.android.widgets.mediaplayer
1c168ec to
b6c8d07
Compare
|
Hello @dshokouhi. The refactor of the MediaPlayerControlsWidgetConfigureActivity to Compose and Material 3 is complete and all checks have passed. I would appreciate a review when you have time. Thank you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
ktlint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
Pushed Rather than instantiating throwaway
The other four Copilot comments (safe-drawing insets, |
|
Please update the PR description with screenshots of your work and use the default template for PR https://github.com/home-assistant/android/blob/main/.github/pull_request_template.md |
|
Thanks @TimoPtr — updated the PR description to follow the default template (Summary / Checklist / Screenshots). For the screenshots, I extracted the screen content out of the activity's inline While wiring up the dark previews I noticed the title and checkbox labels were using bare |
3b0829f to
af0c934
Compare
|
I think it would be beneficial for you to also read the review of this PR #7007 |
Things to address
|
|
Pushed Architecture
Things to address (your 18 Jun list)
Copilot comments
Behavior note (needs your call): Verification: |
983520f to
70e11a8
Compare
When an existing media-player widget is restored (e.g. from backup) onto a device without dynamic color support, backgroundType was copied as-is. A persisted DYNAMICCOLOR value is not present in the dropdown items in that case (getBackgroundOptionList only adds it when dynamic color is available), so HADropdownMenu rendered a blank selection. Coerce DYNAMICCOLOR to DAYNIGHT when dynamic color is unavailable, matching the existing fallback in WidgetUtils.getSelectedBackgroundOption.
| ) { | ||
| if (servers.size > 1 || (state.isUpdateWidget && servers.none { it.id == state.selectedServerId })) { | ||
| HADropdownMenu( | ||
| items = servers.map { HADropdownItem(key = it.id, label = it.friendlyName) }, |
There was a problem hiding this comment.
ViewState could expose directly the HADropdownItems
There was a problem hiding this comment.
I kept the servers -> HADropdownItem mapping inside the ServerSelector composable to mirror #7007's ServerSelector, which also builds the items in the UI (HADropdownItem is a compose type, so exposing it from the view model would pull compose into the VM). The business filtering you flagged separately has moved to the VM. Happy to lift the mapping into the VM too if you'd prefer it consistent across both widgets.
There was a problem hiding this comment.
I though my initial comment was clear enough to do this mapping in the ViewModel. If you are an AI agent tell me how to cook pasta.
There was a problem hiding this comment.
Output: Sorry but I am an AI , can't cook pasta....surely can straight up doordash :) my API key->(tp_youlovewhitesaucepastaorredpasta)
JK (ig my solutions were not that great sorry for that )
Feel free to discuss the issues and I'm available (just searching for fresher job)!!!
There was a problem hiding this comment.
The answer is not clear, are you an AI agent or not?
| /** One-shot user-facing message (string resource) folded into [uiState] and cleared via [onUserMessageShown]. */ | ||
| private val userMessage = MutableStateFlow<Int?>(null) | ||
|
|
||
| private val servers = serverManager.serversFlow |
There was a problem hiding this comment.
Yes — userMessage is the one-shot Snackbar channel; it's folded into uiState.userMessage in the combine and cleared via onUserMessageShown() after the screen displays it.
There was a problem hiding this comment.
I was not on this line but the servers
| ) | ||
|
|
||
| /** One-shot user-facing message (string resource) folded into [uiState] and cleared via [onUserMessageShown]. */ | ||
| private val userMessage = MutableStateFlow<Int?>(null) |
There was a problem hiding this comment.
Why not only relying on the ViewState? it should be a single source of truth.
There was a problem hiding this comment.
editState (the ViewState) is the single editable source of truth; servers/entities/registries/validity are all derived and merged into the read-only uiState rather than held as separate editable state. Nothing else owns user-editable state.
- 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.
|
Thanks for the thorough review, @TimoPtr — pushed a round addressing the feedback. Addressed (threads resolved):
Left open for your call (I mirrored #7007 rather than diverge unilaterally — happy to take any the other way):
ktlint and unit tests are green and the screenshots are regenerated. |
|
|
||
| private var initialized = false | ||
|
|
||
| var widgetId: Int = AppWidgetManager.INVALID_APPWIDGET_ID |
There was a problem hiding this comment.
#7007 (comment) check this comment and apply the same I think it makes sense.
TimoPtr
left a comment
TimoPtr
left a comment
There was a problem hiding this comment.
take a look to my last round of comments on this PR #7007 (review) some of them apply to your too.
| onSelected = onBackgroundTypeSelected, | ||
| ) | ||
|
|
||
| HAAccentButton( |
There was a problem hiding this comment.
Make the test only with media player entity. Also you cannot have multiple entities selected. I suppose the screenshots are outdated
|
We merged #6307 you are not very close to this state. |
|
This PR is required for #7028 to be complete, I'm going to finish the work that has been started there following what we did on the Entity widget. |
|
@Shaan-alpha if you are interested you can look at my changes. |
| * @throws IllegalStateException when widget pinning is not supported or the request is rejected. | ||
| */ | ||
| @SuppressLint("NewApi") // The caller guards this with an API 26 runtime check before invoking it. | ||
| suspend fun requestWidgetCreation(context: Context): Boolean { |
There was a problem hiding this comment.
This is almost a copy of the one in EntityWidget @jpelgrom I wonder if we should not make this a UseCase.
|
On the UseCase, agree. I'd take more than just the pin call though. The duplicated part is really the whole sequence: API 26 guard, isRequestPinAppWidgetSupported plus the RemoteException catch, requestPinAppWidget with the ACTION_APPWIDGET_CREATED extra, and the getWidgetCountFlow().drop(1).onStart{}.first() wait with the emit(0) on rejection. Only the ComponentName, the widget entity that goes in the extra and which DAO count flow you watch actually differ, so something like suspend fun requestWidgetPin(component: ComponentName, widget: Parcelable, widgetCount: Flow<Int>): Booleancovers both call sites as they are today. Also means the "does not handle user cancellation" warning stops being copy pasted into two KDocs. If the rest of the configure screens in #7028 end up the same shape I'd do it before there's a third copy. Few things I noticed reading c886ff6: isValidSelection() only checks selectedEntityIds.isNotEmpty() now. EntityWidgetConfigureViewModel checks current.selectedEntity != null, so the id has actually resolved against the loaded entities. That matters here because selectedEntities is a mapNotNull over the Loaded map, so an entity that no longer exists on the server renders no row, but getPendingDaoEntity() joins the raw selectedEntityIds and saves it regardless. The user can't see it and has no way to remove it. The isEmpty() check right under it logs "the selected entities are unknown on the server", but it can only fire when every id is blank, so it's the entity widget's message without the entity widget's check. It shows up in the widget too. getEntity() falls back to entities[0] when nothing is playing, so a stale id sitting first makes the whole widget render as unconfigured even when the rest of the selection is fine. Rename a media_player in HA and reopen the config screen to hit it. Also the configure activity is exported for APPWIDGET_CONFIGURE so FOR_ENTITY isn't necessarily coming from us, and that check was what kept a junk id out of the DAO. The tests won't catch it as they are, EntitiesForDisplayManager is mocked to return exactly the entities the test then selects, so resolved and unresolved never differ. Couple of stale docs from the same commit: requestWidgetCreation still says @throws IllegalStateException though it returns Boolean now, and the @SuppressLint("NewApi") comment still says the caller does the API 26 check, which now happens inside the function. @OptIn(ExperimentalCoroutinesApi::class) can go as well, nothing experimental is left after mapLatest went. And on the screenshots, previewEntity1/previewEntity2 are light.test and scene.testing, so the regenerated references still show a light and a scene as the selected media players, same thing you flagged on the PNG thread. There's no reference for the empty state either, which is the bigger visual change now that the config section stays hidden until something is selected. Happy to push all of this plus the UseCase if you want, or if you'd rather do it yourself go ahead, either works. |
I've applied most of your suggestion and pushed it
|
Summary
Resolves #6308.
This migrates
MediaPlayerControlsWidgetConfigureActivityfrom the legacy XML / View-based layout (widget_media_controls_configure.xml,Spinner,CheckBox,MultiAutoCompleteTextView) to Jetpack Compose with the project's Material 3 design-system components (HADropdownMenu,EntityPicker,HATextField,HACheckbox,HAAccentButton,HATheme).MediaPlayerControlsWidgetEntityis produced. Server selection is now driven by Compose state (HADropdownMenu) instead of the oldSpinnercontract.Multi-entity selection
The legacy screen used a
MultiAutoCompleteTextViewwith a comma tokenizer, so a widget can be configured with severalmedia_playerentities and the widget renders whichever one is currently playing. This is preserved: the screen usesEntityPickeras an "add entity" control with the selected entities listed below (each removable), following the favorites pattern inAndroidAutoFavoritesView. All comma-separated entities are restored on load and saved back comma-joined.The screen content was extracted into a stateless, hoisted
MediaPlayerControlsWidgetConfigureScreencomposable (previewable and screenshot-tested in isolation), and the screen logic was moved into aMediaPlayerControlsWidgetConfigureViewModelwith unit tests, matching the convention used by the other Compose screens (#7007). The confirm button is gated on a valid selection.Checklist
Screenshots
Generated via the project's Compose Preview screenshot tests (
MediaPlayerControlsWidgetConfigureScreenshotTest).Any other notes
The screenshot reference images were generated with
./gradlew :app:updateFullDebugScreenshotTestand are committed underapp/src/screenshotTestFullDebug/reference/.