This repository has been archived by the owner on Sep 4, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Issue 2585 - Alllowing to clear notification when the app is not running #2619
Open
wolfenrain
wants to merge
2
commits into
phonegap:master
Choose a base branch
from
Mediapioniers:issue-2585
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The way this is right now, and considering the example you used on the OP, I can still use the
clearNotification
attribute on the payload to clear said id if the application is in foreground, however the scenario on this line won't trigger and it'll fall through to the lastelse {
, which will consider foreground:false even though we're in foreground, therefore creating a very inconsistent payload for the js to handle.So, we have a few options:
clearNotification
attribute represents a separate and independent feature on the payload, and a single payload can be {an actual notification with all the other payload attributes available} and {a request to clear a specific notification} at the same time. This would need the firstif
statement here to be isolated as to not race against the existing ones. Check theclearBadge
feature, as that's the closest we have to this option.clearNotification
attribute represents the whole feature, and whenever it is present it will skip all other cases (if/else
statements). This would need the firstif
statement to actually return from the method, or make the lastelse
statement do nothing if it did clear a notification before. This would also make the example used on the OP invalid, as thetitle
andbody
attributes on the payload mean nothing. This would also make it useful to move this feature to as early as possible on the method to avoid useless work (if we're gonna clear a notification and return, why waste resources preping the extras?) This would also make it needed to solve the question "who has the highest priority on the feature order:clearBadge
orclearNotification
?".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.
Will take a look at it tonight or tomorrow! Thanks for taking the time! Will also update the docs :-)
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.
I would consider going for the first option, I would consider that the most likely use-case for people. And that way you don't force people. Will change and test it :-)