Support multiple Template tiles on Wear OS

[slovdahl]

Summary

Support any number of Template tiles in the Wear OS app. Fixes #3291.

Screenshots

Template tiles settings view with 2 tiles:

Companion app:

Wear app:

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

[slovdahl]

Very very early draft, the code is not even compiling right now. But happy to get any kind of feedback anyway 🙂

[slovdahl]

While testing this locally, I somehow ended up with stale data in the HA app 🤔 I removed some Template tiles from the Wear OS tile carousel, but some of them remained in the Home Assistant app data, and hence are visible in the list of Template tiles in the settings. @marazmarci did you see anything like this when implementing support for multiple shortcuts tiles?

[marazmarci]

While testing this locally, I somehow ended up with stale data in the HA app 🤔 I removed some Template tiles from the Wear OS tile carousel, but some of them remained in the Home Assistant app data, and hence are visible in the list of Template tiles in the settings. @marazmarci did you see anything like this when implementing support for multiple shortcuts tiles?

Yes, I encountered this when I was in the middle of development, and the app was crashing when TileService::onTileRemoveEvent was called, so it couldn't remove the tile from the DB 😛

Other than this, I didn't encounter such issues. However, I can imagine that Wear OS may give up calling onTileRemoveEvent in low-memory situations.

[marazmarci]

While testing this locally, I somehow ended up with stale data in the HA app 🤔 I removed some Template tiles from the Wear OS tile carousel, but some of them remained in the Home Assistant app data, and hence are visible in the list of Template tiles in the settings. @marazmarci did you see anything like this when implementing support for multiple shortcuts tiles?

Yes, I encountered this when I was in the middle of development, and the app was crashing when TileService::onTileRemoveEvent was called, so it couldn't remove the tile from the DB 😛

Other than this, I didn't encounter such issues. However, I can imagine that Wear OS may give up calling onTileRemoveEvent in low-memory situations.

To make users able to fix a potential issue like this themselves, maybe we could provide an option to force remove a Template/Shortcut Tile 🤔

[slovdahl]

Yes, I encountered this when I was in the middle of development, and the app was crashing when TileService::onTileRemoveEvent was called, so it couldn't remove the tile from the DB 😛

I still don't understand how it happened for me. It worked for one of the tiles I removed but the other one was left in place 🤷 AFAICT the onTileRemoveEvent method was properly implemented at the time at least.

To make users able to fix a potential issue like this themselves, maybe we could provide an option to force remove a Template/Shortcut Tile 🤔

Yeah, that could be an option 👍

I suppose there is no way to query the currently active tiles? Or could we through some other side-channel keep track of whether a tile is in use or not? 🤔

For Template tiles with a refresh interval set, we could actually keep track of it. But maybe it's overkill, and an option to manually force remove a tile would be better. But still not easy and obvious, how would a user know from the HA Template tile config whether a tile is in use or not? It would be quite easy to remove a tile config for an active tile, and that means that tile is in some kind of limbo state.

[jpelgrom]

Good to know: a new camera tile with support for multiple instances has been merged into the app. As a result there is a merge conflict, but also things like a single launcher activity for the tile settings that would be good to adopt.

[jpelgrom]

Another good to know: the Wear app switched to Material 3 components. While your current build won't fail, please update UI composables to M3 components when updating 🙏

[slovdahl]

Thanks for the heads up 👍 I haven't given up on this, I've just been very short on time and energy lately. If someone else feels like picking this up while I'm dragging my feet, I won't mind. Otherwise I'll try to get it forward in the coming weeks.

[jpelgrom]

Regarding the stale tile discussion: I recently was debugging for issues with the camera tile and found that the current setup with serviceScope + launch in add/remove functions could result in the system destroying the service very quickly, before adding/removing was completed. As a workaround, and because the system plays an animation anyway, we've changed this to runBlocking to ensure the state stays correct. Probably best to implement it here too. See #3974 for an example.

[slovdahl]

Regarding the stale tile discussion: I recently was debugging for issues with the camera tile and found that the current setup with serviceScope + launch in add/remove functions could result in the system destroying the service very quickly, before adding/removing was completed. As a workaround, and because the system plays an animation anyway, we've changed this to runBlocking to ensure the state stays correct. Probably best to implement it here too. See #3974 for an example.

Wow, nice, thank you for that! 👍 This was the thing that made getting this PR over the goal line feel hard or impossible. I'll pick it up again now 🙂

[slovdahl]

Good to know: a new camera tile with support for multiple instances has been merged into the app. As a result there is a merge conflict, but also things like a single launcher activity for the tile settings that would be good to adopt.

Fixed in bee0a79 ✔️

Another good to know: the Wear app switched to Material 3 components. While your current build won't fail, please update UI composables to M3 components when updating 🙏

Fixed in f39f1b4 ✔️

Regarding the stale tile discussion: I recently was debugging for issues with the camera tile and found that the current setup with serviceScope + launch in add/remove functions could result in the system destroying the service very quickly, before adding/removing was completed. As a workaround, and because the system plays an animation anyway, we've changed this to runBlocking to ensure the state stays correct. Probably best to implement it here too. See #3974 for an example.

Fixed in 7ef7a61 ✔️

[dshokouhi]

Had a chance to run a test on the debug APK and the feature is working as expected. Adding tiles updates the phone and watch and templates and options are reflected in teh appropriate location.

[dshokouhi]

Lets just make sure to clean up the TODO comment and also let us know if you had checked the one existing data case mentioned.

[slovdahl]

Had a chance to run a test on the debug APK and the feature is working as expected. Adding tiles updates the phone and watch and templates and options are reflected in teh appropriate location.

Thanks 👍

Lets just make sure to clean up the TODO comment and also let us know if you had checked the one existing data case mentioned.

While testing this I think I found another problem. After upgrading both apps, if the phone settings are opened before an existing template tile on the watch has been refreshed at least once the phone settings will hang because of a crash on the wear side:

    2023-11-06 22:01:19.162  9210-9210  AndroidRuntime          io....stant.companion.android.debug  E  FATAL EXCEPTION: main
      Process: io.homeassistant.companion.android.debug, PID: 9210
      com.fasterxml.jackson.databind.JsonMappingException: Null key for a Map not allowed in JSON (use a converting NullKeySerializer?) (through reference chain: kotlin.collections.builders.MapBuilder["null"])
      	at com.fasterxml.jackson.databind.SerializerProvider.mappingException(SerializerProvider.java:1358)
      	at com.fasterxml.jackson.databind.SerializerProvider.reportMappingProblem(SerializerProvider.java:1254)
      	at com.fasterxml.jackson.databind.ser.impl.FailingSerializer.serialize(FailingSerializer.java:32)
      	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:791)
      	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
      	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
      	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
      	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:480)
      	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:319)
      	at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4568)
      	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3821)
      	at io.homeassistant.companion.android.phone.PhoneSettingsListener$sendPhoneData$1.invokeSuspend(PhoneSettingsListener.kt:93)
      	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
      	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
      	at android.os.Handler.handleCallback(Handler.java:942)
      	at android.os.Handler.dispatchMessage(Handler.java:99)
      	at android.os.Looper.loopOnce(Looper.java:201)
      	at android.os.Looper.loop(Looper.java:288)
      	at android.app.ActivityThread.main(ActivityThread.java:7898)
      	at java.lang.reflect.Method.invoke(Native Method)
      	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
      	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)
      	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@a9ddef7, Dispatchers.Main]

I guess it fails because we have not yet been able to persist the tile ID for the existing template tile, and we cannot serialize a map with a null key to JSON.

I managed to work around the initial problem by using "null" (the string) as key, similar to how WearPrefsRepositoryImpl does internally. But that just opened a new problem instead:

FATAL EXCEPTION: main
Process: io.homeassistant.companion.android.debug, PID: 12498
java.lang.IllegalArgumentException: Navigation destination that matches request NavDeepLinkRequest{ uri=android-app://androidx.navigation/Template/null } cannot be found in the navigation graph ComposeNavGraph(0x0) startDestination={Destination(0xf6edc9ca) route=Landing}
	at androidx.navigation.NavController.navigate(NavController.kt:1814)
	at androidx.navigation.NavController.navigate(NavController.kt:2220)
	at androidx.navigation.NavController.navigate$default(NavController.kt:2215)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearHomeViewKt$LoadSettingsHomeView$1$1$3$1.invoke(SettingsWearHomeView.kt:63)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearHomeViewKt$LoadSettingsHomeView$1$1$3$1.invoke(SettingsWearHomeView.kt:60)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearTemplateTileListKt$SettingsWearTemplateTileList$2$1$1.invoke(SettingsWearTemplateTileList.kt:47)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearTemplateTileListKt$SettingsWearTemplateTileList$2$1$1.invoke(SettingsWearTemplateTileList.kt:42)
	at io.homeassistant.companion.android.settings.views.SettingsRowKt$SettingsRow$1$1.invoke(SettingsRow.kt:38)
	at io.homeassistant.companion.android.settings.views.SettingsRowKt$SettingsRow$1$1.invoke(SettingsRow.kt:38)
	at androidx.compose.foundation.ClickablePointerInputNode$pointerInput$3.invoke-k-4lQ0M(Clickable.kt:895)
	at androidx.compose.foundation.ClickablePointerInputNode$pointerInput$3.invoke(Clickable.kt:889)
	at androidx.compose.foundation.gestures.TapGestureDetectorKt$detectTapAndPress$2$1.invokeSuspend(TapGestureDetector.kt:255)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTaskKt.resume(DispatchedTask.kt:179)

I guess we can handle that too by changing the argument type for SettingsWearTemplateTile from NavType.IntType to NavType.StringType`.. Though I didn't manage to get it working right away

FATAL EXCEPTION: main
Process: io.homeassistant.companion.android.debug, PID: 14435
java.lang.IllegalArgumentException: Wrong argument type for 'tileId' in argument bundle. string expected.
	at androidx.navigation.NavDestination.addInDefaultArgs(NavDestination.kt:602)
	at androidx.navigation.NavController.navigate(NavController.kt:1805)
	at androidx.navigation.NavController.navigate(NavController.kt:2220)
	at androidx.navigation.NavController.navigate$default(NavController.kt:2215)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearHomeViewKt$LoadSettingsHomeView$1$1$3$1.invoke(SettingsWearHomeView.kt:63)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearHomeViewKt$LoadSettingsHomeView$1$1$3$1.invoke(SettingsWearHomeView.kt:60)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearTemplateTileListKt$SettingsWearTemplateTileList$2$1$1.invoke(SettingsWearTemplateTileList.kt:47)
	at io.homeassistant.companion.android.settings.wear.views.SettingsWearTemplateTileListKt$SettingsWearTemplateTileList$2$1$1.invoke(SettingsWearTemplateTileList.kt:42)
	at io.homeassistant.companion.android.settings.views.SettingsRowKt$SettingsRow$1$1.invoke(SettingsRow.kt:38)
	at io.homeassistant.companion.android.settings.views.SettingsRowKt$SettingsRow$1$1.invoke(SettingsRow.kt:38)
	at androidx.compose.foundation.ClickablePointerInputNode$pointerInput$3.invoke-k-4lQ0M(Clickable.kt:895)
	at androidx.compose.foundation.ClickablePointerInputNode$pointerInput$3.invoke(Clickable.kt:889)
	at androidx.compose.foundation.gestures.TapGestureDetectorKt$detectTapAndPress$2$1.invokeSuspend(TapGestureDetector.kt:255)

But this does not feel like a sustainable path forward overall 🤔 Any ideas? Could we somehow trigger a refresh of the tiles after updating the app so we minimize the window for this to happen?

[dshokouhi]

But this does not feel like a sustainable path forward overall 🤔 Any ideas? Could we somehow trigger a refresh of the tiles after updating the app so we minimize the window for this to happen?

IIRC I think the OS handles this outside the freshness interval. Maybe its best not to send any data when the tile hasn't been refreshed yet and just provide a friendly message to the user? Either way though we shouldn't crash in any of these cases so would be good to add some try/catch blocks so we don't have a crash unexpectedly.

[slovdahl]

Using null as key for the template tile that we don't yet know the ID of didn't feel 100% clean from the start, but now I got rid of it and replaced it with -1 instead (0ff5c0a). Hopefully that doesn't clash with a real ID given by Android. All IDs I have seen have been positive integers at least.

That also nicely solved the problem I had with serializing a null key with Jackson.

AFAIK, everything should finally work now and the PR is ready for (final) review.

[jpelgrom]

@slovdahl I don't mind reviewing but you literally pushed a TODO item for what looks like debugging after requesting a review. This does not seem finished? If you want feedback on something specific just ask.

[slovdahl]

@jpelgrom I'm sorry for the confusion, it was not intentional. I didn't notice that there was a rebase conflict before I requested the review, so I rebased it and fixed the trivial import-related conflict locally, and tried to push all the commits except the last REVERTME one. But I obviously failed and managed to push that one as well. 🙈

It should all be fixed now, and the PR is ready for review whenever you have time. 🙇

[jpelgrom]

Sorry for the delay in getting back to the review. As a result I've noticed some new things to address... Thanks for the efforts so far, almost there 🙏

[jpelgrom]

🎉