Add delegate for implementing custom prompting behavior #65

NickEntin · 2018-05-09T00:45:54Z

Add delegate for implementing custom prompting behavior via the ARKEmailBugReporterPromptingDelegate protocol.

Also fixes a bug where the email dialog isn't shown when no log messages are included in the report. Previously, this could happen when the log stores are empty or the emailAttachmentAdditionsDelegate returns false for all log stores. The custom prompt adds another case of this bug by offering users the chance to filter out all log store.

…mailBugReporterPromptingDelegate` protocol. Also fixes a bug where the email dialog isn't shown when no log messages are included in the report. Previously, this could happen when the log stores are empty or the `emailAttachmentAdditionsDelegate` returns false for all log stores. The custom prompt adds another case of this bug by offering users the chance to filter out all log store.

dfed

Dig the concept! A few comments.

Note that nits/supernits are not blocking 😉

dfed · 2018-05-09T00:57:51Z

Aardvark/ARKEmailBugReportConfiguration.h

+/// The log stores that will be included as attachments on the email.
+@property (nonnull, nonatomic, copy) NSArray<ARKLogStore *> *logStores;
+
+/// Controls whether or not a screenshot should be attached to the email, when available.


might be good to add what the default value is.

dfed · 2018-05-09T00:57:55Z

Aardvark/ARKEmailBugReportConfiguration.h

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


might be good to add what the default value is.

dfed · 2018-05-09T00:58:18Z

Aardvark/ARKEmailBugReportConfiguration.h

+@class ARKEmailAttachment;
+
+
+@interface ARKEmailBugReportConfiguration : NSObject


triple-slash comment explaining what this object is?

dfed · 2018-05-09T00:58:59Z

Aardvark/ARKEmailBugReporter.h

+/// Called on the main thread when a bug is filed to signal that a bug report prompt should be presented to the user.
+/// The `completion` should be called with either an updated configuration to present the email dialog, or `nil` to signal that the prompt was cancelled.
+/// When the initial `configuration` has `includesScreenshot` or `includesViewHierarchyDescription` false, setting the field to true will have no effect.
+- (void)showBugReportingPromptForConfiguration:(nonnull ARKEmailBugReportConfiguration *)configuration completion:(nonnull ARKEmailBugReporterCustomPromptCompletionBlock)completion;


(not a blocker) any chance we can use the modern _Nonnull syntax here rather than nonnull?

dfed · 2018-05-09T00:59:14Z

Aardvark/ARKEmailBugReporter.h

+
+@required
+
+/// Called on the main thread when a bug is filed to signal that a bug report prompt should be presented to the user.


👏 for calling out what thread this is called on

dfed · 2018-05-09T01:08:52Z

Aardvark/ARKEmailBugReporter.m


+        dispatch_group_t logStoreRetrievalDispatchGroup = dispatch_group_create();
+        dispatch_group_enter(logStoreRetrievalDispatchGroup);


nit: technically there's no need for this outside enter/leave. As long as you're calling notify after you've entered all your async tasks, you're fine.

that said, I'm pretty sure the surrounding enter/leave is in the style of other code here, so, we're good either way.

I think theoretically it's better because we could change retrieveAllLogMessagesWithCompletionHandler to be synchronous at some point and potentially inadvertently change behavior here.

As long as notify is called after current async work, and after you'd want it to be executed, any changes to sync/async behavior above shouldn't cause a change.

But yeah, the wrapper certainly makes things a bit more foolproof :)

dfed · 2018-05-09T01:09:11Z

Aardvark/ARKEmailBugReporter.m

+
+        // 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];


supernit: const here and... well... everywhere 😉

Added it everywhere it seems relevant in this method.

dfed · 2018-05-09T01:09:29Z

Aardvark/ARKEmailBugReporter.m

+                if (appendToEmailBody) {
+                    [emailBody appendString:emailBodyForLogStore];
+                }
+


supernit: whitespace

dfed · 2018-05-09T01:10:38Z

Aardvark/ARKEmailBugReporter.m

+- (void)_createBugReportWithTitle:(NSString *)title;
+{
+    self.configuration.prefilledEmailSubject = title;
+    self.completion(self.configuration);


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

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.

dfed · 2018-05-09T01:11:17Z

Aardvark/ARKEmailBugReporter.m

+    }]];
+
+    [alertController addAction:[UIAlertAction actionWithTitle:cancelButtonTitle style:UIAlertActionStyleDefault handler:^(UIAlertAction *action) {
+        self.completion(nil);


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

... or consider the no-storage approach.

dfed · 2018-05-09T01:35:11Z

Aardvark/ARKEmailBugReportConfiguration.m

@@ -23,4 +23,17 @@

 @implementation ARKEmailBugReportConfiguration

+- (instancetype)init;


pwesten · 2018-05-09T18:40:24Z

Aardvark/ARKEmailBugReporter.h

@@ -53,6 +54,20 @@
 @end


+typedef void (^ARKEmailBugReporterCustomPromptCompletionBlock)(ARKEmailBugReportConfiguration *_Nullable configuration);
+
+@protocol ARKEmailBugReporterPromptingDelegate <NSObject>


The <NSObject> specifier may be noise here. Not a strong opinion though since it's idiomatic.

Looks like every other protocol in Aardvark does this, so just as well to match.

pwesten · 2018-05-09T18:44:25Z

Aardvark/ARKEmailBugReporter.m

+        if (configuration != nil) {
+            [self _createBugReportWithConfiguration:configuration];
+        } else {
+            // If the configuration is nil, the callee has signaled that we should not show a bug report. In the future, we can clean up any persisted state here as necessary.


In the absence of a specific to-do item, I'd omit the else and put this comment before the if.

pwesten · 2018-05-09T18:49:34Z

Aardvark/ARKEmailBugReporter.m

+        [self.mailComposeViewController setSubject:configuration.prefilledEmailSubject];
+
+        dispatch_group_t logStoreRetrievalDispatchGroup = dispatch_group_create();
+        dispatch_group_enter(logStoreRetrievalDispatchGroup);


It would be a little better to put this outside the canSendMail check so the bug is fixed for both code paths.

pwesten · 2018-05-09T18:51:33Z

Aardvark/ARKEmailBugReporter.m

+{
+    /*
+     iOS 8 often fails to transfer the keyboard from a focused text field to a UIAlertView's text field.
+     Transfer first responder to an invisble view when a debug screenshot is captured to make bug filing itself bug-free.


invisible :-)

pwesten · 2018-05-09T18:53:55Z

Aardvark/ARKEmailBugReporter.m

+
+- (void)showBugReportingPromptForConfiguration:(nonnull ARKEmailBugReportConfiguration *)configuration completion:(nonnull ARKEmailBugReporterCustomPromptCompletionBlock)completion {
+    self.configuration = configuration;
+    self.completion = completion;


It might be a little tidier to not store these in properties on the object, but instead capture them in the completion blocks inside the method below (which could then just be inlined here).

pwesten · 2018-05-09T18:54:25Z

Aardvark/ARKEmailBugReporter.m

+    }]];
+
+    [alertController addAction:[UIAlertAction actionWithTitle:cancelButtonTitle style:UIAlertActionStyleDefault handler:^(UIAlertAction *action) {
+        self.completion(nil);


... or consider the no-storage approach.

…re one-way mutable

NickEntin · 2018-06-22T20:49:39Z

@dfed @pwesten I think I've addressed all of the feedback. Please take another look when you get a chance.

dfed

LGTM!

dfed · 2018-06-22T23:44:56Z

Aardvark/ARKEmailBugReportConfiguration.m

+{
+    self = [super init];
+
+    _prefilledEmailSubject = @"";


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.

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.

Works for me!

pwesten · 2018-06-25T17:28:06Z

Aardvark/ARKEmailBugReportConfiguration.h

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


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

pwesten · 2018-06-25T17:37:42Z

Aardvark/ARKEmailBugReportConfiguration.h

+- (void)removeScreenshot;
+
+/// Removes the view hierarchy description from the bug report, if one is included.
+- (void)removeViewHierarchyDescription;


It's a little confusing to use the word remove when it's just going to flip a flag. This whole object represents a "configuration" of a bug report, not the actual contents. Might want to adjust comments to reflect that very clearly.

Would disable be better than remove here?

Hmmm "omit"?

Works for me 👍

Actually, exclude might be better, since it matches includesScreenshot

NickEntin requested review from pwesten, efirestone and dfed May 9, 2018 00:46

dfed reviewed May 9, 2018

View reviewed changes

Address feedback from @dfed

afe2c79

dfed reviewed May 9, 2018

View reviewed changes

NickEntin added 2 commits May 8, 2018 18:53

Address feedback from @dfed

d412dbb

Bump version to 3.4.0

7ebd7c6

pwesten reviewed May 9, 2018

View reviewed changes

NickEntin added 4 commits May 9, 2018 14:22

Address feedback from @pwesten

0cb6208

Address feedback from @pwesten

74403e4

Make and properties on read only, and provide methods, so that they a…

df47e3d

…re one-way mutable

Add missing nullability specifiers

b17bc05

dfed approved these changes Jun 23, 2018

View reviewed changes

pwesten reviewed Jun 25, 2018

View reviewed changes

Address feedback from @pwesten

de292f4

pwesten approved these changes Jun 25, 2018

View reviewed changes

NickEntin merged commit f65d75e into square:master Jun 25, 2018

NickEntin deleted the entin/custom-prompting branch June 25, 2018 19:25

		@class ARKEmailAttachment;


		@interface ARKEmailBugReportConfiguration : NSObject


		@required

		/// Called on the main thread when a bug is filed to signal that a bug report prompt should be presented to the user.


		dispatch_group_t logStoreRetrievalDispatchGroup = dispatch_group_create();
		dispatch_group_enter(logStoreRetrievalDispatchGroup);

		@@ -23,4 +23,17 @@

		@implementation ARKEmailBugReportConfiguration

		- (instancetype)init;

Add delegate for implementing custom prompting behavior #65

Add delegate for implementing custom prompting behavior #65

Conversation

NickEntin commented May 9, 2018

dfed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickEntin commented Jun 22, 2018

dfed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment