-
-
Notifications
You must be signed in to change notification settings - Fork 383
feat(feedback): implement shake gesture detection #7579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 14 commits
971f5a5
fb973ee
cc676fe
c83f0f0
3575377
67a100d
bcda268
27ff27e
3e4b593
3c24378
a447914
0f86f9f
6a17a68
f0cd795
e47ec7b
9044b8b
36137d7
81be891
1ebfe2f
725eba9
f5eacb9
1a04fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import Foundation | ||
| #if os(iOS) && !SENTRY_NO_UI_FRAMEWORK | ||
| import ObjectiveC | ||
| import QuartzCore | ||
| import UIKit | ||
| #endif | ||
|
|
||
| /// Extension providing the Sentry shake detection notification name. | ||
| public extension NSNotification.Name { | ||
| /// Notification posted when the device detects a shake gesture on iOS/iPadOS. | ||
| /// On non-iOS platforms this notification is never posted. | ||
| static let SentryShakeDetected = NSNotification.Name("SentryShakeDetected") | ||
| } | ||
|
|
||
| /// Detects shake gestures by swizzling `UIWindow.motionEnded(_:with:)` on iOS/iPadOS. | ||
| /// When a shake gesture is detected, posts a `.SentryShakeDetected` notification. | ||
| /// | ||
| /// Use `enable()` to start detection and `disable()` to stop it. | ||
| /// Swizzling is performed at most once regardless of how many times `enable()` is called. | ||
| /// On non-iOS platforms (macOS, tvOS, watchOS), these methods are no-ops. | ||
| @objc(SentryShakeDetector) | ||
| @objcMembers | ||
| public final class SentryShakeDetector: NSObject { | ||
|
|
||
| /// The notification name posted on shake, exposed for ObjC consumers. | ||
| /// In Swift, prefer using `.SentryShakeDetected` on `NSNotification.Name` directly. | ||
| @objc public static let shakeDetectedNotification = NSNotification.Name.SentryShakeDetected | ||
|
|
||
| #if os(iOS) && !SENTRY_NO_UI_FRAMEWORK | ||
| // Both motionEnded (main thread) and enable/disable (main thread in practice) | ||
| // access this flag. UIKit's motionEnded is always dispatched on the main thread, | ||
| // and the SDK calls enable/disable from main-thread integration lifecycle. | ||
| private static var enabled = false | ||
|
|
||
| private static var swizzled = false | ||
| private static var originalIMP: IMP? | ||
| private static var lastShakeTimestamp: CFTimeInterval = 0 | ||
| private static let cooldownSeconds: CFTimeInterval = 1.0 | ||
| private static let lock = NSLock() | ||
|
|
||
| /// Enables shake gesture detection. On iOS/iPadOS, swizzles `UIWindow.motionEnded(_:with:)` | ||
| /// the first time it is called, and from then on posts `.SentryShakeDetected` | ||
| /// whenever a shake is detected. No-op on non-iOS platforms. | ||
| public static func enable() { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
|
|
||
| if !swizzled { | ||
| let windowClass: AnyClass = UIWindow.self | ||
| let selector = #selector(UIResponder.motionEnded(_:with:)) | ||
|
|
||
| guard let inheritedMethod = class_getInstanceMethod(windowClass, selector) else { | ||
| return | ||
| } | ||
|
|
||
| let inheritedIMP = method_getImplementation(inheritedMethod) | ||
| let types = method_getTypeEncoding(inheritedMethod) | ||
| class_addMethod(windowClass, selector, inheritedIMP, types) | ||
|
|
||
| guard let ownMethod = class_getInstanceMethod(windowClass, selector) else { | ||
| return | ||
| } | ||
|
|
||
| let replacementIMP = imp_implementationWithBlock({ (self: UIWindow, motion: UIEvent.EventSubtype, event: UIEvent?) in | ||
| if SentryShakeDetector.enabled && motion == .motionShake { | ||
| let now = CACurrentMediaTime() | ||
| if now - SentryShakeDetector.lastShakeTimestamp > SentryShakeDetector.cooldownSeconds { | ||
| SentryShakeDetector.lastShakeTimestamp = now | ||
| NotificationCenter.default.post(name: .SentryShakeDetected, object: nil) | ||
| } | ||
| } | ||
|
|
||
| if let original = SentryShakeDetector.originalIMP { | ||
| typealias MotionEndedFunc = @convention(c) (Any, Selector, UIEvent.EventSubtype, UIEvent?) -> Void | ||
| let originalFunc = unsafeBitCast(original, to: MotionEndedFunc.self) | ||
| originalFunc(self, selector, motion, event) | ||
| } | ||
| } as @convention(block) (UIWindow, UIEvent.EventSubtype, UIEvent?) -> Void) | ||
|
|
||
| originalIMP = method_setImplementation(ownMethod, replacementIMP) | ||
| swizzled = true | ||
| } | ||
|
|
||
| enabled = true | ||
| } | ||
|
|
||
| /// Disables shake gesture detection. Does not un-swizzle `UIWindow`; it only suppresses | ||
| /// the notification so the overhead is negligible. No-op on non-iOS platforms. | ||
| public static func disable() { | ||
| enabled = false | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I explored implementing unswizzling in disable() by restoring the original IMP like: I think there is a risk of breakage if another library swizzles motionEnded after us. Wdyt?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this could break multi-nested swizzling. Maybe we should defer this and create a proper solution for all swizzling in the future |
||
| } | ||
| #else | ||
| /// No-op on non-iOS platforms. | ||
| @objc public static func enable() {} | ||
| /// No-op on non-iOS platforms. | ||
| @objc public static func disable() {} | ||
| #endif | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ final class SentryUserFeedbackIntegrationDriver: NSObject { | |
| fileprivate let callback: (SentryFeedback) -> Void | ||
| let screenshotSource: SentryScreenshotSource | ||
| weak var customButton: UIButton? | ||
| private var didEnableShakeDetection = false | ||
|
|
||
| init(configuration: SentryUserFeedbackConfiguration, screenshotSource: SentryScreenshotSource, callback: @escaping (SentryFeedback) -> Void) { | ||
| self.configuration = configuration | ||
|
|
@@ -44,6 +45,8 @@ final class SentryUserFeedbackIntegrationDriver: NSObject { | |
| * At the time this integration is being installed, if there is no UIApplicationDelegate and no connected UIScene, it is very likely we are in a SwiftUI app, but it's possible we could instead be in a UIKit app that has some nonstandard launch procedure or doesn't call SentrySDK.start in a place we expect/recommend, in which case they will need to manually display the widget when they're ready by calling SentrySDK.feedback.showWidget. | ||
| */ | ||
| if UIApplication.shared.connectedScenes.isEmpty && UIApplication.shared.delegate == nil { | ||
| observeScreenshots() | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| observeShakeGesture() | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return | ||
| } | ||
|
|
||
|
|
@@ -53,10 +56,15 @@ final class SentryUserFeedbackIntegrationDriver: NSObject { | |
| } | ||
|
|
||
| observeScreenshots() | ||
| observeShakeGesture() | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| deinit { | ||
| customButton?.removeTarget(self, action: #selector(showForm(sender:)), for: .touchUpInside) | ||
|
Comment on lines
58
to
61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The integration's deinitializer unconditionally disables the shared, global Suggested FixImplement reference counting for the Prompt for AI AgentDid we get this right? π / π to inform future reviews. |
||
| if didEnableShakeDetection { | ||
| SentryShakeDetector.disable() | ||
|
||
| } | ||
| NotificationCenter.default.removeObserver(self) | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| func showWidget() { | ||
|
|
@@ -149,6 +157,19 @@ private extension SentryUserFeedbackIntegrationDriver { | |
| } | ||
| } | ||
|
|
||
| func observeShakeGesture() { | ||
| if configuration.useShakeGesture { | ||
| SentryShakeDetector.enable() | ||
| didEnableShakeDetection = true | ||
| NotificationCenter.default.addObserver(self, selector: #selector(handleShakeGesture), name: .SentryShakeDetected, object: nil) | ||
| } | ||
| } | ||
|
|
||
| @objc func handleShakeGesture() { | ||
| guard !displayingForm else { return } | ||
|
||
| showForm(screenshot: nil) | ||
| } | ||
|
|
||
| @objc func userCapturedScreenshot() { | ||
| stopObservingScreenshots() | ||
| showForm(screenshot: screenshotSource.appScreenshots().first) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| @testable import Sentry | ||
| import XCTest | ||
|
|
||
| #if os(iOS) | ||
| import UIKit | ||
|
|
||
| final class SentryShakeDetectorTests: XCTestCase { | ||
|
|
||
| override func tearDown() { | ||
| super.tearDown() | ||
| SentryShakeDetector.disable() | ||
| } | ||
|
|
||
| func testEnable_whenShakeOccurs_shouldPostNotification() { | ||
| let expectation = expectation(forNotification: .SentryShakeDetected, object: nil) | ||
|
|
||
| SentryShakeDetector.enable() | ||
|
|
||
| let window = UIWindow() | ||
| window.motionEnded(.motionShake, with: nil) | ||
|
|
||
| wait(for: [expectation], timeout: 1.0) | ||
| } | ||
|
|
||
| func testDisable_whenShakeOccurs_shouldNotPostNotification() { | ||
| SentryShakeDetector.enable() | ||
| SentryShakeDetector.disable() | ||
|
|
||
| let expectation = expectation(forNotification: .SentryShakeDetected, object: nil) | ||
| expectation.isInverted = true | ||
|
|
||
| let window = UIWindow() | ||
| window.motionEnded(.motionShake, with: nil) | ||
|
|
||
| wait(for: [expectation], timeout: 0.5) | ||
| } | ||
|
|
||
| func testEnable_whenNonShakeMotion_shouldNotPostNotification() { | ||
| SentryShakeDetector.enable() | ||
|
|
||
| let expectation = expectation(forNotification: .SentryShakeDetected, object: nil) | ||
| expectation.isInverted = true | ||
|
|
||
| let window = UIWindow() | ||
| window.motionEnded(.none, with: nil) | ||
|
|
||
| wait(for: [expectation], timeout: 0.5) | ||
| } | ||
|
|
||
| func testEnable_calledMultipleTimes_shouldNotCrash() { | ||
| SentryShakeDetector.enable() | ||
| SentryShakeDetector.enable() | ||
| SentryShakeDetector.enable() | ||
|
|
||
| // Just verify no crash; the swizzle-once guard handles repeated calls | ||
| let window = UIWindow() | ||
| window.motionEnded(.motionShake, with: nil) | ||
| } | ||
|
|
||
| func testDisable_withoutEnable_shouldNotCrash() { | ||
| SentryShakeDetector.disable() | ||
| } | ||
|
|
||
| func testCooldown_whenShakesTooFast_shouldPostOnlyOnce() { | ||
| SentryShakeDetector.enable() | ||
|
|
||
| var notificationCount = 0 | ||
| let observer = NotificationCenter.default.addObserver( | ||
| forName: .SentryShakeDetected, object: nil, queue: nil | ||
| ) { _ in | ||
| notificationCount += 1 | ||
| } | ||
|
|
||
| let window = UIWindow() | ||
| // Rapid shakes within the 1s cooldown | ||
| window.motionEnded(.motionShake, with: nil) | ||
| window.motionEnded(.motionShake, with: nil) | ||
| window.motionEnded(.motionShake, with: nil) | ||
|
|
||
| XCTAssertEqual(notificationCount, 1) | ||
|
|
||
| NotificationCenter.default.removeObserver(observer) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shared static state causes flaky cooldown testMedium Severity
Additional Locations (1) |
||
|
|
||
| func testOriginalImplementation_shouldStillBeCalled() { | ||
| SentryShakeDetector.enable() | ||
|
|
||
| // motionEnded should not crash β the original UIResponder implementation | ||
| // is called after our interceptor | ||
| let window = UIWindow() | ||
| window.motionEnded(.motionShake, with: nil) | ||
| window.motionEnded(.remoteControlBeginSeekingBackward, with: nil) | ||
| } | ||
| } | ||
|
|
||
| #endif | ||


Uh oh!
There was an error while loading. Please reload this page.