-
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 notification open not launching main activity by default on app cold start #1276
Conversation
* Main activity wasn't luanching when tapping on a notification that resulted in the app being cold started. * This was fixed by adding a missing startActivity and removing an unneeded PendingIntent * This was a bug interduced in PR #1192 in 4.0.0 release
* Cleans up unneeded logic and confirmed no behavior changes. * `FLAG_ACTIVITY_NEW_TASK` is not needed as `getLaunchIntentForPackage` already defaults this. - See Android ASOP source code - Android 11 - https://cs.android.com/android/platform/superproject/+/android-11.0.0_r3:frameworks/base/core/java/android/app/ApplicationPackageManager.java;l=251 - Android 4 - https://cs.android.com/android/platform/superproject/+/android-4.0.4_r2.1:frameworks/base/core/java/android/app/ApplicationPackageManager.java;l=121
logger.debug("startOrResumeApp from context: " + activity + " isRoot: " + activity.isTaskRoot() + " with launchIntent: " + launchIntent); | ||
|
||
// Not all apps have a launcher intent, such one that only provides a homescreen widget | ||
if (launchIntent == null) |
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.
How does this prevent the application from being launched from the launching activity when the application is in the background, instead of resuming the activity on top of the stack?
Doesn't this remove the behavior of emulating app icon click when the app is in the background when clicking a notification?
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.
This bug was due to missing a startActivity
in the else
of the isTaskRoot
check. This was fixed in commit 0326d93 of this PR.
I noticed getLaunchIntentForPackage
defaults to FLAG_ACTIVITY_NEW_TASK
so the whole isTaskRoot
check and setting of it isn't needed. Notes to this and ASOP source linked of this in commit b27ac14
I didn't notice any other behavior differences before and after this PR. See the manual test scenarios above for what I tested.
This PR is scoped to fixing this one specific cold start bug. However I did notice an additional resume issue which which I fixed in PR #1277.
Test failing due to unrelated flaky tests. |
* No behavior changes in this commit, just clean up * inContext -> activity - As we are requiring an Activity, not just any Context * Early retrun for launchIntent null check. * Better comment of why launchIntent has a null check
7905deb
to
14c340f
Compare
Ran the following to clean up fixup commits
|
Scope
Fix
This is fixes a bug where the app would not be open when the app was completely closed when tapping on a notification.
Clean up
Clean up to internals of the private
startOrResumeApp
method.Tests
Automated
Normally a failing test should be added before a fix is done but since the bugged code path was completely removed it isn't needed. There is an existing test already covering the launching the main activity so all lines of code are covered in this modification.
Manual
App w/ splash and single Activity (the example app in this repo)
This change is