Page MenuHomePhabricator

Construct local badge-only notif when notification doesn't have body property
ClosedPublic

Authored by marcin on Thu, Apr 18, 3:51 AM.
Tags
None
Referenced Files
F1690458: D11684.id39339.diff
Wed, May 1, 9:42 PM
Unknown Object (File)
Sat, Apr 27, 5:54 PM
Unknown Object (File)
Sat, Apr 27, 12:42 PM
Unknown Object (File)
Fri, Apr 26, 6:11 PM
Unknown Object (File)
Fri, Apr 26, 2:11 PM
Unknown Object (File)
Thu, Apr 25, 6:13 PM
Unknown Object (File)
Thu, Apr 25, 6:09 PM
Unknown Object (File)
Wed, Apr 24, 2:22 PM
Subscribers
None

Details

Summary
Test Plan
  1. Build iOS app.
  2. Make a thread badge only.
  3. Sent notif to this thread.
  4. Ensure that badge is updated and no "ENCRYPTED" notifs are displayed.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

An alternative solution to this problem is to set badgeOnly property on a notif payload. However I am of opinion that if we can solve something without putting additional data to notification payload then it should be a preferred solution. However if reviewers disagree I can proceed with this solution.

ashoat requested changes to this revision.Thu, Apr 18, 5:03 AM

I think the changes are good here, but I feel like the code comments can be improved, and I'd like to review the updated code comments before this is landed

native/ios/NotificationService/NotificationService.mm
257–258 ↗(On Diff #39213)

You're deleting a comment elsewhere that explains why we use a separate notif for the badge count instead of just modifying the remote notif we received. Can you add that explanation here?

563 ↗(On Diff #39213)

Is this function useful anymore? I think we can get rid of it and make the check inline

566–569 ↗(On Diff #39213)

This comment was extremely helpful for debugging this exact issue. Can you find a way to keep it, and in particular the part explaining that we set BODY to ENCRYPTED for internal builds, and that we need to filter the notif to avoid displaying that to the user?

645 ↗(On Diff #39213)

Why would a local badge-only notif need to be created, as opposed to modifying the remote notif we receive?

This revision now requires changes to proceed.Thu, Apr 18, 5:03 AM
native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

Sorry for the confusion. This comment is inaccurate. For notifs that don't have body in payload (rescinds and badgeonly) we costruct new notif locally in this code but we pass it to contentHandler so it is not a local notif but a remote notif. I will delete this comment.

  1. Remove needsSilentBadgeUpdate function and replace it with inline checks.
  2. Bring back useful comment.
ashoat added inline comments.
native/ios/NotificationService/NotificationService.mm
260–264 ↗(On Diff #39271)

Can we say "the server sets" instead of "we set" here?

266–268 ↗(On Diff #39271)

Why aren't we using getBadgeOnlyContentFor here?

This revision is now accepted and ready to land.Fri, Apr 19, 6:36 AM
native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

I'm still confused here, to be honest. If we construct the new notif locally, how is it a remote notif and not a local notif?

native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

Even though it is an object instantiated in the NSE code we pass it to contentHandler given to the NSE byt the OS so the OS treats it as a remote notif.

native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

Okay, I think I was using the terms wrong. Let me ask my question again.

Why would a new remote badge-only notif need to be created, as opposed to modifying the remote notif we receive?

native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

Initially badge only notif/rescind has body set to "ENCRYPTED". I recall that when I was introducing this code I tried first to modify body of received object directly and I found that:

  • setting body to empty string results in empty notif
  • setting body to nil results in bad memory access crash

However creating new notif object without touching its body property worked as desired. Signal does it too: https://github.com/signalapp/Signal-iOS/blob/main/SignalNSE/NotificationService.swift#L80-L83.

I guess that Apple initially sets this field to some flag that prevents system from displaying any visual content.

  1. Update the comment
  2. Use method to get badge only content for notif.
native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

setting body to empty string results in empty notif

Isn't that what we want, though? An empty notif that only updates badge count?

native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

I meant visible empty notif bubble.

native/ios/NotificationService/NotificationService.mm
645 ↗(On Diff #39213)

Okay, got it. It took a lot of clarification but I think I understand now. Next time hopefully there can be less confusion if we use less ambiguous language