Migrate TemplateWidgetConfigureActivity to Compose#6444
Migrate TemplateWidgetConfigureActivity to Compose#6444amlwin wants to merge 7 commits intohome-assistant:mainfrom
Conversation
Migrates the Template Widget configuration screen from XML and Views to Jetpack Compose. This change replaces `TemplateWidgetConfigureActivity`'s XML layout with a new Composable screen, `TemplateWidgetConfigureView`. A new `TemplateWidgetConfigureViewModel` is introduced to manage the screen's state and business logic, including template rendering and widget creation/updating. This also adds unit tests for the new ViewModel to ensure its correctness.
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TimoPtr
left a comment
TimoPtr
left a comment
There was a problem hiding this comment.
Thanks for your work 🎖️, some comments but overall it looks ok.
Would you mind adding a screenshot test too to your compose screen, it should be rather easy?
| <component name="KotlinCommonCompilerArguments"> | ||
| <option name="apiVersion" value="2.3" /> | ||
| <option name="languageVersion" value="2.3" /> | ||
| </component> |
There was a problem hiding this comment.
This should have not be committed but I'm fine about keeping it since I think it became obsolete.
| * Loads existing widget configuration if editing an existing widget. | ||
| */ | ||
| fun onSetup(widgetId: Int, supportedTextColors: List<String>) { | ||
| if (this.widgetId != AppWidgetManager.INVALID_APPWIDGET_ID) return |
There was a problem hiding this comment.
In which scenario this could happen?
There was a problem hiding this comment.
i'm going to remove that line coz view (activity) already have these check.
if (widgetId == AppWidgetManager.INVALID_APPWIDGET_ID && !requestLauncherSetup) {
finish()
return
}
| templateText = getString( | ||
| if (e.cause is SerializationException) { | ||
| commonR.string.template_error | ||
| } else { | ||
| commonR.string.template_render_error | ||
| }, |
There was a problem hiding this comment.
I think it is a good idea to keep this logic that was giving a bit more details to the user making the template on what is the current error.
… move logic to the ViewModel (home-assistant#6454) * Extract `TemplateWidgetConfigureScreen` and `TemplateWidgetConfigureView` to a separate file * Migrate configuration state (server, template text, text size, background type) to a single `TemplateWidgetConfigureUiState` managed by `MutableStateFlow` * Move widget pinning and update logic from the Activity to the ViewModel * Improve template rendering error handling with a new `TemplateRenderError` enum and better exception catching * Update tests to reflect state management and logic changes * Remove obsolete SDK checks and `Action` sealed class --------- Co-authored-by: Joris Pelgröm <jpelgrom@users.noreply.github.com>
Changes Summary1. TemplateWidgetConfigureViewModel.kt - Major refactor:
2. TemplateWidgetConfigureScreen.kt - New file:
3. TemplateWidgetConfigureActivity.kt - Simplified:
4. TemplateWidgetConfigureViewModelTest.kt - Updated:
cc: @TimoPtr |
* Implement `errorMessage` flow in `TemplateWidgetConfigureViewModel` * Add `SnackbarHost` to `TemplateWidgetConfigureScreen` to display one-shot error events * Update `TemplateWidgetConfigureActivity` to use the ViewModel for error reporting instead of manual Toast calls
…te files * Extract TemplateWidgetConfigureUiState and TemplateRenderError to a new file * Handle CancellationException during template rendering in TemplateWidgetConfigureViewModel
|
@amlwin reopen the PR when you think it is ready for review. |
* Do not catch `CancellationException` in `TemplateWidgetConfigureActivity` to ensure coroutines are cancelled properly * Update `TemplateWidgetConfigureViewModel` to use `Duration` for debounce and switch template rendering to `Dispatchers.Default` * Ensure widget configuration is updated even if a `widgetId` was already set in `onSetup`
look like screenshot is not working properly with current gradle version 
|
* Refactor `TemplateWidgetConfigureScreen` into smaller, more maintainable composables: `ServerSelectionSection`, `TemplateInputSection`, `TemplateRenderResult`, and `WidgetAppearanceSection`. * Replace standard `Button` with `HAAccentButton`. * Improve error handling in `TemplateWidgetConfigureActivity` when updating widget configurations.
There was a problem hiding this comment.
Pull request overview
Migrates the Template widget configuration flow from the legacy XML/view-based TemplateWidgetConfigureActivity to a Jetpack Compose screen backed by a new TemplateWidgetConfigureViewModel, with unit tests added for the ViewModel.
Changes:
- Replaces the XML-based configuration UI with a Compose-based
TemplateWidgetConfigureScreen - Introduces
TemplateWidgetConfigureViewModel+TemplateWidgetConfigureUiStateto drive state, template rendering, and widget persistence - Adds unit tests covering ViewModel setup, template rendering, and saving behavior
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/template/TemplateWidgetConfigureActivity.kt | Switches configuration activity to Compose + ViewModel-driven behavior |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/template/TemplateWidgetConfigureScreen.kt | New Compose UI implementation for the configuration screen |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/template/TemplateWidgetConfigureUiState.kt | New state model + error enum for the screen |
| app/src/main/kotlin/io/homeassistant/companion/android/widgets/template/TemplateWidgetConfigureViewModel.kt | New ViewModel handling setup, rendering, saving, and pinning widget creation |
| app/src/test/kotlin/io/homeassistant/companion/android/widgets/template/TemplateWidgetConfigureViewModelTest.kt | New unit tests for the ViewModel |
| app/src/main/res/layout/widget_template_configure.xml | Removes the legacy XML layout |
| .idea/kotlinc.xml | Updates IDE Kotlin compiler settings |
Files not reviewed (1)
- .idea/kotlinc.xml: Language not supported
| return | ||
| } | ||
| if (!serverManager.isRegistered() || serverManager.getServer(serverId) == null) { | ||
| Timber.w("Not rendering template because server is not set") |
There was a problem hiding this comment.
When the server is not registered or getServer(serverId) is null, renderTemplate returns early without updating renderedTemplate/isTemplateValid. If the user previously had a valid render, the UI can keep showing stale data and keep the action button enabled while the selected server is now invalid. Consider explicitly setting isTemplateValid = false (and clearing renderedTemplate/setting an error) before returning.
| Timber.w("Not rendering template because server is not set") | |
| Timber.w("Not rendering template because server is not set") | |
| _uiState.update { | |
| it.copy( | |
| renderedTemplate = null, | |
| isTemplateValid = false, | |
| templateRenderError = TemplateRenderError.RENDER_ERROR, | |
| ) | |
| } |
| if (e is SerializationException) { | ||
| Timber.e(e, "Template syntax error") | ||
| } else { | ||
| Timber.e(e, "Error rendering template") | ||
| } | ||
| val errorRendered = if (e.cause is SerializationException) { | ||
| TemplateRenderError.TEMPLATE_ERROR | ||
| } else { | ||
| TemplateRenderError.RENDER_ERROR | ||
| } | ||
| _uiState.update { |
There was a problem hiding this comment.
The template error classification is inconsistent: the log branch checks e is SerializationException, but the error mapping uses e.cause is SerializationException. This can misclassify template syntax errors depending on where the SerializationException is wrapped. Align both checks to the same condition (matching the previous behavior that looked at the cause).
| @OptIn(FlowPreview::class) | ||
| private fun startTemplateRendering() { | ||
| templateRenderingJob?.cancel() | ||
| templateRenderingJob = viewModelScope.launch { | ||
| combine( | ||
| _uiState.mapField { it.templateText }, | ||
| _uiState.mapField { it.selectedServerId }, | ||
| ) { template, serverId -> template to serverId } | ||
| .debounce(RENDER_DEBOUNCE_MS.milliseconds) | ||
| .collect { (template, serverId) -> | ||
| renderTemplate(template, serverId) | ||
| } | ||
| } |
There was a problem hiding this comment.
startTemplateRendering() uses .collect { renderTemplate(...) }, so if rendering is slow (network) and the user changes input again, older renders will still complete and can overwrite newer results. Using collectLatest (or cancelling in-flight renders) avoids stale/out-of-order UI updates.
| serverId = state.selectedServerId, | ||
| template = state.templateText, | ||
| textSize = state.textSize.toFloatOrNull() ?: DEFAULT_TEXT_SIZE, | ||
| lastUpdate = "Loading", |
There was a problem hiding this comment.
getPendingDaoEntity() always sets lastUpdate = "Loading". Previously, updating an existing widget preserved the prior lastUpdate from the DAO; resetting it here will temporarily replace the displayed widget content with a loading placeholder until the next widget update/render.
| lastUpdate = "Loading", | |
| lastUpdate = state.renderedTemplate ?: "Loading", |
There was a problem hiding this comment.
This comment is kinda weird because the previous code was dao.get(appWidgetId)?.lastUpdate ?: "Loading", please check
| import androidx.annotation.VisibleForTesting | ||
| import androidx.core.text.HtmlCompat | ||
| import androidx.lifecycle.ViewModel | ||
| import androidx.lifecycle.viewModelScope | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||
| import io.homeassistant.companion.android.database.widget.TemplateWidgetDao | ||
| import io.homeassistant.companion.android.database.widget.TemplateWidgetEntity | ||
| import io.homeassistant.companion.android.database.widget.WidgetBackgroundType | ||
| import io.homeassistant.companion.android.widgets.ACTION_APPWIDGET_CREATED | ||
| import io.homeassistant.companion.android.widgets.BaseWidgetProvider.Companion.UPDATE_WIDGETS | ||
| import io.homeassistant.companion.android.widgets.EXTRA_WIDGET_ENTITY | ||
| import javax.inject.Inject | ||
| import kotlin.coroutines.cancellation.CancellationException | ||
| import kotlin.time.Duration | ||
| import kotlin.time.Duration.Companion.milliseconds | ||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.Dispatchers |
There was a problem hiding this comment.
There are several unused imports in this file (e.g., VisibleForTesting, Duration, CoroutineDispatcher). These typically get flagged by ktlint/IDE inspections; please remove unused imports to keep the file clean and avoid CI failures if unused-import rules are enforced.
| private suspend fun onUpdateWidget() { | ||
| try { | ||
| viewModel.updateWidgetConfiguration(this@TemplateWidgetConfigureActivity) | ||
| setResult(RESULT_OK) | ||
| finish() | ||
| } catch (e: IllegalStateException) { |
There was a problem hiding this comment.
When finishing widget configuration (non-launcher flow), the result should include AppWidgetManager.EXTRA_APPWIDGET_ID so the launcher/host knows which widget was configured. Previously this was handled by BaseWidgetConfigureActivity.updateWidget(); now only setResult(RESULT_OK) is returned, which can cause the widget host to treat configuration as failed.
There was a problem hiding this comment.
Good catch 👍🏻, I'm not sure it is needed in the TodoWidgetConfigure because we use Glance and we manually call the update method from the manager.
| private val supportedTextColors: List<String> | ||
| get() = listOf( | ||
| application.getHexForColor(commonR.color.colorWidgetButtonLabelBlack), | ||
| application.getHexForColor(android.R.color.white), | ||
| ) |
There was a problem hiding this comment.
TemplateWidgetConfigureView shows text color choices in the order Black then White, and the Activity passes supportedTextColors in the same order. In the previous XML, the White option was checked by default for transparent widgets; with the new setup the default becomes Black (index 0). If this is unintended, reorder the options or default textColorIndex to preserve the prior default behavior.
| private fun onActionClick(requestLauncherSetup: Boolean) { | ||
| lifecycleScope.launch { | ||
| if (requestLauncherSetup) { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| lifecycleScope.launch { | ||
| requestWidgetCreation() | ||
| } | ||
| } else { | ||
| showAddWidgetError() // this shouldn't be possible | ||
| } | ||
| requestPinWidget() | ||
| } else { | ||
| lifecycleScope.launch { | ||
| updateWidget() | ||
| } | ||
| onUpdateWidget() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun onServerSelected(serverId: Int) = renderTemplateText() | ||
|
|
||
| override suspend fun getPendingDaoEntity(): TemplateWidgetEntity { | ||
| val serverId = checkNotNull(selectedServerId) { "Selected server ID is null" } | ||
| val template = checkNotNull(binding.templateText.text?.toString()) { "Template text is null" } | ||
|
|
||
| return TemplateWidgetEntity( | ||
| id = appWidgetId, | ||
| serverId = serverId, | ||
| template = template, | ||
| textSize = binding.textSize.text.toString().toFloat(), | ||
| backgroundType = when (binding.backgroundType.selectedItem as String?) { | ||
| getString(commonR.string.widget_background_type_dynamiccolor) -> WidgetBackgroundType.DYNAMICCOLOR | ||
| getString(commonR.string.widget_background_type_transparent) -> WidgetBackgroundType.TRANSPARENT | ||
| else -> WidgetBackgroundType.DAYNIGHT | ||
| }, | ||
| textColor = if (binding.backgroundType.selectedItem as String? == | ||
| getString(commonR.string.widget_background_type_transparent) | ||
| ) { | ||
| getHexForColor( | ||
| if (binding.textColorWhite.isChecked) { | ||
| android.R.color.white | ||
| } else { | ||
| commonR.color.colorWidgetButtonLabelBlack | ||
| }, | ||
| ) | ||
| } else { | ||
| null | ||
| }, | ||
| lastUpdate = dao.get(appWidgetId)?.lastUpdate ?: "Loading", | ||
| ) | ||
| } | ||
|
|
||
| override val widgetClass: Class<*> = TemplateWidget::class.java | ||
|
|
||
| private fun renderTemplateText() { | ||
| val editableText = binding.templateText.text ?: return | ||
| if (editableText.isNotEmpty()) { | ||
| renderTemplateText(editableText.toString()) | ||
| } else { | ||
| binding.renderedTemplate.text = getString(commonR.string.empty_template) | ||
| binding.addButton.isEnabled = false | ||
| private suspend fun requestPinWidget() { | ||
| try { | ||
| viewModel.requestWidgetCreation(this@TemplateWidgetConfigureActivity) | ||
| finish() |
There was a problem hiding this comment.
requestPinWidget() is called without an API 26+ guard, but AppWidgetManager.requestPinAppWidget only exists on O+. On pre-O devices this path can crash with NoSuchMethodError. Add a Build.VERSION.SDK_INT >= Build.VERSION_CODES.O check (as other widget configure activities do) or mark the call site @RequiresApi(26) and ensure it’s never invoked below 26.
| private suspend fun requestPinWidget() { | ||
| try { | ||
| viewModel.requestWidgetCreation(this@TemplateWidgetConfigureActivity) | ||
| finish() | ||
| } catch (e: IllegalStateException) { | ||
| if (e is CancellationException) throw e | ||
| showAddWidgetError() | ||
| } | ||
| } | ||
|
|
||
| private fun renderTemplateText(template: String) { | ||
| val serverId = selectedServerId | ||
| if (serverId == null) { | ||
| Timber.w("Not rendering template because server is not set") | ||
| return | ||
| private suspend fun onUpdateWidget() { | ||
| try { | ||
| viewModel.updateWidgetConfiguration(this@TemplateWidgetConfigureActivity) | ||
| setResult(RESULT_OK) | ||
| finish() | ||
| } catch (e: IllegalStateException) { | ||
| if (e is CancellationException) throw e | ||
| showAddWidgetError() | ||
| } |
There was a problem hiding this comment.
Both requestPinWidget() and onUpdateWidget() catch IllegalStateException and then check if (e is CancellationException), but e can never be a CancellationException due to the catch type. Either catch Exception (and rethrow CancellationException) or remove the unreachable cancellation check.
There was a problem hiding this comment.
Remove the if no need if you only catch IllegalStateException. The cancelation is already thrown
It never really worked in the IDE but it works when you execute the update screenshot task. |
| val flags = PendingIntent.FLAG_MUTABLE | ||
| appWidgetManager?.requestPinAppWidget( | ||
| ComponentName(context, TemplateWidget::class.java), | ||
| null, | ||
| PendingIntent.getBroadcast( | ||
| context, | ||
| System.currentTimeMillis().toInt(), | ||
| Intent(context, TemplateWidget::class.java).apply { | ||
| action = ACTION_APPWIDGET_CREATED | ||
| putExtra(EXTRA_WIDGET_ENTITY, pendingEntity) | ||
| }, | ||
| flags, | ||
| ), |
There was a problem hiding this comment.
| val flags = PendingIntent.FLAG_MUTABLE | |
| appWidgetManager?.requestPinAppWidget( | |
| ComponentName(context, TemplateWidget::class.java), | |
| null, | |
| PendingIntent.getBroadcast( | |
| context, | |
| System.currentTimeMillis().toInt(), | |
| Intent(context, TemplateWidget::class.java).apply { | |
| action = ACTION_APPWIDGET_CREATED | |
| putExtra(EXTRA_WIDGET_ENTITY, pendingEntity) | |
| }, | |
| flags, | |
| ), | |
| appWidgetManager?.requestPinAppWidget( | |
| ComponentName(context, TemplateWidget::class.java), | |
| null, | |
| PendingIntent.getBroadcast( | |
| context, | |
| System.currentTimeMillis().toInt(), | |
| Intent(context, TemplateWidget::class.java).apply { | |
| action = ACTION_APPWIDGET_CREATED | |
| putExtra(EXTRA_WIDGET_ENTITY, pendingEntity) | |
| }, | |
| PendingIntent.FLAG_MUTABLE, | |
| ), |
TimoPtr
left a comment
TimoPtr
left a comment
There was a problem hiding this comment.
If you are using AI I think that for this final step you should do it yourself to make sure to fully get my comments.
| } catch (e: IllegalStateException) { | ||
| if (e is CancellationException) throw e |
There was a problem hiding this comment.
| } catch (e: IllegalStateException) { | |
| if (e is CancellationException) throw e | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: IllegalStateException) { |
This is way we use in the repo, please adjust in the activity and the viewModel
| } else { | ||
| showAddWidgetError() // this shouldn't be possible | ||
| } | ||
| requestPinWidget() |
There was a problem hiding this comment.
Why not checking the version anymore? If you do so it would remove the warning in the ViewModel by properly annotating the function requestPinWidget like it is done in TodoWidgetConfigure.
| private suspend fun onUpdateWidget() { | ||
| try { | ||
| viewModel.updateWidgetConfiguration(this@TemplateWidgetConfigureActivity) | ||
| setResult(RESULT_OK) | ||
| finish() | ||
| } catch (e: IllegalStateException) { |
There was a problem hiding this comment.
Good catch 👍🏻, I'm not sure it is needed in the TodoWidgetConfigure because we use Glance and we manually call the update method from the manager.
| binding.addButton.isEnabled = enabled && isValidServerId() | ||
| } | ||
| private fun showAddWidgetError() { | ||
| viewModel.showError(commonR.string.widget_creation_error) |
There was a problem hiding this comment.
The error is always the same and always trigger from the activity I propose you to not use the viewModel and directly send this to the screen, it would simplify things.
| } | ||
|
|
||
| @Composable | ||
| private fun TemplateWidgetConfigureView( |
There was a problem hiding this comment.
| private fun TemplateWidgetConfigureView( | |
| @VisibleForTesting | |
| internal fun TemplateWidgetConfigureScreen( |
Ideally we should write a compose screen test https://github.com/home-assistant/android/blob/457de6f8896328e6ebebeb5f3db8a01a39e7168f/app/src/test/kotlin/io/homeassistant/companion/android/onboarding/sethomenetwork/SetHomeNetworkScreenTest.kt for inspiration
| return | ||
| } | ||
| if (!serverManager.isRegistered() || serverManager.getServer(serverId) == null) { | ||
| Timber.w("Not rendering template because server is not set") |
There was a problem hiding this comment.
Maybe you could set an Error here in the ViewState otherwise the user wouldn't see anything.
| val rendered = HtmlCompat.fromHtml(result, HtmlCompat.FROM_HTML_MODE_LEGACY) | ||
| ?.toString() | ||
| ?.trimEnd() | ||
| ?: result |
There was a problem hiding this comment.
| val rendered = HtmlCompat.fromHtml(result, HtmlCompat.FROM_HTML_MODE_LEGACY) | |
| ?.toString() | |
| ?.trimEnd() | |
| ?: result | |
| val rendered = HtmlCompat.fromHtml(result, HtmlCompat.FROM_HTML_MODE_LEGACY).toString().trimEnd() |
If you check the doc from fromHtml it won't ever return null. Any reason to do otherwise?
| if (e is CancellationException) { | ||
| throw e | ||
| } | ||
| if (e is SerializationException) { |
There was a problem hiding this comment.
Put this in a dedicated catch block above this catch.
| serverId = state.selectedServerId, | ||
| template = state.templateText, | ||
| textSize = state.textSize.toFloatOrNull() ?: DEFAULT_TEXT_SIZE, | ||
| lastUpdate = "Loading", |
There was a problem hiding this comment.
This comment is kinda weird because the previous code was dao.get(appWidgetId)?.lastUpdate ?: "Loading", please check
| /** | ||
| * Emit a one-shot error event to be displayed as a Snackbar. | ||
| * | ||
| * @param messageResId the string resource ID for the error message | ||
| */ | ||
| fun showError(messageResId: Int) { | ||
| viewModelScope.launch { _errorMessage.emit(messageResId) } | ||
| } |
There was a problem hiding this comment.
| /** | |
| * Emit a one-shot error event to be displayed as a Snackbar. | |
| * | |
| * @param messageResId the string resource ID for the error message | |
| */ | |
| fun showError(messageResId: Int) { | |
| viewModelScope.launch { _errorMessage.emit(messageResId) } | |
| } |
I'll check in this weekend |
@amlwin If you merge main in your branch and update your Android Studio to the latest version then it is going to work. It has been fixed this week 🎉 |
Summary
Migrates the Template Widget configuration screen from XML and Views to Jetpack Compose. This change replaces
TemplateWidgetConfigureActivity's XML layout with a new Composable screen,TemplateWidgetConfigureView.A new
TemplateWidgetConfigureViewModelis introduced to manage the screen's state and business logic, including template rendering and widget creation/updating. This also adds unit tests for the new ViewModel to ensure its correctness.Checklist
Screenshots
No Ui changes
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#