Skip to content

Commit 9aa138e

Browse files
antonisclaude
andcommitted
ref(feedback): address review feedback for shake gesture detection
- Reuse single SentryShakeDetector instance across activity transitions instead of re-creating on every resume (reduces allocations) - Memoize SensorManager and Sensor lookups to avoid repeated binder calls - Use getDefaultSensor(TYPE_ACCELEROMETER, false) to avoid wakeup sensor - Deliver sensor events on a background HandlerThread instead of main thread - Use SentryUserFeedbackDialog.Builder directly with tracked activity instead of going through showDialog/CurrentActivityHolder - Merge dialogActivity into currentActivity, use AppState.isInBackground() to gate against background shakes - Fix integration count in SentryAndroidTest (19 -> 20) Tested manually on Pixel 8 Pro by enabling useShakeGesture in the sample app's SentryAndroid.init and verifying shake opens the feedback dialog. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent bac65b8 commit 9aa138e

File tree

4 files changed

+52
-25
lines changed

4 files changed

+52
-25
lines changed

sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,15 @@ public final class FeedbackShakeIntegration
2323
implements Integration, Closeable, Application.ActivityLifecycleCallbacks {
2424

2525
private final @NotNull Application application;
26-
private @Nullable SentryShakeDetector shakeDetector;
26+
private final @NotNull SentryShakeDetector shakeDetector;
2727
private @Nullable SentryAndroidOptions options;
2828
private volatile @Nullable Activity currentActivity;
2929
private volatile boolean isDialogShowing = false;
30-
private volatile @Nullable Activity dialogActivity;
3130
private volatile @Nullable Runnable previousOnFormClose;
3231

3332
public FeedbackShakeIntegration(final @NotNull Application application) {
3433
this.application = Objects.requireNonNull(application, "Application is required");
34+
this.shakeDetector = new SentryShakeDetector(io.sentry.NoOpLogger.getInstance());
3535
}
3636

3737
@Override
@@ -42,6 +42,9 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions
4242
return;
4343
}
4444

45+
// Re-assign a properly configured detector logger now that options are available
46+
shakeDetector.init(application);
47+
4548
addIntegrationToSdkVersion("FeedbackShake");
4649
application.registerActivityLifecycleCallbacks(this);
4750
options.getLogger().log(SentryLevel.DEBUG, "FeedbackShakeIntegration installed.");
@@ -95,9 +98,8 @@ public void onActivitySaveInstanceState(
9598
public void onActivityDestroyed(final @NotNull Activity activity) {
9699
// Only reset if this is the activity that hosts the dialog — the dialog cannot
97100
// outlive its host activity being destroyed.
98-
if (activity == dialogActivity) {
101+
if (isDialogShowing && activity == currentActivity) {
99102
isDialogShowing = false;
100-
dialogActivity = null;
101103
if (options != null) {
102104
options.getFeedbackOptions().setOnFormClose(previousOnFormClose);
103105
}
@@ -109,39 +111,39 @@ private void startShakeDetection(final @NotNull Activity activity) {
109111
if (options == null) {
110112
return;
111113
}
112-
// Stop any existing detector (e.g. when transitioning between activities)
114+
// Stop any existing detection (e.g. when transitioning between activities)
113115
stopShakeDetection();
114-
shakeDetector = new SentryShakeDetector(options.getLogger());
115116
shakeDetector.start(
116117
activity,
117118
() -> {
118119
final Activity active = currentActivity;
119-
if (active != null && options != null && !isDialogShowing) {
120+
final Boolean inBackground = AppState.getInstance().isInBackground();
121+
if (active != null
122+
&& options != null
123+
&& !isDialogShowing
124+
&& !Boolean.TRUE.equals(inBackground)) {
120125
active.runOnUiThread(
121126
() -> {
122127
if (isDialogShowing) {
123128
return;
124129
}
125130
try {
126131
isDialogShowing = true;
127-
dialogActivity = active;
128132
previousOnFormClose = options.getFeedbackOptions().getOnFormClose();
129133
options
130134
.getFeedbackOptions()
131135
.setOnFormClose(
132136
() -> {
133137
isDialogShowing = false;
134-
dialogActivity = null;
135138
options.getFeedbackOptions().setOnFormClose(previousOnFormClose);
136139
if (previousOnFormClose != null) {
137140
previousOnFormClose.run();
138141
}
139142
previousOnFormClose = null;
140143
});
141-
options.getFeedbackOptions().getDialogHandler().showDialog(null, null);
144+
new SentryUserFeedbackDialog.Builder(active).create().show();
142145
} catch (Throwable e) {
143146
isDialogShowing = false;
144-
dialogActivity = null;
145147
options.getFeedbackOptions().setOnFormClose(previousOnFormClose);
146148
previousOnFormClose = null;
147149
options
@@ -154,9 +156,6 @@ private void startShakeDetection(final @NotNull Activity activity) {
154156
}
155157

156158
private void stopShakeDetection() {
157-
if (shakeDetector != null) {
158-
shakeDetector.stop();
159-
shakeDetector = null;
160-
}
159+
shakeDetector.stop();
161160
}
162161
}

sentry-android-core/src/main/java/io/sentry/android/core/SentryShakeDetector.java

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
import android.hardware.SensorEvent;
66
import android.hardware.SensorEventListener;
77
import android.hardware.SensorManager;
8+
import android.os.Handler;
9+
import android.os.HandlerThread;
810
import android.os.SystemClock;
911
import io.sentry.ILogger;
1012
import io.sentry.SentryLevel;
@@ -21,6 +23,9 @@
2123
*
2224
* <p>Requires at least {@link #SHAKE_COUNT_THRESHOLD} accelerometer readings above {@link
2325
* #SHAKE_THRESHOLD_GRAVITY} within {@link #SHAKE_WINDOW_MS} to trigger a shake event.
26+
*
27+
* <p>Sensor events are delivered on a background {@link HandlerThread} to avoid polluting the main
28+
* thread.
2429
*/
2530
@ApiStatus.Internal
2631
public final class SentryShakeDetector implements SensorEventListener {
@@ -31,6 +36,8 @@ public final class SentryShakeDetector implements SensorEventListener {
3136
private static final int SHAKE_COOLDOWN_MS = 1000;
3237

3338
private @Nullable SensorManager sensorManager;
39+
private @Nullable Sensor accelerometer;
40+
private @Nullable HandlerThread handlerThread;
3441
private final @NotNull AtomicLong lastShakeTimestamp = new AtomicLong(0);
3542
private volatile @Nullable Listener listener;
3643
private final @NotNull ILogger logger;
@@ -46,27 +53,46 @@ public SentryShakeDetector(final @NotNull ILogger logger) {
4653
this.logger = logger;
4754
}
4855

56+
/**
57+
* Initializes the sensor manager and accelerometer sensor. This is separated from start() so the
58+
* values can be resolved once and reused across activity transitions.
59+
*/
60+
void init(final @NotNull Context context) {
61+
if (sensorManager == null) {
62+
sensorManager = (SensorManager) context.getSystemService(Context.SENSOR_SERVICE);
63+
}
64+
if (sensorManager != null && accelerometer == null) {
65+
accelerometer = sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER, false);
66+
}
67+
}
68+
4969
public void start(final @NotNull Context context, final @NotNull Listener shakeListener) {
5070
this.listener = shakeListener;
51-
sensorManager = (SensorManager) context.getSystemService(Context.SENSOR_SERVICE);
71+
init(context);
5272
if (sensorManager == null) {
5373
logger.log(SentryLevel.WARNING, "SensorManager is not available. Shake detection disabled.");
5474
return;
5575
}
56-
Sensor accelerometer = sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER);
5776
if (accelerometer == null) {
5877
logger.log(
5978
SentryLevel.WARNING, "Accelerometer sensor not available. Shake detection disabled.");
6079
return;
6180
}
62-
sensorManager.registerListener(this, accelerometer, SensorManager.SENSOR_DELAY_NORMAL);
81+
handlerThread = new HandlerThread("sentry-shake");
82+
handlerThread.start();
83+
final Handler handler = new Handler(handlerThread.getLooper());
84+
sensorManager.registerListener(
85+
this, accelerometer, SensorManager.SENSOR_DELAY_NORMAL, handler);
6386
}
6487

6588
public void stop() {
6689
listener = null;
6790
if (sensorManager != null) {
6891
sensorManager.unregisterListener(this);
69-
sensorManager = null;
92+
}
93+
if (handlerThread != null) {
94+
handlerThread.quitSafely();
95+
handlerThread = null;
7096
}
7197
}
7298

sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ class SentryAndroidTest {
477477
fixture.initSut(context = mock<Application>()) { options ->
478478
optionsRef = options
479479
options.dsn = "https://key@sentry.io/123"
480-
assertEquals(19, options.integrations.size)
480+
assertEquals(20, options.integrations.size)
481481
options.integrations.removeAll {
482482
it is UncaughtExceptionHandlerIntegration ||
483483
it is ShutdownHookIntegration ||

sentry-android-core/src/test/java/io/sentry/android/core/SentryShakeDetectorTest.kt

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@ import android.content.Context
44
import android.hardware.Sensor
55
import android.hardware.SensorEvent
66
import android.hardware.SensorManager
7+
import android.os.Handler
78
import android.os.SystemClock
89
import androidx.test.ext.junit.runners.AndroidJUnit4
910
import io.sentry.ILogger
1011
import kotlin.test.Test
1112
import org.junit.runner.RunWith
1213
import org.mockito.kotlin.any
1314
import org.mockito.kotlin.eq
15+
import org.mockito.kotlin.isA
1416
import org.mockito.kotlin.mock
1517
import org.mockito.kotlin.never
1618
import org.mockito.kotlin.verify
@@ -28,7 +30,7 @@ class SentryShakeDetectorTest {
2830

2931
init {
3032
whenever(context.getSystemService(Context.SENSOR_SERVICE)).thenReturn(sensorManager)
31-
whenever(sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER)).thenReturn(accelerometer)
33+
whenever(sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER, false)).thenReturn(accelerometer)
3234
}
3335

3436
fun getSut(): SentryShakeDetector {
@@ -44,7 +46,7 @@ class SentryShakeDetectorTest {
4446
sut.start(fixture.context, fixture.listener)
4547

4648
verify(fixture.sensorManager)
47-
.registerListener(eq(sut), eq(fixture.accelerometer), eq(SensorManager.SENSOR_DELAY_NORMAL))
49+
.registerListener(eq(sut), eq(fixture.accelerometer), eq(SensorManager.SENSOR_DELAY_NORMAL), isA<Handler>())
4850
}
4951

5052
@Test
@@ -63,17 +65,17 @@ class SentryShakeDetectorTest {
6365
val sut = fixture.getSut()
6466
sut.start(fixture.context, fixture.listener)
6567

66-
verify(fixture.sensorManager, never()).registerListener(any(), any<Sensor>(), any())
68+
verify(fixture.sensorManager, never()).registerListener(any(), any<Sensor>(), any<Int>(), any<Handler>())
6769
}
6870

6971
@Test
7072
fun `does not crash when accelerometer is null`() {
71-
whenever(fixture.sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER)).thenReturn(null)
73+
whenever(fixture.sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER, false)).thenReturn(null)
7274

7375
val sut = fixture.getSut()
7476
sut.start(fixture.context, fixture.listener)
7577

76-
verify(fixture.sensorManager, never()).registerListener(any(), any<Sensor>(), any())
78+
verify(fixture.sensorManager, never()).registerListener(any(), any<Sensor>(), any<Int>(), any<Handler>())
7779
}
7880

7981
@Test

0 commit comments

Comments
 (0)