diff --git a/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt b/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt index 7fb93c96..84d2b841 100644 --- a/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt +++ b/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt @@ -1,12 +1,10 @@ package eu.darken.capod.common.upgrade.core.billing -import android.app.Activity import android.content.ActivityNotFoundException import android.content.Context import android.content.Intent import android.net.Uri import android.provider.Settings -import android.widget.Toast import eu.darken.capod.R import eu.darken.capod.common.debug.logging.Logging.Priority.ERROR import eu.darken.capod.common.debug.logging.log @@ -34,18 +32,23 @@ class GplayServiceUnavailableException(cause: Throwable) : try { activity.startActivity(intent) } catch (e: ActivityNotFoundException) { - onLaunchFailed(activity, e) + onLaunchFailed(e) + throw e } catch (e: SecurityException) { // Play can be installed but unreachable: disabled app, work/restricted profile or a // ROM that guards the settings screen. The launch is denied, not unresolvable. - onLaunchFailed(activity, e) + onLaunchFailed(e) + throw e } }, + // The failure is not presented here: it propagates to the dialog, which renders this inline. + // A Toast caps at 2 lines and clipped this message (in French mid-word, dropping a whole + // condition), so the container was the defect, not the wording. + fixActionErrorMessage = context.getString(R.string.upgrades_gplay_not_installed_message), ) - private fun onLaunchFailed(activity: Activity, e: Exception) { + private fun onLaunchFailed(e: Exception) { log(ERROR) { "Can't launch settings intent for Google Play: $e" } - Toast.makeText(activity, R.string.upgrades_gplay_not_installed_message, Toast.LENGTH_SHORT).show() } companion object { diff --git a/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt b/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt index 6bcd993a..2350d91d 100644 --- a/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt +++ b/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt @@ -1,7 +1,10 @@ package eu.darken.capod.common.error import android.app.Activity +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.padding import androidx.compose.material3.AlertDialog +import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable @@ -10,8 +13,10 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource +import androidx.compose.ui.unit.dp import eu.darken.capod.R import eu.darken.capod.common.debug.logging.Logging.Priority.ERROR import eu.darken.capod.common.debug.logging.asLog @@ -48,24 +53,57 @@ private fun ComposeErrorDialog( // acknowledge-only shape. The activity is required to launch the fix. val hasFix = localizedError.fixAction != null && activity != null + // Keyed on the throwable, not the LocalizedError: the latter is rebuilt (with fresh action + // lambdas, so never equal) on every recomposition, which would wipe the message immediately. + var actionError by remember(throwable) { mutableStateOf(null) } + + // errorMessage is per-dispatch, NOT read from localizedError: fixActionErrorMessage describes + // only the fix action's failure, so every dispatch site passes its own copy (or none) and no + // other action can ever surface it. + fun dispatchAndDismiss( + action: (Activity) -> Unit, + errorMessage: String? = null, + ) { + // Error actions are arbitrary code (intent launches): a throw here would crash the UI + // thread from inside a click handler, and skipping onDismiss() would leave the dialog + // latched on the current error with no way out. + try { + action(activity!!) + } catch (e: Exception) { + log(TAG, ERROR) { "Error action failed: ${e.asLog()}" } + // A dispatch that ships its own failure copy keeps the dialog open and shows it inline + // (no length cap, unlike a Toast). Never latched: the dismiss button stays available. + errorMessage?.let { + actionError = it + return + } + } + onDismiss() + } + AlertDialog( onDismissRequest = onDismiss, title = { Text(text = localizedError.label) }, - text = { Text(text = localizedError.description) }, + text = { + Column { + Text(text = localizedError.description) + actionError?.let { + Text( + text = it, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(top = 8.dp), + ) + } + } + }, confirmButton = { if (hasFix) { TextButton( onClick = { - // Error actions are arbitrary code (intent launches): a throw here would - // crash the UI thread from inside a click handler, and skipping onDismiss() - // would leave the dialog latched on the current error with no way out. - try { - localizedError.fixAction!!.invoke(activity!!) - } catch (e: Exception) { - log(TAG, ERROR) { "Error action failed: ${e.asLog()}" } - } finally { - onDismiss() - } + dispatchAndDismiss( + action = localizedError.fixAction!!, + errorMessage = localizedError.fixActionErrorMessage, + ) }, ) { Text(text = localizedError.fixActionLabel ?: stringResource(android.R.string.ok)) diff --git a/app/src/main/java/eu/darken/capod/common/error/LocalizedError.kt b/app/src/main/java/eu/darken/capod/common/error/LocalizedError.kt index 84192194..3290fed5 100644 --- a/app/src/main/java/eu/darken/capod/common/error/LocalizedError.kt +++ b/app/src/main/java/eu/darken/capod/common/error/LocalizedError.kt @@ -14,6 +14,8 @@ data class LocalizedError( val description: String, val fixActionLabel: String? = null, val fixAction: ((Activity) -> Unit)? = null, + /** Shown inline in the error dialog if the fix action fails, instead of a length-capped toast. */ + val fixActionErrorMessage: String? = null, ) { fun asText() = "$label:\n$description" } diff --git a/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt b/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt index de06b755..9aab58a5 100644 --- a/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt +++ b/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt @@ -3,9 +3,12 @@ package eu.darken.capod.common.error import android.app.Activity import android.content.Context import androidx.compose.ui.test.assertCountEquals +import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick +import androidx.test.core.app.ApplicationProvider +import eu.darken.capod.R import eu.darken.capod.common.compose.PreviewWrapper import eu.darken.capod.common.flow.SingleEventFlow import io.kotest.matchers.shouldBe @@ -22,7 +25,11 @@ class ComposeErrorDialogGuardTest : BaseComposeRobolectricTest() { override val errorEvents = SingleEventFlow() } + private val dismissLabel: String + get() = ApplicationProvider.getApplicationContext().getString(R.string.general_dismiss_action) + private class TestError( + private val fixErrorMessage: String? = null, private val fixAction: (Activity) -> Unit, ) : Exception(ERROR_BODY), HasLocalizedError { override fun getLocalizedError(context: Context): LocalizedError = LocalizedError( @@ -31,6 +38,7 @@ class ComposeErrorDialogGuardTest : BaseComposeRobolectricTest() { description = ERROR_BODY, fixActionLabel = FIX_LABEL, fixAction = fixAction, + fixActionErrorMessage = fixErrorMessage, ) } @@ -66,8 +74,42 @@ class ComposeErrorDialogGuardTest : BaseComposeRobolectricTest() { // caught and onDismiss still ran. composeRule.onAllNodesWithText(FIX_LABEL).assertCountEquals(0) } + + @Test + fun `a throwing fix action with its own message keeps the dialog open and shows it inline`() { + // A Toast caps at 2 lines and clipped this kind of message; the dialog body has no cap. + showError( + TestError(fixErrorMessage = FIX_ERROR_MESSAGE) { + throw IllegalStateException("fix action exploded") + } + ) + + composeRule.onNodeWithText(FIX_LABEL).performClick() + composeRule.waitForIdle() + + composeRule.onNodeWithText(FIX_ERROR_MESSAGE).assertIsDisplayed() + composeRule.onNodeWithText(FIX_LABEL).assertIsDisplayed() + // Not latched: the way out stays available while the message is shown. + composeRule.onNodeWithText(dismissLabel).performClick() + composeRule.waitForIdle() + composeRule.onAllNodesWithText(FIX_LABEL).assertCountEquals(0) + } + + @Test + fun `a fix action that succeeds never shows its failure message`() { + // The message belongs to the fix action's failed dispatch, not to the error itself: a + // dispatch that did not throw must keep the plain dismiss behaviour. + showError(TestError(fixErrorMessage = FIX_ERROR_MESSAGE) { }) + + composeRule.onNodeWithText(FIX_LABEL).performClick() + composeRule.waitForIdle() + + composeRule.onAllNodesWithText(FIX_LABEL).assertCountEquals(0) + composeRule.onAllNodesWithText(FIX_ERROR_MESSAGE).assertCountEquals(0) + } } private const val ERROR_TITLE = "Test error title" private const val ERROR_BODY = "Test error description" private const val FIX_LABEL = "Fix it" +private const val FIX_ERROR_MESSAGE = "Fixing it did not work" diff --git a/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt b/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt index c08080fc..bd9c4603 100644 --- a/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt +++ b/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt @@ -4,6 +4,7 @@ import android.app.Activity import android.content.ActivityNotFoundException import android.content.Intent import eu.darken.capod.R +import io.kotest.assertions.throwables.shouldThrow import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe import org.junit.Test @@ -17,7 +18,8 @@ import testhelpers.TestApplication /** * The error dialog's "Google Play" button runs on an activity context: a device where the launch is - * refused must get a toast, not a crash. The successful launch is covered by ComposeErrorDialogTest. + * refused must get the failure reported through the dialog (which can show the full message), not a + * clipped toast and not a crash. The successful launch is covered by ComposeErrorDialogTest. */ @RunWith(RobolectricTestRunner::class) @Config(sdk = [33], application = TestApplication::class) @@ -35,23 +37,36 @@ class GplayFixActionTest : BaseTest() { private fun activityOf(clazz: Class): T = Robolectric.buildActivity(clazz).setup().get() - private fun assertToastInsteadOfCrash(activity: Activity) { - val fixAction = GplayServiceUnavailableException(RuntimeException("Play hiccup")) - .getLocalizedError(activity).fixAction.shouldNotBeNull() + private fun fixActionOf(activity: Activity) = GplayServiceUnavailableException(RuntimeException("Play hiccup")) + .getLocalizedError(activity).fixAction.shouldNotBeNull() - fixAction.invoke(activity) + @Test + fun `a denied launch reports through the dialog instead of a toast`() { + val activity = activityOf(DeniedLaunchActivity::class.java) - val expected = activity.getString(R.string.upgrades_gplay_not_installed_message) - ShadowToast.getTextOfLatestToast() shouldBe expected + shouldThrow { fixActionOf(activity).invoke(activity) } + + // A toast caps at 2 lines and clipped this message (French lost a whole condition + // mid-word) — the dialog renders it inline instead. + ShadowToast.getLatestToast() shouldBe null } @Test - fun `a denied launch shows the not-installed toast instead of crashing`() { - assertToastInsteadOfCrash(activityOf(DeniedLaunchActivity::class.java)) + fun `an unresolvable launch reports through the dialog instead of a toast`() { + val activity = activityOf(MissingPlayActivity::class.java) + + shouldThrow { fixActionOf(activity).invoke(activity) } + + ShadowToast.getLatestToast() shouldBe null } @Test - fun `an unresolvable launch shows the not-installed toast instead of crashing`() { - assertToastInsteadOfCrash(activityOf(MissingPlayActivity::class.java)) + fun `the failure message travels with the error for the dialog to show`() { + val activity = activityOf(Activity::class.java) + + val message = GplayServiceUnavailableException(RuntimeException("Play hiccup")) + .getLocalizedError(activity).fixActionErrorMessage.shouldNotBeNull() + + message shouldBe activity.getString(R.string.upgrades_gplay_not_installed_message) } }