diff --git a/CHANGELOG.md b/CHANGELOG.md index 35f1f30820b..abee0c33746 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 ([#5827](https://github.com/getsentry/sentry-java/pull/5827)) + ### Improvements - Skip building Android manifest metadata debug log messages when debug logging is disabled, reducing allocations during SDK init ([#5790](https://github.com/getsentry/sentry-java/pull/5790)) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index adebedf2700..0c2890427d4 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 @@ -304,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 { @@ -553,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/build.gradle.kts b/sentry-android-core/build.gradle.kts index 23248d6dae4..0e3708a89bf 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..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,9 +4,13 @@ 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; +import io.sentry.SentryFeedbackOptions; import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.util.Objects; @@ -17,18 +21,30 @@ 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()}. + * + *

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, 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 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"); @@ -46,13 +62,74 @@ 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; + startDetecting(options); + } + + @Override + public synchronized void disable() { + if (!enabled) { return; } + enabled = false; + if (!dialogRequestedShakeDetection || getDialog() == null) { + stopDetecting(); + } + } - // 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. + @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 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(); // Resolving the accelerometer is the most expensive part of init (the first SensorManager @@ -72,7 +149,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); @@ -80,35 +157,27 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions } } - @Override - public void close() throws IOException { + private synchronized void stopDetecting() { + if (!detecting) { + return; + } + 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. - if (isDialogShowing) { - isDialogShowing = false; - if (options != null) { - options.getFeedbackOptions().setOnFormClose(previousOnFormClose); - } - previousOnFormClose = null; - } currentActivityRef = null; } + @Override + 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); } @@ -118,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; } } @@ -147,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) { @@ -166,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 43500d50ebc..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,109 +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(); - if (!resolvedFeedbackOptions.isUseShakeGesture() || globalFeedbackOptions.isUseShakeGesture()) { - 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(); - } + // 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; } - - @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 @@ -332,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 170211abf55..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,8 +2,10 @@ 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 import io.sentry.Scopes import io.sentry.SentryFeedbackOptions import io.sentry.test.DeferredExecutorService @@ -16,8 +18,10 @@ 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 +import org.robolectric.Robolectric @RunWith(AndroidJUnit4::class) class FeedbackShakeIntegrationTest { @@ -165,4 +169,224 @@ 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() + } + + 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-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..2039a80df4b 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,17 @@ 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 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 { 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..c66ce6e979d 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,41 @@ 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(); + + /** + * 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. */ 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..7d55c1464a7 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3496,7 +3496,32 @@ 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; + } + + @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) { 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()