From 849f6b8041f16244cd1c152a81d7dc1f14113067 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 23 Jul 2026 18:06:45 +0200 Subject: [PATCH 01/13] 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 02/13] 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 03/13] 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 04/13] 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 05/13] 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 06/13] 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 07/13] 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)) + } } From e5d6f39abcb3ba98789271edc9bc8e5f3864900e Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 11 Aug 2026 14:21:37 +0200 Subject: [PATCH 08/13] fix(feedback): Address PR feedback on shake-to-report runtime API - Scope shake suppression to the activity a feedback form is showing on, instead of a global paused flag. A dialog lives in its activity's window and is never notified when that activity is backgrounded, so tracking the owning activity removes the lifecycle heuristics entirely - Re-check enabled on shake dispatch so a concurrent disableOnShake() cannot still show a form - Narrow user-callback guards from Throwable to Exception so Errors propagate - Rename isFeedbackOnShakeEnabled() to isOnShakeEnabled() for consistency with enableOnShake()/disableOnShake(), and drop @ApiStatus.Internal Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- .../api/sentry-android-core.api | 9 +- .../core/FeedbackShakeIntegration.java | 75 ++++++++++---- .../android/core/SentryUserFeedbackForm.java | 26 ++--- .../core/FeedbackShakeIntegrationTest.kt | 98 +++++++++++++++---- .../io/sentry/samples/android/MainActivity.kt | 8 +- sentry/api/sentry.api | 20 ++-- .../src/main/java/io/sentry/FeedbackApi.java | 12 +-- .../src/main/java/io/sentry/IFeedbackApi.java | 8 +- .../main/java/io/sentry/NoOpFeedbackApi.java | 6 +- .../java/io/sentry/SentryFeedbackOptions.java | 8 +- .../main/java/io/sentry/SentryOptions.java | 8 +- .../test/java/io/sentry/FeedbackApiTest.kt | 22 ++--- .../test/java/io/sentry/SentryOptionsTest.kt | 2 +- 14 files changed, 201 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abee0c33746..db6ce7aff28 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 ([#5827](https://github.com/getsentry/sentry-java/pull/5827)) +- Add `Sentry.feedback().enableOnShake()`, `Sentry.feedback().disableOnShake()`, and `Sentry.feedback().isOnShakeEnabled()` to toggle and query shake-to-report at runtime ([#5827](https://github.com/getsentry/sentry-java/pull/5827)) ### Improvements diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 6f5227ec9ac..364b149eafb 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -296,9 +296,9 @@ public abstract class io/sentry/android/core/EnvelopeFileObserverIntegration : i 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 disableOnShake ()V + public fun enableOnShake ()V + public fun isOnShakeEnabled ()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 @@ -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 setOnShakePaused (Z)V } public abstract interface class io/sentry/android/core/IDebugImagesLoader { @@ -559,6 +559,7 @@ public abstract interface class io/sentry/android/core/SentryUserFeedbackDialog$ public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog { protected fun onCreate (Landroid/os/Bundle;)V + public fun onDetachedFromWindow ()V protected fun onStart ()V protected fun onStop ()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 b0ccda495e1..3f93f96f294 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 @@ -16,16 +16,19 @@ import java.lang.ref.WeakReference; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.TestOnly; /** * 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()}. + * toggled at runtime via {@code Sentry.feedback().enableOnShake()} and {@code + * Sentry.feedback().disableOnShake()}. * - *

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. + *

Shake detection is scoped to the resumed activity: a dialog belongs to the window of the + * activity that created it, so it can only ever be visible while that activity is resumed. Forms + * report themselves via {@link #setOnShakePaused(boolean)} and detection is then suppressed for + * that one activity, which keeps a shake from stacking a second dialog on top of a visible one + * without letting a dialog on a backgrounded activity suppress detection elsewhere. */ public final class FeedbackShakeIntegration implements Integration, @@ -37,9 +40,11 @@ public final class FeedbackShakeIntegration private final @NotNull SentryShakeDetector shakeDetector; private @Nullable SentryAndroidOptions options; private volatile boolean enabled = false; - private volatile boolean paused = false; private volatile @Nullable WeakReference currentActivityRef; + /** The activity a feedback form is currently showing on, if any. */ + private volatile @Nullable WeakReference formActivityRef; + public FeedbackShakeIntegration(final @NotNull Application application) { this.application = Objects.requireNonNull(application, "Application is required"); this.shakeDetector = new SentryShakeDetector(io.sentry.NoOpLogger.getInstance()); @@ -60,12 +65,12 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions options.getFeedbackOptions().setShakeController(this); if (options.getFeedbackOptions().isUseShakeGesture()) { - enable(); + enableOnShake(); } } @Override - public synchronized void enable() { + public synchronized void enableOnShake() { final @Nullable SentryAndroidOptions options = this.options; if (enabled || options == null) { return; @@ -103,7 +108,7 @@ public synchronized void enable() { } @Override - public synchronized void disable() { + public synchronized void disableOnShake() { if (!enabled) { return; } @@ -115,18 +120,42 @@ public synchronized void disable() { } @Override - public boolean isEnabled() { + public boolean isOnShakeEnabled() { return enabled; } @Override - public void pauseDetection(final boolean paused) { - this.paused = paused; + public void setOnShakePaused(final boolean paused) { + if (paused) { + final @Nullable Activity activity = CurrentActivityHolder.getInstance().getActivity(); + formActivityRef = activity == null ? null : new WeakReference<>(activity); + stopShakeDetection(); + } else { + formActivityRef = null; + // The form is gone, so detection can resume for whichever activity is currently resumed. + final @Nullable WeakReference currentRef = currentActivityRef; + final @Nullable Activity current = currentRef == null ? null : currentRef.get(); + if (enabled && current != null) { + startShakeDetection(current); + } + } + } + + private boolean hasFormOn(final @NotNull Activity activity) { + final @Nullable WeakReference ref = formActivityRef; + return ref != null && ref.get() == activity; + } + + @TestOnly + @Nullable + Activity getFormActivity() { + final @Nullable WeakReference ref = formActivityRef; + return ref == null ? null : ref.get(); } @Override public void close() throws IOException { - disable(); + disableOnShake(); } @Override @@ -171,20 +200,32 @@ private void startShakeDetection(final @NotNull Activity activity) { } // Stop any existing detection (e.g. when transitioning between activities) stopShakeDetection(); + // A form is already visible here, so a shake could only stack a second one on top of it. + // The form has no detector of its own in this case: SentryUserFeedbackForm only starts one + // while shake-to-report is globally disabled, which is exactly when this integration is not + // detecting either. + if (hasFormOn(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 || paused || Boolean.TRUE.equals(inBackground)) { + if (active == null + || options == null + || !enabled + || hasFormOn(active) + || Boolean.TRUE.equals(inBackground)) { return; } active.runOnUiThread( () -> { - // 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()) { + // Re-check on the main thread: shake-to-report may have been disabled, or an + // earlier queued shake may have shown a form in the meantime (the form reports + // itself synchronously in onStart). + if (!enabled || hasFormOn(active) || active.isFinishing() || active.isDestroyed()) { return; } try { 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 2ad374d5393..7772005cd1e 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 @@ -60,14 +60,16 @@ public class SentryUserFeedbackForm extends AlertDialog { } 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. + // Only a per-form opt-in on top of a globally disabled shake gesture starts a detector for + // this form. Both other cases defer to FeedbackShakeIntegration: while shake-to-report is + // enabled it already shows a form on shake, and once it has been disabled at runtime that + // must stay disabled. Note resolvedFeedbackOptions is a copy of the global options, so its + // isUseShakeGesture() is indistinguishable from the global one unless a configurator set it. final @NotNull SentryFeedbackOptions globalFeedbackOptions = Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); if (!resolvedFeedbackOptions.isUseShakeGesture() || globalFeedbackOptions.isUseShakeGesture() - || globalFeedbackOptions.getShakeController().isEnabled()) { + || globalFeedbackOptions.getShakeController().isOnShakeEnabled()) { return; } final @Nullable Activity activity = getActivity(context); @@ -106,7 +108,7 @@ private void stopShakeDetection() { .getOptions() .getFeedbackOptions() .getShakeController() - .isEnabled()) { + .isOnShakeEnabled()) { return; } final @Nullable Activity active = activityRef.get(); @@ -300,7 +302,7 @@ protected void onCreate(Bundle savedInstanceState) { if (onSubmitSuccess != null) { try { onSubmitSuccess.call(feedback); - } catch (Throwable e) { + } catch (Exception e) { Sentry.getCurrentScopes() .getOptions() .getLogger() @@ -313,7 +315,7 @@ protected void onCreate(Bundle savedInstanceState) { if (onSubmitError != null) { try { onSubmitError.call(feedback); - } catch (Throwable e) { + } catch (Exception e) { Sentry.getCurrentScopes() .getOptions() .getLogger() @@ -342,7 +344,7 @@ public void setOnDismissListener(final @Nullable OnDismissListener listener) { // cleanup and the user's own dismiss listener below try { onFormClose.run(); - } catch (Throwable e) { + } catch (Exception e) { options .getLogger() .log(SentryLevel.ERROR, "onFormClose callback threw an exception.", e); @@ -370,12 +372,12 @@ protected void onStart() { final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); // 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); + feedbackOptions.getShakeController().setOnShakePaused(true); final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { try { onFormOpen.run(); - } catch (Throwable e) { + } catch (Exception e) { options.getLogger().log(SentryLevel.ERROR, "onFormOpen callback threw an exception.", e); } } @@ -390,7 +392,7 @@ protected void onStop() { .getOptions() .getFeedbackOptions() .getShakeController() - .pauseDetection(false); + .setOnShakePaused(false); } @Override @@ -403,7 +405,7 @@ public void onDetachedFromWindow() { .getOptions() .getFeedbackOptions() .getShakeController() - .pauseDetection(false); + .setOnShakePaused(false); } @Override 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 2b3b629cdd2..e7efc4ce183 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 @@ -174,7 +174,7 @@ class FeedbackShakeIntegrationTest { sut.register(fixture.scopes, fixture.options) assertThat(fixture.options.feedbackOptions.shakeController).isSameInstanceAs(sut) - assertThat(sut.isEnabled).isFalse() + assertThat(sut.isOnShakeEnabled).isFalse() } @Test @@ -186,9 +186,9 @@ class FeedbackShakeIntegrationTest { sut.register(fixture.scopes, fixture.options) verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) - sut.enable() + sut.enableOnShake() - assertThat(sut.isEnabled).isTrue() + assertThat(sut.isOnShakeEnabled).isTrue() verify(fixture.application).registerActivityLifecycleCallbacks(any()) // Hooks into the already-resumed activity verify(fixture.activity).getSystemService(eq(Context.SENSOR_SERVICE)) @@ -199,8 +199,8 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = false) sut.register(fixture.scopes, fixture.options) - sut.enable() - sut.enable() + sut.enableOnShake() + sut.enableOnShake() verify(fixture.application, times(1)).registerActivityLifecycleCallbacks(any()) } @@ -210,9 +210,9 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.disable() + sut.disableOnShake() - assertThat(sut.isEnabled).isFalse() + assertThat(sut.isOnShakeEnabled).isFalse() verify(fixture.application).unregisterActivityLifecycleCallbacks(any()) } @@ -221,8 +221,8 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.disable() - sut.disable() + sut.disableOnShake() + sut.disableOnShake() verify(fixture.application, times(1)).unregisterActivityLifecycleCallbacks(any()) } @@ -232,7 +232,7 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = false) sut.register(fixture.scopes, fixture.options) - sut.disable() + sut.disableOnShake() verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) } @@ -241,9 +241,9 @@ class FeedbackShakeIntegrationTest { fun `enable before register is a no-op`() { val sut = fixture.getSut(useShakeGesture = false) - sut.enable() + sut.enableOnShake() - assertThat(sut.isEnabled).isFalse() + assertThat(sut.isOnShakeEnabled).isFalse() verify(fixture.application, never()).registerActivityLifecycleCallbacks(any()) } @@ -255,12 +255,12 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.disable() - sut.enable() + sut.disableOnShake() + sut.enableOnShake() deferredExecutor.runAll() - assertThat(sut.isEnabled).isTrue() + assertThat(sut.isOnShakeEnabled).isTrue() verify(fixture.application, atLeastOnce()).getSystemService(eq(Context.SENSOR_SERVICE)) } @@ -271,18 +271,74 @@ class FeedbackShakeIntegrationTest { sut.close() - assertThat(sut.isEnabled).isFalse() + assertThat(sut.isOnShakeEnabled).isFalse() } @Test - fun `pauseDetection toggles the paused state`() { + fun `a visible form does not tear down the detection machinery`() { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.pauseDetection(true) - sut.pauseDetection(false) - // Pausing only gates shake handling; detection machinery stays untouched + sut.setOnShakePaused(true) + sut.setOnShakePaused(false) + verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) - assertThat(sut.isEnabled).isTrue() + assertThat(sut.isOnShakeEnabled).isTrue() + } + + @Test + fun `a form suppresses detection on the activity it belongs to`() { + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.setOnShakePaused(true) + assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + + // Coming back to the activity the form is on (e.g. screen off/on) must not re-arm detection, + // otherwise a shake would stack a second form on top of the visible one. + sut.onActivityResumed(fixture.activity) + + verify(fixture.activity, never()).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `a form on a backgrounded activity does not suppress detection on the next one`() { + // A dialog lives in the window of the activity that created it, so once that activity is no + // longer resumed the dialog cannot be seen - it must not keep detection off on the activity + // now in front. Android's order is A.onPause() -> B.onResume(), so exercise exactly that. + val otherActivity = mock() + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + whenever(otherActivity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.onActivityResumed(fixture.activity) + sut.setOnShakePaused(true) + + sut.onActivityPaused(fixture.activity) + sut.onActivityResumed(otherActivity) + + verify(otherActivity).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `dismissing a form re-arms detection on the current activity`() { + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.onActivityResumed(fixture.activity) + sut.setOnShakePaused(true) + sut.setOnShakePaused(false) + + assertThat(sut.formActivity).isNull() + verify(fixture.activity, atLeastOnce()).getSystemService(eq(Context.SENSOR_SERVICE)) } } 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 6b2147edb19..b38f17ed64c 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 @@ -807,18 +807,18 @@ fun UserFeedbackScreen() { // Toggle shake-to-show at runtime using the global Sentry.feedback() API item(span = { GridItemSpan(maxLineSpan) }) { - var shakeEnabled by remember { mutableStateOf(Sentry.feedback().isFeedbackOnShakeEnabled) } + var shakeEnabled by remember { mutableStateOf(Sentry.feedback().isOnShakeEnabled) } Button( modifier = Modifier, onClick = { if (shakeEnabled) { - Sentry.feedback().disableFeedbackOnShake() + Sentry.feedback().disableOnShake() } else { - Sentry.feedback().enableFeedbackOnShake() + Sentry.feedback().enableOnShake() Toast.makeText(activity, "Shake your device to open the form!", Toast.LENGTH_SHORT) .show() } - shakeEnabled = Sentry.feedback().isFeedbackOnShakeEnabled + shakeEnabled = Sentry.feedback().isOnShakeEnabled }, ) { 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 11c58ddf325..a91b4b87746 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -853,9 +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 disableOnShake ()V + public abstract fun enableOnShake ()V + public abstract fun isOnShakeEnabled ()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 @@ -1607,10 +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 fun disableOnShake ()V + public fun enableOnShake ()V public static fun getInstance ()Lio/sentry/NoOpFeedbackApi; - public fun isFeedbackOnShakeEnabled ()Z + public fun isOnShakeEnabled ()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 @@ -3269,10 +3269,10 @@ public abstract interface class io/sentry/SentryFeedbackOptions$IFormHandler { } 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 pauseDetection (Z)V + public abstract fun disableOnShake ()V + public abstract fun enableOnShake ()V + public abstract fun isOnShakeEnabled ()Z + public abstract fun setOnShakePaused (Z)V } public abstract interface class io/sentry/SentryFeedbackOptions$OptionsConfigurator { diff --git a/sentry/src/main/java/io/sentry/FeedbackApi.java b/sentry/src/main/java/io/sentry/FeedbackApi.java index d6b3cc658bd..3822c6fd7c7 100644 --- a/sentry/src/main/java/io/sentry/FeedbackApi.java +++ b/sentry/src/main/java/io/sentry/FeedbackApi.java @@ -32,18 +32,18 @@ public void show( } @Override - public void enableFeedbackOnShake() { - scopes.getOptions().getFeedbackOptions().getShakeController().enable(); + public void enableOnShake() { + scopes.getOptions().getFeedbackOptions().getShakeController().enableOnShake(); } @Override - public void disableFeedbackOnShake() { - scopes.getOptions().getFeedbackOptions().getShakeController().disable(); + public void disableOnShake() { + scopes.getOptions().getFeedbackOptions().getShakeController().disableOnShake(); } @Override - public boolean isFeedbackOnShakeEnabled() { - return scopes.getOptions().getFeedbackOptions().getShakeController().isEnabled(); + public boolean isOnShakeEnabled() { + return scopes.getOptions().getFeedbackOptions().getShakeController().isOnShakeEnabled(); } @Override diff --git a/sentry/src/main/java/io/sentry/IFeedbackApi.java b/sentry/src/main/java/io/sentry/IFeedbackApi.java index 80fd980d3b5..335ba510b10 100644 --- a/sentry/src/main/java/io/sentry/IFeedbackApi.java +++ b/sentry/src/main/java/io/sentry/IFeedbackApi.java @@ -2,7 +2,6 @@ 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; @@ -21,14 +20,14 @@ void show( * SentryFeedbackOptions#isUseShakeGesture()}. Only supported on Android; no-op on other * platforms. */ - void enableFeedbackOnShake(); + void enableOnShake(); /** * 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(); + void disableOnShake(); /** * Whether showing the feedback form on a shake gesture is currently enabled. Always {@code false} @@ -36,8 +35,7 @@ void show( * * @return true if the feedback form is shown when a shake gesture is detected */ - @ApiStatus.Internal - boolean isFeedbackOnShakeEnabled(); + boolean isOnShakeEnabled(); @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 6c884181a39..23ea5cb47eb 100644 --- a/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java +++ b/sentry/src/main/java/io/sentry/NoOpFeedbackApi.java @@ -27,13 +27,13 @@ public void show( final @Nullable SentryFeedbackOptions.OptionsConfigurator configurator) {} @Override - public void enableFeedbackOnShake() {} + public void enableOnShake() {} @Override - public void disableFeedbackOnShake() {} + public void disableOnShake() {} @Override - public boolean isFeedbackOnShakeEnabled() { + public boolean isOnShakeEnabled() { return false; } diff --git a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java index 35fa8d0df4b..04898bac65a 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -643,11 +643,11 @@ void showForm( /** Controls shake-to-report at runtime, overriding {@link #isUseShakeGesture()}. */ @ApiStatus.Internal public interface IShakeController { - void enable(); + void enableOnShake(); - void disable(); + void disableOnShake(); - boolean isEnabled(); + boolean isOnShakeEnabled(); /** * Pauses or resumes reacting to detected shakes without tearing down shake detection. Feedback @@ -656,7 +656,7 @@ public interface IShakeController { * * @param paused true to ignore detected shakes, false to react to them again */ - void pauseDetection(boolean paused); + void setOnShakePaused(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 9e99473fdb8..55d9c9f4448 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3499,22 +3499,22 @@ private SentryOptions(final boolean empty) { logger.log(SentryLevel.WARNING, "showForm() can only be called in Android."), new SentryFeedbackOptions.IShakeController() { @Override - public void enable() { + public void enableOnShake() { logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); } @Override - public void disable() { + public void disableOnShake() { logger.log(SentryLevel.WARNING, "Shake to report is only supported on Android."); } @Override - public boolean isEnabled() { + public boolean isOnShakeEnabled() { return false; } @Override - public void pauseDetection(final boolean paused) {} + public void setOnShakePaused(final boolean paused) {} }); if (!empty) { diff --git a/sentry/src/test/java/io/sentry/FeedbackApiTest.kt b/sentry/src/test/java/io/sentry/FeedbackApiTest.kt index dd588dfd2ed..762e5241b4e 100644 --- a/sentry/src/test/java/io/sentry/FeedbackApiTest.kt +++ b/sentry/src/test/java/io/sentry/FeedbackApiTest.kt @@ -19,29 +19,29 @@ class FeedbackApiTest { private val fixture = Fixture() @Test - fun `enableFeedbackOnShake delegates to the shake controller`() { - fixture.getSut().enableFeedbackOnShake() + fun `enableOnShake delegates to the shake controller`() { + fixture.getSut().enableOnShake() - verify(fixture.shakeController).enable() + verify(fixture.shakeController).enableOnShake() } @Test - fun `disableFeedbackOnShake delegates to the shake controller`() { - fixture.getSut().disableFeedbackOnShake() + fun `disableOnShake delegates to the shake controller`() { + fixture.getSut().disableOnShake() - verify(fixture.shakeController).disable() + verify(fixture.shakeController).disableOnShake() } @Test - fun `isFeedbackOnShakeEnabled delegates to the shake controller`() { - whenever(fixture.shakeController.isEnabled).thenReturn(true) + fun `isOnShakeEnabled delegates to the shake controller`() { + whenever(fixture.shakeController.isOnShakeEnabled).thenReturn(true) - assertThat(fixture.getSut().isFeedbackOnShakeEnabled).isTrue() - verify(fixture.shakeController).isEnabled + assertThat(fixture.getSut().isOnShakeEnabled).isTrue() + verify(fixture.shakeController).isOnShakeEnabled } @Test fun `default shake controller is disabled`() { - assertThat(SentryOptions().feedbackOptions.shakeController.isEnabled).isFalse() + assertThat(SentryOptions().feedbackOptions.shakeController.isOnShakeEnabled).isFalse() } } diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index 0ae479d848a..43ceaff28ce 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -944,7 +944,7 @@ class SentryOptionsTest { setLogger(logger) isDebug = true } - options.feedbackOptions.shakeController.enable() + options.feedbackOptions.shakeController.enableOnShake() verify(logger).log(eq(SentryLevel.WARNING), eq("Shake to report is only supported on Android.")) } From 77d3ba18d9728651012a25683b57e3c94f63b183 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 12 Aug 2026 13:42:49 +0200 Subject: [PATCH 09/13] meta: Restore performance changelog entries dropped in merge Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19f4cb0fc5a..37a8f5f3b6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,9 @@ ### Performance +- Read the clock once per performance collection round instead of once per in-flight transaction ([#5934](https://github.com/getsentry/sentry-java/pull/5934)) - Reduce allocations while collecting cpu usage during transactions by reading the process cpu time via `Process.getElapsedCpuTime()` instead of parsing `/proc/self/stat` (33.6kB to 16 bytes per sample on a Pixel 3) ([#5926](https://github.com/getsentry/sentry-java/pull/5926)) +- Store performance measurements as primitives, removing a boxed allocation per measurement per performance sample ([#5935](https://github.com/getsentry/sentry-java/pull/5935)) ### Dependencies From 99dd1c5ae2fdbd6be0b301aed70a019dfba60528 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 13 Aug 2026 09:29:30 +0200 Subject: [PATCH 10/13] fix(feedback): Track visible feedback forms per form and host activity Shake suppression while a form is visible was keyed on a single activity ref resolved from CurrentActivityHolder, which is not necessarily the activity the form is showing on, and any form teardown cleared it. With two forms visible at once - e.g. showForm() called while a form is already up - the first one going away re-armed detection under the second, so a shake could stack another form on top. Forms now report themselves and their host activity to FeedbackShakeIntegration, which tracks them individually and only re-arms detection once no form remains on the current activity. Since the host activity is Android-only, pausing moves off the cross-platform IShakeController onto the integration itself. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/sentry-android-core.api | 1 - .../core/FeedbackShakeIntegration.java | 90 ++++++++++++++----- .../android/core/SentryUserFeedbackForm.java | 40 ++++++--- .../core/FeedbackShakeIntegrationTest.kt | 80 +++++++++++++++-- .../core/SentryUserFeedbackFormTest.kt | 22 +++++ sentry/api/sentry.api | 1 - .../java/io/sentry/SentryFeedbackOptions.java | 9 -- .../main/java/io/sentry/SentryOptions.java | 3 - 8 files changed, 190 insertions(+), 56 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 9cfb41806f9..65bf072f0a0 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -307,7 +307,6 @@ 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 setOnShakePaused (Z)V } public abstract interface class io/sentry/android/core/IDebugImagesLoader { 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 3f93f96f294..8f4d6eb3582 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,7 @@ import android.app.Activity; import android.app.Application; +import android.app.Dialog; import android.os.Bundle; import io.sentry.IScopes; import io.sentry.Integration; @@ -14,6 +15,7 @@ import java.io.Closeable; import java.io.IOException; import java.lang.ref.WeakReference; +import java.util.concurrent.CopyOnWriteArrayList; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; @@ -26,9 +28,10 @@ * *

Shake detection is scoped to the resumed activity: a dialog belongs to the window of the * activity that created it, so it can only ever be visible while that activity is resumed. Forms - * report themselves via {@link #setOnShakePaused(boolean)} and detection is then suppressed for - * that one activity, which keeps a shake from stacking a second dialog on top of a visible one - * without letting a dialog on a backgrounded activity suppress detection elsewhere. + * report themselves via {@link #onFormVisible(Activity, Dialog)} / {@link #onFormGone(Dialog)} and + * detection is then suppressed for the activity hosting them, which keeps a shake from stacking a + * second dialog on top of a visible one without letting a dialog on a backgrounded activity + * suppress detection elsewhere. */ public final class FeedbackShakeIntegration implements Integration, @@ -42,8 +45,13 @@ public final class FeedbackShakeIntegration private volatile boolean enabled = false; private volatile @Nullable WeakReference currentActivityRef; - /** The activity a feedback form is currently showing on, if any. */ - private volatile @Nullable WeakReference formActivityRef; + /** + * The feedback forms that are currently visible, together with the activity hosting them. More + * than one can be visible at a time, e.g. when the app calls {@code Sentry.feedback().showForm()} + * while another form is already showing. + */ + private final @NotNull CopyOnWriteArrayList visibleForms = + new CopyOnWriteArrayList<>(); public FeedbackShakeIntegration(final @NotNull Application application) { this.application = Objects.requireNonNull(application, "Application is required"); @@ -124,33 +132,71 @@ public boolean isOnShakeEnabled() { return enabled; } - @Override - public void setOnShakePaused(final boolean paused) { - if (paused) { - final @Nullable Activity activity = CurrentActivityHolder.getInstance().getActivity(); - formActivityRef = activity == null ? null : new WeakReference<>(activity); - stopShakeDetection(); - } else { - formActivityRef = null; - // The form is gone, so detection can resume for whichever activity is currently resumed. - final @Nullable WeakReference currentRef = currentActivityRef; - final @Nullable Activity current = currentRef == null ? null : currentRef.get(); - if (enabled && current != null) { - startShakeDetection(current); + /** + * Reports a feedback form as visible on {@code host}. Shake detection is suppressed for that + * activity until the form reports back via {@link #onFormGone(Dialog)}, so a shake can never + * stack a second form on top of a visible one — no matter how the visible one was opened. + */ + void onFormVisible(final @NotNull Activity host, final @NotNull Dialog form) { + visibleForms.add(new VisibleForm(host, form)); + stopShakeDetection(); + } + + /** Reports a feedback form as no longer visible. Safe to call more than once per form. */ + void onFormGone(final @NotNull Dialog form) { + if (!removeForm(form)) { + return; + } + final @Nullable WeakReference currentRef = currentActivityRef; + final @Nullable Activity current = currentRef == null ? null : currentRef.get(); + if (enabled && current != null) { + startShakeDetection(current); + } + } + + private boolean removeForm(final @NotNull Dialog form) { + boolean removed = false; + for (final @NotNull VisibleForm visibleForm : visibleForms) { + // Drop entries whose form was collected without reporting back, so they can't suppress + // detection forever. + final @Nullable Dialog trackedForm = visibleForm.formRef.get(); + if (trackedForm == form) { + removed = visibleForms.remove(visibleForm) || removed; + } else if (trackedForm == null) { + visibleForms.remove(visibleForm); } } + return removed; } private boolean hasFormOn(final @NotNull Activity activity) { - final @Nullable WeakReference ref = formActivityRef; - return ref != null && ref.get() == activity; + for (final @NotNull VisibleForm visibleForm : visibleForms) { + if (visibleForm.formRef.get() != null && visibleForm.activityRef.get() == activity) { + return true; + } + } + return false; } @TestOnly @Nullable Activity getFormActivity() { - final @Nullable WeakReference ref = formActivityRef; - return ref == null ? null : ref.get(); + for (final @NotNull VisibleForm visibleForm : visibleForms) { + if (visibleForm.formRef.get() != null) { + return visibleForm.activityRef.get(); + } + } + return null; + } + + private static final class VisibleForm { + private final @NotNull WeakReference activityRef; + private final @NotNull WeakReference

formRef; + + VisibleForm(final @NotNull Activity activity, final @NotNull Dialog form) { + this.activityRef = new WeakReference<>(activity); + this.formRef = new WeakReference<>(form); + } } @Override 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 7772005cd1e..bee4593cbd6 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 @@ -370,9 +370,13 @@ protected void onStart() { final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); - // Pause shake-to-report while this form is visible, so a shake can't stack a second - // form on top of it - feedbackOptions.getShakeController().setOnShakePaused(true); + // Pause shake-to-report on this form's activity while it is visible, so a shake can't stack a + // second form on top of it + final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + final @Nullable Activity activity = getActivity(getContext()); + if (integration != null && activity != null) { + integration.onFormVisible(activity, this); + } final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { try { @@ -388,11 +392,10 @@ protected void onStart() { @Override protected void onStop() { super.onStop(); - Sentry.getCurrentScopes() - .getOptions() - .getFeedbackOptions() - .getShakeController() - .setOnShakePaused(false); + final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + if (integration != null) { + integration.onFormGone(this); + } } @Override @@ -401,11 +404,22 @@ public void 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() - .setOnShakePaused(false); + final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + if (integration != null) { + integration.onFormGone(this); + } + } + + /** + * The shake integration to report this form's visibility to, or null when shake-to-report isn't + * available (non-Android controller, or the integration was never installed). + */ + private @Nullable FeedbackShakeIntegration shakeIntegration() { + final @NotNull SentryFeedbackOptions.IShakeController controller = + Sentry.getCurrentScopes().getOptions().getFeedbackOptions().getShakeController(); + return controller instanceof FeedbackShakeIntegration + ? (FeedbackShakeIntegration) controller + : null; } @Override 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 e7efc4ce183..b700d454488 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 @@ -279,8 +280,9 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.setOnShakePaused(true) - sut.setOnShakePaused(false) + val form = mock() + sut.onFormVisible(fixture.activity, form) + sut.onFormGone(form) verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) assertThat(sut.isOnShakeEnabled).isTrue() @@ -293,8 +295,7 @@ class FeedbackShakeIntegrationTest { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - CurrentActivityHolder.getInstance().setActivity(fixture.activity) - sut.setOnShakePaused(true) + sut.onFormVisible(fixture.activity, mock()) assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) // Coming back to the activity the form is on (e.g. screen off/on) must not re-arm detection, @@ -318,7 +319,7 @@ class FeedbackShakeIntegrationTest { CurrentActivityHolder.getInstance().setActivity(fixture.activity) sut.onActivityResumed(fixture.activity) - sut.setOnShakePaused(true) + sut.onFormVisible(fixture.activity, mock()) sut.onActivityPaused(fixture.activity) sut.onActivityResumed(otherActivity) @@ -326,6 +327,26 @@ class FeedbackShakeIntegrationTest { verify(otherActivity).getSystemService(eq(Context.SENSOR_SERVICE)) } + @Test + fun `a form reports the activity it is showing on, not the current one`() { + // The form's host activity is what a stacked dialog would land on, so a mid-transition + // CurrentActivityHolder must not decide which activity detection is suppressed for. + val otherActivity = mock() + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(otherActivity) + sut.onFormVisible(fixture.activity, mock()) + + assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + + sut.onActivityResumed(fixture.activity) + + verify(fixture.activity, never()).getSystemService(eq(Context.SENSOR_SERVICE)) + } + @Test fun `dismissing a form re-arms detection on the current activity`() { whenever(fixture.activity.getSystemService(any())).thenReturn(null) @@ -335,10 +356,55 @@ class FeedbackShakeIntegrationTest { CurrentActivityHolder.getInstance().setActivity(fixture.activity) sut.onActivityResumed(fixture.activity) - sut.setOnShakePaused(true) - sut.setOnShakePaused(false) + val form = mock() + sut.onFormVisible(fixture.activity, form) + sut.onFormGone(form) assertThat(sut.formActivity).isNull() verify(fixture.activity, atLeastOnce()).getSystemService(eq(Context.SENSOR_SERVICE)) } + + @Test + fun `dismissing one of two visible forms keeps detection suppressed`() { + // Two forms can be visible at once, e.g. when the app calls showForm() while a form is + // already up. The first one going away must not re-arm detection under the second. + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.onActivityResumed(fixture.activity) + val first = mock() + val second = mock() + sut.onFormVisible(fixture.activity, first) + sut.onFormVisible(fixture.activity, second) + + sut.onFormGone(first) + assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + verify(fixture.activity, times(1)).getSystemService(eq(Context.SENSOR_SERVICE)) + + sut.onFormGone(second) + assertThat(sut.formActivity).isNull() + verify(fixture.activity, times(2)).getSystemService(eq(Context.SENSOR_SERVICE)) + } + + @Test + fun `reporting the same form gone twice re-arms detection only once`() { + // A dismissed dialog reports back from both onStop() and onDetachedFromWindow(). + whenever(fixture.activity.getSystemService(any())).thenReturn(null) + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.onActivityResumed(fixture.activity) + val form = mock() + sut.onFormVisible(fixture.activity, form) + sut.onFormGone(form) + sut.onFormGone(form) + + // Once for the resume, once for the single re-arm - the second report is a no-op. + verify(fixture.activity, times(2)).getSystemService(eq(Context.SENSOR_SERVICE)) + } } 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 4fb96ab8e90..9f6de8f18f5 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,5 +1,7 @@ package io.sentry.android.core +import android.app.Activity +import android.app.Application import android.content.Context import android.os.Looper import android.view.WindowManager @@ -20,6 +22,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue import org.junit.runner.RunWith import org.mockito.Mockito.mockStatic @@ -29,6 +32,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever +import org.robolectric.Robolectric import org.robolectric.Shadows.shadowOf @RunWith(AndroidJUnit4::class) @@ -191,4 +195,22 @@ class SentryUserFeedbackFormTest { // The form open must still complete its own work after the callback crash verify(fixture.mockReplayController).captureReplay(eq(false)) } + + @Test + fun `form reports its own host activity to the shake integration while visible`() { + fixture.options.isEnabled = true + val integration = FeedbackShakeIntegration(fixture.application as Application) + fixture.options.feedbackOptions.setShakeController(integration) + val activity = Robolectric.buildActivity(Activity::class.java).setup().get() + + val sut = SentryUserFeedbackForm(activity, 0, null, null, null) + sut.show() + + assertEquals(activity, integration.formActivity) + + sut.dismiss() + shadowOf(Looper.getMainLooper()).idle() + + assertNull(integration.formActivity) + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 60240c18baa..b02815a73e3 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3283,7 +3283,6 @@ public abstract interface class io/sentry/SentryFeedbackOptions$IShakeController public abstract fun disableOnShake ()V public abstract fun enableOnShake ()V public abstract fun isOnShakeEnabled ()Z - public abstract fun setOnShakePaused (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 04898bac65a..69b16b87ac1 100644 --- a/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java +++ b/sentry/src/main/java/io/sentry/SentryFeedbackOptions.java @@ -648,15 +648,6 @@ public interface IShakeController { void disableOnShake(); boolean isOnShakeEnabled(); - - /** - * 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. - * - * @param paused true to ignore detected shakes, false to react to them again - */ - void setOnShakePaused(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 77969ccff02..49b1d0deaf2 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3554,9 +3554,6 @@ public void disableOnShake() { public boolean isOnShakeEnabled() { return false; } - - @Override - public void setOnShakePaused(final boolean paused) {} }); if (!empty) { From 2a0cd43d10a500a8ff467d213d7ee7536349b42f Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 13 Aug 2026 10:12:15 +0200 Subject: [PATCH 11/13] ref(feedback): Use Dialog instead of Form in shake integration internals On Android these are dialogs, so name the internal members after what they are. The public API keeps "form" (SentryUserFeedbackForm, onFormOpen/onFormClose, showForm) since renaming it would be breaking and it is not Android-specific. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/FeedbackShakeIntegration.java | 83 ++++++++++--------- .../android/core/SentryUserFeedbackForm.java | 30 +++---- .../core/FeedbackShakeIntegrationTest.kt | 66 +++++++-------- .../core/SentryUserFeedbackFormTest.kt | 6 +- 4 files changed, 94 insertions(+), 91 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 8f4d6eb3582..cad881f1abb 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 @@ -27,10 +27,10 @@ * Sentry.feedback().disableOnShake()}. * *

Shake detection is scoped to the resumed activity: a dialog belongs to the window of the - * activity that created it, so it can only ever be visible while that activity is resumed. Forms - * report themselves via {@link #onFormVisible(Activity, Dialog)} / {@link #onFormGone(Dialog)} and - * detection is then suppressed for the activity hosting them, which keeps a shake from stacking a - * second dialog on top of a visible one without letting a dialog on a backgrounded activity + * activity that created it, so it can only ever be visible while that activity is resumed. Dialogs + * report themselves via {@link #onDialogVisible(Activity, Dialog)} / {@link #onDialogGone(Dialog)} + * and detection is then suppressed for the activity hosting them, which keeps a shake from stacking + * a second dialog on top of a visible one without letting a dialog on a backgrounded activity * suppress detection elsewhere. */ public final class FeedbackShakeIntegration @@ -46,11 +46,11 @@ public final class FeedbackShakeIntegration private volatile @Nullable WeakReference currentActivityRef; /** - * The feedback forms that are currently visible, together with the activity hosting them. More + * The feedback dialogs that are currently visible, together with the activity hosting them. More * than one can be visible at a time, e.g. when the app calls {@code Sentry.feedback().showForm()} - * while another form is already showing. + * while another dialog is already showing. */ - private final @NotNull CopyOnWriteArrayList visibleForms = + private final @NotNull CopyOnWriteArrayList visibleDialogs = new CopyOnWriteArrayList<>(); public FeedbackShakeIntegration(final @NotNull Application application) { @@ -133,18 +133,18 @@ public boolean isOnShakeEnabled() { } /** - * Reports a feedback form as visible on {@code host}. Shake detection is suppressed for that - * activity until the form reports back via {@link #onFormGone(Dialog)}, so a shake can never - * stack a second form on top of a visible one — no matter how the visible one was opened. + * Reports a feedback dialog as visible on {@code host}. Shake detection is suppressed for that + * activity until the dialog reports back via {@link #onDialogGone(Dialog)}, so a shake can never + * stack a second dialog on top of a visible one — no matter how the visible one was opened. */ - void onFormVisible(final @NotNull Activity host, final @NotNull Dialog form) { - visibleForms.add(new VisibleForm(host, form)); + void onDialogVisible(final @NotNull Activity host, final @NotNull Dialog dialog) { + visibleDialogs.add(new VisibleDialog(host, dialog)); stopShakeDetection(); } - /** Reports a feedback form as no longer visible. Safe to call more than once per form. */ - void onFormGone(final @NotNull Dialog form) { - if (!removeForm(form)) { + /** Reports a feedback dialog as no longer visible. Safe to call more than once per dialog. */ + void onDialogGone(final @NotNull Dialog dialog) { + if (!removeDialog(dialog)) { return; } final @Nullable WeakReference currentRef = currentActivityRef; @@ -154,24 +154,24 @@ void onFormGone(final @NotNull Dialog form) { } } - private boolean removeForm(final @NotNull Dialog form) { + private boolean removeDialog(final @NotNull Dialog dialog) { boolean removed = false; - for (final @NotNull VisibleForm visibleForm : visibleForms) { - // Drop entries whose form was collected without reporting back, so they can't suppress + for (final @NotNull VisibleDialog visibleDialog : visibleDialogs) { + // Drop entries whose dialog was collected without reporting back, so they can't suppress // detection forever. - final @Nullable Dialog trackedForm = visibleForm.formRef.get(); - if (trackedForm == form) { - removed = visibleForms.remove(visibleForm) || removed; - } else if (trackedForm == null) { - visibleForms.remove(visibleForm); + final @Nullable Dialog trackedDialog = visibleDialog.dialogRef.get(); + if (trackedDialog == dialog) { + removed = visibleDialogs.remove(visibleDialog) || removed; + } else if (trackedDialog == null) { + visibleDialogs.remove(visibleDialog); } } return removed; } - private boolean hasFormOn(final @NotNull Activity activity) { - for (final @NotNull VisibleForm visibleForm : visibleForms) { - if (visibleForm.formRef.get() != null && visibleForm.activityRef.get() == activity) { + private boolean hasDialogOn(final @NotNull Activity activity) { + for (final @NotNull VisibleDialog visibleDialog : visibleDialogs) { + if (visibleDialog.dialogRef.get() != null && visibleDialog.activityRef.get() == activity) { return true; } } @@ -180,22 +180,22 @@ private boolean hasFormOn(final @NotNull Activity activity) { @TestOnly @Nullable - Activity getFormActivity() { - for (final @NotNull VisibleForm visibleForm : visibleForms) { - if (visibleForm.formRef.get() != null) { - return visibleForm.activityRef.get(); + Activity getDialogActivity() { + for (final @NotNull VisibleDialog visibleDialog : visibleDialogs) { + if (visibleDialog.dialogRef.get() != null) { + return visibleDialog.activityRef.get(); } } return null; } - private static final class VisibleForm { + private static final class VisibleDialog { private final @NotNull WeakReference activityRef; - private final @NotNull WeakReference

formRef; + private final @NotNull WeakReference dialogRef; - VisibleForm(final @NotNull Activity activity, final @NotNull Dialog form) { + VisibleDialog(final @NotNull Activity activity, final @NotNull Dialog dialog) { this.activityRef = new WeakReference<>(activity); - this.formRef = new WeakReference<>(form); + this.dialogRef = new WeakReference<>(dialog); } } @@ -246,11 +246,11 @@ private void startShakeDetection(final @NotNull Activity activity) { } // Stop any existing detection (e.g. when transitioning between activities) stopShakeDetection(); - // A form is already visible here, so a shake could only stack a second one on top of it. - // The form has no detector of its own in this case: SentryUserFeedbackForm only starts one + // A dialog is already visible here, so a shake could only stack a second one on top of it. + // The dialog has no detector of its own in this case: SentryUserFeedbackForm only starts one // while shake-to-report is globally disabled, which is exactly when this integration is not // detecting either. - if (hasFormOn(activity)) { + if (hasDialogOn(activity)) { return; } shakeDetector.start( @@ -262,16 +262,19 @@ private void startShakeDetection(final @NotNull Activity activity) { if (active == null || options == null || !enabled - || hasFormOn(active) + || hasDialogOn(active) || Boolean.TRUE.equals(inBackground)) { return; } active.runOnUiThread( () -> { // Re-check on the main thread: shake-to-report may have been disabled, or an - // earlier queued shake may have shown a form in the meantime (the form reports + // earlier queued shake may have shown a dialog in the meantime (the dialog reports // itself synchronously in onStart). - if (!enabled || hasFormOn(active) || active.isFinishing() || active.isDestroyed()) { + if (!enabled + || hasDialogOn(active) + || active.isFinishing() + || active.isDestroyed()) { return; } try { 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 bee4593cbd6..0ea5029ef86 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 @@ -60,9 +60,9 @@ public class SentryUserFeedbackForm extends AlertDialog { } private void maybeStartShakeDetection(final @NotNull Context context) { - // Only a per-form opt-in on top of a globally disabled shake gesture starts a detector for - // this form. Both other cases defer to FeedbackShakeIntegration: while shake-to-report is - // enabled it already shows a form on shake, and once it has been disabled at runtime that + // Only a per-dialog opt-in on top of a globally disabled shake gesture starts a detector for + // this dialog. Both other cases defer to FeedbackShakeIntegration: while shake-to-report is + // enabled it already shows a dialog on shake, and once it has been disabled at runtime that // must stay disabled. Note resolvedFeedbackOptions is a copy of the global options, so its // isUseShakeGesture() is indistinguishable from the global one unless a configurator set it. final @NotNull SentryFeedbackOptions globalFeedbackOptions = @@ -362,7 +362,7 @@ public void setOnDismissListener(final @Nullable OnDismissListener listener) { @Override protected void onStart() { super.onStart(); - // Clear the message field so subsequent show() calls start with a fresh form + // Clear the message field so subsequent show() calls start with a fresh dialog final @NotNull EditText edtMessage = findViewById(R.id.sentry_dialog_user_feedback_edt_description); edtMessage.getText().clear(); @@ -370,12 +370,12 @@ protected void onStart() { final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions(); final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions(); - // Pause shake-to-report on this form's activity while it is visible, so a shake can't stack a - // second form on top of it - final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + // Pause shake-to-report on this dialog's activity while it is visible, so a shake can't stack + // a second dialog on top of it + final @Nullable FeedbackShakeIntegration integration = getFeedbackShakeIntegration(); final @Nullable Activity activity = getActivity(getContext()); if (integration != null && activity != null) { - integration.onFormVisible(activity, this); + integration.onDialogVisible(activity, this); } final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen(); if (onFormOpen != null) { @@ -392,9 +392,9 @@ protected void onStart() { @Override protected void onStop() { super.onStop(); - final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + final @Nullable FeedbackShakeIntegration integration = getFeedbackShakeIntegration(); if (integration != null) { - integration.onFormGone(this); + integration.onDialogGone(this); } } @@ -402,19 +402,19 @@ protected void onStop() { 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 — + // dialog is still showing): onStop never fires then, but the window is still detached — // without this, shake-to-report would stay paused forever. - final @Nullable FeedbackShakeIntegration integration = shakeIntegration(); + final @Nullable FeedbackShakeIntegration integration = getFeedbackShakeIntegration(); if (integration != null) { - integration.onFormGone(this); + integration.onDialogGone(this); } } /** - * The shake integration to report this form's visibility to, or null when shake-to-report isn't + * The shake integration to report this dialog's visibility to, or null when shake-to-report isn't * available (non-Android controller, or the integration was never installed). */ - private @Nullable FeedbackShakeIntegration shakeIntegration() { + private @Nullable FeedbackShakeIntegration getFeedbackShakeIntegration() { final @NotNull SentryFeedbackOptions.IShakeController controller = Sentry.getCurrentScopes().getOptions().getFeedbackOptions().getShakeController(); return controller instanceof FeedbackShakeIntegration 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 b700d454488..6fb4d49572f 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 @@ -276,37 +276,37 @@ class FeedbackShakeIntegrationTest { } @Test - fun `a visible form does not tear down the detection machinery`() { + fun `a visible dialog does not tear down the detection machinery`() { val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - val form = mock() - sut.onFormVisible(fixture.activity, form) - sut.onFormGone(form) + val dialog = mock() + sut.onDialogVisible(fixture.activity, dialog) + sut.onDialogGone(dialog) verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any()) assertThat(sut.isOnShakeEnabled).isTrue() } @Test - fun `a form suppresses detection on the activity it belongs to`() { + fun `a dialog suppresses detection on the activity it belongs to`() { whenever(fixture.activity.getSystemService(any())).thenReturn(null) val sut = fixture.getSut(useShakeGesture = true) sut.register(fixture.scopes, fixture.options) - sut.onFormVisible(fixture.activity, mock()) - assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + sut.onDialogVisible(fixture.activity, mock()) + assertThat(sut.dialogActivity).isSameInstanceAs(fixture.activity) - // Coming back to the activity the form is on (e.g. screen off/on) must not re-arm detection, - // otherwise a shake would stack a second form on top of the visible one. + // Coming back to the activity the dialog is on (e.g. screen off/on) must not re-arm detection, + // otherwise a shake would stack a second dialog on top of the visible one. sut.onActivityResumed(fixture.activity) verify(fixture.activity, never()).getSystemService(eq(Context.SENSOR_SERVICE)) } @Test - fun `a form on a backgrounded activity does not suppress detection on the next one`() { + fun `a dialog on a backgrounded activity does not suppress detection on the next one`() { // A dialog lives in the window of the activity that created it, so once that activity is no // longer resumed the dialog cannot be seen - it must not keep detection off on the activity // now in front. Android's order is A.onPause() -> B.onResume(), so exercise exactly that. @@ -319,7 +319,7 @@ class FeedbackShakeIntegrationTest { CurrentActivityHolder.getInstance().setActivity(fixture.activity) sut.onActivityResumed(fixture.activity) - sut.onFormVisible(fixture.activity, mock()) + sut.onDialogVisible(fixture.activity, mock()) sut.onActivityPaused(fixture.activity) sut.onActivityResumed(otherActivity) @@ -328,8 +328,8 @@ class FeedbackShakeIntegrationTest { } @Test - fun `a form reports the activity it is showing on, not the current one`() { - // The form's host activity is what a stacked dialog would land on, so a mid-transition + fun `a dialog reports the activity it is showing on, not the current one`() { + // The dialog's host activity is what a stacked dialog would land on, so a mid-transition // CurrentActivityHolder must not decide which activity detection is suppressed for. val otherActivity = mock() whenever(fixture.activity.getSystemService(any())).thenReturn(null) @@ -338,9 +338,9 @@ class FeedbackShakeIntegrationTest { sut.register(fixture.scopes, fixture.options) CurrentActivityHolder.getInstance().setActivity(otherActivity) - sut.onFormVisible(fixture.activity, mock()) + sut.onDialogVisible(fixture.activity, mock()) - assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + assertThat(sut.dialogActivity).isSameInstanceAs(fixture.activity) sut.onActivityResumed(fixture.activity) @@ -348,7 +348,7 @@ class FeedbackShakeIntegrationTest { } @Test - fun `dismissing a form re-arms detection on the current activity`() { + fun `dismissing a dialog re-arms detection on the current activity`() { whenever(fixture.activity.getSystemService(any())).thenReturn(null) val sut = fixture.getSut(useShakeGesture = true) @@ -356,17 +356,17 @@ class FeedbackShakeIntegrationTest { CurrentActivityHolder.getInstance().setActivity(fixture.activity) sut.onActivityResumed(fixture.activity) - val form = mock() - sut.onFormVisible(fixture.activity, form) - sut.onFormGone(form) + val dialog = mock() + sut.onDialogVisible(fixture.activity, dialog) + sut.onDialogGone(dialog) - assertThat(sut.formActivity).isNull() + assertThat(sut.dialogActivity).isNull() verify(fixture.activity, atLeastOnce()).getSystemService(eq(Context.SENSOR_SERVICE)) } @Test - fun `dismissing one of two visible forms keeps detection suppressed`() { - // Two forms can be visible at once, e.g. when the app calls showForm() while a form is + fun `dismissing one of two visible dialogs keeps detection suppressed`() { + // Two dialogs can be visible at once, e.g. when the app calls showForm() while a dialog is // already up. The first one going away must not re-arm detection under the second. whenever(fixture.activity.getSystemService(any())).thenReturn(null) @@ -377,20 +377,20 @@ class FeedbackShakeIntegrationTest { sut.onActivityResumed(fixture.activity) val first = mock() val second = mock() - sut.onFormVisible(fixture.activity, first) - sut.onFormVisible(fixture.activity, second) + sut.onDialogVisible(fixture.activity, first) + sut.onDialogVisible(fixture.activity, second) - sut.onFormGone(first) - assertThat(sut.formActivity).isSameInstanceAs(fixture.activity) + sut.onDialogGone(first) + assertThat(sut.dialogActivity).isSameInstanceAs(fixture.activity) verify(fixture.activity, times(1)).getSystemService(eq(Context.SENSOR_SERVICE)) - sut.onFormGone(second) - assertThat(sut.formActivity).isNull() + sut.onDialogGone(second) + assertThat(sut.dialogActivity).isNull() verify(fixture.activity, times(2)).getSystemService(eq(Context.SENSOR_SERVICE)) } @Test - fun `reporting the same form gone twice re-arms detection only once`() { + fun `reporting the same dialog gone twice re-arms detection only once`() { // A dismissed dialog reports back from both onStop() and onDetachedFromWindow(). whenever(fixture.activity.getSystemService(any())).thenReturn(null) @@ -399,10 +399,10 @@ class FeedbackShakeIntegrationTest { CurrentActivityHolder.getInstance().setActivity(fixture.activity) sut.onActivityResumed(fixture.activity) - val form = mock() - sut.onFormVisible(fixture.activity, form) - sut.onFormGone(form) - sut.onFormGone(form) + val dialog = mock() + sut.onDialogVisible(fixture.activity, dialog) + sut.onDialogGone(dialog) + sut.onDialogGone(dialog) // Once for the resume, once for the single re-arm - the second report is a no-op. verify(fixture.activity, times(2)).getSystemService(eq(Context.SENSOR_SERVICE)) 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 9f6de8f18f5..852b4e7e8f0 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 @@ -197,7 +197,7 @@ class SentryUserFeedbackFormTest { } @Test - fun `form reports its own host activity to the shake integration while visible`() { + fun `dialog reports its own host activity to the shake integration while visible`() { fixture.options.isEnabled = true val integration = FeedbackShakeIntegration(fixture.application as Application) fixture.options.feedbackOptions.setShakeController(integration) @@ -206,11 +206,11 @@ class SentryUserFeedbackFormTest { val sut = SentryUserFeedbackForm(activity, 0, null, null, null) sut.show() - assertEquals(activity, integration.formActivity) + assertEquals(activity, integration.dialogActivity) sut.dismiss() shadowOf(Looper.getMainLooper()).idle() - assertNull(integration.formActivity) + assertNull(integration.dialogActivity) } } From 92fc15f0ee3a1fc02f7b92727f4a50ab1cf22c06 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 13 Aug 2026 11:02:22 +0200 Subject: [PATCH 12/13] fix(feedback): Re-arm shake detection when showing a dialog fails Dialog.show() runs onStart() - which reports the dialog as visible and stops shake detection - before adding the window, so a failure in addView() left the dialog registered as visible with no callback ever reporting it gone. Shake detection then stayed off for the rest of the activity's lifetime while isOnShakeEnabled() still reported true. Also drop the global useShakeGesture check from the dialog's own shake opt-in: whether the integration is currently detecting is the only thing that matters there, and a dialog configured for shake detection should get it once the integration is off. Document on disableOnShake() that it only turns off the SDK-wide detection. Co-Authored-By: Claude Opus 5 (1M context) --- .../android/core/FeedbackShakeIntegration.java | 12 +++++++++--- .../android/core/SentryUserFeedbackForm.java | 14 +++++--------- sentry/src/main/java/io/sentry/IFeedbackApi.java | 7 ++++++- 3 files changed, 20 insertions(+), 13 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 cad881f1abb..e8d41bf52e0 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 @@ -47,7 +47,7 @@ public final class FeedbackShakeIntegration /** * The feedback dialogs that are currently visible, together with the activity hosting them. More - * than one can be visible at a time, e.g. when the app calls {@code Sentry.feedback().showForm()} + * than one can be visible at a time, e.g. when the app calls {@code Sentry.feedback().show()} * while another dialog is already showing. */ private final @NotNull CopyOnWriteArrayList visibleDialogs = @@ -85,7 +85,7 @@ public synchronized void enableOnShake() { } enabled = 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 disableOnShake() 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(); @@ -202,6 +202,7 @@ private static final class VisibleDialog { @Override public void close() throws IOException { disableOnShake(); + visibleDialogs.clear(); } @Override @@ -277,9 +278,14 @@ private void startShakeDetection(final @NotNull Activity activity) { || active.isDestroyed()) { return; } + @Nullable Dialog dialog = null; try { - new SentryUserFeedbackForm.Builder(active).create().show(); + dialog = new SentryUserFeedbackForm.Builder(active).create(); + dialog.show(); } catch (Throwable e) { + if (dialog != null) { + onDialogGone(dialog); + } 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 0ea5029ef86..01d4546d877 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 @@ -60,15 +60,11 @@ public class SentryUserFeedbackForm extends AlertDialog { } private void maybeStartShakeDetection(final @NotNull Context context) { - // Only a per-dialog opt-in on top of a globally disabled shake gesture starts a detector for - // this dialog. Both other cases defer to FeedbackShakeIntegration: while shake-to-report is - // enabled it already shows a dialog on shake, and once it has been disabled at runtime that - // must stay disabled. Note resolvedFeedbackOptions is a copy of the global options, so its - // isUseShakeGesture() is indistinguishable from the global one unless a configurator set it. + // Only start shake detection if it's enabled within the options, + // and not already running globally final @NotNull SentryFeedbackOptions globalFeedbackOptions = Sentry.getCurrentScopes().getOptions().getFeedbackOptions(); if (!resolvedFeedbackOptions.isUseShakeGesture() - || globalFeedbackOptions.isUseShakeGesture() || globalFeedbackOptions.getShakeController().isOnShakeEnabled()) { return; } @@ -401,9 +397,9 @@ protected void onStop() { @Override public void onDetachedFromWindow() { super.onDetachedFromWindow(); - // Safety net for teardown without a dismiss (e.g. the host activity is destroyed while the - // dialog is still showing): onStop never fires then, but the window is still detached — - // without this, shake-to-report would stay paused forever. + // Runs on every teardown: on dismiss the decor view is removed before onStop(), and when the + // host activity is destroyed with the dialog still showing this is the only callback that + // fires. onDialogGone is idempotent, so reporting from both here and onStop() is safe. final @Nullable FeedbackShakeIntegration integration = getFeedbackShakeIntegration(); if (integration != null) { integration.onDialogGone(this); diff --git a/sentry/src/main/java/io/sentry/IFeedbackApi.java b/sentry/src/main/java/io/sentry/IFeedbackApi.java index 335ba510b10..b0915bc43c2 100644 --- a/sentry/src/main/java/io/sentry/IFeedbackApi.java +++ b/sentry/src/main/java/io/sentry/IFeedbackApi.java @@ -26,12 +26,17 @@ void show( * Disables showing the feedback form when a shake gesture is detected, overriding {@link * SentryFeedbackOptions#isUseShakeGesture()}. Only supported on Android; no-op on other * platforms. + * + *

This turns off the SDK-wide shake detection only. A feedback form whose own options enable + * the shake gesture still runs its own detection while it is showing and while its host activity + * is alive, so shaking can re-open that form. */ void disableOnShake(); /** * Whether showing the feedback form on a shake gesture is currently enabled. Always {@code false} - * on non-Android platforms. + * on non-Android platforms. Reflects the SDK-wide shake detection only; a form running its own + * shake detection is not covered by this. * * @return true if the feedback form is shown when a shake gesture is detected */ From 239881dce36d3acab1fc59fd0c8cafd6a45a2c32 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 13 Aug 2026 13:54:29 +0200 Subject: [PATCH 13/13] test(feedback): Cover shake detection re-arming after a failed dialog show Drives a real shake through the detector and injects a dialog that reports itself visible and then fails to show, the way Android runs onStart() before adding the window. Verified to fail without the re-arm in the catch block. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/FeedbackShakeIntegration.java | 16 +++- .../core/FeedbackShakeIntegrationTest.kt | 82 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) 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 e8d41bf52e0..4405cd19309 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 @@ -189,6 +189,20 @@ Activity getDialogActivity() { return null; } + /** Creates the dialog shown on shake. Replaceable in tests to simulate a failing show(). */ + interface DialogFactory { + @NotNull + Dialog create(final @NotNull Activity activity); + } + + private @NotNull DialogFactory dialogFactory = + activity -> new SentryUserFeedbackForm.Builder(activity).create(); + + @TestOnly + void setDialogFactory(final @NotNull DialogFactory dialogFactory) { + this.dialogFactory = dialogFactory; + } + private static final class VisibleDialog { private final @NotNull WeakReference activityRef; private final @NotNull WeakReference

dialogRef; @@ -280,7 +294,7 @@ private void startShakeDetection(final @NotNull Activity activity) { } @Nullable Dialog dialog = null; try { - dialog = new SentryUserFeedbackForm.Builder(active).create(); + dialog = dialogFactory.create(active); dialog.show(); } catch (Throwable e) { if (dialog != null) { 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 6fb4d49572f..f2118fb76de 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,12 @@ import android.app.Activity import android.app.Application import android.app.Dialog import android.content.Context +import android.hardware.Sensor +import android.hardware.SensorEvent +import android.hardware.SensorEventListener +import android.hardware.SensorManager +import android.os.Handler +import android.view.WindowManager import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat import io.sentry.Scopes @@ -14,8 +20,11 @@ import kotlin.test.BeforeTest import kotlin.test.Test import org.junit.runner.RunWith import org.mockito.kotlin.any +import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.atLeastOnce +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq +import org.mockito.kotlin.isA import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.times @@ -407,4 +416,77 @@ class FeedbackShakeIntegrationTest { // Once for the resume, once for the single re-arm - the second report is a no-op. verify(fixture.activity, times(2)).getSystemService(eq(Context.SENSOR_SERVICE)) } + + @Test + fun `a dialog that fails to show does not leave detection suppressed`() { + // Dialog.show() runs onStart() - which reports the dialog as visible and stops detection - + // before the window is added, so an addView() failure hits with the dialog already tracked + // and no lifecycle callback left to report it gone. + val sensorManager = mock() + val accelerometer = mock() + whenever(fixture.activity.getSystemService(Context.SENSOR_SERVICE)).thenReturn(sensorManager) + whenever(sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER, false)) + .thenReturn(accelerometer) + whenever(fixture.activity.runOnUiThread(any())).thenAnswer { + (it.arguments[0] as Runnable).run() + null + } + + val sut = fixture.getSut(useShakeGesture = true) + sut.register(fixture.scopes, fixture.options) + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + sut.onActivityResumed(fixture.activity) + + val dialog = mock() + doAnswer { + sut.onDialogVisible(fixture.activity, dialog) + throw WindowManager.BadTokenException("Unable to add window") + } + .whenever(dialog) + .show() + sut.setDialogFactory { dialog } + + val listener = argumentCaptor() + verify(sensorManager) + .registerListener( + listener.capture(), + eq(accelerometer), + eq(SensorManager.SENSOR_DELAY_NORMAL), + isA(), + ) + shake(listener.lastValue) + + verify(dialog).show() + assertThat(sut.dialogActivity).isNull() + verify(sensorManager, times(2)) + .registerListener( + any(), + eq(accelerometer), + eq(SensorManager.SENSOR_DELAY_NORMAL), + isA(), + ) + } + + private fun shake(listener: SensorEventListener) { + val baseTimestamp = 1_000_000_000L + val intervalNs = 20_000_000L + for (i in 0 until 20) { + listener.onSensorChanged( + createSensorEvent(floatArrayOf(20f, 0f, 0f), baseTimestamp + i * intervalNs) + ) + } + } + + private fun createSensorEvent(values: FloatArray, timestamp: Long): SensorEvent { + val sensor = mock() + whenever(sensor.type).thenReturn(Sensor.TYPE_ACCELEROMETER) + + val constructor = SensorEvent::class.java.getDeclaredConstructor(Int::class.javaPrimitiveType) + constructor.isAccessible = true + val event = constructor.newInstance(values.size) + values.copyInto(event.values) + SensorEvent::class.java.getField("sensor").set(event, sensor) + SensorEvent::class.java.getField("timestamp").set(event, timestamp) + return event + } }