-
Notifications
You must be signed in to change notification settings - Fork 368
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
Fix HMS activity open #1533
Fix HMS activity open #1533
Conversation
@jkasten2 will love your point of view in this PR 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jeasmine Thanks for diving in and fixing the issue! Just some things to clean up in the PR and we can get it merged in.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Jeasmine)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2339 at r1 (raw file):
* Method called when opening a notification */ static void handleNotificationOpen(final Activity context, final JSONArray data, final boolean fromHMSMessage, @Nullable final String notificationId) {
Instead of naming this fromHMSMessage
we should come up with a boolean name that is more generic, to separate concerns. Something like startLauncherActivity
?
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2368 at r1 (raw file):
applicationOpenedByNotification(notificationId); // HMS notification with Message Type being Message won't trigger Activity reverse trampolining logic
This comment would be better fit if it was added to notificationPayloadProcessorHMS.java
instead.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2372 at r1 (raw file):
// Last EMUI (12 to the date) is based on Android 10, so no // Activity trampolining restriction exist for HMS devices if (OSUtils.isHuaweiDeviceType() && fromHMSMessage) {
Checking OSUtils.isHuaweiDeviceType()
is redundant, we can remove it.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2381 at r1 (raw file):
} // This opens the app in the same way an Android home screen launcher does.
Let's add a comment that reverse activity traplining is used for most notifications, this method is only used if the push provider does support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @emawby and @jkasten2)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2339 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Instead of naming this
fromHMSMessage
we should come up with a boolean name that is more generic, to separate concerns. Something likestartLauncherActivity
?
Done.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2368 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
This comment would be better fit if it was added to
notificationPayloadProcessorHMS.java
instead.
Done.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2372 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Checking
OSUtils.isHuaweiDeviceType()
is redundant, we can remove it.
Done.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2381 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Let's add a comment that reverse activity traplining is used for most notifications, this method is only used if the push provider does support it.
Nice catch
Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jkasten2)
* HMS notification with Message type "Message" won't trigger activity trampolining logic * Start application from NotificationOpenedActivityHMS
9102a12
to
0c7aecf
Compare
@peter-rosendahl Just wanted to call out to you this should fix the issue in the #1494 (comment) you left when sending HMS notification types (not data type). It is now available in the 4.7.0 release: You can manually bump the OneSignal version in your |
@jkasten2 I have a similar or the same issue as @peter-rosendahl with the OneSignal SDK for Android. I hoped it is fixed now with the 4.7.0 release, but the issue is still present on my Huawei device. The notification is present, but when clicking on it, the app is not opened. |
@dkoeltringer Thanks for reporting. Is the issue happening with "message" and / or "data" Huawei HMS notification types?
The different is important as they work much differently from another. Questions
|
@jkasten2 Thank you for your quick reply! Here the answers for your questions:
Thank you very much for your help! |
Description
One Line Summary
HMS notification with Message Type Message doesn't open the app.
Details
Motivation
HMS notification with Message Type Message, don't trigger OS logic. It doesn't enter on the activity reverse trampolining logic. Notification open depends on the start activity that was still implemented in the release 4.4.2. Also, the HMS notification trampolining point of entrance is NotificationOpenedActivityHMS configured by the backend.
Since the last EMUI is based on Android 10 and the coming HarmonyOS version doesn't specify activity trampolining requirements, the last logic was pulled back again.
Scope
HMS notification with Message Type Message
Manual testing
Test app opens by notification with data and message, message type, with application in the background and swiped away. On Huawei P20 Lite Android 9
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is