orientation change breadcrumb PR feedback

bugsnag · Mar 20, 2020 · 04c3285 · 04c3285
1 parent 5625833
commit 04c3285
Show file tree

Hide file tree

Showing 3 changed files with 122 additions and 59 deletions.
diff --git a/Source/BugsnagClient.h b/Source/BugsnagClient.h
@@ -28,6 +28,13 @@
 
 #import "BugsnagConfiguration.h"
 #import "BugsnagMetadata.h"
+#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
+#import <UIKit/UIKit.h>
+#elif TARGET_OS_MAC
+#import <AppKit/AppKit.h>
+#endif
+
+NSString * _Nullable BSGOrientationNameFromEnum(UIDeviceOrientation deviceOrientation);
 
 @class BugsnagSessionTracker;
 

diff --git a/Source/BugsnagClient.m b/Source/BugsnagClient.m
@@ -42,12 +42,6 @@
 #import "BSG_KSSystemInfo.h"
 #import "BSG_KSMach.h"
 
-#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
-#import <UIKit/UIKit.h>
-#elif TARGET_OS_MAC
-#import <AppKit/AppKit.h>
-#endif
-
 NSString *const NOTIFIER_VERSION = @"5.23.0";
 NSString *const NOTIFIER_URL = @"https://github.com/bugsnag/bugsnag-cocoa";
 NSString *const BSTabCrash = @"crash";
@@ -84,10 +78,6 @@
 static NSUInteger handledCount;
 static NSUInteger unhandledCount;
 static bool hasRecordedSessions;
-#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
-// The previous device orientation - iOS only
-static NSString *lastOrientation = NULL;
-#endif
 
 /**
  *  Handler executed when the application crashes. Writes information about the
@@ -147,6 +137,40 @@ void BSSerializeDataCrashHandler(const BSG_KSCrashReportWriter *writer, int type
     }
 }
 
+/**
+ * Convert a device orientation into its Bugsnag string representation
+ *
+ * @param deviceOrientation The platform device orientation
+ *
+ * @returns A string representing the device orientation or nil if there's no equivalent
+ */
+NSString *BSGOrientationNameFromEnum(UIDeviceOrientation deviceOrientation)
+{
+    NSString *orientation;
+    switch (deviceOrientation) {
+    case UIDeviceOrientationPortraitUpsideDown:
+        orientation = @"portraitupsidedown";
+        break;
+    case UIDeviceOrientationPortrait:
+        orientation = @"portrait";
+        break;
+    case UIDeviceOrientationLandscapeRight:
+        orientation = @"landscaperight";
+        break;
+    case UIDeviceOrientationLandscapeLeft:
+        orientation = @"landscapeleft";
+        break;
+    case UIDeviceOrientationFaceUp:
+        orientation = @"faceup";
+        break;
+    case UIDeviceOrientationFaceDown:
+        orientation = @"facedown";
+        break;
+    default:
+        return nil; // always ignore unknown breadcrumbs
+    }
+    return orientation;
+}
 /**
  *  Writes a dictionary to a destination using the BSG_KSCrash JSON encoding
  *
@@ -213,6 +237,10 @@ @interface BugsnagClient ()
 @property (nonatomic, strong) BSGOutOfMemoryWatchdog *oomWatchdog;
 @property (nonatomic, strong) BugsnagPluginClient *pluginClient;
 @property (nonatomic) BOOL appCrashedLastLaunch;
+#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
+// The previous device orientation - iOS only
+@property (class, nonatomic, strong) NSString *lastOrientation;
+#endif
 @end
 
 @interface BugsnagConfiguration ()
@@ -221,6 +249,23 @@ @interface BugsnagConfiguration ()
 
 @implementation BugsnagClient
 
+#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
+/**
+ * Storage for the device orientation.  It is "last" whenever an orientation change is received
+ */
+static NSString *_lastOrientation = nil;
+
++ (NSString *)lastOrientation
+{
+    return _lastOrientation;
+}
+
++ (void)setLastOrientation:(NSString *)lastOrientation
+{
+    _lastOrientation = lastOrientation;
+}
+#endif
+
 @synthesize configuration;
 
 - (id)initWithConfiguration:(BugsnagConfiguration *)initConfiguration {
@@ -280,6 +325,10 @@ - (id)initWithConfiguration:(BugsnagConfiguration *)initConfiguration {
         [self metadataChanged:self.configuration.config];
         [self metadataChanged:self.state];
         self.pluginClient = [[BugsnagPluginClient alloc] initWithPlugins:self.configuration.plugins];
+
+#if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE
+        _lastOrientation = BSGOrientationNameFromEnum([UIDevice currentDevice].orientation);
+#endif
     }
     return self;
 }
@@ -390,7 +439,6 @@ - (void)start {
     [[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications];
 
     [self batteryChanged:nil];
-    [self orientationChanged:nil];
     [self addTerminationObserver:UIApplicationWillTerminateNotification];
 
 #elif TARGET_OS_MAC
@@ -778,63 +826,46 @@ - (void)batteryChanged:(NSNotification *)notif {
                  toTabWithName:BSGKeyDeviceState];
 }
 
-- (void)orientationChanged:(NSNotification *)notif {
-    NSString *orientation;
-
+/**
+ * Called when an orientation change notification is received to record an
+ * equivalent breadcrumb.
+ *
+ * @param notification The orientation-change notification
+ */
+- (void)orientationChanged:(NSNotification *)notification {
     UIDeviceOrientation currentDeviceOrientation = [UIDevice currentDevice].orientation;
-
-    switch (currentDeviceOrientation) {
-    case UIDeviceOrientationPortraitUpsideDown:
-        orientation = @"portraitupsidedown";
-        break;
-    case UIDeviceOrientationPortrait:
-        orientation = @"portrait";
-        break;
-    case UIDeviceOrientationLandscapeRight:
-        orientation = @"landscaperight";
-        break;
-    case UIDeviceOrientationLandscapeLeft:
-        orientation = @"landscapeleft";
-        break;
-    case UIDeviceOrientationFaceUp:
-        orientation = @"faceup";
-        break;
-    case UIDeviceOrientationFaceDown:
-        orientation = @"facedown";
-        break;
-    default:
-        return; // always ignore unknown breadcrumbs
+    NSString *orientation = BSGOrientationNameFromEnum(currentDeviceOrientation);
+
+    // Short-circuit the exit if we don't have enough info to record a full breadcrumb
+    // or the orientation hasn't changed (false positive).
+    if (!orientation) {
+        return;
+    }
+    else if (!_lastOrientation || [orientation isEqualToString:_lastOrientation]) {
+        _lastOrientation = orientation;
+        return;
     }
 
-    NSDictionary *lastBreadcrumb =
-        [[self.configuration.breadcrumbs arrayValue] lastObject];
-    NSString *orientationNotifName =
-        BSGBreadcrumbNameForNotificationName(notif.name);
+    NSDictionary *lastBreadcrumb = [[self.configuration.breadcrumbs arrayValue] lastObject];
+    NSString *orientationNotifName = BSGBreadcrumbNameForNotificationName(notification.name);
 
-    if (lastBreadcrumb &&
-        [orientationNotifName isEqualToString:lastBreadcrumb[BSGKeyName]]) {
+    if (lastBreadcrumb && [orientationNotifName isEqualToString:lastBreadcrumb[BSGKeyName]])
+    {
         NSDictionary *metadata = lastBreadcrumb[BSGKeyMetadata];
 
-        if ([orientation isEqualToString:metadata[BSGKeyOrientation]])
+        if ([orientation isEqualToString:metadata[BSGKeyOrientation]]) {
             return; // ignore duplicate orientation event
+        }
     }
-
-    // It's not a change
-    if ([orientation isEqualToString:lastOrientation])
-        return;
-
-    // We previously had an orientation
-    if (lastOrientation) {
-        [[self state] addAttribute:BSGKeyOrientationChange
-                         withValue:@{@"from" : lastOrientation,
-                                     @"to" : orientation}
-                     toTabWithName:BSGKeyDeviceState];
-    }
 
-    // We shouldn't get here without orientation being set, but to be on the safe side:
-    if (orientation)
-        // Preserve the orientation
-        lastOrientation = orientation;
+    // Record the breadcrumb
+    [[self state] addAttribute:BSGKeyOrientationChange
+                     withValue:@{@"from" : _lastOrientation,
+                                 @"to" : orientation}
+                 toTabWithName:BSGKeyDeviceState];
+
+    // Preserve the orientation
+    _lastOrientation = orientation;
 }
 
 - (void)lowMemoryWarning:(NSNotification *)notif {

diff --git a/Tests/BugsnagTests.m b/Tests/BugsnagTests.m
@@ -8,6 +8,7 @@
 // Unit tests of global Bugsnag behaviour
 
 #import "Bugsnag.h"
+#import "BugsnagClient.h"
 #import "BugsnagTestConstants.h"
 #import <XCTest/XCTest.h>
 
@@ -21,6 +22,11 @@ @interface BugsnagConfiguration ()
 @property(nonatomic, readwrite, strong) NSMutableArray *onSendBlocks;
 @end
 
+@interface BugsnagClient ()
++ (NSString *)lastOrientation;
++ (void)setLastOrientation:(NSString *)lastOrientation;
+@end
+
 @interface BugsnagTests : XCTestCase
 @end
 
@@ -406,4 +412,23 @@ - (void) testOnSendBlocks {
     [self waitForExpectations:@[expectation5, expectation6] timeout:1.0];
 }
 
+/**
+ * Test that the Orientation -> string mapping is as expected
+ * NOTE: should be moved to BugsnagClientTests when that file exists
+ */
+- (void)testBSGOrientationNameFromEnum {
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationPortraitUpsideDown), @"portraitupsidedown");
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationPortrait), @"portrait");
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationLandscapeRight), @"landscaperight");
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationLandscapeLeft), @"landscapeleft");
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationFaceUp), @"faceup");
+    XCTAssertEqualObjects(BSGOrientationNameFromEnum(UIDeviceOrientationFaceDown), @"facedown");
+
+    XCTAssertNil(BSGOrientationNameFromEnum(-1));
+    XCTAssertNil(BSGOrientationNameFromEnum(99));
+
+    [BugsnagClient setLastOrientation:@"testOrientation"];
+    XCTAssertEqualObjects([BugsnagClient lastOrientation], @"testOrientation");
+}
+
 @end