Add communication Notification context to message notifications #4819
Conversation
|
Related issue: #4710 I'll have a look through this next week, although given that the CI very much needs Xcode 13, no promises on how long it would be until we could merge it. |
kloenk
force-pushed
the
communication
branch
from
f773f88
to
cb35ba0
Compare
Signed-off-by: Finn Behrens <[email protected]>
kloenk
force-pushed
the
communication
branch
2 times, most recently
from
c1f42b8
to
67399c5
Compare
Signed-off-by: Finn Behrens <[email protected]>
kloenk
force-pushed
the
communication
branch
from
67399c5
to
8c62814
Compare
There was a problem hiding this comment.
Hey Finn, thanks very much for this contribution. 🤩
I've had a look through and added a few comments (not yet set up to test it out in action). It may be a while until we're ready to adopt Xcode 13 for our releases so merging won't be possible until then, but I'd be very happy to iterate on this with you in the mean time 🙂
One comment not added inline is that it would be desirable for this update to not change the behaviour of notifications for iOS 14 and lower, e.g. keep the title to display "Sender in Room"
| @@ -581,6 +591,9 @@ class NotificationService: UNNotificationServiceExtension { | |||
| if let title = title { | |||
| notificationContent.title = title | |||
| } | |||
| if let subTitle = subTitle { | |||
| notificationContent.subtitle = subTitle | |||
There was a problem hiding this comment.
Would you be able to make the various uses of subtitle all lower case, so that they match the UNNotificationContent property?
| notificationContent.subtitle = subTitle | |
| notificationContent.subtitle = subtitle |
There was a problem hiding this comment.
Yes, I think I meant to do it, but coding to fast :-)
| notificationTitle = self.messageTitle(for: eventSenderName, in: roomDisplayName) | ||
| notificationTitle = eventSenderName | ||
| } | ||
| if !(roomSummary?.isDirect ?? false) { |
There was a problem hiding this comment.
Could these tests match the old logic from the messageTitle/replyTitle methods and compare sender name to room name:
if let roomDisplayName = roomDisplayName, roomDisplayName != eventSenderName {
This would avoid duplicating the title into the subtitle for 1:1 rooms that aren't DMs.
There was a problem hiding this comment.
I think I rememberer I had one direct message room, which failed this test. But I can't find it right now, so yeah. Can use the old test.
There was a problem hiding this comment.
Ok, just had a notification where this check failed (build from the AppStore, did not show the room name, but only the sender name)
| self.bestAttemptContents[event.eventId] = content | ||
| if let notificationContent = notificationContent { | ||
| // TODO: this will most likely break the notification context maybe there is a better way | ||
| self.bestAttemptContents[event.eventId] = notificationContent.mutableCopy() as? UNMutableNotificationContent |
There was a problem hiding this comment.
Is it possible to leave it how it was before? I guess the removed block above doesn't copy the extra data filled from the Intent?
There was a problem hiding this comment.
Yes, the internal type does change. And apple says don't touch it.
| if event.isVoiceMessage() { | ||
| return .outgoingMessageAudio | ||
| } | ||
| return .unknown |
There was a problem hiding this comment.
The docs for .outgoingMessageAudio say "An audio recording". Could this cover all audio messages?
There was a problem hiding this comment.
Probably, will change it
| } | ||
|
|
||
| @available(iOS 14, *) | ||
| private func getMessageIntentType(_ msgType: String?, _ event: MXEvent) -> INOutgoingMessageType { |
There was a problem hiding this comment.
| private func getMessageIntentType(_ msgType: String?, _ event: MXEvent) -> INOutgoingMessageType { | |
| private func messageIntentType(for msgType: String?, _ event: MXEvent) -> INOutgoingMessageType { |
|
|
||
|
|
||
| @available(iOS 15, *) | ||
| private func getMessageIntent(forEvent event: MXEvent, |
There was a problem hiding this comment.
| private func getMessageIntent(forEvent event: MXEvent, | |
| private func messageIntent(forEvent event: MXEvent, |
| self.makeCommunicationNotification( | ||
| forEvent: event, | ||
| // TODO: use real room/user avatar | ||
| senderImage: INImage.systemImageNamed("person.circle.fill"), |
There was a problem hiding this comment.
If this was nil would it give the full sized app icon until the real avatar is available?
There was a problem hiding this comment.
Yes, I still did not figure out what one should do there. So if nil or this is the better approach
There was a problem hiding this comment.
I'd go with nil for now as it will look familiar. Having a template image may give the impression that something has broken.
Just wondering, as the intent is only created for message events, perhaps at the appropriate point around here maybe it would make sense to do a check for iOS 15 and update the content with an intent, otherwise carry on as before? |
I would like to add the call events rather soon, just did not get around to it yet. (I will try to add your comment's this weekend, uni just started for me 😄 ) |
Ah, yes that makes sense then. No rush, have fun! 🎓 |
|
|
Pull Request Checklist