diff --git a/app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt index 04c9083f50..18a30b5afa 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt @@ -111,9 +111,7 @@ class PathActivity : ScreenLockActionBarActivity() { binding.pathRowsContainer.layoutParams = params } - lifecycleScope.launch { - inAppReviewManager.onEvent(InAppReviewManager.Event.PathScreenVisited) - } + inAppReviewManager.onEvent(InAppReviewManager.Event.PathScreenVisited) } private fun registerObservers() { diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsViewModel.kt index 125b2fca6c..45c271e65c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsViewModel.kt @@ -639,9 +639,7 @@ class SettingsViewModel @Inject constructor( } is Commands.OnDonateClicked -> { - viewModelScope.launch { - inAppReviewManager.onEvent(InAppReviewManager.Event.DonateButtonClicked) - } + inAppReviewManager.onEvent(InAppReviewManager.Event.DonateButtonClicked) showUrlDialog(URL_DONATE) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt index 4dafa60910..c38d751440 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt @@ -2,11 +2,9 @@ package org.thoughtcrime.securesms.preferences.appearance import androidx.annotation.StyleRes import androidx.lifecycle.ViewModel -import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.launch import org.session.libsession.utilities.TextSecurePreferences import org.thoughtcrime.securesms.reviews.InAppReviewManager import org.thoughtcrime.securesms.ui.theme.invalidateComposeThemeColors @@ -30,9 +28,7 @@ class AppearanceSettingsViewModel @Inject constructor( invalidateComposeThemeColors() - viewModelScope.launch { - inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged) - } + inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged) } fun setNewStyle(newThemeStyle: String) { @@ -42,9 +38,7 @@ class AppearanceSettingsViewModel @Inject constructor( invalidateComposeThemeColors() - viewModelScope.launch { - inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged) - } + inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged) } fun setNewFollowSystemSettings(followSystemSettings: Boolean) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/reviews/InAppReviewManager.kt b/app/src/main/java/org/thoughtcrime/securesms/reviews/InAppReviewManager.kt index 4c2b1997b8..7a607ef70d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/reviews/InAppReviewManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/reviews/InAppReviewManager.kt @@ -70,7 +70,11 @@ class InAppReviewManager @Inject constructor( .stateIn(scope, SharingStarted.Eagerly, false) init { - val channel = Channel() + // UNLIMITED, not the default rendezvous. On a rendezvous channel `send` does not complete when + // it is called — it suspends until this collector receives — so an event emitted from a screen's + // scope is lost if that screen goes away in between. Buffering makes the handoff finish at the + // call, which is what lets [onEvent] be non-suspending. + val channel = Channel(capacity = Channel.UNLIMITED) eventsChannel = channel scope.launch { @@ -143,8 +147,24 @@ class InAppReviewManager @Inject constructor( } } - suspend fun onEvent(event: Event) { - eventsChannel.send(event) + /** + * Record something the user did that might earn a review prompt. + * + * Deliberately NOT suspending, and deliberately not requiring a coroutine at the call site. Every + * caller is a screen, and a screen's scope dies when the user leaves it — which is exactly when these + * events happen. Changing the theme and pressing back immediately used to lose the event entirely, + * because the emission had to outlive the screen that triggered it. + * + * The channel is UNLIMITED, so this hands off and returns rather than waiting for the collector. + */ + fun onEvent(event: Event) { + val result = eventsChannel.trySend(event) + + // Unreachable on an unlimited channel short of the manager being closed, but silence here would + // look exactly like the bug this replaced: a trigger that simply never arrives. + if (result.isFailure) { + Log.w(TAG, "Dropped review event $event: ${result.exceptionOrNull()}") + } } enum class Event { diff --git a/app/src/test/java/org/thoughtcrime/securesms/reviews/InAppReviewManagerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/reviews/InAppReviewManagerTest.kt index 456e2abdeb..155b909d07 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/reviews/InAppReviewManagerTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/reviews/InAppReviewManagerTest.kt @@ -3,6 +3,7 @@ package org.thoughtcrime.securesms.reviews import android.content.pm.PackageInfo import android.content.pm.PackageManager import app.cash.turbine.test +import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest @@ -216,6 +217,43 @@ class InAppReviewManagerAppUpdatedOverrideTest { } } +/** + * That an event survives being fired from a caller that does not stick around. + * + * This is the regression: a theme change emitted from a screen's own scope was lost if the user left the + * screen immediately, because the channel was rendezvous and the send did not complete until the collector + * received it. The prompt then never appeared — for anyone, not only a test. + */ +@RunWith(JUnit4::class) +class InAppReviewManagerEventDeliveryTest { + @get:Rule + val mockLoggingRule = MockLoggingRule() + + @Test + fun `an event fired before the collector is ready is still delivered`() = runTest { + val manager = createManager(isFreshInstall = true) + + // Deliberately no yield between construction and the event: the collector has not started, so this + // is the moment a rendezvous channel had nowhere to put it. `first { it }` waits rather than + // sampling, so a lost event fails by timing out instead of by reading a stale false. + manager.onEvent(InAppReviewManager.Event.ThemeChanged) + + assertTrue(manager.shouldShowPrompt.first { it }) + } + + @Test + fun `onEvent needs no coroutine at the call site`() = runTest { + // Compilation is half the assertion: onEvent must stay non-suspending, because every caller is a + // screen and wrapping it in that screen's scope is what made the event losable. + val manager = createManager(isFreshInstall = true) + val fire: () -> Unit = { manager.onEvent(InAppReviewManager.Event.PathScreenVisited) } + + fire() + + assertTrue(manager.shouldShowPrompt.first { it }) + } +} + fun TestScope.createManager( isFreshInstall: Boolean, supportInAppReviewFlow: Boolean = true,