-
-
Notifications
You must be signed in to change notification settings - Fork 861
Multi-action grid widget implementation #4783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mrdanielps
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!
8fa697b to
c3eb2e3
Compare
|
Looking great! Will the state of each item reflect in realtime? for example if the light is ON |
Not yet. As mentioned in the additional notes, I limited the scope of this PR for the initial review, just to get some feedback. Although looking at the entity widget implementation, I see it should be pretty straightforward to add, so I'll look into as soon as I can. |
|
How often can a widget update in Android? In iOS the minimum auto-refresh is 15 minutes (besides other techniques like updating through push notification trigger) |
|
It's actually quite similar on Android. The fastest auto-refresh is 30 minutes, but AFAIK anything that wakes up the app can manually trigger an update, like broadcasts or push notifications. Widget interactions can also update it, so pressing the button will allow for the state to be updated immediately. |
|
The template widget updates instantly for me, there is no delay there. Our Template widget and entity state widget use a websocket subscription to update instantly. |
|
@dshokouhi so you keep a websocket connection all the time for the widget? 🪫😭 |
For widgets only when the screen turns on, then we stop the connection when the screen turns off. Its our best effort to keep them as up to as possible while not draining too much battery
yup same for local push we never stop the connection and also send a ping to the server every 30 seconds to ensure its active and current. |
Done! Screen_recording_20241111_005218.webm |
|
Gave this PR a quick test run. I think you have captured the obvious missing features like auto complete, ideally the behavior for each action should mimic the existing button widget with how fields are populated. I think that will match general user expectation. For example that widget handles when a custom integration provides incomplete action data and also when a user provides invalid action data. Looking forward to seeing this PR progress! |
7b0edda to
0257657
Compare
I see, I was thinking of implementing it like the entity widget, which only takes an ID, but I'll certainly take that into account. |
|
There is no progress in this? 🫠 It's a really interesting improvement |
|
Wish this could be merged.... |
|
Sorry for the long wait! I hope to have it ready soon enough, but for those interested, here's a sneak peek. glance-grid-widget.mp4 |
|
This is awesome! Also like the idea of the reload button to prevent excessive connections to check status. Can't wait! |
|
Any update on this? Would love to see it merged. |
72731f6 to
e0b9479
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a multi-action grid widget for the Home Assistant Companion Android app. The widget allows users to configure multiple Home Assistant entity actions in a responsive grid layout that adapts based on widget size and becomes scrollable when necessary.
Changes:
- Adds complete database layer for grid widget (Entity, DAO, TypeConverters)
- Implements Jetpack Glance-based widget UI with responsive layout
- Provides configuration UI using Compose for widget setup
- Includes comprehensive unit and screenshot tests
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| GridWidgetEntity.kt | Database entity for storing grid widget configuration |
| GridWidgetDao.kt | Room DAO interface for grid widget database operations |
| GridWidgetItemConverter.kt | Type converter for serializing/deserializing widget items |
| AppDatabase.kt | Updated database to include grid widget table |
| DatabaseModule.kt | Added Hilt provider for GridWidgetDao |
| GridConfiguration.kt | Data models for widget configuration |
| GridConfigurationViewModel.kt | ViewModel managing widget configuration state |
| GridWidgetConfigureActivity.kt | Activity for widget configuration |
| GridConfigurationScreen.kt | Compose UI for widget configuration |
| GridWidget.kt | Widget receiver implementing BaseGlanceEntityWidgetReceiver |
| GridGlanceAppWidget.kt | Glance-based widget UI implementation with responsive layout |
| GridWidgetState.kt | State models for widget UI |
| GridWidgetStateUpdater.kt | Manages widget state updates from entity changes |
| GridWidgetActions.kt | Glance actions for entity press and refresh |
| GridWidgetUtils.kt | Utility functions for converting between models |
| Tests | Comprehensive unit and UI tests for all components |
| Manifest & Resources | Widget registration and string resources |
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridWidgetState.kt
Fixed
Show fixed
Hide fixed
...ain/kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationScreen.kt
Fixed
Show fixed
Hide fixed
...ain/kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationScreen.kt
Fixed
Show fixed
Hide fixed
...ain/kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationScreen.kt
Fixed
Show fixed
Hide fixed
...ain/kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationScreen.kt
Fixed
Show fixed
Hide fixed
...ain/kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationScreen.kt
Fixed
Show fixed
Hide fixed
Adds a new widget that allows to configure multiple actions. Fixes home-assistant#1193 home-assistant#4549
Fixes lint issue.
e0b9479 to
208aeed
Compare
|
I've rebased the branch and added tests, so the PR should now be ready for review. I've also updated the lint baseline to ignore some warnings relating to stability of lists in composables which should be safe. However, I can correct them if needed. |
| column="21"/> | ||
| </issue> | ||
|
|
||
| <issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's address the issue not ignoring them.
| @Composable | ||
| private fun LoadingScreen() { | ||
| Column( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| modifier = GlanceModifier.gridWidgetBackground().semantics { testTag = "LoadingScreen" }, | ||
| ) { | ||
| CircularProgressIndicator( | ||
| color = GlanceTheme.colors.primary, | ||
| modifier = GlanceModifier.size(HomeAssistantGlanceTheme.dimensions.iconSize), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid duplication this since it's a duplicate of the TODOWidget and create a shared GlanceLoadingScreen.
| CircleIconButton( | ||
| imageProvider = ImageProvider(R.drawable.ic_refresh), | ||
| contentDescription = glanceStringResource(commonR.string.refresh), | ||
| onClick = actionRefreshGrid(), | ||
| backgroundColor = null, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above you could reuse the same as the one in TODO widget, it would also be helpful to add the sync_problem state that is quite important, since today the WebSocket sometimes is not working well, it helps to know when the state is not up to date.
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/widgets/grid/GridGlanceAppWidget.kt
Show resolved
Hide resolved
| .getEntities() | ||
| .orEmpty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we accept all kind of entities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we make it possible to attach an action to an element (another PR 🙏🏻 ) we could simply display them a bit differently that makes it clear that it is not clickable.
|
|
||
| fun onSetup(widgetId: Int) { | ||
| if (this.widgetId == AppWidgetManager.INVALID_APPWIDGET_ID) { | ||
| loadPreviousState(widgetId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the TodoConfigureViewModel we do use a Mutex to ensure that the thread safety, we probably should apply some here too. I didn't look into all the details of the VM here, but most probably you have multiple sources touching the same thing, you want to avoid race condition as much as possible.
| fun setServer(serverId: Int) = _gridConfig.update { currentConfig -> | ||
| currentConfig.copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could simply check if the sever is the same in that case return the currentConfig instead of making a new copy?
.../kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationViewModel.kt
Show resolved
Hide resolved
.../kotlin/io/homeassistant/companion/android/widgets/grid/config/GridConfigurationViewModel.kt
Show resolved
Hide resolved
...kotlin/io/homeassistant/companion/android/widgets/grid/config/GridWidgetConfigureActivity.kt
Show resolved
Hide resolved
| import kotlinx.coroutines.launch | ||
|
|
||
| @AndroidEntryPoint | ||
| class GridWidgetConfigureActivity : BaseActivity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to start this activity from the Settings, like all the other widgets.
...kotlin/io/homeassistant/companion/android/widgets/grid/config/GridWidgetConfigureActivity.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| android:minWidth="40dp" | ||
| android:minHeight="40dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...src/test/kotlin/io/homeassistant/companion/android/widgets/grid/GridWidgetPressActionTest.kt
Show resolved
Hide resolved
app/src/test/kotlin/io/homeassistant/companion/android/widgets/grid/GridWidgetStateTest.kt
Show resolved
Hide resolved
| import org.junit.jupiter.api.Assertions.assertTrue | ||
| import org.junit.jupiter.api.Test | ||
|
|
||
| class GridWidgetStateUpdaterTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like some test
- to test the error scenarios.
- test that verify that we are not updating more than we need when things doesn't change and verify the number of interaction we make with the integrationRepo to avoid making call that we shouldn't.
- Making multiple update of the same entity is making the right amount of call on the integrationRepo (to verify that the collect within the transformLatest is properly canceled it's important if one day we change the operator to have this test).
| <issue | ||
| id="UnusedResources" | ||
| message="The resource `R.xml.grid_widget_info` appears to be unused" | ||
| errorLine1="<appwidget-provider xmlns:android="http://schemas.android.com/apk/res/android"" | ||
| errorLine2="^"> | ||
| <location | ||
| file="${:automotive*fullDebug*MAIN*sourceProvider*0*resDir*1}/xml/grid_widget_info.xml" | ||
| line="2" | ||
| column="1"/> | ||
| </issue> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't have this here. Could you revert the changes it should be used, like the other widgets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not referenced in the automotive manifest, so I'm guessing that's why it complains about the unused resource.
I can add it to the grid_widget_info.xml like in the TODO widget:
tools:ignore="UnusedAttribute,UnusedResources"I wonder if auto can benefit from this widget though.
common/src/main/kotlin/io/homeassistant/companion/android/database/AppDatabase.kt
Show resolved
Hide resolved
TimoPtr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you contribution it's a a very nice improvements 💪🏻 and I'm sure many people are waiting for it (including myself)
It would be nice that when we click on an action only the action show the loading animation.
Few other things to fix on top of the comments
- Add an entry in the changelog xml file since it's a nice new feature
- Support empty content
- Support out of sync or sync failure
common/src/main/kotlin/io/homeassistant/companion/android/database/widget/GridWidgetDao.kt
Show resolved
Hide resolved
common/src/main/kotlin/io/homeassistant/companion/android/database/widget/GridWidgetDao.kt
Show resolved
Hide resolved
| val label: String?, | ||
| @ColumnInfo(name = "items") | ||
| val items: List<Item>, | ||
| ) : WidgetEntity<GridWidgetEntity> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) : WidgetEntity<GridWidgetEntity> { | |
| ) : WidgetEntity<GridWidgetEntity>, ThemeableWidgetEntity { |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |



Summary
Adds a new widget that allows the user to configure multiple actions in a grid. It's somewhat based on the existing ButtonWidget.
The widget is responsive, adjusting the number of columns based on size, and becomes scrollable when it doesn't fit all the actions.
Fixes #1193, #4549
Screenshots
Dynamic colors - light

Dynamic colors - dark

API <31

Configuration and resizing
Screen_recording_20241102_185549.mp4
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#1132
Any other notes
I've limited the scope of this PR for easier review, hence the draft status, but I will update it based on feedback.
The main features I think are necessary before merging are auto-completion for icons and action data, feedback when the button is pressed, and maybe entity states.
Additionally, the existing button widget could be updated to reflect the UI changes and share some of their code. However, I didn't want to change too much considering a proposal exists to refactor this code (#4640).