Skip to content
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

Allow preventDefault to be fired up to two times #2138

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 28, 2024

Description

One Line Summary

Allow preventDefault to be fired up to two times with the discard parameter of false and then true, for the Received Event and the Display Event.

Details

  • The issue was that the event is passed to the developer's callback and the discard parameter is read when the callback returns. Then the logic continues on with that state.
  • So, a second call to preventDefault(true) had no effect.
  • Modify the notification display waiter to accept this second prevent default call.
  • This is necessary for wrappers which may need to call this twice, once to capture the event internally and once from the developer's intent.
  • preventDefault(false) can be called multiple times and has no effects due to no state change.

Motivation

To allow #2094 to work for wrappers.

Scope

Calling preventDefault twice where the first does not discard and the second discards.

Testing

Unit testing

  • Cleaned up NotificationGenerationProcessorTests helper methods
  • Added 2 tests

Manual testing

  1. Test with NotificationLifecycleListener for the will display event.
  2. Call preventDefault()
  3. Delay 3 seconds and call preventDefault(true)
  4. Delay 3 seconds and call display()
  5. See that the notification will not display.
  6. Do the same manual test with NotificationServiceExtension for the received event.

Also test there is no breaking of current behavior when preventDefault(discard: false) is called followed by display, will display the notification.

@Override
public void onWillDisplay(@NonNull INotificationWillDisplayEvent event) {
    Log.v(Tag.LOG_TAG, "APP onWillDisplay fired with event: " + event);

    IDisplayableNotification notification = event.getNotification();
    JSONObject data = notification.getAdditionalData();

    event.preventDefault();

    Runnable preventDefaultAgain = () -> {
        try {
            Thread.sleep(3000);
        } catch (InterruptedException ignored) { }
        Log.v(Tag.LOG_TAG, "Dev App INotificationLifecycleListener calling prevent default: discard = true" );
        event.preventDefault(true);
    };

    Runnable callDisplay = () -> {
        try {
            Thread.sleep(6000);
        } catch (InterruptedException ignored) { }
        Log.v(Tag.LOG_TAG, "Dev App INotificationLifecycleListener calling DISPLAY");
        notification.display();
    };

    Thread t1 = new Thread(preventDefaultAgain);
    t1.start();
    Thread t2 = new Thread(callDisplay);
    t2.start();
}

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

* The preventDefault(discard) method can now be called up to two times with false and then true.
* This is necessary for the `NotificationWillDisplayEvent` for wrappers which may need to call this twice, once to capture the event internally and once from the developer's intent.
* For consistency, this ability is added to the `NotificationReceivedEvent` as well.
* There is a lot of duplicated setup code, so let's extract common ones into a helper class
* There are no logic changes in this commit.
@nan-li nan-li changed the title [WIP] Allow preventDefault to be fired up to two times Allow preventDefault to be fired up to two times Jul 9, 2024
* The tests ensure calling preventDefault twice work when called in different threads
* And a followup call to display does not display the notification
@nan-li nan-li force-pushed the feat/call_prevent_default_multiple_times branch from 7d4391d to 9b7b91d Compare July 9, 2024 18:35
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.

Looks good to me to account for the wrapper use-case. Would be a nice to have if preventDefault(false) could be called any number of times, but that is pretty minor and shouldn't hold up getting this PR out there.

@nan-li
Copy link
Contributor Author

nan-li commented Jul 18, 2024

preventDefault(false) can be called multiple times, and basically no-op since there is no changes in state.
The waiter will wake once it is called with discard = true

@nan-li nan-li merged commit 7491122 into main Jul 18, 2024
2 checks passed
@nan-li nan-li deleted the feat/call_prevent_default_multiple_times branch July 18, 2024 18:21
@jinliu9508 jinliu9508 mentioned this pull request Jul 26, 2024
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.

3 participants