Skip to content

Fix handlers #375

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

Merged
merged 5 commits into from
Apr 14, 2021
Merged

Fix handlers #375

merged 5 commits into from
Apr 14, 2021

Conversation

Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented Apr 5, 2021

Fix Android handler

  • Add in-app message handler listener
  • Fix grouped notification parsing, not add to JSON if is an empty array
  • Fix action id, make it optional
  • Add notification open handler listener

Fix iOS Notification open handler

  • Add open handler listener
  • Add cold start support --> Not working

On iOS notification, the open handler is not being called when the app is swiped away.

This change is Reviewable

@Jeasmine Jeasmine requested a review from jkasten2 April 5, 2021 21:02
Jeasmine added 2 commits April 5, 2021 18:03
* Add in-app message handler listener
* Fix grouped notification parsing, not add to JSON if is an empty array
* Fix action id, make it optional
* Add notification open handler listener
* Add open handler listener
* Add cold start support
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 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @rgomezp)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 46 at r1 (raw file):

  /** Plugin registration. */
  private OSNotificationOpenedResult coldStartNotificationResult;

See my iOS note about cold start Notification handling and match this with Android.


ios/Classes/OneSignalPlugin.m, line 332 at r1 (raw file):

- (void)handleNotificationOpened:(OSNotificationOpenedResult *)result {
    if (!self.hasSetNotificationOpenedHandler) {
        _coldStartOpenResult = result;

The wrapper SDK shouldn't have to handle the cold start as the main SDK already handles this.
OneSignal/OneSignal-iOS-SDK#826

For this to work correctly however we should call this code when the flutter developer setups the handler, not in addObservers above.

    [OneSignal setNotificationOpenedHandler:^(OSNotificationOpenedResult * _Nonnull result) {
       [OneSignalPlugin.sharedInstance handleNotificationOpened:result];
    }];

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.

The comments above are something I think we should do before the final release. But ok to merge this PR as-is now to not block a Beta release.

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @rgomezp)

Copy link
Contributor Author

@Jeasmine Jeasmine 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 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @rgomezp)


ios/Classes/OneSignalPlugin.m, line 332 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

The wrapper SDK shouldn't have to handle the cold start as the main SDK already handles this.
OneSignal/OneSignal-iOS-SDK#826

For this to work correctly however we should call this code when the flutter developer setups the handler, not in addObservers above.

    [OneSignal setNotificationOpenedHandler:^(OSNotificationOpenedResult * _Nonnull result) {
       [OneSignalPlugin.sharedInstance handleNotificationOpened:result];
    }];

Nice catch, 100% right, will remove this

* Cold start handling is being done by native iOS implementation
Copy link
Contributor Author

@Jeasmine Jeasmine 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 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @jkasten2, and @rgomezp)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 46 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

See my iOS note about cold start Notification handling and match this with Android.

Done.


ios/Classes/OneSignalPlugin.m, line 332 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

Nice catch, 100% right, will remove this

After the change, on iOS, it still doesn't trigger the notification open handler when the app is swiped again under cold start. I also tested it while in the foreground.

Run application without setting the notification open handler.
Send a notification, and the device receives the notification
Open notification
Set open handler
Expected behavior, notification open handler callback is being called
Actual behavior only "Notification will show in foreground handler set successfully" msg appears on the log

Copy link
Contributor Author

@Jeasmine Jeasmine 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 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @jkasten2, and @rgomezp)


ios/Classes/OneSignalPlugin.m, line 332 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

After the change, on iOS, it still doesn't trigger the notification open handler when the app is swiped again under cold start. I also tested it while in the foreground.

Run application without setting the notification open handler.
Send a notification, and the device receives the notification
Open notification
Set open handler
Expected behavior, notification open handler callback is being called
Actual behavior only "Notification will show in foreground handler set successfully" msg appears on the log

This scenario does work on Android

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.

I am pretty sure the reason why iOS Notification open cold starts are not working as OneSignal.initWithLaunchOptions is being called too late.

In the 3.0.0 branch it is being called from setAppId:

[OneSignal initWithLaunchOptions:nil];

We used to call OneSignal init from registerWithRegistrar in the 2.x.x SDK:
https://github.com/OneSignal/OneSignal-Flutter-SDK/blob/2.6.3/ios/Classes/OneSignalPlugin.m#L85

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @jkasten2, and @rgomezp)

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 1 of 4 files at r1.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @rgomezp)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 46 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

Done.

I see the code changes where the use is now removed for coldStartNotificationResult. Now we just need to delete this unused definition 😄

@MeAtPros
Copy link

@Jeasmine Can you fix this error quickly? My client is waiting for the application update and we are unable to release the correct version due to this problem.

* Cold start handling is being done by native Android implementation
Copy link
Contributor Author

@Jeasmine Jeasmine 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: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @emawby, @jkasten2, and @rgomezp)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 46 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

I see the code changes where the use is now removed for coldStartNotificationResult. Now we just need to delete this unused definition 😄

Done.


ios/Classes/OneSignalPlugin.m, line 332 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

This scenario does work on Android

Done. Fixed by moving initWithLaunchOptions

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.

Just one logging message to clean up. Feel free to merge in after fixing without another approval.

Reviewed 1 of 4 files at r1, 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @rgomezp)


ios/Classes/OneSignalPlugin.m, line 295 at r4 (raw file):

- (void)initNotificationOpenedHandlerParams {
    [OneSignal setNotificationOpenedHandler:^(OSNotificationOpenedResult * _Nonnull result) {
        [OneSignal onesignalLog:ONE_S_LL_VERBOSE message:@"setNotificationOpenedHandler called from addObservers"];

Logging isn't correct. Replace "addObservers" with "initNotificationOpenedHandlerParams".

Copy link
Contributor Author

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

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

That log is not needed anymore since it is used to track open handler on cold start. But that is already fixed. I will remove the log

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @rgomezp)

* Fix open handler not called on cold start
@Jeasmine Jeasmine merged commit 303c9cb into major-release-3.0.0 Apr 14, 2021
@Jeasmine Jeasmine deleted the fix/handlers branch April 14, 2021 14:22
@youssefali424
Copy link

when would it be merged to master? or published to pub?

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.

4 participants