Bump deployment target to iOS 12 #92
Conversation
| @@ -47,13 +47,8 @@ extension UIApplication { | |||
| UIApplication.bugReporterToGestureRecognizerMap.setObject(bugReportingGestureRecognizer, forKey: bugReporter) | |||
|
|
|||
| if !UIApplication.observingKeyWindowNotifications { | |||
| #if swift(>=4.2) | |||
| @@ -27,6 +27,12 @@ | |||
| #import "ARKScreenshotViewController.h" | |||
| #import "UIActivityViewController+ARKAdditions.h" | |||
|
|
|||
| typedef NS_ENUM(NSUInteger, ARKLogExportOption) { | |||
There was a problem hiding this comment.
can you help me understand why this change was needed? This code looks reasonable but I'm not following why this was required.
There was a problem hiding this comment.
This is one of @JustinDSN's commits, so he could explain the thinking behind the change better than I can. The short version is that UIActionSheet and UIPopoverController are deprecated in iOS 8.3 and iOS 9, respectively, so this replaces their usage with UIAlertControllers.
This specific change (the enum of export options) could be implemented as two separate methods fairly easily. This route adds a bit more structure in order to avoid repeating a bit of logic. I don't have strong feelings either way given it's not a ton of structure but also not a ton of repeated logic.
There was a problem hiding this comment.
You summarized the specific trade offs I was trying to balance here. It's really good feedback to see the feedback on repeated logic and that would have been acceptable. Thanks for representing the thought process as well as seeing both sides of the same coin.
There was a problem hiding this comment.
A comment explaining the why would be helpful for future maintainers 🙂
|
|
||
| - (void)actionSheet:(UIActionSheet *)actionSheet clickedButtonAtIndex:(NSInteger)buttonIndex; | ||
| { | ||
| #if TARGET_IPHONE_SIMULATOR |
There was a problem hiding this comment.
looks like this code block was moved down to _exportLogsWithOption with no meaningful changes
|
|
||
| if (actionSheet == self.clearLogsConfirmationActionSheet) { | ||
| if (buttonIndex == actionSheet.destructiveButtonIndex) { | ||
| [self.logStore clearLogsWithCompletionHandler:NULL]; |
There was a problem hiding this comment.
this code moved down into an UIAlertAction handler with no changes
| @@ -100,7 +100,7 @@ typedef void (^ARKEmailBugReporterCustomPromptCompletionBlock)(ARKEmailBugReport | |||
| /// Controls the number of recent error logs per log distributor to include in the email body of a bug report composed in a mail client that does not allow attachments. Defaults to 15. | |||
| @property (nonatomic) NSUInteger numberOfRecentErrorLogsToIncludeInEmailBodyWhenAttachmentsAreUnavailable; | |||
|
|
|||
| /// The window level for the email composer on iOS 7 or later. Defaults to UIWindowLevelStatusBar + 3.0. | |||
| /// The window level for the email composer. Defaults to UIWindowLevelStatusBar + 3.0. | |||
There was a problem hiding this comment.
the + 3 hack continues to give me joy. So many years later and it still works.
| NSError *error = nil; | ||
| NSData *data = [NSKeyedArchiver archivedDataWithRootObject:object requiringSecureCoding:NO error:&error]; | ||
|
|
||
| ARKCheckCondition(error == nil, , @"Couldn't archive object %@", object); |
| @@ -143,7 +139,10 @@ - (void)readObjectsFromArchiveWithCompletionHandler:(nonnull void (^)(NSArray * | |||
|
|
|||
| id object = nil; | |||
| @try { | |||
| #pragma clang diagnostic push | |||
| #pragma clang diagnostic ignored "-Wdeprecated" | |||
Co-authored-by: Dan Federman <dfed@me.com>
Co-authored-by: Dan Federman <dfed@me.com>
* Bump deployment target to iOS 12 and watchOS 4 * Bump Swift version to 5.0 * Adopt recommended project setting * Migrate to stringByAddingPercentEncodingWithAllowedCharacters * Migrate to UIAlertController * Add watchOS version requirement to README Co-authored-by: Dan Federman <dfed@me.com> Co-authored-by: Justin Steffen <jsteffen@squareup.com>
Some commits in here are cherry picked off of @JustinDSN's #82 and rebased on top of the Aardvark 4.0 development branch. Resolves #83.