Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

V1.x Android: handle dismissed notifications #1816

Merged
merged 3 commits into from
Jun 30, 2017
Merged

Conversation

BenWildeman
Copy link
Contributor

Description

When a user dismisses notifications, you would expect them to go away. Currently when using Inbox style, previous notifications will be included, even though the user has dismissed the notification. This fix handles dismissed notifications, and clears the internal list.

Related Issue

#1665

Motivation and Context

With the related issue, originally the dismissal of the notification would result in the app opening which is obviously the incorrect way to handle this, but the "fix" applied was just to remove the deleteIntent altogether, which is also the incorrect way to handle this.

How Has This Been Tested?

I sent a bunch of inbox style notifications with the same notId, dismissed the notification, then resent a bunch more. Checked to see if the previous dismissed notifications were gone. They were.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@BenWildeman BenWildeman changed the title V1.x handle dismissed notifications V1.x Android: handle dismissed notifications Jun 30, 2017
@fredgalvao
Copy link
Collaborator

Doesn't this go against some common cases of conversation/threaded applications? Such as:

  • Maria sends you "Hey"
  • Maria sends you "What's up"
  • You dismiss the single inbox-style notification that had those two messages
  • Maria sends you "Call me back"
  • ?

At this point, most applications would bring back the 2 you dismissed (as they actually weren't processed, just dismissed/delayed), agregate the third one, and present the user a new inbox-style notification with all 3 of them.

The interpretation of what dismissing a notification means is subjective, I know, and multiple scenarios would argue to both sides, and thus I'd say we have to at least let the user decide instead of assume.

@fredgalvao
Copy link
Collaborator

Also, we might need to bring whatever is decided here to the master branch too.

@macdonst
Copy link
Member

@fredgalvao this PR deals with Inbox style notifications not Android N groupings. When you dismiss an inbox style notification all of the notifications should be dismissed from the plugin as they are only contained in one notification. In Andoid N they can be expanded to have multiple lines in the notification center.

This also fixes the setDeleteIntent problem. This is a good basis for either fire the notification event with dismissed = true or adding a new dismissed event.

@macdonst macdonst merged commit d3ad767 into phonegap:v1.x Jun 30, 2017
@fredgalvao
Copy link
Collaborator

I should've read the related issue at full before commenting. Nevermind my daydreams.

@BenWildeman
Copy link
Contributor Author

Would you like me to do a v2.x pull request also for this?

@macdonst
Copy link
Member

@NiTrOGhost nope, I was doing a cherry-pick and my computer crashed. Should be committed soon.

@BenWildeman
Copy link
Contributor Author

that's great 😁

@macdonst
Copy link
Member

@NiTrOGhost harsh! You are happy my computer crashed 😉

macdonst pushed a commit that referenced this pull request Jun 30, 2017
* handle dismissed notifications

* added missing const

* 🐧 🐛 obligatory commit message
@macdonst
Copy link
Member

@NiTrOGhost merged into v2.x

@lock
Copy link

lock bot commented Jun 3, 2018

This thread has been automatically locked.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants