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

Jump to bottom

[5.0.0] Notification Foreground Listener - API update #1258

Merged
merged 10 commits into from
May 1, 2023
Merged

[5.0.0] Notification Foreground Listener - API update #1258

merged 10 commits into from
May 1, 2023

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Apr 28, 2023 •
edited
Loading

Description

One Line Summary

Update Notification Foreground Listener API.

Details

Motivation

Update APi.

Scope

  • Rename notificationWillShowInForegroundHandler to OSNotificationLifecycleListener
  • It will have the onWillDisplay event.
  • I had trouble putting OSDisplayableNotification into OneSignalCore where OSNotification lived, so it is in OneSignalNotifications.
  • Moved some methods out of OSNotification and into OSDisplayableNotification that are for the displaying logic.
  • Files structures aren't great and can be refactored, I refrained from deleting renaming or adding files to avoid conflicts while making many changes at once.
  • I think doesn't support multiple listeners, so that should be modified.

NOTE: I did not update the timeout time and it is still 25 seconds to call display() and this time is not updated to 30 seconds due to iOS limitation. For example, calling display() 29 seconds later would not display the notification.

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Unit testing

OPTIONAL - Explain unit tests added, if not clear in the code.

Manual testing

RECOMMEND - OPTIONAL - Explain what scenarios were tested and the environment.
Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@nan-li
Notification Foreground Listener - API update
2cc2c36 
* Rename notificationWillShowInForegroundHandler to OSNotificationLifecycleListener
* It will have the `onWillDisplay` event.
nan-li
nan-li commented Apr 28, 2023
View reviewed changes
iOS_SDK/OneSignalSDK/OneSignalNotifications/OSNotificationsManager.m Outdated

- (void)preventDefault {
[OneSignalLog onesignalLog:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"OSNotificationWillDisplayEvent.preventDefault called."]];
_isPreventDefault = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need for both

@nan-li
Copy link
Contributor Author

nan-li commented Apr 28, 2023 •
edited
Loading

✍️ TODO: To support multiple listeners, we will prevent default if any of them call preventDefault(). Then, if any calls display(), we will display it. If additional listeners call preventDefault() or display(), nothing will happen.

@emawby emawby self-requested a review April 28, 2023 20:24
@nan-li
Push Subscription Observer - API update
c5dad91 
* Rename OSPushSubscriptionStateChanges to OSPushSubscriptionChangedState
* Rename event from onOSPushSubscriptionChanged to onPushSubscriptionDidChange
* Use current and previous instead of to and from
* No other logic changes, only naming
nan-li and others added 8 commits April 28, 2023 14:39
@nan-li
[nit] display event does not need preventDefault state
9d27761 
* It can just use the `wantsToDisplay` property of the notification it owns.
@nan-li
IAM Click Listener - API update
9695f85 
* Rename click block to `OSInAppMessageClickListener`
* Rename `setClickHandler` to  `addClickListener`
* Add a remove method for the click listener
* OSInAppMessageAction has basically been renamed to OSInAppMessageClickResult
* OSInAppMessageClickEvent will be passed to developers instead of OSInAppMessageAction and the event will contain an in app message and a click result.
@nan-li
[nit] remove some old non-existent methods
58cad6f 
OSInAppMessageDelegate  and handleMessageAction not used by anything
@nan-li
make a fix to IAM click listener
f5c34df 
* We have some legacy logic about IAM direct influence but if there are multiple click listeners added, we only want this to update once, and not for every listener.
@nan-li
Notification Click Listener - API update
95efdd1 
* Rename setNotificationOpenedHandler -> addClickListener, removeClickListener
* Rename object `OSNotificationOpenedResult` -> `OSNotificationClickEvent`
* Rename object `OSNotificationAction` -> `OSNotificationClickResult`
* Support having multiple listeners
@emawby
Merge pull request #1262 from OneSignal/5.0.0/notification_click_list…
553d31c 
…ener_api

[5.0.0] Notification Click Listener - API update
@emawby
Merge pull request #1261 from OneSignal/5.0.0/iam_click_listener_api
494bf10 
[5.0.0] IAM Click Listener - API update
@emawby
Merge pull request #1260 from OneSignal/5.0.0/push_subscription_obser…
d3613e2 
…ver_api

[5.0.0] Push Subscription Observer - API update
@emawby emawby merged commit 412e65a into 5.0.0/iam_lifecycle_listener_api May 1, 2023
@emawby emawby deleted the 5.0.0/notification_foreground_listener_api branch May 1, 2023 20:28
@emawby emawby mentioned this pull request May 1, 2023
Merged
@nan-li nan-li mentioned this pull request Jul 19, 2023
Merged
18 tasks
nan-li pushed a commit that referenced this pull request Oct 30, 2023
@emawby @nan-li
Merge pull request #1258 from OneSignal/5.0.0/notification_foreground…
7185348 
…_listener_api

[5.0.0] Notification Foreground Listener - API update
nan-li pushed a commit that referenced this pull request Oct 30, 2023
@emawby @nan-li
Merge pull request #1258 from OneSignal/5.0.0/notification_foreground…
969b324 
…_listener_api

[5.0.0] Notification Foreground Listener - API update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emawby emawby emawby approved these changes

@jkasten2 jkasten2 Awaiting requested review from jkasten2

@shepherd-l shepherd-l Awaiting requested review from shepherd-l

@jennantilla jennantilla Awaiting requested review from jennantilla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nan-li @emawby