-
-
Notifications
You must be signed in to change notification settings - Fork 735
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
onPushOpen of push notification no longer able to start activity in Android 12 #1152
Comments
Hey @breumr thanks for opening this. I have introduced some changes in supporting push notifications for Android 12+ and I will be more than happy if you will find some time to open a PR for fixing this. I will glad to review it. |
Hi @L3K0V thanks for responding. As much as I'd love to be able to fix it, I'm only a part time Android dev and am not actually confident of exactly how this should be fixed :( |
This seems to be a critical bug, I'm adding a bounty to it. |
In order to solve it Google folks recommend removing the delegation of opening the notification from the broadcast receiver and do it directly from the Service (in our case from |
I just finished taking a deep look at the code and found where is the gotcha. @L3K0V I already saw your latest changes as well.
The android studio shows a warning on this line contains the following message
What Google folks want us to do here is to pass our activity intent (Wrapped by PendingIntent) inside the notification if ( Build.VERSION.SDK_INT >= Build.VERSION_CODES.O){
pContentIntent = PendingIntent.getActivity(context,
contentIntentRequestCode,
activityIntent, // The activity intent is the one we build inside the onPushOpen but now we should build it in onPushReceive
pendingIntentFlags
);
} else {
pContentIntent = PendingIntent.getBroadcast(
context,
contentIntentRequestCode,
contentIntent,
pendingIntentFlags
);
} And then we pass that down to the notification builder.
What is the problem with this ? |
@bahaa-kallas from what I remember and seeing now the Broadcast receiver was used to track push notifications open/dismiss for analytics like here:
parse-dashboard or parse-server repos .
parse-community/parse-dashboard#1474 @mtrezza commented there:
And he can elaborate a little more if remember. Everything until now means that actually the implementation can be adjusted to use activities directly instead of the Parse Broadcast Receiver if analytics are abandoned. A workaround also, if we switch to activity implementation for newer Android versions will be to tell developers if they want analytics to call |
As I am not part of the board here. It's not my decision to make (Whether to break push open tracking completely in this sdk Or putting the responsibility of tracking this event on the developers in their codebase but not at the sdk level) I can open a PR right now that will include the fix above but I would vote for waiting a little bit. Maybe someone can come up with a better solution that won't introduce breaking changes (I have been disconnected for a while from the android eco-system so it's really likely that my naive solution above is not the only solution to the problem) In a different context but relevant to our discussion here . Someone came up with (even worse than using broadcast receivers) idea from UI/UX perspective. |
Let's wait for @mtrezza what have to say about Analytics.
I'm thinking about push notification to open a special Parse Activity which handles the analytics. Like |
What you are thinking about is exactly what I have found in the stackoverflow link. I am not sure how bad is the UI/UX in that case (How much time it will take to reach the desired activity after clicking on the notification). But it seems like a solution for what we face here. |
@bahaa-kallas I'm not taking about invisible activity, but Base activity which the developer should extend, so we can inject some logic. Of course it would be great to hear different options and opinions. How do you imagine it, do you have some ideas after checking the code? |
Regarding Analytics... Parse Server:
Community:
Strategy:
Maybe we can think about removing any analytics legacy code from server and SDKs. We probably should consider it a breaking change, just in case someone has created their own analytics adapter based on the existing implementation. More specifically regarding this issue; I think whatever solution we take, it should be possible to track push opens with popular 3rd party analytics tools, such as Google Analytics, AWS Mobile Analytics, or Mixpanel. |
Related to the analytics discussion, we have an open source analytics adapter we released recently that plugs into the Parse Server, https://github.com/netreconlab/parse-server-any-analytics-adapter |
So maybe we should keep the analytics route? In any case, to me this seems to be a primitive implementation of analytics, which may be enough for certain use cases. In a more sophisticated app I think analytics is usually comprised of much more than tracking events. Also, in a pay-per-request setting, the Parse Server intermediary that just passes requests on from the client to a 3rd party may cost more than it's worth, especially when collection many data points. |
My vote would be to keep as it's low maintenance from a Parse Server development perspective and isn't hurting anything. I think the usage of analytics on the server vs client side is a case-by-case situation for developers. For the apps my teams and collaborators build, we prefer the current server-side setup over the bloat (multiple dependencies) client side SDK's like Firebase provide to capture analytics. The beauty of Parse-Server is the flexibility it provides the developer for making such decisions and enables them to consider the tradeoffs of costs, requests, bloat, functionality, etc. |
If we decide to keep it, I think we'd at least need to document it officially. In server docs and client SDK docs. Currently it appears like a half-way implemented feature artifact. We should probably also add the (only known?) Analytics adapter to our list of 3rd party adapters on our website. |
The PR thread that added the Analytics Adapter configuration may provide some insight for documentation, parse-community/parse-server#2327. I currently don’t know of other adapters. |
Great discussion and most importantly I believe we are on the same page, so I propose this: As I wrote somewhere above we could patch and for other versions of Android to leave the default old behaviour and for Android 12+ we will add support for Activities and we will document it, so if a developer wants to preserve the behaviour AND they are using analytics in some form, they should handle it by them selves in the Activity. No ParseActivity, no invisible activities. Who is using the analytics will must do additional small adjustments. How this sounds to you @mtrezza, @bahaa-kallas? |
Sounds good |
Is there any update on this? I'm currently using v4.3.0 but it still won't open the app on notifications in Android 12 to 14. |
Could someone re-describe the scope of this issue as of today? So we can prioritize this issue accordingly. This is an older issue, so I assume it doesn't affect push notifications in general, otherwise I guess there would have been stronger feedback. Is this only a Parse Analytics issue, or also for other certain scenarios that open activities based on push? |
Issue Description
Notification trampoline restrictions have been introduced in Android 12 which restricts how activities can be started. As this article describes it:
To improve app performance and UX, apps that target Android 12 or higher can't start activities from services or broadcast receivers that are used as notification trampolines. In other words, after the user taps on a notification, or an action button within the notification, your app cannot call startActivity() inside of a service or broadcast receiver.
Steps to reproduce
Trigger a push notification and tap on the notification on the Android device. This triggers onPushOpen in the ParsePushBroadcastReceiver
Actual Outcome
Nothing happens. Logcat displays error:
E/NotificationService: Indirect notification activity start (trampoline) from {insert package name} blocked
Expected Outcome
App should open
Environment
Parse Android SDK
The text was updated successfully, but these errors were encountered: