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 2 commits
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
8 changes: 8 additions & 0 deletions Aardvark.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
objects = {

/* Begin PBXBuildFile section */
3D6E2D0E20868335007B8013 /* ARKEmailBugReportConfiguration.m in Sources */ = {isa = PBXBuildFile; fileRef = 3D6E2D0C20868335007B8013 /* ARKEmailBugReportConfiguration.m */; };
3D6E2D0F20868335007B8013 /* ARKEmailBugReportConfiguration.h in Headers */ = {isa = PBXBuildFile; fileRef = 3D6E2D0D20868335007B8013 /* ARKEmailBugReportConfiguration.h */; settings = {ATTRIBUTES = (Public, ); }; };
3DED97142006D35B007FC95C /* ARKEmailAttachment.h in Headers */ = {isa = PBXBuildFile; fileRef = 3DED97122006D35B007FC95C /* ARKEmailAttachment.h */; };
3DED97152006D35B007FC95C /* ARKEmailAttachment.m in Sources */ = {isa = PBXBuildFile; fileRef = 3DED97132006D35B007FC95C /* ARKEmailAttachment.m */; };
4551A2D91BDAD10E00F216D0 /* Aardvark.h in Headers */ = {isa = PBXBuildFile; fileRef = EAD1442419E073FB0065A1FF /* Aardvark.h */; settings = {ATTRIBUTES = (Public, ); }; };
Expand Down Expand Up @@ -94,6 +96,8 @@
/* End PBXContainerItemProxy section */

/* Begin PBXFileReference section */
3D6E2D0C20868335007B8013 /* ARKEmailBugReportConfiguration.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = ARKEmailBugReportConfiguration.m; path = Aardvark/ARKEmailBugReportConfiguration.m; sourceTree = "<group>"; };
3D6E2D0D20868335007B8013 /* ARKEmailBugReportConfiguration.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ARKEmailBugReportConfiguration.h; path = Aardvark/ARKEmailBugReportConfiguration.h; sourceTree = "<group>"; };
3DED97122006D35B007FC95C /* ARKEmailAttachment.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARKEmailAttachment.h; path = Aardvark/ARKEmailAttachment.h; sourceTree = "<group>"; };
3DED97132006D35B007FC95C /* ARKEmailAttachment.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = ARKEmailAttachment.m; path = Aardvark/ARKEmailAttachment.m; sourceTree = "<group>"; };
4551A2C21BDACF9000F216D0 /* Aardvark.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Aardvark.framework; sourceTree = BUILT_PRODUCTS_DIR; };
Expand Down Expand Up @@ -295,6 +299,8 @@
EA98B9311D4BEB6E00B3A390 /* ARKDefaultLogFormatter.h */,
EA98B9321D4BEB6E00B3A390 /* ARKDefaultLogFormatter.m */,
EA98B9331D4BEB6E00B3A390 /* ARKEmailBugReporter_Testing.h */,
3D6E2D0D20868335007B8013 /* ARKEmailBugReportConfiguration.h */,
3D6E2D0C20868335007B8013 /* ARKEmailBugReportConfiguration.m */,
EA98B9341D4BEB6E00B3A390 /* ARKEmailBugReporter.h */,
EA98B9351D4BEB6E00B3A390 /* ARKEmailBugReporter.m */,
3DED97122006D35B007FC95C /* ARKEmailAttachment.h */,
Expand Down Expand Up @@ -419,6 +425,7 @@
EA98B9161D4BEB4900B3A390 /* ARKIndividualLogViewController.h in Headers */,
EA98B9181D4BEB4900B3A390 /* ARKLogTableViewController.h in Headers */,
EA98B93A1D4BEB6E00B3A390 /* ARKDefaultLogFormatter.h in Headers */,
3D6E2D0F20868335007B8013 /* ARKEmailBugReportConfiguration.h in Headers */,
EA98B93E1D4BEB6E00B3A390 /* ARKEmailBugReporter_Testing.h in Headers */,
EA98B9141D4BEB4100B3A390 /* ARKLogDistributor+UIAdditions.h in Headers */,
EA98B9381D4BEB6E00B3A390 /* ARKBugReporter.h in Headers */,
Expand Down Expand Up @@ -622,6 +629,7 @@
EA98B9121D4BEB3D00B3A390 /* ARKLogDistributor+UIAdditions.m in Sources */,
EA98B94E1D4BF37000B3A390 /* UIApplication+ARKAdditions.swift in Sources */,
EA98B9621D4BFA1700B3A390 /* Aardvark.swift in Sources */,
3D6E2D0E20868335007B8013 /* ARKEmailBugReportConfiguration.m in Sources */,
EA98B91A1D4BEB4900B3A390 /* ARKLogTableViewController.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down
45 changes: 45 additions & 0 deletions Aardvark/ARKEmailBugReportConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//
// ARKEmailBugReportConfiguration.h
// Aardvark
//
// Created by Nick Entin on 4/14/18.
// Copyright 2018 Square, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//    http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

#import <Foundation/Foundation.h>

@class ARKLogStore;
@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. Defaults to an empty string.
@property (nonnull, nonatomic, copy) NSString *prefilledEmailSubject;

/// 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. Defaults to NO.
@property (nonatomic) BOOL includesScreenshot;

/// 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. 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
39 changes: 39 additions & 0 deletions Aardvark/ARKEmailBugReportConfiguration.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//
// ARKEmailBugReportConfiguration.m
// Aardvark
//
// Created by Nick Entin on 4/14/18.
// Copyright 2018 Square, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//    http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

#import "ARKEmailBugReportConfiguration.h"


@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
18 changes: 18 additions & 0 deletions Aardvark/ARKEmailBugReporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@


@class ARKEmailAttachment;
@class ARKEmailBugReportConfiguration;
@class ARKEmailBugReporter;
@class ARKLogStore;
@protocol ARKLogFormatter;
Expand Down Expand Up @@ -53,6 +54,20 @@
@end


typedef void (^ARKEmailBugReporterCustomPromptCompletionBlock)(ARKEmailBugReportConfiguration *_Nullable configuration);

@protocol ARKEmailBugReporterPromptingDelegate <NSObject>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


@required

/// 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:(ARKEmailBugReportConfiguration *_Nonnull)configuration completion:(ARKEmailBugReporterCustomPromptCompletionBlock _Nonnull)completion;

@end


/// Composes a bug report that is sent via email.
@interface ARKEmailBugReporter : NSObject <ARKBugReporter>

Expand All @@ -73,6 +88,9 @@
/// The email attachment delegate, responsible for providing additional attachments and filtering which log stores to include in the bug report at the time the bug is filed.
@property (nullable, nonatomic, weak) id <ARKEmailBugReporterEmailAttachmentAdditionsDelegate> emailAttachmentAdditionsDelegate;

/// The prompting delegate, responsible for showing a prompt to file a bug report.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a default behavior if/when this is nil?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, default behavior is the existing behavior (showing an alert asking for the bug title). Added to comment.

@property (nullable, nonatomic, weak) id <ARKEmailBugReporterPromptingDelegate> promptingDelegate;

/// The formatter used to prepare the log for entry into an email. Defaults to a vanilla instance of ARKDefaultLogFormatter.
@property (nonnull, nonatomic) id <ARKLogFormatter> logFormatter;

Expand Down
Loading