Skip to content

Android 12: Performance - Remove notification trampoline #16090

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
Merged
AjeshRPai merged 7 commits into from
Jun 2, 2022
Merged

Android 12: Performance - Remove notification trampoline #16090

AjeshRPai merged 7 commits into from
Jun 2, 2022

Conversation

@ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Mar 11, 2022 •
edited
Loading

Closes #15875
Closes #16069

Description

This PR

  1. Removes ShareAndDismissNotificationReceiver (that acted as a notification trampoline) and starts the post published share action without having to depend on a broadcast receiver. - 235231b
  2. Removes logic to start comments activity on an empty reply from NotificationsProcessingService that acted as a notification trampoline. - 2db34f6 (see commit message for details)

Also,

  1. Removes BROADCAST_CLOSE_SYSTEM_DIALOGS permission. - 0a43a35 (see commit message for details)
    Since we removed Intent.ACTION_CLOSE_SYSTEM_DIALOGS at both places: NativeNotificationsUtils, ShareAndDismissNotificationReceiver, this PR closes issue Android 12: Security and privacy - Apps can't close system dialogs #16069 as well.
  2. Fixes a bug by waiting for note download response before displaying the notification as the notification actions info is available only in the full note. - 579372b

To test

Test.1 Post published share action

  1. Publish a post (optionally include an image) and put the app in the background so that the post published notification is shown.
  2. Click the Share action button from the notification.
  3. Complete the action.
  4. Make sure that the behavior remains the same as before.

Before

before

After

out

Test.2 Inline actions present in comment notification

  1. Login using a wpcom account having sites on your device.
  2. Comment on a site post using another account from Calypso.
  3. Wait for the comment notification to arrive on the device.
  4. Expand the notification immediately.
  5. Notice that reply action is present in the notification.
  6. Tap on the Reply action.
  7. Directly reply to the comment from the notification.
  8. Notice that the comment is posted properly.

Steps 2-5 can be repeated a few times to check that comment actions are present in the notification every time.

Test.3 Post empty comment

The failsafe logic that started comments activity on an empty reply from inside a notification service is removed. Below are a few tests to check that one cannot post an empty comment from the app or from the website.

Test.3.1 Reply from notification
  1. Repeat steps 1-6 from Test.2.
  2. Notice that the send button is disabled until a character is typed into the comment reply box.
Test.3.2 Reply from website
  1. Try to post a comment reply using Calypso.
  2. Notice that the send button is disabled until a character is typed into the comment reply box.
Test.3.2 Reply from Comments section in the app
  1. Try to post a comment reply using the app (from My Site -> Comments -> Comments Details).
  2. Notice that an empty reply cannot be sent.

Test.4 Check other services and broadcast receivers for start activity logic

I checked that we're not starting an activity from inside any other services(1, 2) and broadcast receivers (1, 2) by checking the code itself. I'll appreciate another pair of eyes that confirms it. 🙏

Regression Notes

  1. Potential unintended areas of impact
    Changes are limited to
  • Post published share action from the notification
  • Direct reply from the notification

I've tried to include tests in the To test section to test these areas and can't think of potential unintended areas at this point.

  1. What I did to test those areas of impact (or what existing automated tests I relied on) - N/A
    See To test section

  2. What automated tests I added (or what prevented me from doing so) - N/A
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ashiagr
Start the share action directly using a pending intent instead of fro…
235231b 
…m a broadcast receiver.

Share action from post published notification was started from a broadcast receiver that acted as a notification trampoline. It is now directly started using a pending intent without having to depend on a broadcast receiver.

For more details see: https://developer.android.com/about/versions/12/behavior-changes-12#notification-trampolines
@ashiagr ashiagr added this to the Future milestone Mar 11, 2022
@ashiagr ashiagr requested a review from ParaskP7 March 11, 2022 12:22
@ashiagr ashiagr self-assigned this Mar 11, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 11, 2022 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 11, 2022 •
edited
Loading

You can test the changes on this Pull Request by downloading the APKs:

@ashiagr ashiagr removed the request for review from ParaskP7 March 11, 2022 13:44
@ashiagr ashiagr marked this pull request as draft March 11, 2022 13:51
ashiagr added 6 commits May 31, 2022 15:59
@ashiagr
Merge branch 'trunk' into issue/15875-remove-notif-trampoline
b2d3129 
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java
@ashiagr
Remove suppress notification trampoline after merge
97c2896 
Notification trampoline in this method is addressed in 235231b.
@ashiagr
Refactor: Extract show notification logic for note data from buildAnd…
2d9bbcd 
…ShowNotificationFromNoteData(...)

This new method will be needed in the next commit that fixes a bug.
@ashiagr
Bug Fix: Show notification for note after full note is downloaded
579372b 
This fixes a bug where inline actions were not added to notification because the notification was sometimes shown before full note with actions info was downloaded.
@ashiagr
Return with no action if reply text is empty
2db34f6 
Opening comments screen when there's an empty message looks like a fail-safe. We cannot reply with an empty message either from the app or from the website as send button is disabled till we enter a text. Even in the case of voice reply from a wearable, an empty voice reply doesn't make sense.

User can tap the notification itself to go straight to the comments screen in case there's ever an empty reply and so this logic to open comments activity from service on empty reply is removed.
@ashiagr
Remove BROADCAST_CLOSE_SYSTEM_DIALOGS permission
0a43a35 
This was added in PR#15602 to fix lint issues complaining of a missing permission as the app used Intent.ACTION_CLOSE_SYSTEM_DIALOGS at two places: NativeNotificationsUtils, ShareAndDismissNotificationReceiver.

As we've removed Intent.ACTION_CLOSE_SYSTEM_DIALOGS in both the classes, we can remove this permission now.
@ashiagr ashiagr marked this pull request as ready for review June 1, 2022 13:13
@ashiagr ashiagr requested a review from AjeshRPai June 1, 2022 13:14
AjeshRPai
AjeshRPai reviewed Jun 2, 2022
View reviewed changes
Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

@ashiagr I have tested the changes as described.

  • I checked that we're not starting an activity from inside any other services by checking the code itself 🟢
  • I have tested the notification reply and share option. Its working as expected. 🟢

However I came across two issues which I couldn't find the root cause. I am not sure whether the changes in this PR is the root cause of the issue

Issue 1

  • Go to MySite-> Comments
  • Select a comment("Test Comment") to reply to from list
  • Type reply -> Click send
  • Notice that the reply is send but the edit text is still filled with the comment
  • Go back and come back to reply to the original comment("Test Comment")
  • Notice that the reply is still filled
Screen_Recording_20220602-123327_WordPress.Pre-Alpha.mp4
Screen_Recording_20220602-133140_WordPress.Pre-Alpha.mp4

First video capture is from the build from this PR
Second Video capture Time stamp 0:00 - 0:30 - This is the behaviour of the comment reply in the current Playstore app.

Issue 2

When the user logs out from the app, the comment notification doesnt go away.

Screen_Recording_20220602-121643_WordPress.Pre-Alpha.mp4

Let me know if these issues are not related to this PR, I will approve the PR and we can take it up as a seperate task.

@AjeshRPai AjeshRPai self-requested a review June 2, 2022 11:06
AjeshRPai
AjeshRPai approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

@ashiagr

Issue 1

I am not creating an issue to track this bug as I couldn't reproduce this issue.

Issue 2

I have created an issue to track this bug here.

As both of the Issues are not due to the changes from this PR. Am approving the PR. 👍🏼

@AjeshRPai AjeshRPai merged commit adc446b into trunk Jun 2, 2022
@AjeshRPai AjeshRPai deleted the issue/15875-remove-notif-trampoline branch June 2, 2022 11:18
@ashiagr ashiagr modified the milestones: Future, 20.1 Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@AjeshRPai AjeshRPai AjeshRPai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ashiagr ashiagr

Labels

None yet

Projects

None yet

Milestone

20.1

Development

Successfully merging this pull request may close these issues.

Android 12: Security and privacy - Apps can't close system dialogs Android 12: Performance - Notification trampoline restrictions

3 participants

@ashiagr @AjeshRPai