-
Notifications
You must be signed in to change notification settings - Fork 155
Add a new APNs configuration option push_with_badge to suppress sending aps.badge in APNs messages.
#358
base: main
Are you sure you want to change the base?
Conversation
| # # | ||
| # # Specifies whether to send the badge key in the APNs message. | ||
| # # Defaults to True, set this to False to suppress sending the badge count. | ||
| # #push_with_badge: false |
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.
| # # | |
| # # Specifies whether to send the badge key in the APNs message. | |
| # # Defaults to True, set this to False to suppress sending the badge count. | |
| # #push_with_badge: false | |
| # # | |
| # # Specifies whether to update the notification count badge on the app upon sending | |
| # # an APNs message (if notification counts can be calculated). Specifically, | |
| # # whether or not the `aps.badge` field is set. | |
| # # | |
| # # If this is False, additional `unread_count` and `missed_calls` fields will be added to | |
| # # the APNS message. | |
| # # | |
| # # Defaults to True, set this to False to suppress sending the badge count. | |
| # #push_with_badge: false |
Some slight wording adjustments. You may also want to explain why this could be desirable when "not using mutable-content"?
Though after writing this out, I feel like we're conflating two different behaviours in a single config option:
- Suppression of
aps.badgefield. - Inclusion of additional
unread_countandmissed_callsfields.
Could you say why you want the unread_count and missed_calls fields? Or even what the overall goal for this PR is with respect to your implementation?
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 agree this comes off as a bit confusing.
The primary focus of this commit to suppress the aps.badge field so iOS does not automatically draw the badge on the home screen. In the event an iOS app is receiving APNS from multiple independent sources, the badge value is incorrect. Whether the other source is sending its own aps.badge or not, sygnal sending its own aps.badge still conflicts with the true badge value expected by the user.
When suppressing that field, I wanted to ensure the underlying data was still available to the iOS client, so it could be used, if desired. The inclusion of the unread_count and missed_calls fields were to represent the expanded form of what aps.badge represents here .
I agree the inclusion of unread_count and missed_calls could become their own setting, but I was unsure about APNS message length assumptions.
- If length assumptions are not a concern, it seems like always including
unread_countandmissed_callsin APNS would be reasonable (I can submit a separate PR). Then this PR would simply be about toggling the inclusion ofaps.badge. - If length assumptions are a concern, I'm open to opinions of how to move forward. We can drop the extra fields entirely, or add multiple settings controlling the inclusion of fields. But neither of these options feel very elegant.
| payload["aps"].setdefault("alert", {})["loc-args"] = loc_args | ||
|
|
||
| if badge is not None: | ||
| if self.get_config("push_with_badge", bool, True) and badge is not None: |
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.
Rather than pulling this option value out of the config on each request, please do so in __init__ and set to a class variable, then reference that class variable instead. i.e.:
def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]) -> None:
# ...
self.push_with_badge = self.get_config("push_with_badge", bool, True)def _get_payload_event_id_only(
self, n: Notification, default_payload: Dict[str, Any]
) -> Dict[str, Any]:
# ...
if self.push_with_badge and badge is not None: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 be in the next commit
…ding `aps.badge` in APNs messages. Signed-off-by: Chris Ennis <cennis91@gmail.com>
608dee5 to
12c0b05
Compare
For situations when not using
mutable-contentand updating the badge count is not desired.This also adds
unread_countandmissed_callsto the push payload whenpush_with_badgeisFalse.