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.

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 Display Type Overrider Delegate #488

Merged
merged 12 commits into from
Apr 1, 2019

Conversation

Nightsd01
Copy link
Contributor

@Nightsd01 Nightsd01 commented Mar 10, 2019

• This PR adds the ability to customize the display type for individual push notifications that are received while the app is open.
• This is useful for example in messaging apps, where you do not need to show an alert/notification for messages received if you are already viewing a thread with the same person.

• This PR adds the following method to OneSignal:

[OneSignal setNotificationDisplayTypeDelegate:delegate]

• When a notification is received, the following method on the delegate will fire:

- (void)willPresentInFocusNotificationWithPayload:(OSNotificationPayload *)payload forDisplayType:(OSNotificationDisplayType *)type

• The display type is passed in as a pointer that the app can set (or not). If the developer wants to set the NONE display type for this specific notification, they would do so like this:

- (void)willPresentInFocusNotificationWithPayload:(OSNotificationPayload *)payload forDisplayType:(OSNotificationDisplayType *)type {
   *type = OSNotificationDisplayTypeNone;
}

• Setting the display type for this notification does not effect any future notifications that may be received, and the SDK will always default to whatever was set with [OneSignal setInFocusDisplayOption:]

• Adds a test to ensure that the behavior works correctly
• Adds this delegate to the demo project, it can be tested by sending overrideDisplayType in the additionalData of a push notification and setting it to 0 (none), 1 (alert), or 2 (notification)


This change is Reviewable

• Adds a new delegate to the OneSignal SDK that allows you to customize the display type for notifications received while the app is in focus.
• Adds a test to make sure the display type works correctly
• Noticed a copyright comment was incorrect on a file I made over a year ago!
• Adds a notification center category that was mistakenly left out of the previous commit
• Adds the display type delegate to the example project
• Adds a check to make sure the delegate is only called if the app is in focus (and not just in the background)
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @Nightsd01)


iOS_SDK/OneSignalSDK/Source/OneSignal.h, line 219 at r1 (raw file):

// Allows apps to customize per-notification display-type
@protocol OSNotificationDisplayTypeDelegate <NSObject>
- (void)willPresentInFocusNotificationWithPayload:(OSNotificationPayload *)payload forDisplayType:(OSNotificationDisplayType *)type;

What you think about using a callback like userNotificationCenter:willPresentNotification:withCompletionHandler: has? Instead of a pointer to forDisplayType.

This will make it possible for someone to do some work what might take longer so they don't block the main thread.

@Nightsd01
Copy link
Contributor Author

@jkasten2 we could do that, but it would complicate things. I’m using a pointer because I’m guessing that in a lot of cases they’ll just want to use the default display type in which case they don’t need to dereference the pointer or do anything with it.

If we use a block however, that makes things a bit more complex since we’ll have to do something if they never call it, do we check after a timer? Etc. so I figured this was simpler and faster.

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Hmm, true, keeping it simple is probably best. I think returning value instead of having to update a pointer would be simpler. What do you think? Also I am guess this would be easier to Swift too, since otherwise you would have to use UnsafePointer?

I think a timmer would work if we did do a callback. I can't remember if Apple documented theirs somewhere for their userNotificationCenter:willPresentNotification:withCompletionHandler: but I want to say it is 30 seconds. Anyway, probably ok to keep it a simple API. A 2nd callback typed method could always be added later.

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @Nightsd01)

@Nightsd01
Copy link
Contributor Author

@jkasten2 now that I think about it a bit more, I think a callback might actually be best since it will make it easier to use with wrapper SDK’s that can’t do synchronous returns. That should make it easy to implement in the wrapper SDK’s.

I will update the PR this weekend to use a callback with a 30 second timer - that sound good? The only complexity is that multiple notifications can arrive pretty quickly so I’ll keep that in mind.

@jkasten2
Copy link
Member

@Nightsd01 It might be worth trying to see what happens when the withCompletionHandler does not fire to get the time down for sure, then add a bit of buffer to it.

Also I wonder if iOS will wait until the app has called the withCompletionHandler callback before calling userNotificationCenter:willPresentNotification:withCompletionHandler: again. If it does do this it will save on the SDK needing to keep track of a callback list. 😄

• Instead of using a pointer based approach to setting the display type for a given notification, we are switching to using a completion block (callback)
• This commit changes the OSNotificationDisplayTypeDelegate to accept the completion block. If the delegate has been set, the SDK will wait until the delegate calls the completion block to deliver a notification.
• Updates the dev project to use the completion block approach
• Adds a 25 second timeout to make sure that even if an app implements the OSNotificationDisplayTypeDelegate but doesn't execute the callback, the SDK will timeout and will still show the notification
• The complicating factor is that multiple notifications can arrive rapidly, so the app needs to be able to map the correct display types to previously received notifications.
• TODO: Before this PR gets reviewed, I've been working on a test for the timeout edge case but it is a complex timing situation so this test will be added in the next day or two.
• I noticed that the dev app had full compiler optimization enabled - this makes debugging very difficult so since this is a purely dev app, I've reduced the Clang optimization level to NONE
@Nightsd01
Copy link
Contributor Author

@jkasten2 updated the diff! I’ll be adding a test soon but it should be good to take a look at the code now

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @Nightsd01)


iOS_SDK/OneSignalSDK/Source/OneSignal.h, line 221 at r2 (raw file):

// Allows apps to customize per-notification display-type
@protocol OSNotificationDisplayTypeDelegate <NSObject>
- (void)willPresentInFocusNotificationWithPayload:(OSNotificationPayload *)payload

willPresentInFocusNotificationWithPayload 👍 on the naming, very clear on what this does!


iOS_SDK/OneSignalSDK/Source/OneSignal.h, line 222 at r2 (raw file):

@protocol OSNotificationDisplayTypeDelegate <NSObject>
- (void)willPresentInFocusNotificationWithPayload:(OSNotificationPayload *)payload
                           withDefaultDisplayType:(OSNotificationDisplayType)displayType

Thinking we can drop withDefaultDisplayType as I think it is something that wouldn't be used. It also wouldn't be different per notification so if it needs to be read it should be something they can read from calling something on the OneSignal class.

@Nightsd01
Copy link
Contributor Author


iOS_SDK/OneSignalSDK/Source/OneSignal.h, line 222 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Thinking we can drop withDefaultDisplayType as I think it is something that wouldn't be used. It also wouldn't be different per notification so if it needs to be read it should be something they can read from calling something on the OneSignal class.

The reason I included the default display type as a parameter here is to make it easy for developers to just call the completion block with the default value if they don't want to override it in some situations.

They could also just call the completion block with OneSignal.inFocusDisplayType but I think this is less obvious. What do you think?

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @Nightsd01)


iOS_SDK/OneSignalSDK/Source/OneSignal.h, line 222 at r2 (raw file):

Previously, Nightsd01 (Brad Hesse) wrote…

The reason I included the default display type as a parameter here is to make it easy for developers to just call the completion block with the default value if they don't want to override it in some situations.

They could also just call the completion block with OneSignal.inFocusDisplayType but I think this is less obvious. What do you think?

I think anyone who sets up this delegate will know the display type they will want to set. In both cases of if it meets some special payload or if it doesn't.

• Removes the `withDefaultDisplayType` convenience parameter from `OSNotificationDisplayTypeDelegate`'s `willPresentInFocusNotificationWithPayload:withCompletion` method
• This parameter was provided as a convenience for developers so that if they wanted to use the existing default display type, they could just call the callback with this value
• However to simplify things we will remove it, and developers can just call the callback with OneSignal.inFocusDisplayType to achieve the same goal
• Fixes an issue where the delegate could get executed twice under some situations when the timeout is reached (if they implement the `OSNotificationDisplayTypeDelegate` but don't ever call the callback, the SDK times out after a while and uses the default display type)
• This is a *temporary* solution to a bug we've had with an old test.
• The `testSwizzling` test causes the `UNUserNotificationCenterDelegate` to get nullified on the notification center singleton.
• Since some tests use this delegate to mock a push notification delivery, this can break these tests
• Since XCTest cases are run in alphabetical order, we've been lucky that we've never noticed this in the past since `testSwizzling` was run AFTER all of our tests that use the `UNUserNotificationCenterDelegate.willPresentNotification...` method are alphabetically BEFORE the testSwizzling test

• TODO: The notification center delegate gets set to a dummy instance then nullified. We COULD fix this by simply resetting it back to the original value - but this causes issues with swizzling
• We will probably need to change the `setOneSignalUNDelegate` swizzle method to make sure it allows the UNUserNotificationCenterDelegate to be reset without messing up the swizzling which causes infinite loops, etc.
• Adds a new test case to make sure that if a developer implements the `OSNotificationDisplayTypeDelegate`, and their `willPresentInFocusNotificationWithPayload:` method gets called but they somehow don't call the callback, this test makes sure the SDK correctly waits for a while but then times out and uses the default display type
• Fixes the test project's AppDelegate to fix the delegate method signature
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 11 files at r1, 3 of 9 files at r2, 6 of 6 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@jkasten2 jkasten2 merged commit add0227 into OneSignal:master Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants