From 849f6b8041f16244cd1c152a81d7dc1f14113067 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 23 Jul 2026 18:06:45 +0200 Subject: [PATCH 1/7] feat(feedback): Support runtime enable/disable of shake-to-report --- CHANGELOG.md | 4 + .../api/sentry-android-core.api | 5 +- sentry-android-core/build.gradle.kts | 1 + .../core/FeedbackShakeIntegration.java | 55 +++++++-- .../android/core/SentryUserFeedbackForm.java | 4 +- .../core/FeedbackShakeIntegrationTest.kt | 108 ++++++++++++++++++ .../io/sentry/samples/android/MainActivity.kt | 20 ++-- sentry/api/sentry.api | 14 +++ .../src/main/java/io/sentry/FeedbackApi.java | 15 +++ .../src/main/java/io/sentry/IFeedbackApi.java | 24 ++++ .../main/java/io/sentry/NoOpFeedbackApi.java | 11 ++ .../java/io/sentry/SentryFeedbackOptions.java | 37 +++++- .../main/java/io/sentry/SentryOptions.java | 18 ++- .../test/java/io/sentry/FeedbackApiTest.kt | 47 ++++++++ .../io/sentry/SentryFeedbackOptionsTest.kt | 6 +- .../test/java/io/sentry/SentryOptionsTest.kt | 12 ++ 16 files changed, 356 insertions(+), 25 deletions(-) create mode 100644 sentry/src/test/java/io/sentry/FeedbackApiTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index f46c77b306c..51076ce6f45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Add `Sentry.feedback().enableFeedbackOnShake()` and `Sentry.feedback().disableFeedbackOnShake()` to toggle shake-to-report at runtime, e.g. based on an asynchronously fetched feature flag ([#5486](https://github.com/getsentry/sentry-java/issues/5486)) + ### Fixes - Pin the published Sentry Android SDK's AAR metadata `minCompileSdk` to our `minSdk` (`21`) instead of AGP 9's new default of the SDK's own `compileSdk` (`37`), so apps that depend on the SDK aren't forced to raise their `compileSdk` ([#5823](https://github.com/getsentry/sentry-java/pull/5823)) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index adebedf2700..8e513cacfae 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -293,9 +293,12 @@ public abstract class io/sentry/android/core/EnvelopeFileObserverIntegration : i public final fun register (Lio/sentry/IScopes;Lio/sentry/SentryOptions;)V } -public final class io/sentry/android/core/FeedbackShakeIntegration : android/app/Application$ActivityLifecycleCallbacks, io/sentry/Integration, java/io/Closeable { +public final class io/sentry/android/core/FeedbackShakeIntegration : android/app/Application$ActivityLifecycleCallbacks, io/sentry/Integration, io/sentry/SentryFeedbackOptions$IShakeController, java/io/Closeable { public fun (Landroid/app/Application;)V public fun close ()V + public fun disable ()V + public fun enable ()V + public fun isEnabled ()Z public fun onActivityCreated (Landroid/app/Activity;Landroid/os/Bundle;)V public fun onActivityDestroyed (Landroid/app/Activity;)V public fun onActivityPaused (Landroid/app/Activity;)V diff --git a/sentry-android-core/build.gradle.kts b/sentry-android-core/build.gradle.kts index f92876530fd..9af0a714b25 100644 --- a/sentry-android-core/build.gradle.kts +++ b/sentry-android-core/build.gradle.kts @@ -115,6 +115,7 @@ dependencies { testImplementation(libs.androidx.test.ext.junit) testImplementation(libs.androidx.test.runner) testImplementation(libs.awaitility.kotlin) + testImplementation(libs.google.truth) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) testImplementation(projects.sentryTestSupport) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java index b059b0104da..7ec9e3800aa 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java @@ -7,6 +7,7 @@ import android.os.Bundle; import io.sentry.IScopes; import io.sentry.Integration; +import io.sentry.SentryFeedbackOptions; import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.util.Objects; @@ -17,15 +18,21 @@ import org.jetbrains.annotations.Nullable; /** - * Detects shake gestures and shows the user feedback dialog when a shake is detected. Only active - * when {@link io.sentry.SentryFeedbackOptions#isUseShakeGesture()} returns {@code true}. + * Detects shake gestures and shows the user feedback dialog when a shake is detected. {@link + * io.sentry.SentryFeedbackOptions#isUseShakeGesture()} determines the initial state; it can be + * toggled at runtime via {@code Sentry.feedback().enableFeedbackOnShake()} and {@code + * Sentry.feedback().disableFeedbackOnShake()}. */ public final class FeedbackShakeIntegration - implements Integration, Closeable, Application.ActivityLifecycleCallbacks { + implements Integration, + Closeable, + Application.ActivityLifecycleCallbacks, + SentryFeedbackOptions.IShakeController { private final @NotNull Application application; private final @NotNull SentryShakeDetector shakeDetector; private @Nullable SentryAndroidOptions options; + private volatile boolean enabled = false; private volatile @Nullable WeakReference currentActivityRef; private volatile boolean isDialogShowing = false; private volatile @Nullable Runnable previousOnFormClose; @@ -46,13 +53,25 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions final @NotNull SentryAndroidOptions options = this.options; - if (!options.getFeedbackOptions().isUseShakeGesture()) { + // Always expose the runtime toggle, even when the option starts out disabled. + options.getFeedbackOptions().setShakeController(this); + + if (options.getFeedbackOptions().isUseShakeGesture()) { + enable(); + } + } + + @Override + public synchronized void enable() { + final @Nullable SentryAndroidOptions options = this.options; + if (enabled || options == null) { return; } + enabled = true; - // Re-arm the detector in case this integration is being re-registered after a previous close() - // (e.g. a second Sentry.init reusing the same options), otherwise the closed latch would keep - // shake detection off permanently. + // Re-arm the detector in case it was closed before, either by disable() or by a previous + // close() (e.g. a second Sentry.init reusing the same options), otherwise the closed latch + // would keep shake detection off permanently. shakeDetector.reopen(); // Resolving the accelerometer is the most expensive part of init (the first SensorManager @@ -72,7 +91,7 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions application.registerActivityLifecycleCallbacks(this); options.getLogger().log(SentryLevel.DEBUG, "FeedbackShakeIntegration installed."); - // In case of a deferred init, hook into any already-resumed activity + // In case of a deferred init or runtime enable, hook into any already-resumed activity final @Nullable Activity activity = CurrentActivityHolder.getInstance().getActivity(); if (activity != null) { currentActivityRef = new WeakReference<>(activity); @@ -81,11 +100,17 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions } @Override - public void close() throws IOException { + public synchronized void disable() { + if (!enabled) { + return; + } + enabled = false; + application.unregisterActivityLifecycleCallbacks(this); shakeDetector.close(); // Restore onFormClose if a dialog is still showing, since lifecycle callbacks - // are now unregistered and onActivityDestroyed cleanup won't fire. + // are now unregistered and onActivityDestroyed cleanup won't fire. The dialog + // itself stays on screen; only future shake detection stops. if (isDialogShowing) { isDialogShowing = false; if (options != null) { @@ -96,6 +121,16 @@ public void close() throws IOException { currentActivityRef = null; } + @Override + public boolean isEnabled() { + return enabled; + } + + @Override + public void close() throws IOException { + disable(); + } + @Override public void onActivityResumed(final @NotNull Activity activity) { // If a dialog is showing on a different activity (e.g. user navigated via notification), diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java index 43500d50ebc..0c14e4fd3c4 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java @@ -62,7 +62,9 @@ public class SentryUserFeedbackForm extends AlertDialog { private void maybeStartShakeDetection(final @NotNull Context context) { final @NotNull SentryFeedbackOptions globalFeedbackOptions = Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); - if (!resolvedFeedbackOptions.isUseShakeGesture() || globalFeedbackOptions.isUseShakeGesture()) { + + if (!resolvedFeedbackOptions.isUseShakeGesture() + || globalFeedbackOptions.getShakeController().isEnabled()) { return; } final @Nullable Activity activity = getActivity(context); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt index 170211abf55..6f96f2300d1 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt @@ -4,6 +4,7 @@ import android.app.Activity import android.app.Application import android.content.Context import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.Scopes import io.sentry.SentryFeedbackOptions import io.sentry.test.DeferredExecutorService @@ -16,6 +17,7 @@ import org.mockito.kotlin.atLeastOnce import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -165,4 +167,110 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut() sut.close() } + + @Test + fun `register sets itself as shake controller even when useShakeGesture is disabled`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + + assertThat(fixture.options.feedbackOptions.shakeController).isSameInstanceAs(sut) + assertThat(sut.isEnabled).isFalse() + } + + @Test + fun `enable after register starts shake detection at runtime`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + + sut.enable() + + assertThat(sut.isEnabled).isTrue() + verify(fixture.application).registerActivityLifecycleCallbacks(any()) + // Hooks into the already-resumed activity + verify(fixture.activity).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `enable is idempotent`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + + sut.enable() + sut.enable() + + verify(fixture.application, times(1)).registerActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable stops shake detection at runtime`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + sut.disable() + + assertThat(sut.isEnabled).isFalse() + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable is idempotent`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + sut.disable() + sut.disable() + + verify(fixture.application, times(1)).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable when never enabled does not unregister callbacks`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + + sut.disable() + + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `enable before register is a no-op`() { + val sut = fixture.getSut(useShakeGesture = false) + + sut.enable() + + assertThat(sut.isEnabled).isFalse() + verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + } + + @Test + fun `re-enable after disable re-arms shake detection`() { + val deferredExecutor = DeferredExecutorService() + fixture.options.executorService = deferredExecutor + whenever(fixture.application.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + sut.disable() + sut.enable() + + deferredExecutor.runAll() + + assertThat(sut.isEnabled).isTrue() + verify(fixture.application, atLeastOnce()).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `close disables shake detection`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + sut.close() + + assertThat(sut.isEnabled).isFalse() + } } diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.kt index 90e75feee71..6b2147edb19 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.kt @@ -805,21 +805,23 @@ fun UserFeedbackScreen() { } } - // Enable shake-to-show for a specific form instance + // Toggle shake-to-show at runtime using the global Sentry.feedback() API item(span = { GridItemSpan(maxLineSpan) }) { + var shakeEnabled by remember { mutableStateOf(Sentry.feedback().isFeedbackOnShakeEnabled) } Button( modifier = Modifier, onClick = { - SentryUserFeedbackForm.Builder(activity) - .configurator { options -> - options.isUseShakeGesture = true - options.formTitle = "Shake Feedback" - } - .create() - Toast.makeText(activity, "Shake your device to open the form!", Toast.LENGTH_SHORT).show() + if (shakeEnabled) { + Sentry.feedback().disableFeedbackOnShake() + } else { + Sentry.feedback().enableFeedbackOnShake() + Toast.makeText(activity, "Shake your device to open the form!", Toast.LENGTH_SHORT) + .show() + } + shakeEnabled = Sentry.feedback().isFeedbackOnShakeEnabled }, ) { - Text(text = "Enable Shake-to-Show") + Text(text = if (shakeEnabled) "Disable Shake-to-Show" else "Enable Shake-to-Show") } } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index c623e71d08f..761737de232 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -853,6 +853,9 @@ public abstract interface class io/sentry/IFeedbackApi { public abstract fun capture (Lio/sentry/protocol/Feedback;)Lio/sentry/protocol/SentryId; public abstract fun capture (Lio/sentry/protocol/Feedback;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public abstract fun capture (Lio/sentry/protocol/Feedback;Lio/sentry/Hint;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; + public abstract fun disableFeedbackOnShake ()V + public abstract fun enableFeedbackOnShake ()V + public abstract fun isFeedbackOnShakeEnabled ()Z public abstract fun show ()V public abstract fun show (Lio/sentry/SentryFeedbackOptions$OptionsConfigurator;)V public abstract fun show (Lio/sentry/protocol/SentryId;Lio/sentry/SentryFeedbackOptions$OptionsConfigurator;)V @@ -1604,7 +1607,10 @@ public final class io/sentry/NoOpFeedbackApi : io/sentry/IFeedbackApi { public fun capture (Lio/sentry/protocol/Feedback;)Lio/sentry/protocol/SentryId; public fun capture (Lio/sentry/protocol/Feedback;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun capture (Lio/sentry/protocol/Feedback;Lio/sentry/Hint;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; + public fun disableFeedbackOnShake ()V + public fun enableFeedbackOnShake ()V public static fun getInstance ()Lio/sentry/NoOpFeedbackApi; + public fun isFeedbackOnShakeEnabled ()Z public fun show ()V public fun show (Lio/sentry/SentryFeedbackOptions$OptionsConfigurator;)V public fun show (Lio/sentry/protocol/SentryId;Lio/sentry/SentryFeedbackOptions$OptionsConfigurator;)V @@ -3221,6 +3227,7 @@ public final class io/sentry/SentryFeedbackOptions { public fun getOnFormOpen ()Ljava/lang/Runnable; public fun getOnSubmitError ()Lio/sentry/SentryFeedbackOptions$SentryFeedbackCallback; public fun getOnSubmitSuccess ()Lio/sentry/SentryFeedbackOptions$SentryFeedbackCallback; + public fun getShakeController ()Lio/sentry/SentryFeedbackOptions$IShakeController; public fun getSubmitButtonLabel ()Ljava/lang/CharSequence; public fun getSuccessMessageText ()Ljava/lang/CharSequence; public fun isEmailRequired ()Z @@ -3246,6 +3253,7 @@ public final class io/sentry/SentryFeedbackOptions { public fun setOnFormOpen (Ljava/lang/Runnable;)V public fun setOnSubmitError (Lio/sentry/SentryFeedbackOptions$SentryFeedbackCallback;)V public fun setOnSubmitSuccess (Lio/sentry/SentryFeedbackOptions$SentryFeedbackCallback;)V + public fun setShakeController (Lio/sentry/SentryFeedbackOptions$IShakeController;)V public fun setShowBranding (Z)V public fun setShowEmail (Z)V public fun setShowName (Z)V @@ -3260,6 +3268,12 @@ public abstract interface class io/sentry/SentryFeedbackOptions$IFormHandler { public abstract fun showForm (Lio/sentry/protocol/SentryId;Lio/sentry/SentryFeedbackOptions$OptionsConfigurator;)V } +public abstract interface class io/sentry/SentryFeedbackOptions$IShakeController { + public abstract fun disable ()V + public abstract fun enable ()V + public abstract fun isEnabled ()Z +} + public abstract interface class io/sentry/SentryFeedbackOptions$OptionsConfigurator { public abstract fun configure (Lio/sentry/SentryFeedbackOptions;)V } diff --git a/sentry/src/main/java/io/sentry/FeedbackApi.java b/sentry/src/main/java/io/sentry/FeedbackApi.java index b8b8a3c9b9a..d6b3cc658bd 100644 --- a/sentry/src/main/java/io/sentry/FeedbackApi.java +++ b/sentry/src/main/java/io/sentry/FeedbackApi.java @@ -31,6 +31,21 @@ public void show( options.getFeedbackOptions().getFormHandler().showForm(associatedEventId, configurator); } + @Override + public void enableFeedbackOnShake() { + scopes.getOptions().getFeedbackOptions().getShakeController().enable(); + } + + @Override + public void disableFeedbackOnShake() { + scopes.getOptions().getFeedbackOptions().getShakeController().disable(); + } + + @Override + public boolean isFeedbackOnShakeEnabled() { + return scopes.getOptions().getFeedbackOptions().getShakeController().isEnabled(); + } + @Override public @NotNull SentryId capture(final @NotNull Feedback feedback) { return scopes.captureFeedback(feedback); diff --git a/sentry/src/main/java/io/sentry/IFeedbackApi.java b/sentry/src/main/java/io/sentry/IFeedbackApi.java index 5bab630fa8b..80fd980d3b5 100644 --- a/sentry/src/main/java/io/sentry/IFeedbackApi.java +++ b/sentry/src/main/java/io/sentry/IFeedbackApi.java @@ -2,6 +2,7 @@ import io.sentry.protocol.Feedback; import io.sentry.protocol.SentryId; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -15,6 +16,29 @@ void show( final @Nullable SentryId associatedEventId, final @Nullable SentryFeedbackOptions.OptionsConfigurator configurator); + /** + * Enables showing the feedback form when a shake gesture is detected, overriding {@link + * SentryFeedbackOptions#isUseShakeGesture()}. Only supported on Android; no-op on other + * platforms. + */ + void enableFeedbackOnShake(); + + /** + * Disables showing the feedback form when a shake gesture is detected, overriding {@link + * SentryFeedbackOptions#isUseShakeGesture()}. Only supported on Android; no-op on other + * platforms. + */ + void disableFeedbackOnShake(); + + /** + * Whether showing the feedback form on a shake gesture is currently enabled. Always {@code false} + * on non-Android platforms. + * + * @return true if the feedback form is shown when a shake gesture is detected + */ + @ApiStatus.Internal + boolean isFeedbackOnShakeEnabled(); + @NotNull SentryId capture(final @NotNull Feedback feedback); diff --git a/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java b/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java index bdef5d37590..6c884181a39 100644 --- a/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java +++ b/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java @@ -26,6 +26,17 @@ public void show( final @Nullable SentryId associatedEventId, final @Nullable SentryFeedbackOptions.OptionsConfigurator configurator) {} + @Override + public void enableFeedbackOnShake() {} + + @Override + public void disableFeedbackOnShake() {} + + @Override + public boolean isFeedbackOnShakeEnabled() { + return false; + } + @Override public @NotNull SentryId capture(final @NotNull Feedback feedback) { return SentryId.EMPTY_ID; diff --git a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java index a72b352317e..2f6b4e53976 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -93,8 +93,12 @@ public final class SentryFeedbackOptions { private @NotNull IFormHandler iFormHandler; - SentryFeedbackOptions(@NotNull IFormHandler iFormHandler) { + private @NotNull IShakeController shakeController; + + SentryFeedbackOptions( + final @NotNull IFormHandler iFormHandler, final @NotNull IShakeController shakeController) { this.iFormHandler = iFormHandler; + this.shakeController = shakeController; } /** Creates a copy of the passed {@link SentryFeedbackOptions}. */ @@ -122,6 +126,7 @@ public SentryFeedbackOptions(final @NotNull SentryFeedbackOptions other) { this.onSubmitSuccess = other.onSubmitSuccess; this.onSubmitError = other.onSubmitError; this.iFormHandler = other.iFormHandler; + this.shakeController = other.shakeController; } /** @@ -554,6 +559,26 @@ public void setFormHandler(final @NotNull IFormHandler iFormHandler) { return iFormHandler; } + /** + * Sets the controller to be used to enable/disable shake-to-report at runtime. + * + * @param shakeController the controller to be used to enable/disable shake-to-report at runtime + */ + @ApiStatus.Internal + public void setShakeController(final @NotNull IShakeController shakeController) { + this.shakeController = shakeController; + } + + /** + * Gets the controller to be used to enable/disable shake-to-report at runtime. + * + * @return the controller to be used to enable/disable shake-to-report at runtime + */ + @ApiStatus.Internal + public @NotNull IShakeController getShakeController() { + return shakeController; + } + @Override public String toString() { return "SentryFeedbackOptions{" @@ -615,6 +640,16 @@ void showForm( final @Nullable SentryFeedbackOptions.OptionsConfigurator configurator); } + /** Controls shake-to-report at runtime, overriding {@link #isUseShakeGesture()}. */ + @ApiStatus.Internal + public interface IShakeController { + void enable(); + + void disable(); + + boolean isEnabled(); + } + /** Configuration callback for feedback options. */ public interface OptionsConfigurator { diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index f10f2aede05..2e3f45d01fd 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3496,7 +3496,23 @@ private SentryOptions(final boolean empty) { feedbackOptions = new SentryFeedbackOptions( (associatedEventId, configurator) -> - logger.log(SentryLevel.WARNING, "showForm() can only be called in Android.")); + logger.log(SentryLevel.WARNING, "showForm() can only be called in Android."), + new SentryFeedbackOptions.IShakeController() { + @Override + public void enable() { + logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); + } + + @Override + public void disable() { + logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); + } + + @Override + public boolean isEnabled() { + return false; + } + }); if (!empty) { setSpanFactory(SpanFactoryFactory.create(new LoadClass(), NoOpLogger.getInstance())); diff --git a/sentry/src/test/java/io/sentry/FeedbackApiTest.kt b/sentry/src/test/java/io/sentry/FeedbackApiTest.kt new file mode 100644 index 00000000000..dd588dfd2ed --- /dev/null +++ b/sentry/src/test/java/io/sentry/FeedbackApiTest.kt @@ -0,0 +1,47 @@ +package io.sentry + +import com.google.common.truth.Truth.assertThat +import kotlin.test.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +class FeedbackApiTest { + + private class Fixture { + val shakeController = mock() + val options = SentryOptions().apply { feedbackOptions.setShakeController(shakeController) } + val scopes = mock().also { whenever(it.options).thenReturn(options) } + + fun getSut(): FeedbackApi = FeedbackApi(scopes) + } + + private val fixture = Fixture() + + @Test + fun `enableFeedbackOnShake delegates to the shake controller`() { + fixture.getSut().enableFeedbackOnShake() + + verify(fixture.shakeController).enable() + } + + @Test + fun `disableFeedbackOnShake delegates to the shake controller`() { + fixture.getSut().disableFeedbackOnShake() + + verify(fixture.shakeController).disable() + } + + @Test + fun `isFeedbackOnShakeEnabled delegates to the shake controller`() { + whenever(fixture.shakeController.isEnabled).thenReturn(true) + + assertThat(fixture.getSut().isFeedbackOnShakeEnabled).isTrue() + verify(fixture.shakeController).isEnabled + } + + @Test + fun `default shake controller is disabled`() { + assertThat(SentryOptions().feedbackOptions.shakeController.isEnabled).isFalse() + } +} diff --git a/sentry/src/test/java/io/sentry/SentryFeedbackOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryFeedbackOptionsTest.kt index e4b96cb17d0..c3e0dbd7303 100644 --- a/sentry/src/test/java/io/sentry/SentryFeedbackOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryFeedbackOptionsTest.kt @@ -1,6 +1,7 @@ package io.sentry import io.sentry.SentryFeedbackOptions.IFormHandler +import io.sentry.SentryFeedbackOptions.IShakeController import kotlin.test.Test import kotlin.test.assertEquals import org.mockito.kotlin.mock @@ -8,7 +9,7 @@ import org.mockito.kotlin.mock class SentryFeedbackOptionsTest { @Test fun `feedback options is initialized with default values`() { - val options = SentryFeedbackOptions(mock()) + val options = SentryFeedbackOptions(mock(), mock()) assertEquals(false, options.isNameRequired) assertEquals(true, options.isShowName) assertEquals(false, options.isEmailRequired) @@ -35,7 +36,7 @@ class SentryFeedbackOptionsTest { @Test fun `feedback options copy constructor`() { val options = - SentryFeedbackOptions(mock()).apply { + SentryFeedbackOptions(mock(), mock()).apply { isNameRequired = true isShowName = false isEmailRequired = true @@ -81,5 +82,6 @@ class SentryFeedbackOptionsTest { assertEquals(options.onSubmitSuccess, optionsCopy.onSubmitSuccess) assertEquals(options.onSubmitError, optionsCopy.onSubmitError) assertEquals(options.formHandler, optionsCopy.formHandler) + assertEquals(options.shakeController, optionsCopy.shakeController) } } diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index 9402c6fee9b..0ae479d848a 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -936,6 +936,18 @@ class SentryOptionsTest { verify(logger).log(eq(SentryLevel.WARNING), eq("showForm() can only be called in Android.")) } + @Test + fun `default shake controller logs a warning`() { + val logger = mock() + val options = + SentryOptions.empty().apply { + setLogger(logger) + isDebug = true + } + options.feedbackOptions.shakeController.enable() + verify(logger).log(eq(SentryLevel.WARNING), eq("Shake to report is only supported on Android.")) + } + @Test fun `autoTransactionDeadlineTimeoutMillis option defaults to 30000`() { val options = SentryOptions.empty() From 876b66dfdcf126881846e2caebb771c82a302e50 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 23 Jul 2026 18:12:41 +0200 Subject: [PATCH 2/7] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51076ce6f45..ee9191498ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Features -- Add `Sentry.feedback().enableFeedbackOnShake()` and `Sentry.feedback().disableFeedbackOnShake()` to toggle shake-to-report at runtime, e.g. based on an asynchronously fetched feature flag ([#5486](https://github.com/getsentry/sentry-java/issues/5486)) +- Add `Sentry.feedback().enableFeedbackOnShake()` and `Sentry.feedback().disableFeedbackOnShake()` to toggle shake-to-report at runtime, e.g. based on an asynchronously fetched feature flag ([#5827](https://github.com/getsentry/sentry-java/pull/5827)) ### Fixes From 6176c1dcc431a514263fdaf41a84067553532a9d Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 23 Jul 2026 22:03:08 +0200 Subject: [PATCH 3/7] Unify shake detection in FeedbackShakeIntegration via IShakeController.setDialog to prevent overlapping feedback dialogs --- .../api/sentry-android-core.api | 3 +- .../core/FeedbackShakeIntegration.java | 203 +++++++++++------- .../android/core/SentryUserFeedbackForm.java | 115 +--------- .../core/FeedbackShakeIntegrationTest.kt | 116 ++++++++++ sentry/api/sentry.api | 5 + .../java/io/sentry/SentryFeedbackOptions.java | 25 +++ .../main/java/io/sentry/SentryOptions.java | 9 + 7 files changed, 291 insertions(+), 185 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 8e513cacfae..0c2890427d4 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -307,6 +307,7 @@ public final class io/sentry/android/core/FeedbackShakeIntegration : android/app public fun onActivityStarted (Landroid/app/Activity;)V public fun onActivityStopped (Landroid/app/Activity;)V public fun register (Lio/sentry/IScopes;Lio/sentry/SentryOptions;)V + public fun setDialog (Lio/sentry/SentryFeedbackOptions$IShakeDialog;Z)V } public abstract interface class io/sentry/android/core/IDebugImagesLoader { @@ -556,7 +557,7 @@ public class io/sentry/android/core/SentryUserFeedbackDialog$Builder : io/sentry public abstract interface class io/sentry/android/core/SentryUserFeedbackDialog$OptionsConfiguration : io/sentry/android/core/SentryUserFeedbackForm$OptionsConfiguration { } -public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog { +public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog, io/sentry/SentryFeedbackOptions$IShakeDialog { protected fun onCreate (Landroid/os/Bundle;)V protected fun onStart ()V public fun setCancelable (Z)V diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java index 7ec9e3800aa..a228110513a 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java @@ -4,6 +4,9 @@ import android.app.Activity; import android.app.Application; +import android.app.Dialog; +import android.content.Context; +import android.content.ContextWrapper; import android.os.Bundle; import io.sentry.IScopes; import io.sentry.Integration; @@ -22,6 +25,11 @@ * io.sentry.SentryFeedbackOptions#isUseShakeGesture()} determines the initial state; it can be * toggled at runtime via {@code Sentry.feedback().enableFeedbackOnShake()} and {@code * Sentry.feedback().disableFeedbackOnShake()}. + * + *

A single detector serves both the global toggle and individual dialogs set via {@link + * #setDialog(SentryFeedbackOptions.IShakeDialog, boolean)}. While a dialog is tracked, a shake on + * its host activity re-shows that dialog instead of creating a new form — a no-op when it is + * already visible, so a shake can never stack a second form on top of one that is showing. */ public final class FeedbackShakeIntegration implements Integration, @@ -33,9 +41,10 @@ public final class FeedbackShakeIntegration private final @NotNull SentryShakeDetector shakeDetector; private @Nullable SentryAndroidOptions options; private volatile boolean enabled = false; + private boolean detecting = false; + private volatile @Nullable WeakReference dialogRef; + private boolean dialogRequestedShakeDetection = false; private volatile @Nullable WeakReference currentActivityRef; - private volatile boolean isDialogShowing = false; - private volatile @Nullable Runnable previousOnFormClose; public FeedbackShakeIntegration(final @NotNull Application application) { this.application = Objects.requireNonNull(application, "Application is required"); @@ -68,8 +77,57 @@ public synchronized void enable() { return; } enabled = true; + startDetecting(options); + } + + @Override + public synchronized void disable() { + if (!enabled) { + return; + } + enabled = false; + if (!dialogRequestedShakeDetection || getDialog() == null) { + stopDetecting(); + } + } + + @Override + public boolean isEnabled() { + return enabled; + } + + @Override + public synchronized void setDialog( + final @Nullable SentryFeedbackOptions.IShakeDialog dialog, + final boolean startShakeDetection) { + final @Nullable SentryAndroidOptions options = this.options; + if (options == null) { + return; + } + if (dialog == null) { + dialogRef = null; + dialogRequestedShakeDetection = false; + if (!enabled) { + stopDetecting(); + } + return; + } + dialogRef = new WeakReference<>(dialog); + if (startShakeDetection) { + dialogRequestedShakeDetection = true; + startDetecting(options); + } else { + dialogRequestedShakeDetection = false; + } + } + + private synchronized void startDetecting(final @NotNull SentryAndroidOptions options) { + if (detecting) { + return; + } + detecting = true; - // Re-arm the detector in case it was closed before, either by disable() or by a previous + // Re-arm the detector in case it was closed before, either by stopDetecting() or by a previous // close() (e.g. a second Sentry.init reusing the same options), otherwise the closed latch // would keep shake detection off permanently. shakeDetector.reopen(); @@ -99,51 +157,27 @@ public synchronized void enable() { } } - @Override - public synchronized void disable() { - if (!enabled) { + private synchronized void stopDetecting() { + if (!detecting) { return; } - enabled = false; + detecting = false; application.unregisterActivityLifecycleCallbacks(this); shakeDetector.close(); - // Restore onFormClose if a dialog is still showing, since lifecycle callbacks - // are now unregistered and onActivityDestroyed cleanup won't fire. The dialog - // itself stays on screen; only future shake detection stops. - if (isDialogShowing) { - isDialogShowing = false; - if (options != null) { - options.getFeedbackOptions().setOnFormClose(previousOnFormClose); - } - previousOnFormClose = null; - } currentActivityRef = null; } @Override - public boolean isEnabled() { - return enabled; - } - - @Override - public void close() throws IOException { - disable(); + public synchronized void close() throws IOException { + enabled = false; + dialogRef = null; + dialogRequestedShakeDetection = false; + stopDetecting(); } @Override public void onActivityResumed(final @NotNull Activity activity) { - // If a dialog is showing on a different activity (e.g. user navigated via notification), - // clean up since the dialog's host activity is going away and onActivityDestroyed - // won't match currentActivity anymore. - final @Nullable Activity current = currentActivityRef != null ? currentActivityRef.get() : null; - if (isDialogShowing && current != null && current != activity) { - isDialogShowing = false; - if (options != null) { - options.getFeedbackOptions().setOnFormClose(previousOnFormClose); - } - previousOnFormClose = null; - } currentActivityRef = new WeakReference<>(activity); startShakeDetection(activity); } @@ -153,16 +187,11 @@ public void onActivityPaused(final @NotNull Activity activity) { // Only stop if this is the activity we're tracking. When transitioning between // activities, B.onResume may fire before A.onPause — stopping unconditionally // would kill shake detection for the new activity. - final @Nullable Activity current = currentActivityRef != null ? currentActivityRef.get() : null; + final @Nullable WeakReference currentRef = currentActivityRef; + final @Nullable Activity current = currentRef != null ? currentRef.get() : null; if (activity == current) { stopShakeDetection(); - // Keep currentActivityRef set when a dialog is showing so onActivityDestroyed - // can still match and clean up. Otherwise the cleanup condition - // (activity == current) would always be false since onPause fires - // before onDestroy. - if (!isDialogShowing) { - currentActivityRef = null; - } + currentActivityRef = null; } } @@ -182,17 +211,31 @@ public void onActivitySaveInstanceState( @Override public void onActivityDestroyed(final @NotNull Activity activity) { - // Only reset if this is the activity that hosts the dialog — the dialog cannot - // outlive its host activity being destroyed. - final @Nullable Activity current = currentActivityRef != null ? currentActivityRef.get() : null; - if (isDialogShowing && activity == current) { - isDialogShowing = false; - currentActivityRef = null; - if (options != null) { - options.getFeedbackOptions().setOnFormClose(previousOnFormClose); + // A tracked dialog cannot outlive its host activity; drop it so detection doesn't keep + // running for it (and a shake can't try to show a dead dialog). + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + if (dialog != null && findDialogActivity(dialog) == activity) { + setDialog(null, false); + } + } + + private @Nullable SentryFeedbackOptions.IShakeDialog getDialog() { + final @Nullable WeakReference ref = dialogRef; + return ref != null ? ref.get() : null; + } + + private static @Nullable Activity findDialogActivity( + final @NotNull SentryFeedbackOptions.IShakeDialog dialog) { + if (dialog instanceof Dialog) { + @Nullable Context context = ((Dialog) dialog).getContext(); + while (context instanceof ContextWrapper) { + if (context instanceof Activity) { + return (Activity) context; + } + context = ((ContextWrapper) context).getBaseContext(); } - previousOnFormClose = null; } + return null; } private void startShakeDetection(final @NotNull Activity activity) { @@ -201,47 +244,47 @@ private void startShakeDetection(final @NotNull Activity activity) { } // Stop any existing detection (e.g. when transitioning between activities) stopShakeDetection(); + // When detection runs only for a tracked dialog, don't listen on other activities — + // a shake there couldn't show the dialog anyway. + if (!enabled) { + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + if (dialog == null || findDialogActivity(dialog) != activity) { + return; + } + } shakeDetector.start( activity, () -> { final @Nullable WeakReference ref = currentActivityRef; final Activity active = ref != null ? ref.get() : null; final Boolean inBackground = AppState.getInstance().isInBackground(); - if (active != null - && options != null - && !isDialogShowing - && !Boolean.TRUE.equals(inBackground)) { - active.runOnUiThread( - () -> { - if (isDialogShowing || active.isFinishing() || active.isDestroyed()) { - return; - } + if (active == null || options == null || Boolean.TRUE.equals(inBackground)) { + return; + } + // Decide on the main thread: show() sets the tracked dialog synchronously, so a + // second queued shake sees the form shown by the first instead of creating another. + active.runOnUiThread( + () -> { + if (active.isFinishing() || active.isDestroyed()) { + return; + } + // A dialog tracked for the active activity takes precedence over creating a + // new form — re-showing it is a no-op while it's already visible. + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + if (dialog != null && findDialogActivity(dialog) == active) { + dialog.show(); + return; + } + if (enabled) { try { - isDialogShowing = true; - final Runnable captured = options.getFeedbackOptions().getOnFormClose(); - previousOnFormClose = captured; - options - .getFeedbackOptions() - .setOnFormClose( - () -> { - isDialogShowing = false; - options.getFeedbackOptions().setOnFormClose(captured); - if (captured != null) { - captured.run(); - } - previousOnFormClose = null; - }); new SentryUserFeedbackForm.Builder(active).create().show(); } catch (Throwable e) { - isDialogShowing = false; - options.getFeedbackOptions().setOnFormClose(previousOnFormClose); - previousOnFormClose = null; options .getLogger() .log(SentryLevel.ERROR, "Failed to show feedback dialog on shake.", e); } - }); - } + } + }); }); } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java index 0c14e4fd3c4..ade5bbc9990 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java @@ -1,10 +1,7 @@ package io.sentry.android.core; -import android.app.Activity; import android.app.AlertDialog; -import android.app.Application; import android.content.Context; -import android.content.ContextWrapper; import android.os.Bundle; import android.view.View; import android.view.Window; @@ -23,11 +20,11 @@ import io.sentry.protocol.Feedback; import io.sentry.protocol.SentryId; import io.sentry.protocol.User; -import java.lang.ref.WeakReference; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public class SentryUserFeedbackForm extends AlertDialog { +public class SentryUserFeedbackForm extends AlertDialog + implements SentryFeedbackOptions.IShakeDialog { private boolean isCancelable = false; private @Nullable SentryId currentReplayId; @@ -36,9 +33,6 @@ public class SentryUserFeedbackForm extends AlertDialog { private final @NotNull SentryFeedbackOptions resolvedFeedbackOptions; - private @Nullable SentryShakeDetector shakeDetector; - private @Nullable Application.ActivityLifecycleCallbacks shakeLifecycleCallbacks; - SentryUserFeedbackForm( final @NotNull Context context, final int themeResId, @@ -56,111 +50,22 @@ public class SentryUserFeedbackForm extends AlertDialog { configurator.configure(resolvedFeedbackOptions); } SentryIntegrationPackageStorage.getInstance().addIntegration("UserFeedbackWidget"); - maybeStartShakeDetection(context); + maybeEnableShakeToShow(); } - private void maybeStartShakeDetection(final @NotNull Context context) { + private void maybeEnableShakeToShow() { final @NotNull SentryFeedbackOptions globalFeedbackOptions = Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); + // Only an explicit per-form opt-in registers this dialog for shake detection. When shake + // is configured globally (via the option or the runtime toggle), the integration already + // shows a form on shake and this dialog defers to it. if (!resolvedFeedbackOptions.isUseShakeGesture() + || globalFeedbackOptions.isUseShakeGesture() || globalFeedbackOptions.getShakeController().isEnabled()) { return; } - final @Nullable Activity activity = getActivity(context); - if (activity == null) { - return; - } - final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); - shakeDetector = new SentryShakeDetector(options.getLogger()); - final @NotNull WeakReference activityRef = new WeakReference<>(activity); - shakeDetector.start(activity, shakeListener(activityRef)); - final @NotNull Application app = activity.getApplication(); - shakeLifecycleCallbacks = new ShakeLifecycleCallbacks(activityRef); - app.registerActivityLifecycleCallbacks(shakeLifecycleCallbacks); - } - - private void stopShakeDetection() { - if (shakeDetector != null) { - shakeDetector.close(); - shakeDetector = null; - } - if (shakeLifecycleCallbacks != null) { - final @Nullable Activity activity = getActivity(getContext()); - if (activity != null) { - activity.getApplication().unregisterActivityLifecycleCallbacks(shakeLifecycleCallbacks); - } - shakeLifecycleCallbacks = null; - } - } - - private @NotNull SentryShakeDetector.Listener shakeListener( - final @NotNull WeakReference activityRef) { - return () -> { - final @Nullable Activity active = activityRef.get(); - if (active != null && !active.isFinishing() && !active.isDestroyed()) { - active.runOnUiThread( - () -> { - if (!active.isFinishing() && !active.isDestroyed()) { - show(); - } - }); - } - }; - } - - private static @Nullable Activity getActivity(final @NotNull Context context) { - Context current = context; - while (current instanceof ContextWrapper) { - if (current instanceof Activity) { - return (Activity) current; - } - current = ((ContextWrapper) current).getBaseContext(); - } - return null; - } - - private class ShakeLifecycleCallbacks implements Application.ActivityLifecycleCallbacks { - private final @NotNull WeakReference activityRef; - - ShakeLifecycleCallbacks(final @NotNull WeakReference activityRef) { - this.activityRef = activityRef; - } - - @Override - public void onActivityResumed(final @NotNull Activity activity) { - if (activity == activityRef.get() && shakeDetector != null) { - shakeDetector.start(activity, shakeListener(activityRef)); - } - } - - @Override - public void onActivityPaused(final @NotNull Activity activity) { - if (activity == activityRef.get() && shakeDetector != null) { - shakeDetector.stop(); - } - } - - @Override - public void onActivityDestroyed(final @NotNull Activity activity) { - if (activity == activityRef.get()) { - stopShakeDetection(); - } - } - - @Override - public void onActivityCreated( - final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {} - - @Override - public void onActivityStarted(final @NotNull Activity activity) {} - - @Override - public void onActivityStopped(final @NotNull Activity activity) {} - - @Override - public void onActivitySaveInstanceState( - final @NotNull Activity activity, final @NotNull Bundle outState) {} + globalFeedbackOptions.getShakeController().setDialog(this, true); } @Override @@ -334,6 +239,8 @@ protected void onStart() { final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); + // Track this form so a shake re-shows it instead of stacking a second one on top + feedbackOptions.getShakeController().setDialog(this, false); final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { onFormOpen.run(); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt index 6f96f2300d1..309ec64d903 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt @@ -2,6 +2,7 @@ package io.sentry.android.core import android.app.Activity import android.app.Application +import android.app.Dialog import android.content.Context import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat @@ -20,6 +21,7 @@ import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever +import org.robolectric.Robolectric @RunWith(AndroidJUnit4::class) class FeedbackShakeIntegrationTest { @@ -273,4 +275,118 @@ class FeedbackShakeIntegrationTest { assertThat(sut.isEnabled).isFalse() } + + private fun createShakeDialog(): TestShakeDialog { + val activity = Robolectric.buildActivity(Activity::class.java).setup().get() + return TestShakeDialog(activity) + } + + private class TestShakeDialog(val activity: Activity) : + Dialog(activity), SentryFeedbackOptions.IShakeDialog + + @Test + fun `setDialog with startShakeDetection starts detection without enabling the global toggle`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + + sut.setDialog(createShakeDialog(), true) + + verify(fixture.application).registerActivityLifecycleCallbacks(any()) + assertThat(sut.isEnabled).isFalse() + } + + @Test + fun `setDialog without startShakeDetection only tracks the dialog`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + + sut.setDialog(createShakeDialog(), false) + + verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + } + + @Test + fun `setDialog with null stops shake detection when globally disabled`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), true) + + sut.setDialog(null, false) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `setDialog with null keeps shake detection when globally enabled`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), true) + + sut.setDialog(null, false) + + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable keeps shake detection while an opted-in dialog is tracked`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, true) + + sut.disable() + + assertThat(sut.isEnabled).isFalse() + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + + // Once the dialog is cleared, nothing keeps detection alive anymore + sut.setDialog(null, false) + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable stops shake detection when the tracked dialog did not opt in`() { + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), false) + + sut.disable() + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `re-setting the same opted-in dialog keeps detection alive`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, true) + + // The dialog reports itself again when shown + sut.setDialog(dialog, false) + sut.disable() + + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `destroying the dialog host activity clears the dialog and stops detection`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, true) + + sut.onActivityDestroyed(dialog.activity) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `setDialog before register is a no-op`() { + val sut = fixture.getSut(useShakeGesture = false) + + sut.setDialog(createShakeDialog(), true) + + verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 761737de232..2039a80df4b 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3272,6 +3272,11 @@ public abstract interface class io/sentry/SentryFeedbackOptions$IShakeController public abstract fun disable ()V public abstract fun enable ()V public abstract fun isEnabled ()Z + public abstract fun setDialog (Lio/sentry/SentryFeedbackOptions$IShakeDialog;Z)V +} + +public abstract interface class io/sentry/SentryFeedbackOptions$IShakeDialog { + public abstract fun show ()V } public abstract interface class io/sentry/SentryFeedbackOptions$OptionsConfigurator { diff --git a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java index 2f6b4e53976..c66ce6e979d 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -648,6 +648,31 @@ public interface IShakeController { void disable(); boolean isEnabled(); + + /** + * Sets the dialog a detected shake should (re-)show instead of creating a new one. Re-showing + * an already visible dialog is a no-op, so a shake can never stack a second dialog on top of + * it. The controller tracks at most one dialog: the one most recently set. The dialog is + * tracked until its host activity is destroyed or it is replaced by another dialog. + * + *

With {@code startShakeDetection} set to {@code true} (a per-dialog shake opt-in), shake + * detection is also started and kept alive independently of the global enable/disable toggle. + * With {@code false} (a dialog merely became visible), the detection state is left untouched. + * + *

Passing a {@code null} dialog clears the tracked dialog and stops shake detection unless + * it is enabled globally. + * + * @param dialog the dialog to show on shake, or {@code null} to clear + * @param startShakeDetection whether the dialog should also start and keep alive shake + * detection + */ + void setDialog(@Nullable IShakeDialog dialog, boolean startShakeDetection); + } + + /** A dialog that can be shown when a shake gesture is detected. */ + @ApiStatus.Internal + public interface IShakeDialog { + void show(); } /** Configuration callback for feedback options. */ diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 2e3f45d01fd..7d55c1464a7 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3512,6 +3512,15 @@ public void disable() { public boolean isEnabled() { return false; } + + @Override + public void setDialog( + final @Nullable SentryFeedbackOptions.IShakeDialog dialog, + final boolean startShakeDetection) { + if (startShakeDetection) { + logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); + } + } }); if (!empty) { From b7096cba664d64056714e38524859d9ea5d470fe Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 24 Jul 2026 07:19:21 +0200 Subject: [PATCH 4/7] Hold tracked shake dialog strongly and re-register opt-in on every show --- .../core/FeedbackShakeIntegration.java | 31 +++++++++-------- .../android/core/SentryUserFeedbackForm.java | 24 +++++++------- .../core/FeedbackShakeIntegrationTest.kt | 33 +++++++++++++++++-- .../java/io/sentry/SentryFeedbackOptions.java | 8 +++-- 4 files changed, 62 insertions(+), 34 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java index a228110513a..3674c56b251 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java @@ -42,7 +42,10 @@ public final class FeedbackShakeIntegration private @Nullable SentryAndroidOptions options; private volatile boolean enabled = false; private boolean detecting = false; - private volatile @Nullable WeakReference dialogRef; + // Strong reference on purpose: for a per-form opt-in the caller may not retain the created + // form, so the controller must keep it alive to be able to show it on shake. Cleared when the + // host activity is destroyed, when replaced by another dialog, and on close(). + private volatile @Nullable SentryFeedbackOptions.IShakeDialog trackedDialog; private boolean dialogRequestedShakeDetection = false; private volatile @Nullable WeakReference currentActivityRef; @@ -86,7 +89,7 @@ public synchronized void disable() { return; } enabled = false; - if (!dialogRequestedShakeDetection || getDialog() == null) { + if (!dialogRequestedShakeDetection || trackedDialog == null) { stopDetecting(); } } @@ -105,19 +108,20 @@ public synchronized void setDialog( return; } if (dialog == null) { - dialogRef = null; + trackedDialog = null; dialogRequestedShakeDetection = false; if (!enabled) { stopDetecting(); } return; } - dialogRef = new WeakReference<>(dialog); + trackedDialog = dialog; + dialogRequestedShakeDetection = startShakeDetection; if (startShakeDetection) { - dialogRequestedShakeDetection = true; startDetecting(options); - } else { - dialogRequestedShakeDetection = false; + } else if (!enabled) { + // The previous dialog may have been the only reason detection was running. + stopDetecting(); } } @@ -171,7 +175,7 @@ private synchronized void stopDetecting() { @Override public synchronized void close() throws IOException { enabled = false; - dialogRef = null; + trackedDialog = null; dialogRequestedShakeDetection = false; stopDetecting(); } @@ -213,17 +217,12 @@ public void onActivitySaveInstanceState( public void onActivityDestroyed(final @NotNull Activity activity) { // A tracked dialog cannot outlive its host activity; drop it so detection doesn't keep // running for it (and a shake can't try to show a dead dialog). - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; if (dialog != null && findDialogActivity(dialog) == activity) { setDialog(null, false); } } - private @Nullable SentryFeedbackOptions.IShakeDialog getDialog() { - final @Nullable WeakReference ref = dialogRef; - return ref != null ? ref.get() : null; - } - private static @Nullable Activity findDialogActivity( final @NotNull SentryFeedbackOptions.IShakeDialog dialog) { if (dialog instanceof Dialog) { @@ -247,7 +246,7 @@ private void startShakeDetection(final @NotNull Activity activity) { // When detection runs only for a tracked dialog, don't listen on other activities — // a shake there couldn't show the dialog anyway. if (!enabled) { - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; if (dialog == null || findDialogActivity(dialog) != activity) { return; } @@ -270,7 +269,7 @@ private void startShakeDetection(final @NotNull Activity activity) { } // A dialog tracked for the active activity takes precedence over creating a // new form — re-showing it is a no-op while it's already visible. - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = getDialog(); + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; if (dialog != null && findDialogActivity(dialog) == active) { dialog.show(); return; diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java index ade5bbc9990..a531de7260d 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java @@ -33,6 +33,9 @@ public class SentryUserFeedbackForm extends AlertDialog private final @NotNull SentryFeedbackOptions resolvedFeedbackOptions; + /** Whether this form instance opted into shake-to-show independently of the global toggle. */ + private final boolean useShakeGesture; + SentryUserFeedbackForm( final @NotNull Context context, final int themeResId, @@ -50,22 +53,19 @@ public class SentryUserFeedbackForm extends AlertDialog configurator.configure(resolvedFeedbackOptions); } SentryIntegrationPackageStorage.getInstance().addIntegration("UserFeedbackWidget"); - maybeEnableShakeToShow(); - } - - private void maybeEnableShakeToShow() { - final @NotNull SentryFeedbackOptions globalFeedbackOptions = - Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); // Only an explicit per-form opt-in registers this dialog for shake detection. When shake // is configured globally (via the option or the runtime toggle), the integration already // shows a form on shake and this dialog defers to it. - if (!resolvedFeedbackOptions.isUseShakeGesture() - || globalFeedbackOptions.isUseShakeGesture() - || globalFeedbackOptions.getShakeController().isEnabled()) { - return; + final @NotNull SentryFeedbackOptions globalFeedbackOptions = + Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); + this.useShakeGesture = + resolvedFeedbackOptions.isUseShakeGesture() + && !globalFeedbackOptions.isUseShakeGesture() + && !globalFeedbackOptions.getShakeController().isEnabled(); + if (useShakeGesture) { + globalFeedbackOptions.getShakeController().setDialog(this, true); } - globalFeedbackOptions.getShakeController().setDialog(this, true); } @Override @@ -240,7 +240,7 @@ protected void onStart() { final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); // Track this form so a shake re-shows it instead of stacking a second one on top - feedbackOptions.getShakeController().setDialog(this, false); + feedbackOptions.getShakeController().setDialog(this, useShakeGesture); final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { onFormOpen.run(); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt index 309ec64d903..98ba2665cfb 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt @@ -356,14 +356,41 @@ class FeedbackShakeIntegrationTest { } @Test - fun `re-setting the same opted-in dialog keeps detection alive`() { + fun `re-setting an opted-in dialog keeps detection alive`() { val sut = fixture.getSut(useShakeGesture = false) sut.register(fixture.scopes, fixture.options) val dialog = createShakeDialog() sut.setDialog(dialog, true) - // The dialog reports itself again when shown - sut.setDialog(dialog, false) + // An opted-in dialog reports itself again with startShakeDetection on every show + sut.setDialog(dialog, true) + + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + verify(fixture.application, times(1)).registerActivityLifecycleCallbacks(any()) + } + + @Test + fun `replacing an opted-in dialog with a tracking-only one stops detection when globally disabled`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), true) + + // A different dialog only reporting visibility no longer justifies detection + sut.setDialog(createShakeDialog(), false) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `disable keeps detection alive after an opted-in dialog re-registers on show`() { + // Regression: global toggle on, opted-in dialog shown (re-registers), runtime disable — + // the opt-in must keep detection running. + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, true) + sut.setDialog(dialog, true) + sut.disable() verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) diff --git a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java index c66ce6e979d..b016ef163ae 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -656,11 +656,13 @@ public interface IShakeController { * tracked until its host activity is destroyed or it is replaced by another dialog. * *

With {@code startShakeDetection} set to {@code true} (a per-dialog shake opt-in), shake - * detection is also started and kept alive independently of the global enable/disable toggle. - * With {@code false} (a dialog merely became visible), the detection state is left untouched. + * detection is also started and kept alive independently of the global enable/disable toggle. A + * dialog that opted in must pass {@code true} again whenever it re-registers (e.g. on every + * show); the flag always reflects the latest call. * *

Passing a {@code null} dialog clears the tracked dialog and stops shake detection unless - * it is enabled globally. + * it is enabled globally. The controller holds a strong reference to the dialog until then, so + * an opted-in form stays reachable even if the creating code does not retain it. * * @param dialog the dialog to show on shake, or {@code null} to clear * @param startShakeDetection whether the dialog should also start and keep alive shake From 2d4e9fb715ad784e38116ebb6ae018aa8bf94729 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 24 Jul 2026 07:43:20 +0200 Subject: [PATCH 5/7] Keep lifecycle callbacks registered while a dialog is tracked so the strong dialog reference cannot leak its activity --- .../core/FeedbackShakeIntegration.java | 42 ++++++++-- .../core/FeedbackShakeIntegrationTest.kt | 76 +++++++++++++++++-- 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java index 3674c56b251..f28acfb12b7 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java @@ -43,10 +43,12 @@ public final class FeedbackShakeIntegration private volatile boolean enabled = false; private boolean detecting = false; // Strong reference on purpose: for a per-form opt-in the caller may not retain the created - // form, so the controller must keep it alive to be able to show it on shake. Cleared when the - // host activity is destroyed, when replaced by another dialog, and on close(). + // form, so the controller must keep it alive to be able to show it on shake. Lifecycle + // callbacks stay registered as long as a dialog is tracked, so the reference is guaranteed + // to be cleared once the host activity goes away (or another activity is created on top). private volatile @Nullable SentryFeedbackOptions.IShakeDialog trackedDialog; private boolean dialogRequestedShakeDetection = false; + private boolean callbacksRegistered = false; private volatile @Nullable WeakReference currentActivityRef; public FeedbackShakeIntegration(final @NotNull Application application) { @@ -113,6 +115,7 @@ public synchronized void setDialog( if (!enabled) { stopDetecting(); } + updateCallbackRegistration(); return; } trackedDialog = dialog; @@ -123,6 +126,24 @@ public synchronized void setDialog( // The previous dialog may have been the only reason detection was running. stopDetecting(); } + // Even without detection, keep listening for the tracked dialog's host activity being + // destroyed, so the strong dialog reference can never outlive it (no activity leak). + updateCallbackRegistration(); + } + + /** + * Lifecycle callbacks are needed while shake detection runs (to follow the current activity) or + * while a dialog is tracked (to release it when its host activity goes away). + */ + private synchronized void updateCallbackRegistration() { + final boolean needed = detecting || trackedDialog != null; + if (needed && !callbacksRegistered) { + callbacksRegistered = true; + application.registerActivityLifecycleCallbacks(this); + } else if (!needed && callbacksRegistered) { + callbacksRegistered = false; + application.unregisterActivityLifecycleCallbacks(this); + } } private synchronized void startDetecting(final @NotNull SentryAndroidOptions options) { @@ -150,7 +171,7 @@ private synchronized void startDetecting(final @NotNull SentryAndroidOptions opt } addIntegrationToSdkVersion("FeedbackShake"); - application.registerActivityLifecycleCallbacks(this); + updateCallbackRegistration(); options.getLogger().log(SentryLevel.DEBUG, "FeedbackShakeIntegration installed."); // In case of a deferred init or runtime enable, hook into any already-resumed activity @@ -167,7 +188,7 @@ private synchronized void stopDetecting() { } detecting = false; - application.unregisterActivityLifecycleCallbacks(this); + updateCallbackRegistration(); shakeDetector.close(); currentActivityRef = null; } @@ -178,6 +199,9 @@ public synchronized void close() throws IOException { trackedDialog = null; dialogRequestedShakeDetection = false; stopDetecting(); + // stopDetecting is a no-op when detection wasn't running, but a tracking-only dialog may + // still have kept the callbacks registered. + updateCallbackRegistration(); } @Override @@ -201,7 +225,15 @@ public void onActivityPaused(final @NotNull Activity activity) { @Override public void onActivityCreated( - final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {} + final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) { + // The user is navigating to a new activity: a dialog hosted by a different activity can't + // be shown there, so stop tracking it (also releasing the strong reference early instead + // of waiting for the host activity to be destroyed). + final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; + if (dialog != null && findDialogActivity(dialog) != activity) { + setDialog(null, false); + } + } @Override public void onActivityStarted(final @NotNull Activity activity) {} diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt index 98ba2665cfb..53cf337dd3c 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt @@ -296,13 +296,66 @@ class FeedbackShakeIntegrationTest { } @Test - fun `setDialog without startShakeDetection only tracks the dialog`() { + fun `setDialog without startShakeDetection tracks the dialog but does not start detection`() { + whenever(fixture.application.getSystemService(any())).thenReturn(null) val sut = fixture.getSut(useShakeGesture = false) sut.register(fixture.scopes, fixture.options) sut.setDialog(createShakeDialog(), false) - verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + // Callbacks are registered to release the dialog on activity destroy, but the shake + // detector itself is not started. + verify(fixture.application).registerActivityLifecycleCallbacks(any()) + verify(fixture.application, never()).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `clearing a tracking-only dialog unregisters the callbacks`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), false) + + sut.setDialog(null, false) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `destroying the host activity of a tracking-only dialog releases it`() { + // Guards against leaking the dialog (and its activity) through the strong reference when + // detection is not running. + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, false) + + sut.onActivityDestroyed(dialog.activity) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `creating a different activity releases the tracked dialog`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + sut.setDialog(createShakeDialog(), false) + + val otherActivity = Robolectric.buildActivity(Activity::class.java).setup().get() + sut.onActivityCreated(otherActivity, null) + + verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) + } + + @Test + fun `creating the dialog's own host activity keeps the tracked dialog`() { + val sut = fixture.getSut(useShakeGesture = false) + sut.register(fixture.scopes, fixture.options) + val dialog = createShakeDialog() + sut.setDialog(dialog, false) + + sut.onActivityCreated(dialog.activity, null) + + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) } @Test @@ -345,13 +398,18 @@ class FeedbackShakeIntegrationTest { } @Test - fun `disable stops shake detection when the tracked dialog did not opt in`() { + fun `disable keeps callbacks registered while a tracking-only dialog is set`() { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), false) + val dialog = createShakeDialog() + sut.setDialog(dialog, false) sut.disable() + // Detection stops, but the callbacks stay registered to release the tracked dialog once + // its host activity goes away. + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + sut.onActivityDestroyed(dialog.activity) verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) } @@ -370,14 +428,18 @@ class FeedbackShakeIntegrationTest { } @Test - fun `replacing an opted-in dialog with a tracking-only one stops detection when globally disabled`() { + fun `replacing an opted-in dialog with a tracking-only one drops the opt-in`() { val sut = fixture.getSut(useShakeGesture = false) sut.register(fixture.scopes, fixture.options) sut.setDialog(createShakeDialog(), true) - // A different dialog only reporting visibility no longer justifies detection - sut.setDialog(createShakeDialog(), false) + // A different dialog only reporting visibility no longer justifies detection; the + // callbacks stay registered only to track the new dialog's host activity. + val dialog = createShakeDialog() + sut.setDialog(dialog, false) + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) + sut.setDialog(null, false) verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) } From 62205866315ff42cc822824b249a2b0bfb224425 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 24 Jul 2026 08:21:21 +0200 Subject: [PATCH 6/7] Replace dialog tracking with IShakeController.pauseDetection to prevent stacked feedback dialogs --- .../api/sentry-android-core.api | 5 +- .../core/FeedbackShakeIntegration.java | 191 +++------------- .../android/core/SentryUserFeedbackForm.java | 161 ++++++++++++-- .../core/FeedbackShakeIntegrationTest.kt | 203 +----------------- sentry/api/sentry.api | 6 +- .../java/io/sentry/SentryFeedbackOptions.java | 28 +-- .../main/java/io/sentry/SentryOptions.java | 8 +- 7 files changed, 196 insertions(+), 406 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 0c2890427d4..6f5227ec9ac 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -306,8 +306,8 @@ public final class io/sentry/android/core/FeedbackShakeIntegration : android/app public fun onActivitySaveInstanceState (Landroid/app/Activity;Landroid/os/Bundle;)V public fun onActivityStarted (Landroid/app/Activity;)V public fun onActivityStopped (Landroid/app/Activity;)V + public fun pauseDetection (Z)V public fun register (Lio/sentry/IScopes;Lio/sentry/SentryOptions;)V - public fun setDialog (Lio/sentry/SentryFeedbackOptions$IShakeDialog;Z)V } public abstract interface class io/sentry/android/core/IDebugImagesLoader { @@ -557,9 +557,10 @@ public class io/sentry/android/core/SentryUserFeedbackDialog$Builder : io/sentry public abstract interface class io/sentry/android/core/SentryUserFeedbackDialog$OptionsConfiguration : io/sentry/android/core/SentryUserFeedbackForm$OptionsConfiguration { } -public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog, io/sentry/SentryFeedbackOptions$IShakeDialog { +public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog { protected fun onCreate (Landroid/os/Bundle;)V protected fun onStart ()V + protected fun onStop ()V public fun setCancelable (Z)V public fun setOnDismissListener (Landroid/content/DialogInterface$OnDismissListener;)V public fun show ()V diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java index f28acfb12b7..b0ccda495e1 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java @@ -4,9 +4,6 @@ import android.app.Activity; import android.app.Application; -import android.app.Dialog; -import android.content.Context; -import android.content.ContextWrapper; import android.os.Bundle; import io.sentry.IScopes; import io.sentry.Integration; @@ -26,10 +23,9 @@ * toggled at runtime via {@code Sentry.feedback().enableFeedbackOnShake()} and {@code * Sentry.feedback().disableFeedbackOnShake()}. * - *

A single detector serves both the global toggle and individual dialogs set via {@link - * #setDialog(SentryFeedbackOptions.IShakeDialog, boolean)}. While a dialog is tracked, a shake on - * its host activity re-shows that dialog instead of creating a new form — a no-op when it is - * already visible, so a shake can never stack a second form on top of one that is showing. + *

While any feedback dialog is visible it pauses shake handling via {@link + * #pauseDetection(boolean)}, so a shake can never stack a second dialog on top of one that is + * already showing — no matter how the visible dialog was opened. */ public final class FeedbackShakeIntegration implements Integration, @@ -41,14 +37,7 @@ public final class FeedbackShakeIntegration private final @NotNull SentryShakeDetector shakeDetector; private @Nullable SentryAndroidOptions options; private volatile boolean enabled = false; - private boolean detecting = false; - // Strong reference on purpose: for a per-form opt-in the caller may not retain the created - // form, so the controller must keep it alive to be able to show it on shake. Lifecycle - // callbacks stay registered as long as a dialog is tracked, so the reference is guaranteed - // to be cleared once the host activity goes away (or another activity is created on top). - private volatile @Nullable SentryFeedbackOptions.IShakeDialog trackedDialog; - private boolean dialogRequestedShakeDetection = false; - private boolean callbacksRegistered = false; + private volatile boolean paused = false; private volatile @Nullable WeakReference currentActivityRef; public FeedbackShakeIntegration(final @NotNull Application application) { @@ -82,77 +71,8 @@ public synchronized void enable() { return; } enabled = true; - startDetecting(options); - } - - @Override - public synchronized void disable() { - if (!enabled) { - return; - } - enabled = false; - if (!dialogRequestedShakeDetection || trackedDialog == null) { - stopDetecting(); - } - } - - @Override - public boolean isEnabled() { - return enabled; - } - - @Override - public synchronized void setDialog( - final @Nullable SentryFeedbackOptions.IShakeDialog dialog, - final boolean startShakeDetection) { - final @Nullable SentryAndroidOptions options = this.options; - if (options == null) { - return; - } - if (dialog == null) { - trackedDialog = null; - dialogRequestedShakeDetection = false; - if (!enabled) { - stopDetecting(); - } - updateCallbackRegistration(); - return; - } - trackedDialog = dialog; - dialogRequestedShakeDetection = startShakeDetection; - if (startShakeDetection) { - startDetecting(options); - } else if (!enabled) { - // The previous dialog may have been the only reason detection was running. - stopDetecting(); - } - // Even without detection, keep listening for the tracked dialog's host activity being - // destroyed, so the strong dialog reference can never outlive it (no activity leak). - updateCallbackRegistration(); - } - - /** - * Lifecycle callbacks are needed while shake detection runs (to follow the current activity) or - * while a dialog is tracked (to release it when its host activity goes away). - */ - private synchronized void updateCallbackRegistration() { - final boolean needed = detecting || trackedDialog != null; - if (needed && !callbacksRegistered) { - callbacksRegistered = true; - application.registerActivityLifecycleCallbacks(this); - } else if (!needed && callbacksRegistered) { - callbacksRegistered = false; - application.unregisterActivityLifecycleCallbacks(this); - } - } - private synchronized void startDetecting(final @NotNull SentryAndroidOptions options) { - if (detecting) { - return; - } - detecting = true; - - // Re-arm the detector in case it was closed before, either by stopDetecting() or by a previous + // Re-arm the detector in case it was closed before, either by disable() or by a previous // close() (e.g. a second Sentry.init reusing the same options), otherwise the closed latch // would keep shake detection off permanently. shakeDetector.reopen(); @@ -171,7 +91,7 @@ private synchronized void startDetecting(final @NotNull SentryAndroidOptions opt } addIntegrationToSdkVersion("FeedbackShake"); - updateCallbackRegistration(); + application.registerActivityLifecycleCallbacks(this); options.getLogger().log(SentryLevel.DEBUG, "FeedbackShakeIntegration installed."); // In case of a deferred init or runtime enable, hook into any already-resumed activity @@ -182,26 +102,31 @@ private synchronized void startDetecting(final @NotNull SentryAndroidOptions opt } } - private synchronized void stopDetecting() { - if (!detecting) { + @Override + public synchronized void disable() { + if (!enabled) { return; } - detecting = false; + enabled = false; - updateCallbackRegistration(); + application.unregisterActivityLifecycleCallbacks(this); shakeDetector.close(); currentActivityRef = null; } @Override - public synchronized void close() throws IOException { - enabled = false; - trackedDialog = null; - dialogRequestedShakeDetection = false; - stopDetecting(); - // stopDetecting is a no-op when detection wasn't running, but a tracking-only dialog may - // still have kept the callbacks registered. - updateCallbackRegistration(); + public boolean isEnabled() { + return enabled; + } + + @Override + public void pauseDetection(final boolean paused) { + this.paused = paused; + } + + @Override + public void close() throws IOException { + disable(); } @Override @@ -225,15 +150,7 @@ public void onActivityPaused(final @NotNull Activity activity) { @Override public void onActivityCreated( - final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) { - // The user is navigating to a new activity: a dialog hosted by a different activity can't - // be shown there, so stop tracking it (also releasing the strong reference early instead - // of waiting for the host activity to be destroyed). - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; - if (dialog != null && findDialogActivity(dialog) != activity) { - setDialog(null, false); - } - } + final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {} @Override public void onActivityStarted(final @NotNull Activity activity) {} @@ -246,28 +163,7 @@ public void onActivitySaveInstanceState( final @NotNull Activity activity, final @NotNull Bundle outState) {} @Override - public void onActivityDestroyed(final @NotNull Activity activity) { - // A tracked dialog cannot outlive its host activity; drop it so detection doesn't keep - // running for it (and a shake can't try to show a dead dialog). - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; - if (dialog != null && findDialogActivity(dialog) == activity) { - setDialog(null, false); - } - } - - private static @Nullable Activity findDialogActivity( - final @NotNull SentryFeedbackOptions.IShakeDialog dialog) { - if (dialog instanceof Dialog) { - @Nullable Context context = ((Dialog) dialog).getContext(); - while (context instanceof ContextWrapper) { - if (context instanceof Activity) { - return (Activity) context; - } - context = ((ContextWrapper) context).getBaseContext(); - } - } - return null; - } + public void onActivityDestroyed(final @NotNull Activity activity) {} private void startShakeDetection(final @NotNull Activity activity) { if (options == null) { @@ -275,45 +171,28 @@ private void startShakeDetection(final @NotNull Activity activity) { } // Stop any existing detection (e.g. when transitioning between activities) stopShakeDetection(); - // When detection runs only for a tracked dialog, don't listen on other activities — - // a shake there couldn't show the dialog anyway. - if (!enabled) { - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; - if (dialog == null || findDialogActivity(dialog) != activity) { - return; - } - } shakeDetector.start( activity, () -> { final @Nullable WeakReference ref = currentActivityRef; final Activity active = ref != null ? ref.get() : null; final Boolean inBackground = AppState.getInstance().isInBackground(); - if (active == null || options == null || Boolean.TRUE.equals(inBackground)) { + if (active == null || options == null || paused || Boolean.TRUE.equals(inBackground)) { return; } - // Decide on the main thread: show() sets the tracked dialog synchronously, so a - // second queued shake sees the form shown by the first instead of creating another. active.runOnUiThread( () -> { - if (active.isFinishing() || active.isDestroyed()) { - return; - } - // A dialog tracked for the active activity takes precedence over creating a - // new form — re-showing it is a no-op while it's already visible. - final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog; - if (dialog != null && findDialogActivity(dialog) == active) { - dialog.show(); + // Re-check on the main thread: an earlier queued shake may have shown a form + // in the meantime (the form pauses detection synchronously in onStart). + if (paused || active.isFinishing() || active.isDestroyed()) { return; } - if (enabled) { - try { - new SentryUserFeedbackForm.Builder(active).create().show(); - } catch (Throwable e) { - options - .getLogger() - .log(SentryLevel.ERROR, "Failed to show feedback dialog on shake.", e); - } + try { + new SentryUserFeedbackForm.Builder(active).create().show(); + } catch (Throwable e) { + options + .getLogger() + .log(SentryLevel.ERROR, "Failed to show feedback dialog on shake.", e); } }); }); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java index a531de7260d..c6453ef1849 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java @@ -1,7 +1,10 @@ package io.sentry.android.core; +import android.app.Activity; import android.app.AlertDialog; +import android.app.Application; import android.content.Context; +import android.content.ContextWrapper; import android.os.Bundle; import android.view.View; import android.view.Window; @@ -20,11 +23,11 @@ import io.sentry.protocol.Feedback; import io.sentry.protocol.SentryId; import io.sentry.protocol.User; +import java.lang.ref.WeakReference; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public class SentryUserFeedbackForm extends AlertDialog - implements SentryFeedbackOptions.IShakeDialog { +public class SentryUserFeedbackForm extends AlertDialog { private boolean isCancelable = false; private @Nullable SentryId currentReplayId; @@ -33,8 +36,8 @@ public class SentryUserFeedbackForm extends AlertDialog private final @NotNull SentryFeedbackOptions resolvedFeedbackOptions; - /** Whether this form instance opted into shake-to-show independently of the global toggle. */ - private final boolean useShakeGesture; + private @Nullable SentryShakeDetector shakeDetector; + private @Nullable Application.ActivityLifecycleCallbacks shakeLifecycleCallbacks; SentryUserFeedbackForm( final @NotNull Context context, @@ -53,19 +56,123 @@ public class SentryUserFeedbackForm extends AlertDialog configurator.configure(resolvedFeedbackOptions); } SentryIntegrationPackageStorage.getInstance().addIntegration("UserFeedbackWidget"); + maybeStartShakeDetection(context); + } - // Only an explicit per-form opt-in registers this dialog for shake detection. When shake - // is configured globally (via the option or the runtime toggle), the integration already - // shows a form on shake and this dialog defers to it. + private void maybeStartShakeDetection(final @NotNull Context context) { + // Only an explicit per-form opt-in starts a detector for this form. When shake is + // configured globally (via the option or the runtime toggle), FeedbackShakeIntegration + // already shows a form on shake and this form defers to it. final @NotNull SentryFeedbackOptions globalFeedbackOptions = Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); - this.useShakeGesture = - resolvedFeedbackOptions.isUseShakeGesture() - && !globalFeedbackOptions.isUseShakeGesture() - && !globalFeedbackOptions.getShakeController().isEnabled(); - if (useShakeGesture) { - globalFeedbackOptions.getShakeController().setDialog(this, true); + if (!resolvedFeedbackOptions.isUseShakeGesture() + || globalFeedbackOptions.isUseShakeGesture() + || globalFeedbackOptions.getShakeController().isEnabled()) { + return; + } + final @Nullable Activity activity = getActivity(context); + if (activity == null) { + return; } + final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); + shakeDetector = new SentryShakeDetector(options.getLogger()); + final @NotNull WeakReference activityRef = new WeakReference<>(activity); + shakeDetector.start(activity, shakeListener(activityRef)); + final @NotNull Application app = activity.getApplication(); + shakeLifecycleCallbacks = new ShakeLifecycleCallbacks(activityRef); + app.registerActivityLifecycleCallbacks(shakeLifecycleCallbacks); + } + + private void stopShakeDetection() { + if (shakeDetector != null) { + shakeDetector.close(); + shakeDetector = null; + } + if (shakeLifecycleCallbacks != null) { + final @Nullable Activity activity = getActivity(getContext()); + if (activity != null) { + activity.getApplication().unregisterActivityLifecycleCallbacks(shakeLifecycleCallbacks); + } + shakeLifecycleCallbacks = null; + } + } + + private @NotNull SentryShakeDetector.Listener shakeListener( + final @NotNull WeakReference activityRef) { + return () -> { + // If shake-to-report got enabled globally in the meantime, FeedbackShakeIntegration + // reacts to the same shake — don't show a second dialog for it. + if (Sentry.getCurrentScopes() + .getOptions() + .getFeedbackOptions() + .getShakeController() + .isEnabled()) { + return; + } + final @Nullable Activity active = activityRef.get(); + if (active != null && !active.isFinishing() && !active.isDestroyed()) { + active.runOnUiThread( + () -> { + if (!active.isFinishing() && !active.isDestroyed()) { + show(); + } + }); + } + }; + } + + private static @Nullable Activity getActivity(final @NotNull Context context) { + Context current = context; + while (current instanceof ContextWrapper) { + if (current instanceof Activity) { + return (Activity) current; + } + current = ((ContextWrapper) current).getBaseContext(); + } + return null; + } + + private class ShakeLifecycleCallbacks implements Application.ActivityLifecycleCallbacks { + private final @NotNull WeakReference activityRef; + + ShakeLifecycleCallbacks(final @NotNull WeakReference activityRef) { + this.activityRef = activityRef; + } + + @Override + public void onActivityResumed(final @NotNull Activity activity) { + if (activity == activityRef.get() && shakeDetector != null) { + shakeDetector.start(activity, shakeListener(activityRef)); + } + } + + @Override + public void onActivityPaused(final @NotNull Activity activity) { + if (activity == activityRef.get() && shakeDetector != null) { + shakeDetector.stop(); + } + } + + @Override + public void onActivityDestroyed(final @NotNull Activity activity) { + if (activity == activityRef.get()) { + stopShakeDetection(); + } + } + + @Override + public void onActivityCreated( + final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {} + + @Override + public void onActivityStarted(final @NotNull Activity activity) {} + + @Override + public void onActivityStopped(final @NotNull Activity activity) {} + + @Override + public void onActivitySaveInstanceState( + final @NotNull Activity activity, final @NotNull Bundle outState) {} } @Override @@ -239,8 +346,9 @@ protected void onStart() { final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); - // Track this form so a shake re-shows it instead of stacking a second one on top - feedbackOptions.getShakeController().setDialog(this, useShakeGesture); + // Pause shake-to-report while this form is visible, so a shake can't stack a second + // form on top of it + feedbackOptions.getShakeController().pauseDetection(true); final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { onFormOpen.run(); @@ -249,6 +357,29 @@ protected void onStart() { currentReplayId = options.getReplayController().getReplayId(); } + @Override + protected void onStop() { + super.onStop(); + Sentry.getCurrentScopes() + .getOptions() + .getFeedbackOptions() + .getShakeController() + .pauseDetection(false); + } + + @Override + public void onDetachedFromWindow() { + super.onDetachedFromWindow(); + // Safety net for teardown without a dismiss (e.g. the host activity is destroyed while the + // form is still showing): onStop never fires then, but the window is still detached — + // without this, shake-to-report would stay paused forever. + Sentry.getCurrentScopes() + .getOptions() + .getFeedbackOptions() + .getShakeController() + .pauseDetection(false); + } + @Override public void show() { // If Sentry is disabled, don't show the dialog, but log a warning diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt index 53cf337dd3c..2b3b629cdd2 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/FeedbackShakeIntegrationTest.kt @@ -2,7 +2,6 @@ package io.sentry.android.core import android.app.Activity import android.app.Application -import android.app.Dialog import android.content.Context import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat @@ -21,7 +20,6 @@ import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever -import org.robolectric.Robolectric @RunWith(AndroidJUnit4::class) class FeedbackShakeIntegrationTest { @@ -276,206 +274,15 @@ class FeedbackShakeIntegrationTest { assertThat(sut.isEnabled).isFalse() } - private fun createShakeDialog(): TestShakeDialog { - val activity = Robolectric.buildActivity(Activity::class.java).setup().get() - return TestShakeDialog(activity) - } - - private class TestShakeDialog(val activity: Activity) : - Dialog(activity), SentryFeedbackOptions.IShakeDialog - - @Test - fun `setDialog with startShakeDetection starts detection without enabling the global toggle`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - - sut.setDialog(createShakeDialog(), true) - - verify(fixture.application).registerActivityLifecycleCallbacks(any()) - assertThat(sut.isEnabled).isFalse() - } - - @Test - fun `setDialog without startShakeDetection tracks the dialog but does not start detection`() { - whenever(fixture.application.getSystemService(any())).thenReturn(null) - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - - sut.setDialog(createShakeDialog(), false) - - // Callbacks are registered to release the dialog on activity destroy, but the shake - // detector itself is not started. - verify(fixture.application).registerActivityLifecycleCallbacks(any()) - verify(fixture.application, never()).getSystemService(eq(Context.SENSOR_SERVICE)) - } - - @Test - fun `clearing a tracking-only dialog unregisters the callbacks`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), false) - - sut.setDialog(null, false) - - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `destroying the host activity of a tracking-only dialog releases it`() { - // Guards against leaking the dialog (and its activity) through the strong reference when - // detection is not running. - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, false) - - sut.onActivityDestroyed(dialog.activity) - - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `creating a different activity releases the tracked dialog`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), false) - - val otherActivity = Robolectric.buildActivity(Activity::class.java).setup().get() - sut.onActivityCreated(otherActivity, null) - - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `creating the dialog's own host activity keeps the tracked dialog`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, false) - - sut.onActivityCreated(dialog.activity, null) - - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `setDialog with null stops shake detection when globally disabled`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), true) - - sut.setDialog(null, false) - - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `setDialog with null keeps shake detection when globally enabled`() { - val sut = fixture.getSut(useShakeGesture = true) - sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), true) - - sut.setDialog(null, false) - - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `disable keeps shake detection while an opted-in dialog is tracked`() { - val sut = fixture.getSut(useShakeGesture = true) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, true) - - sut.disable() - - assertThat(sut.isEnabled).isFalse() - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - - // Once the dialog is cleared, nothing keeps detection alive anymore - sut.setDialog(null, false) - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `disable keeps callbacks registered while a tracking-only dialog is set`() { - val sut = fixture.getSut(useShakeGesture = true) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, false) - - sut.disable() - - // Detection stops, but the callbacks stay registered to release the tracked dialog once - // its host activity goes away. - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - sut.onActivityDestroyed(dialog.activity) - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `re-setting an opted-in dialog keeps detection alive`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, true) - - // An opted-in dialog reports itself again with startShakeDetection on every show - sut.setDialog(dialog, true) - - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - verify(fixture.application, times(1)).registerActivityLifecycleCallbacks(any()) - } - - @Test - fun `replacing an opted-in dialog with a tracking-only one drops the opt-in`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - sut.setDialog(createShakeDialog(), true) - - // A different dialog only reporting visibility no longer justifies detection; the - // callbacks stay registered only to track the new dialog's host activity. - val dialog = createShakeDialog() - sut.setDialog(dialog, false) - verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - - sut.setDialog(null, false) - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - @Test - fun `disable keeps detection alive after an opted-in dialog re-registers on show`() { - // Regression: global toggle on, opted-in dialog shown (re-registers), runtime disable — - // the opt-in must keep detection running. + fun `pauseDetection toggles the paused state`() { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, true) - sut.setDialog(dialog, true) - - sut.disable() + sut.pauseDetection(true) + sut.pauseDetection(false) + // Pausing only gates shake handling; detection machinery stays untouched verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `destroying the dialog host activity clears the dialog and stops detection`() { - val sut = fixture.getSut(useShakeGesture = false) - sut.register(fixture.scopes, fixture.options) - val dialog = createShakeDialog() - sut.setDialog(dialog, true) - - sut.onActivityDestroyed(dialog.activity) - - verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) - } - - @Test - fun `setDialog before register is a no-op`() { - val sut = fixture.getSut(useShakeGesture = false) - - sut.setDialog(createShakeDialog(), true) - - verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) + assertThat(sut.isEnabled).isTrue() } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 2039a80df4b..11c58ddf325 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3272,11 +3272,7 @@ public abstract interface class io/sentry/SentryFeedbackOptions$IShakeController public abstract fun disable ()V public abstract fun enable ()V public abstract fun isEnabled ()Z - public abstract fun setDialog (Lio/sentry/SentryFeedbackOptions$IShakeDialog;Z)V -} - -public abstract interface class io/sentry/SentryFeedbackOptions$IShakeDialog { - public abstract fun show ()V + public abstract fun pauseDetection (Z)V } public abstract interface class io/sentry/SentryFeedbackOptions$OptionsConfigurator { diff --git a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java index b016ef163ae..35fa8d0df4b 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -650,31 +650,13 @@ public interface IShakeController { boolean isEnabled(); /** - * Sets the dialog a detected shake should (re-)show instead of creating a new one. Re-showing - * an already visible dialog is a no-op, so a shake can never stack a second dialog on top of - * it. The controller tracks at most one dialog: the one most recently set. The dialog is - * tracked until its host activity is destroyed or it is replaced by another dialog. + * Pauses or resumes reacting to detected shakes without tearing down shake detection. Feedback + * dialogs pause detection while they are visible, so a shake can never stack a second dialog on + * top of one that is already showing — no matter how the visible dialog was opened. * - *

With {@code startShakeDetection} set to {@code true} (a per-dialog shake opt-in), shake - * detection is also started and kept alive independently of the global enable/disable toggle. A - * dialog that opted in must pass {@code true} again whenever it re-registers (e.g. on every - * show); the flag always reflects the latest call. - * - *

Passing a {@code null} dialog clears the tracked dialog and stops shake detection unless - * it is enabled globally. The controller holds a strong reference to the dialog until then, so - * an opted-in form stays reachable even if the creating code does not retain it. - * - * @param dialog the dialog to show on shake, or {@code null} to clear - * @param startShakeDetection whether the dialog should also start and keep alive shake - * detection + * @param paused true to ignore detected shakes, false to react to them again */ - void setDialog(@Nullable IShakeDialog dialog, boolean startShakeDetection); - } - - /** A dialog that can be shown when a shake gesture is detected. */ - @ApiStatus.Internal - public interface IShakeDialog { - void show(); + void pauseDetection(boolean paused); } /** Configuration callback for feedback options. */ diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 7d55c1464a7..9e99473fdb8 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3514,13 +3514,7 @@ public boolean isEnabled() { } @Override - public void setDialog( - final @Nullable SentryFeedbackOptions.IShakeDialog dialog, - final boolean startShakeDetection) { - if (startShakeDetection) { - logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); - } - } + public void pauseDetection(final boolean paused) {} }); if (!empty) { From 252b24c0013949f8124e36e0bc21510514352d3a Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 24 Jul 2026 08:31:19 +0200 Subject: [PATCH 7/7] Guard user-facing feedback form callbacks with try-catch so a crashing callback cannot crash the app --- .../android/core/SentryUserFeedbackForm.java | 34 +++++++++++-- .../core/SentryUserFeedbackFormTest.kt | 48 +++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java index c6453ef1849..2ad374d5393 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java @@ -298,13 +298,27 @@ protected void onCreate(Bundle savedInstanceState) { final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitSuccess = feedbackOptions.getOnSubmitSuccess(); if (onSubmitSuccess != null) { - onSubmitSuccess.call(feedback); + try { + onSubmitSuccess.call(feedback); + } catch (Throwable e) { + Sentry.getCurrentScopes() + .getOptions() + .getLogger() + .log(SentryLevel.ERROR, "onSubmitSuccess callback threw an exception.", e); + } } } else { final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitError = feedbackOptions.getOnSubmitError(); if (onSubmitError != null) { - onSubmitError.call(feedback); + try { + onSubmitError.call(feedback); + } catch (Throwable e) { + Sentry.getCurrentScopes() + .getOptions() + .getLogger() + .log(SentryLevel.ERROR, "onSubmitError callback threw an exception.", e); + } } } cancel(); @@ -324,7 +338,15 @@ public void setOnDismissListener(final @Nullable OnDismissListener listener) { if (onFormClose != null) { super.setOnDismissListener( dialog -> { - onFormClose.run(); + // User-provided callback: a crash in it must not take down the app or skip the + // cleanup and the user's own dismiss listener below + try { + onFormClose.run(); + } catch (Throwable e) { + options + .getLogger() + .log(SentryLevel.ERROR, "onFormClose callback threw an exception.", e); + } currentReplayId = null; if (delegate != null) { delegate.onDismiss(dialog); @@ -351,7 +373,11 @@ protected void onStart() { feedbackOptions.getShakeController().pauseDetection(true); final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { - onFormOpen.run(); + try { + onFormOpen.run(); + } catch (Throwable e) { + options.getLogger().log(SentryLevel.ERROR, "onFormOpen callback threw an exception.", e); + } } options.getReplayController().captureReplay(false); currentReplayId = options.getReplayController().getReplayId(); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SentryUserFeedbackFormTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SentryUserFeedbackFormTest.kt index 9df2a16d72e..4fb96ab8e90 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SentryUserFeedbackFormTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SentryUserFeedbackFormTest.kt @@ -1,6 +1,7 @@ package io.sentry.android.core import android.content.Context +import android.os.Looper import android.view.WindowManager import android.widget.TextView import androidx.test.core.app.ApplicationProvider @@ -19,13 +20,16 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals import kotlin.test.assertNotNull +import kotlin.test.assertTrue import org.junit.runner.RunWith import org.mockito.Mockito.mockStatic +import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever +import org.robolectric.Shadows.shadowOf @RunWith(AndroidJUnit4::class) class SentryUserFeedbackFormTest { @@ -143,4 +147,48 @@ class SentryUserFeedbackFormTest { val flags = window.attributes.flags assertEquals(0, flags and WindowManager.LayoutParams.FLAG_ALT_FOCUSABLE_IM) } + + @Test + fun `a crashing onFormClose callback does not crash the app when the dialog is closed`() { + fixture.options.isEnabled = true + fixture.options.feedbackOptions.onFormClose = Runnable { throw RuntimeException("user bug") } + val sut = fixture.getSut() + sut.show() + + sut.dismiss() + // The dismiss listener is dispatched via a Handler message + shadowOf(Looper.getMainLooper()).idle() + + verify(fixture.mockLogger) + .log(eq(SentryLevel.ERROR), eq("onFormClose callback threw an exception."), any()) + } + + @Test + fun `a crashing onFormClose callback still runs the user's dismiss listener`() { + fixture.options.isEnabled = true + fixture.options.feedbackOptions.onFormClose = Runnable { throw RuntimeException("user bug") } + val sut = fixture.getSut() + var dismissed = false + sut.setOnDismissListener { dismissed = true } + sut.show() + + sut.dismiss() + shadowOf(Looper.getMainLooper()).idle() + + assertTrue(dismissed) + } + + @Test + fun `a crashing onFormOpen callback does not crash the app when the dialog is shown`() { + fixture.options.isEnabled = true + fixture.options.feedbackOptions.onFormOpen = Runnable { throw RuntimeException("user bug") } + val sut = fixture.getSut() + + sut.show() + + verify(fixture.mockLogger) + .log(eq(SentryLevel.ERROR), eq("onFormOpen callback threw an exception."), any()) + // The form open must still complete its own work after the callback crash + verify(fixture.mockReplayController).captureReplay(eq(false)) + } }