Skip to content
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.

Sign up for GitHub

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

Add delegate for implementing custom prompting behavior #65

Merged
merged 9 commits into from
Jun 25, 2018
Merged

Add delegate for implementing custom prompting behavior #65

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f84ce1c
Add delegate for implementing custom prompting behavior via the `ARKE…
NickEntin May 6, 2018
afe2c79
Address feedback from @dfed
NickEntin May 9, 2018
d412dbb
Address feedback from @dfed
NickEntin May 9, 2018
7ebd7c6
Bump version to 3.4.0
NickEntin May 9, 2018
0cb6208
Address feedback from @pwesten
NickEntin May 9, 2018
74403e4
Address feedback from @pwesten
NickEntin May 9, 2018
df47e3d
Make and properties on read only, and provide methods, so that th…
NickEntin May 15, 2018
b17bc05
Add missing nullability specifiers
NickEntin May 15, 2018
de292f4
Address feedback from @pwesten
NickEntin Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions Aardvark/ARKEmailBugReportConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,22 @@
@class ARKEmailAttachment;


/// Configuration object describing the contents of an email bug report.
@interface ARKEmailBugReportConfiguration : NSObject
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

triple-slash comment explaining what this object is?


/// The email subject that will be prefilled when the email dialog is presented to the user.
/// The email subject that will be prefilled when the email dialog is presented to the user. Defaults to an empty string.
@property (nonnull, nonatomic, copy) NSString *prefilledEmailSubject;

/// The log stores that will be included as attachments on the email.
/// The log stores that will be included as attachments on the email. Defaults to an empty array.
@property (nonnull, nonatomic, copy) NSArray<ARKLogStore *> *logStores;

/// Controls whether or not a screenshot should be attached to the email, when available.
/// Controls whether or not a screenshot should be attached to the email, when available. Defaults to NO.
@property (nonatomic) BOOL includesScreenshot;

/// Controls whether or not a view hierarchy description should be attached to the email, when available.
/// Controls whether or not a view hierarchy description should be attached to the email, when available. Defaults to NO.
@property (nonatomic) BOOL includesViewHierarchyDescription;

/// Additional attachments to include on the email.
/// Additional attachments to include on the email. Defaults to nil.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a little more convenient to make it nonnull and default to an empty array?

@property (nullable, nonatomic, copy) NSArray<ARKEmailAttachment *> *additionalAttachments;

@end
13 changes: 13 additions & 0 deletions Aardvark/ARKEmailBugReportConfiguration.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,17 @@

@implementation ARKEmailBugReportConfiguration

- (instancetype)init;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 😄

{
self = [super init];

_prefilledEmailSubject = @"";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be worth having a quick comment here stating: "If any of the below default values change, remember to update the header file"

That may also be overkill. I leave it up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like making behavioral changes like changing default values should imply needing to update the header docs, and if it doesn't then you don't have enough header docs. Maybe that's just optimistic thinking, but adding comments saying that things are documented feels backwards.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

_logStores = @[];
_includesScreenshot = NO;
_includesViewHierarchyDescription = NO;
_additionalAttachments = nil;

return self;
}

@end
2 changes: 1 addition & 1 deletion Aardvark/ARKEmailBugReporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ typedef void (^ARKEmailBugReporterCustomPromptCompletionBlock)(ARKEmailBugReport
/// Called on the main thread when a bug is filed to signal that a bug report prompt should be presented to the user.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 for calling out what thread this is called on

/// The `completion` should be called with either an updated configuration to present the email dialog, or `nil` to signal that the prompt was cancelled.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on what thread should completion be called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main 👍

/// When the initial `configuration` has `includesScreenshot` or `includesViewHierarchyDescription` false, setting the field to true will have no effect.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"set to false"?

But also: I'm not sure I get this comment. Why does changing some fields work, and some other fields not? Where should includesScreenshot or includesViewHierarchyDescription be set? Kinda feels like includesScreenshot and includesViewHierarchyDescription shouldn't be mutable... but maybe I'm missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it's a one-way door. If there is a screenshot, the initial configuration will have includesScreenshot set to true. The prompt can leave this alone to include the screenshot or set it to false so the screenshot won't be included. If there is no screenshot to include, the initial configuration will have includesScreenshot set to false. Setting it to true will have no effect since there's nothing to attach.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then, why are these fields mutable at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want the prompt to be able to remove the screenshot and view hierarchy. If we want to make the API more explicit here, we could do something like:

/// Whether or not there is a screenshot attached to the bug report.
@property (nonatomic, readonly) BOOL includesScreenshot;

/// Removes the screenshot from the bug report if one is attached.
- (void)removeScreenshot;

/// Whether or not there is a view hierarchy description attached to the bug report.
@property (nonatomic, readonly) BOOL includesViewHierarchyDescription;

/// Removes the view hierarchy description from the bug report if one is attached.
- (void)removeViewHierarchyDescription;

Alternatively, we could just attach the screenshot data, which would have the added advantage of allowing the custom prompt to modify the image. Although this would probably be dependent on #67 (and maybe #66).

@property (nullable, nonatomic, copy) UIImage *screenshot;

@property (nullable, nonatomic, copy) NSString *viewHierarchyDescription;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked about this offline with @efirestone and landed on going to remove* route for now, and eventually moving to including the screenshot and hierarchy themselves with some other breaking changes in the next major release.

- (void)showBugReportingPromptForConfiguration:(nonnull ARKEmailBugReportConfiguration *)configuration completion:(nonnull ARKEmailBugReporterCustomPromptCompletionBlock)completion;
- (void)showBugReportingPromptForConfiguration:(ARKEmailBugReportConfiguration *_Nonnull)configuration completion:(ARKEmailBugReporterCustomPromptCompletionBlock _Nonnull)completion;

@end

Expand Down
17 changes: 9 additions & 8 deletions Aardvark/ARKEmailBugReporter.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,10 @@ - (void)_createBugReportWithConfiguration:(ARKEmailBugReportConfiguration *)conf

// Once all log messages have been retrieved, attach the data and show the compose window.
dispatch_group_notify(logStoreRetrievalDispatchGroup, dispatch_get_main_queue(), ^{
NSMutableString *emailBody = [self _prefilledEmailBodyWithEmailBodyAdditions:emailBodyAdditions];
NSMutableString *const emailBody = [self _prefilledEmailBodyWithEmailBodyAdditions:emailBodyAdditions];

for (ARKLogStore *logStore in logStores) {
NSArray *logMessages = [logStoresToLogMessagesMap objectForKey:logStore];
NSArray *const logMessages = [logStoresToLogMessagesMap objectForKey:logStore];

NSString *screenshotFileName = [NSLocalizedString(@"screenshot", @"File name of a screenshot") stringByAppendingPathExtension:@"png"];
NSString *logsFileName = [NSLocalizedString(@"logs", @"File name for logs attachments") stringByAppendingPathExtension:[self formattedLogMessagesAttachmentExtension]];
Expand All @@ -412,7 +412,7 @@ - (void)_createBugReportWithConfiguration:(ARKEmailBugReportConfiguration *)conf
logsFileName = [logStore.name stringByAppendingFormat:@"_%@", logsFileName];
}

NSString *recentErrorLogs = [self _recentErrorLogMessagesAsPlainText:logMessages count:self.numberOfRecentErrorLogsToIncludeInEmailBodyWhenAttachmentsAreAvailable];
NSString *const recentErrorLogs = [self _recentErrorLogMessagesAsPlainText:logMessages count:self.numberOfRecentErrorLogsToIncludeInEmailBodyWhenAttachmentsAreAvailable];
if (recentErrorLogs.length) {
[emailBodyForLogStore appendFormat:@"%@\n", recentErrorLogs];
appendToEmailBody = YES;
Expand All @@ -422,22 +422,21 @@ - (void)_createBugReportWithConfiguration:(ARKEmailBugReportConfiguration *)conf
[emailBody appendString:emailBodyForLogStore];
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit: whitespace

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch


if (configuration.includesScreenshot && self.attachScreenshotToNextBugReport) {
NSData *const mostRecentImage = [self _mostRecentImageAsPNG:logMessages];
if (mostRecentImage.length > 0) {
[self.mailComposeViewController addAttachmentData:mostRecentImage mimeType:@"image/png" fileName:screenshotFileName];
}
}

NSData *formattedLogs = [self formattedLogMessagesAsData:logMessages];
NSData *const formattedLogs = [self formattedLogMessagesAsData:logMessages];
if (formattedLogs.length) {
[self.mailComposeViewController addAttachmentData:formattedLogs mimeType:[self formattedLogMessagesDataMIMEType] fileName:logsFileName];
}
}

if (configuration.includesViewHierarchyDescription && self.viewHierarchyDescription != nil) {
NSString *viewHierarchyFileName = [NSLocalizedString(@"view_hierarchy", @"File name for view hierarchy attachment") stringByAppendingPathExtension:@"txt"];
NSString *const viewHierarchyFileName = [NSLocalizedString(@"view_hierarchy", @"File name for view hierarchy attachment") stringByAppendingPathExtension:@"txt"];
NSData *const viewHierarchyData = [self.viewHierarchyDescription dataUsingEncoding:NSUTF8StringEncoding];
if (viewHierarchyData.length > 0) {
[self.mailComposeViewController addAttachmentData:viewHierarchyData mimeType:@"text/plain" fileName:viewHierarchyFileName];
Expand All @@ -464,10 +463,10 @@ - (void)_createBugReportWithConfiguration:(ARKEmailBugReportConfiguration *)conf

// Only append logs once all log messages have been retrieved.
if (logStoresToLogMessagesMap.count == logStores.count) {
NSMutableString *emailBody = [self _prefilledEmailBodyWithEmailBodyAdditions:emailBodyAdditions];
NSMutableString *const emailBody = [self _prefilledEmailBodyWithEmailBodyAdditions:emailBodyAdditions];

for (ARKLogStore *logStore in logStores) {
NSArray *logMessages = [logStoresToLogMessagesMap objectForKey:logStore];
NSArray *const logMessages = [logStoresToLogMessagesMap objectForKey:logStore];
[emailBody appendFormat:@"%@\n", [self _recentErrorLogMessagesAsPlainText:logMessages count:self.numberOfRecentErrorLogsToIncludeInEmailBodyWhenAttachmentsAreUnavailable]];
}

Expand Down Expand Up @@ -652,6 +651,7 @@ - (void)_showBugTitleCaptureAlert;

[alertController addAction:[UIAlertAction actionWithTitle:cancelButtonTitle style:UIAlertActionStyleDefault handler:^(UIAlertAction *action) {
self.completion(nil);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we set completion to nil after this line to prevent accidentally calling it twice?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... or consider the no-storage approach.

self.completion = nil;
}]];

[alertController addTextFieldWithConfigurationHandler:^(UITextField *textField) {
Expand Down Expand Up @@ -690,6 +690,7 @@ - (void)_createBugReportWithTitle:(NSString *)title;
{
self.configuration.prefilledEmailSubject = title;
self.completion(self.configuration);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we set completion to nil after this line to prevent accidentally calling it twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a short-lived object, so I don't think there's any change in calling it twice. No harm in setting it to nil though, so probably better to be safe.

self.completion = nil;
}

@end