Introduce PushProvider interface for push notification providers #5301

lone-faerie · 2025-05-11T20:43:53Z

Summary

This PR introduces a generic interface for push notification providers. Currently, the only provider is FCM, but an interface allows supporting additional push providers in the future, such as UnifiedPush.

The implementation is based on the SensorManager as only one instance of each provider is needed but they should still be able to be enabled for only a subset of servers. A PushManager class has also been added to coordinate the various push providers, which is currently just hard coded to use FCM but would provide the primary API for managing push providers.

Checklist

New or updated tests have been added to cover the changes following the testing guidelines.
The code follows the project's code style and best_practices.
The changes have been thoroughly tested, and edge cases have been considered.
Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Any other notes

This came as a suggestion in this PR for adding UnifiedPush as an alternative push provider.

TimoPtr · 2025-05-12T08:33:15Z

...ll/kotlin/io/homeassistant/companion/android/notifications/FirebaseCloudMessagingProvider.kt

+    override fun isAvailable(context: Context): Boolean = true
+
+    private var token: String? = null
+    private val tokenMutex = Mutex()


Why do you need a mutex? Add documentation explaining why is this needed.

A mutex might not be necessary, as I'm not 100% positive on the flow of Android services and coroutine scopes. To my understanding, there is potential for a race condition if onNewToken() in FirebaseCloudMessagingService gets called at the same time as getToken(), such as at launch or during onboarding.

TimoPtr · 2025-05-12T08:37:24Z

...ll/kotlin/io/homeassistant/companion/android/notifications/FirebaseCloudMessagingProvider.kt

+    override suspend fun getToken(): String {
+        return tokenMutex.withLock { token } ?: try {
+            FirebaseMessaging.getInstance().token.await()
+        } catch (e: Exception) {
+            Timber.e(e, "Issue getting token")
+            ""
+        }
+    }


You are not storing the new token in this class?

This function was primarily to move getMessagingToken() from onboarding into the push provider. The token gets stored with the device registration, and I believe the Firebase library also stores the token.

TimoPtr · 2025-05-12T08:38:29Z

...ll/kotlin/io/homeassistant/companion/android/notifications/FirebaseCloudMessagingProvider.kt

+import kotlinx.coroutines.tasks.await
+import timber.log.Timber
+
+class FirebaseCloudMessagingProvider @Inject constructor(


The class is named MessagingProvider but at first glance it simply store a token in memory and forward a message to the messagingManager with the right SOURCE. We probably change the name of the class.

Also could you add top level documentation about the behavior of the class especially about the token is stored and what the lifecycle of the token. You can also add links to Firebase documentation.

I named it that based on FirebaseCloudMessaging and PushProvider, but yeah it is a bit misleading. Something like FirebasePushProviderImpl would probably be clearer.

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushModule.kt

TimoPtr · 2025-05-12T08:46:04Z

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushManager.kt

+class PushManager @Inject constructor(
+    val providers: Map<String, @JvmSuppressWildcards PushProvider>
+) {


Could you create an instrumentation test that checks how the map looks like. It will be useful in the future to make sure the map contains the proper values. One test for each flavor

TimoPtr · 2025-05-12T08:46:42Z

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushManager.kt

+import io.homeassistant.companion.android.common.notifications.PushProvider
+import javax.inject.Inject
+
+class PushManager @Inject constructor(


Add documentation about the purpose of this class.

TimoPtr · 2025-05-12T08:48:00Z

...ll/kotlin/io/homeassistant/companion/android/notifications/FirebaseCloudMessagingProvider.kt

+    companion object {
+        const val SOURCE = "FCM"
+    }
+
+    override val setting = PushProviderSetting.FCM


It's confusing what is setting here?

It is used in isEnabled() for checking against the database. That way each implementation just has to define setting instead of implementing its own isEnabled(). I'll add documentation to explain that, or there could be a better way of handling it.

TimoPtr · 2025-05-12T08:53:04Z

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushManager.kt

+    companion object {
+        private const val FCM_SOURCE = FirebaseCloudMessagingProvider.SOURCE
+    }
+
+    val defaultProvider: PushProvider get() {
+        return providers[FCM_SOURCE]!!
+    }
+
+    suspend fun getToken(): String {
+        return providers[FCM_SOURCE]!!.getToken()
+    }


Could you already prepare in this PR the fact that FCM is only available in full by keeping this class agnostic from FCM?

TimoPtr · 2025-05-12T08:53:23Z

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushModule.kt

+    @Singleton
+    @IntoMap
+    @StringKey(FirebaseCloudMessagingProvider.SOURCE)
+    abstract fun bindFirebasePushProvider(firebaseCloudMessagingProvider: FirebaseCloudMessagingProvider): PushProvider


Why this is not in full?

Any more push providers that are added would also need to be bound in the module, which may not be limited to full. I'm not fully familiar with hilt/dagger, but I imagine it would work to put bindFirebasePushProvider in its own module in full and any providers that work in both full and minimal could be put in one in main.

You can Inject into the map from multiple places. Here we should have a PushModule in full for FCM and another one in minimal without FCM. We don't want FCM reference in minimal.

TimoPtr · 2025-05-12T08:58:18Z

app/src/minimal/kotlin/io/homeassistant/companion/android/launch/LaunchPresenterImpl.kt

 class LaunchPresenterImpl @Inject constructor(
    view: LaunchView,
-    serverManager: ServerManager
-) : LaunchPresenterBase(view, serverManager) {
+    serverManager: ServerManager,
+    pushManager: PushManager
+) : LaunchPresenterBase(view, serverManager, pushManager) {


Now that you have an abstraction you could potentially already remove the two version of the LaunchPresenterImpl and keep only the one from full. Let's avoid as much as possible duplicates. It makes maintenance of minimal easier.

TimoPtr · 2025-05-12T08:59:28Z

...al/kotlin/io/homeassistant/companion/android/notifications/FirebaseCloudMessagingProvider.kt

+import javax.inject.Inject
+import kotlinx.coroutines.CoroutineScope
+
+class FirebaseCloudMessagingProvider @Inject constructor() : PushProvider {


Let's not create a fake FCM in minimal create a No-Op MessagingProvider if you need one that does nothing so it's explicit what it does.

Actually, I don't think a No-op provider is even necessary. The only thing it would be needed for is returning an empty token, but that can be avoided by just adding null checks elsewhere.

TimoPtr · 2025-05-12T09:03:04Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+import kotlinx.coroutines.launch
+import timber.log.Timber
+
+interface PushProvider {


Let's make sure this public interface is properly documented. Also it would be nice to create a section in https://developers.home-assistant.io/docs/android/ about how to add a new provider if this is meant to evolve.

TimoPtr · 2025-05-12T09:07:56Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+    fun isEnabled(context: Context): Boolean {
+        val settingsDao = AppDatabase.getInstance(context).settingsDao()
+        return serverManager(context).defaultServers.any {
+            settingsDao.get(it.id)?.pushProvider == setting


Maybe we could consider using a sealed interface instead of an enum, so we don't need to store within the class what the class is already.

I'm not sure that'd work, since the implementations are in a different module than the interface. Though that could be solved by moving PushProvider from :common to :app

Do we need to have PushProvider in common?

If we wanted to use this same architecture for Wear, then PushProvider would either have to be in :common or duplicated in :wear

Another option could be implementing an id() function like with SensorManager:

fun PushProvider.id(): String { val simpleName = this::class.simpleName ?: this::class.java.name return simpleName.lowercase(Locale.US).replace(" ", "_") }

This way the class doesn't need to store what it is, it's just derived from the class itself.

TimoPtr · 2025-05-12T09:08:48Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+    /** @return Set of server IDs for which this provider is enabled */
+    fun getEnabledServers(context: Context): Set<Int> {


Why not returning the Servers directly?

This function signature was adapted from the one in SensorManager. The only place it's currently used is in updateRegistration() which only needs the server ID.

TimoPtr · 2025-05-12T09:10:13Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+                    } catch (e: Exception) {
+                        Timber.e(e, "Issue updating token")
+                    }


Could you narrow done the catch to only the potential exception that can occur?

TimoPtr · 2025-05-12T09:11:40Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+    suspend fun updateRegistration(context: Context, coroutineScope: CoroutineScope) {
+        coroutineScope.launch {


Why do you need to get the scope here? Either you enforce the scope within the interface so you control it or you delegate to the caller function to decide which scope to use.

This could be me not fully understanding how Kotlin handles coroutines, but does taking the coroutine scope as an argument not delegate it to the caller function?
Or do you just mean the initial coroutineScope.launch? Yeah, that probably isn't needed. I just took onNewToken() from FirebaseCloudMessagingService and adapted it for the interface.

Let's remove the scope given as parameter.

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

common/src/main/kotlin/io/homeassistant/companion/android/database/settings/Setting.kt

TimoPtr · 2025-05-12T09:16:19Z

common/src/main/kotlin/io/homeassistant/companion/android/database/settings/Setting.kt

@@ -12,5 +12,7 @@ data class Setting(
    @ColumnInfo(name = "websocket_setting")
    var websocketSetting: WebsocketSetting,
    @ColumnInfo(name = "sensor_update_frequency")
-    var sensorUpdateFrequency: SensorUpdateFrequencySetting
+    var sensorUpdateFrequency: SensorUpdateFrequencySetting,
+    @ColumnInfo(name = "push_provider", defaultValue = "FCM")


Could we avoid using a string here for the defaultValue, it's a source of typo and will make refactor harder if one day we want to refactor.

Unfortunately, defaultValue must be a compile-time constant string.

Ok the main issue is that the constant is in :app so you don't have access it here. I don't like having a reference to FCM in common because it will also end up into minimal and it doesn't make sense to have FCM in minimal as default.

I would encourage you to handle the case where the push provider is not set in the :app so each flavor can have a proper default value. Also it creates a proper separation from app and common.

TimoPtr · 2025-05-12T09:24:28Z

app/src/main/kotlin/io/homeassistant/companion/android/settings/SettingViewModel.kt

@@ -42,4 +48,11 @@ class SettingViewModel @Inject constructor(
            settingsDao.update(it)
        }
    }
+
+    fun updatePushProviderSetting(id: Int, setting: PushProviderSetting) {


Suggested change

fun updatePushProviderSetting(id: Int, setting: PushProviderSetting) {

fun updatePushProviderSetting(serverId: Int, setting: PushProviderSetting) {

Should the rest of the methods in SettingViewModel also be changed to match this?

You can yes

TimoPtr

Thanks for pushing forward onto the suggestion made in your previous PR.

lone-faerie · 2025-05-21T05:38:07Z

app/src/test/kotlin/io/homeassistant/companion/android/notifications/PushManagerTest.kt

I'm not sure if this is the best way to test the structure of the providers map since it relies on PushManager being in LaunchActivity. I think a better way would be to follow Android's Hilt testing guide but it requires adding the hilt-android-testing dependency.

You can add the new dependency if you need it. The test that I would like to have is a test that gets the map injected by hilt where you can assert the content of the map.

TimoPtr · 2025-05-21T08:25:44Z

Multiple comments aren't addressed. Please take a look to them before asking for re-review. Also I would like @jpelgrom opinion on the architecture since he was the one initially thinking about introducing this concept. So take a look to my comments and ask us to review again your changes :)

jpelgrom · 2025-05-22T22:10:53Z

How do you imagine UnifiedPush would work with this architecture for choosing between multiple distributors? The current architecture forces each push provider to have a separate class, while my first expectation would be that each distributor could be considered a push provider (for example, you get a list: none / FCM / ntfy / Sunup / ...).

lone-faerie · 2025-05-23T00:21:51Z

The reason for each push provider having its own class is to support multiple protocols, as well as helping to separate things between minimal and full. Like @TimoPtr said, it's a good idea to keep any references to FCM out of minimal. And all of the UnifiedPush distributors (ntfy, Sunup, etc.) would be covered under one class since they all use the same protocol.

The PushProvider interface is primarily for translating the various protocols into a common interface. For example, Firebase messages are received as a map but UnifiedPush messages are received as raw bytes, and Firebase just provides a token while UnifiedPush provides a url and public key. The PushManager class would then be responsible for coordinating the various push providers. It will provide the list of available providers/distributors as well as handling enabling the selected one.

As for the UI aspect of choosing a distributor, I imagined a setting similar to what I had in my previous PR though more generic to include other push providers apart from just UnifiedPush. This architecture would also open up the possibility of using a different provider for each server.

jpelgrom · 2025-05-23T18:50:50Z

I understand the advantages and was the one suggesting a generic interface on your previous PR. But how would this setup work for for choosing a distributor, which is a concept specific to UnifiedPush, if the other parts of the app don't know about them as the provider interface doesn't have a way to expose it?

(I tried do something similar for my UP proof of concept, though that has flaws in other ways, so I might simply be looking for something that you didn't add for a good reason and it hasn't clicked yet.)

jpelgrom · 2025-05-23T18:55:05Z

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushManager.kt

+    }
+
+    /**
+     * Get the push url for the default push provider.


The function returns the token

jpelgrom · 2025-05-23T18:58:35Z

...io/homeassistant/companion/android/common/data/integration/impl/IntegrationRepositoryImpl.kt


        val appData = mutableMapOf<String, Any>("push_websocket_channel" to deviceRegistration.pushWebsocket)
        if (!pushToken.isNullOrBlank()) {
-            appData["push_url"] = PUSH_URL
+            appData["push_url"] = pushUrl?.ifBlank { PUSH_URL } ?: PUSH_URL


If there is going to be a generic interface, the app shouldn't fall back to a FCM URL. You could end up with a different push provider token + FCM URL.

jpelgrom · 2025-05-23T18:59:41Z

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt

+        }.map { it.id }.toSet()
+    }
+
+    suspend fun getUrl(): String = BuildConfig.PUSH_URL


This should not default to the FCM URL, but be abstract.

lone-faerie · 2025-05-23T23:03:25Z

Ok, I get what you're saying. I was planning on initially just using the default UnifiedPush distributor with UnifiedPush.tryUseCurrentOrDefaultDistributor(context) just to get things working at first. But there's a couple ways I could see for the user choosing a distributor. We could add a getDistributors() method to PushProvider or the UnifiedPush library includes its own dialog for choosing a distributor that we could use.

jpelgrom · 2025-05-26T17:39:19Z

We could add a getDistributors() method to PushProvider

That could be an option. What would you return for that function in the FCM push provider?

or the UnifiedPush library includes its own dialog for choosing a distributor

That would require specific UI work outside the provider so kind of defeats the point, I think?

lone-faerie · 2025-05-27T02:34:27Z

That could be an option. What would you return for that function in the FCM push provider?

For the FCM provider, I'd either return null or "FCM" (or "Firebase"). If we allow null, then we could display it to the user as 'Provider (Distributor)' if not null, otherwise just 'Provider'. So for example, 'FCM' and 'UnifiedPush (ntfy)'. If getDistributor() always returns a list, so FCM returns "FCM", then we'd just display to the user each distributor.

That would require specific UI work outside the provider so kind of defeats the point, I think?

For UnifiedPush, the library handles the UI work, though for other providers, yeah it would require work outside of it. The library's UI probably doesn't align with Home Assistant's, so it's probably not the right choice, just wanted to mention it.

lone-faerie and others added 6 commits May 10, 2025 20:17

Add push provider setting to database

6262f96

Introduce PushProvider interface for push notification providers

59c74ef

Lint code

85ff4b5

Add documentation for PushProvider

649d788

Merge branch 'home-assistant:main' into feature/push_provider_interface

8e31d9b

Add stub FirebaseCloudMessagingProvider for minimal flavor

f91721a

home-assistant bot added the cla-signed label May 11, 2025

TimoPtr reviewed May 12, 2025

View reviewed changes

app/src/main/kotlin/io/homeassistant/companion/android/notifications/PushModule.kt Outdated Show resolved Hide resolved

TimoPtr reviewed May 12, 2025

View reviewed changes

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt Show resolved Hide resolved

TimoPtr reviewed May 12, 2025

View reviewed changes

common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/PushProvider.kt Outdated Show resolved Hide resolved

TimoPtr reviewed May 12, 2025

View reviewed changes

common/src/main/kotlin/io/homeassistant/companion/android/database/settings/Setting.kt Outdated Show resolved Hide resolved

TimoPtr reviewed May 12, 2025

View reviewed changes

Apply suggestions

e013074

lone-faerie and others added 3 commits May 20, 2025 20:15

Add unit and instrumentation tests

9a3ddf3

Merge branch 'home-assistant:main' into feature/push_provider_interface

2b66420

Update tests to use testing-unit

6a5af75

lone-faerie commented May 21, 2025

View reviewed changes

lone-faerie requested a review from TimoPtr May 21, 2025 05:38

jpelgrom reviewed May 23, 2025

View reviewed changes

		/** @return Set of server IDs for which this provider is enabled */
		fun getEnabledServers(context: Context): Set<Int> {

		suspend fun updateRegistration(context: Context, coroutineScope: CoroutineScope) {
		coroutineScope.launch {

	fun updatePushProviderSetting(id: Int, setting: PushProviderSetting) {
	fun updatePushProviderSetting(serverId: Int, setting: PushProviderSetting) {

Uh oh!

Introduce PushProvider interface for push notification providers #5301

Are you sure you want to change the base?

Introduce PushProvider interface for push notification providers #5301

Uh oh!

Conversation

lone-faerie commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Any other notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoPtr May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TimoPtr May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoPtr May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lone-faerie commented May 11, 2025 •

edited

Loading

TimoPtr May 12, 2025 •

edited

Loading

TimoPtr May 12, 2025 •

edited

Loading

TimoPtr May 12, 2025 •

edited

Loading