Introduce PushProvider interface for push notification providers #5301
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package io.homeassistant.companion.android.notifications | ||
|
|
||
| import android.content.Context | ||
| import com.google.firebase.messaging.FirebaseMessaging | ||
| import io.homeassistant.companion.android.common.notifications.PushProvider | ||
| import io.homeassistant.companion.android.database.settings.PushProviderSetting | ||
| import javax.inject.Inject | ||
| import kotlinx.coroutines.sync.Mutex | ||
| import kotlinx.coroutines.sync.withLock | ||
| import kotlinx.coroutines.tasks.await | ||
| import timber.log.Timber | ||
|
|
||
| class FirebaseCloudMessagingProvider @Inject constructor( | ||
| private val messagingManager: MessagingManager | ||
| ) : PushProvider { | ||
|
|
||
| companion object { | ||
| const val SOURCE = "FCM" | ||
| } | ||
|
|
||
| override val setting = PushProviderSetting.FCM | ||
|
||
|
|
||
| override fun isAvailable(context: Context): Boolean = true | ||
|
|
||
| private var token: String? = null | ||
| private val tokenMutex = Mutex() | ||
|
Member
There was a problem hiding this comment. Why do you need a mutex? Add documentation explaining why is this needed.
Author
There was a problem hiding this comment. 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 |
||
|
|
||
| suspend fun setToken(token: String) = tokenMutex.withLock { | ||
| this.token = token | ||
| } | ||
|
|
||
| override suspend fun getToken(): String { | ||
| return tokenMutex.withLock { token } ?: try { | ||
| FirebaseMessaging.getInstance().token.await() | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Issue getting token") | ||
| "" | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. You are not storing the new token in this class?
Author
There was a problem hiding this comment. This function was primarily to move |
||
|
|
||
| override fun onMessage(context: Context, notificationData: Map<String, String>) { | ||
| messagingManager.handleMessage(notificationData, SOURCE) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package io.homeassistant.companion.android.notifications | ||
|
|
||
| import io.homeassistant.companion.android.common.notifications.PushProvider | ||
| import javax.inject.Inject | ||
|
|
||
| class PushManager @Inject constructor( | ||
|
||
| val providers: Map<String, @JvmSuppressWildcards PushProvider> | ||
| ) { | ||
|
||
|
|
||
| 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() | ||
| } | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package io.homeassistant.companion.android.notifications | ||
|
|
||
| import dagger.Binds | ||
| import dagger.Module | ||
| import dagger.hilt.InstallIn | ||
| import dagger.hilt.components.SingletonComponent | ||
| import dagger.multibindings.IntoMap | ||
| import dagger.multibindings.StringKey | ||
| import io.homeassistant.companion.android.common.notifications.PushProvider | ||
| import javax.inject.Singleton | ||
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| abstract class PushModule { | ||
| @Binds | ||
| @Singleton | ||
| @IntoMap | ||
| @StringKey(FirebaseCloudMessagingProvider.SOURCE) | ||
lone-faerie marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| abstract fun bindFirebasePushProvider(firebaseCloudMessagingProvider: FirebaseCloudMessagingProvider): PushProvider | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,13 @@ import io.homeassistant.companion.android.database.server.ServerConnectionInfo | |
| import io.homeassistant.companion.android.database.server.ServerSessionInfo | ||
| import io.homeassistant.companion.android.database.server.ServerType | ||
| import io.homeassistant.companion.android.database.server.ServerUserInfo | ||
| import io.homeassistant.companion.android.database.settings.PushProviderSetting | ||
| import io.homeassistant.companion.android.database.settings.SensorUpdateFrequencySetting | ||
| import io.homeassistant.companion.android.database.settings.Setting | ||
| import io.homeassistant.companion.android.database.settings.SettingsDao | ||
| import io.homeassistant.companion.android.database.settings.WebsocketSetting | ||
| import io.homeassistant.companion.android.notifications.PushManager | ||
| import io.homeassistant.companion.android.onboarding.OnboardApp | ||
| import io.homeassistant.companion.android.sensors.LocationSensorManager | ||
| import io.homeassistant.companion.android.settings.language.LanguagesManager | ||
| import io.homeassistant.companion.android.themes.ThemesManager | ||
|
|
@@ -50,6 +51,7 @@ class SettingsPresenterImpl @Inject constructor( | |
| private val prefsRepository: PrefsRepository, | ||
| private val themesManager: ThemesManager, | ||
| private val langsManager: LanguagesManager, | ||
| private val pushManager: PushManager, | ||
| private val changeLog: ChangeLog, | ||
| private val settingsDao: SettingsDao, | ||
| private val sensorDao: SensorDao | ||
|
|
@@ -162,7 +164,7 @@ class SettingsPresenterImpl @Inject constructor( | |
| override suspend fun addServer(result: OnboardApp.Output?) { | ||
| if (result != null) { | ||
| val (url, authCode, deviceName, deviceTrackingEnabled, notificationsEnabled) = result | ||
| val messagingToken = pushManager.getToken() | ||
| var serverId: Int? = null | ||
| try { | ||
| val formattedUrl = UrlUtil.formattedUrlString(url) | ||
|
|
@@ -179,9 +181,9 @@ class SettingsPresenterImpl @Inject constructor( | |
| serverManager.authenticationRepository(serverId).registerAuthorizationCode(authCode) | ||
| serverManager.integrationRepository(serverId).registerDevice( | ||
| DeviceRegistration( | ||
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", | ||
| deviceName = deviceName, | ||
| pushToken = messagingToken | ||
| ) | ||
| ) | ||
| serverManager.getServer()?.id?.let { | ||
|
|
@@ -228,7 +230,8 @@ class SettingsPresenterImpl @Inject constructor( | |
| Setting( | ||
| serverId, | ||
| if (enabled) WebsocketSetting.ALWAYS else WebsocketSetting.NEVER, | ||
| SensorUpdateFrequencySetting.NORMAL, | ||
| PushProviderSetting.NONE | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,22 +3,24 @@ package io.homeassistant.companion.android.launch | |
| import io.homeassistant.companion.android.BuildConfig | ||
| import io.homeassistant.companion.android.common.data.integration.DeviceRegistration | ||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||
| import io.homeassistant.companion.android.notifications.PushManager | ||
| import javax.inject.Inject | ||
| import kotlinx.coroutines.launch | ||
| import timber.log.Timber | ||
|
|
||
| class LaunchPresenterImpl @Inject constructor( | ||
| view: LaunchView, | ||
| serverManager: ServerManager, | ||
| pushManager: PushManager | ||
| ) : LaunchPresenterBase(view, serverManager, pushManager) { | ||
|
||
| override fun resyncRegistration() { | ||
| if (!serverManager.isRegistered()) return | ||
| serverManager.defaultServers.forEach { | ||
| ioScope.launch { | ||
| try { | ||
| serverManager.integrationRepository(it.id).updateRegistration( | ||
| DeviceRegistration( | ||
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})" | ||
| ) | ||
| ) | ||
| serverManager.integrationRepository(it.id).getConfig() // Update cached data | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package io.homeassistant.companion.android.notifications | ||
|
|
||
| import android.content.Context | ||
| import io.homeassistant.companion.android.common.notifications.PushProvider | ||
| import io.homeassistant.companion.android.database.settings.PushProviderSetting | ||
| import javax.inject.Inject | ||
| import kotlinx.coroutines.CoroutineScope | ||
|
|
||
| class FirebaseCloudMessagingProvider @Inject constructor() : PushProvider { | ||
|
||
|
|
||
| // Firebase Cloud Messaging depends on Google Play Services, | ||
| // and as a result FCM is not supported with the minimal flavor | ||
|
|
||
| companion object { | ||
| const val SOURCE = "FCM" | ||
| } | ||
|
|
||
| override val setting = PushProviderSetting.FCM | ||
|
|
||
| override fun isAvailable(context: Context): Boolean = false | ||
|
|
||
| override fun isEnabled(context: Context): Boolean = false | ||
|
|
||
| override fun isEnabled(context: Context, serverId: Int): Boolean = false | ||
|
|
||
| override fun getEnabledServers(context: Context): Set<Int> = emptySet() | ||
|
|
||
| override suspend fun getToken(): String = "" | ||
|
|
||
| override fun onMessage(context: Context, notificationData: Map<String, String>) {} | ||
|
|
||
| override suspend fun updateRegistration(context: Context, coroutineScope: CoroutineScope) {} | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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 named it that based on FirebaseCloudMessaging and PushProvider, but yeah it is a bit misleading. Something like FirebasePushProviderImpl would probably be clearer.