Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add communication Notification context to message notifications #4819

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add communication Notification context to message notifications #4819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c32047e
Add communication Notification context to message notifications
kloenk Sep 10, 2021
8c62814
github ci: change to Xcode 13 beta
kloenk Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ env:
jobs:
build:
name: Build
runs-on: macos-latest
runs-on: macos-11

steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ env:
jobs:
tests:
name: Tests
runs-on: macos-latest
runs-on: macos-11

steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release-alpha.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ env:
jobs:
build:
name: Release
runs-on: macos-latest
runs-on: macos-11

steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 2 additions & 0 deletions Riot/SupportingFiles/Riot.entitlements
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
</array>
<key>com.apple.developer.ubiquity-kvstore-identifier</key>
<string>$(TeamIdentifierPrefix)$(CFBundleIdentifier)</string>
<key>com.apple.developer.usernotifications.communication</key>
<true/>
<key>com.apple.security.application-groups</key>
<array>
<string>$(APPLICATION_GROUP_IDENTIFIER)</string>
Expand Down
11 changes: 11 additions & 0 deletions RiotNSE/Info.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@
<string>$(CURRENT_PROJECT_VERSION)</string>
<key>NSExtension</key>
<dict>
<key>NSExtensionAttributes</key>
<dict>
<key>IntentsRestrictedWhileLocked</key>
<array/>
<key>IntentsSupported</key>
<array>
<string>INStartAudioCallIntent</string>
<string>INStartVideoCallIntent</string>
<string>INSendMessageIntent</string>
</array>
</dict>
<key>NSExtensionPointIdentifier</key>
<string>com.apple.usernotifications.service</string>
<key>NSExtensionPrincipalClass</key>
Expand Down
199 changes: 156 additions & 43 deletions RiotNSE/NotificationService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import UserNotifications
import MatrixKit
import MatrixSDK
import Intents

/// The number of milliseconds in one second.
private let MSEC_PER_SEC: TimeInterval = 1000
Expand Down Expand Up @@ -225,31 +226,30 @@ class NotificationService: UNNotificationServiceExtension {
self.notificationContent(forEvent: event, forAccount: userAccount) { (notificationContent) in
var isUnwantedNotification = false

// Modify the notification content here...
if let newContent = notificationContent {
content.title = newContent.title
content.subtitle = newContent.subtitle
content.body = newContent.body
content.threadIdentifier = newContent.threadIdentifier
content.categoryIdentifier = newContent.categoryIdentifier
content.userInfo = newContent.userInfo
content.sound = newContent.sound
} else {
// this is an unwanted notification, mark as to be deleted when app is foregrounded again OR a new push came
if notificationContent == nil {
content.categoryIdentifier = Constants.toBeRemovedNotificationCategoryIdentifier
isUnwantedNotification = true
}

if self.ongoingVoIPPushRequests[event.eventId] == true {
// modify the best attempt content, to be able to use in the future
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the internal type does change. And apple says don't touch it.

} else {
self.bestAttemptContents[event.eventId] = content
}

// There is an ongoing VoIP Push request for this event, wait for it to be completed.
// When it completes, it'll continue with the bestAttemptContent.
return
} else {
MXLog.debug("[NotificationService] processEvent: Calling content handler for: \(String(describing: event.eventId)), isUnwanted: \(isUnwantedNotification)")
self.contentHandlers[event.eventId]?(content)
if let notificationContent = notificationContent {
self.contentHandlers[event.eventId]?(notificationContent)
} else {
self.contentHandlers[event.eventId]?(content)
}
// clear maps
self.contentHandlers.removeValue(forKey: event.eventId)
self.bestAttemptContents.removeValue(forKey: event.eventId)
Expand Down Expand Up @@ -296,6 +296,7 @@ class NotificationService: UNNotificationServiceExtension {
switch response {
case .success(let (roomState, eventSenderName)):
var notificationTitle: String?
var notificationSubTitle: String?
var notificationBody: String?
var additionalUserInfo: [AnyHashable: Any]?

Expand Down Expand Up @@ -361,9 +362,12 @@ class NotificationService: UNNotificationServiceExtension {
let isReply = event.isReply()

if isReply {
notificationTitle = self.replyTitle(for: eventSenderName, in: roomDisplayName)
notificationTitle = self.replyTitle(for: eventSenderName)
} else {
notificationTitle = self.messageTitle(for: eventSenderName, in: roomDisplayName)
notificationTitle = eventSenderName
}
if !(roomSummary?.isDirect ?? false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, just had a notification where this check failed (build from the AppStore, did not show the room name, but only the sender name)

notificationSubTitle = roomDisplayName
}

if event.isEncrypted && !self.showDecryptedContentInNotifications {
Expand Down Expand Up @@ -402,7 +406,10 @@ class NotificationService: UNNotificationServiceExtension {
// If the current user is already joined, display updated displayname/avatar events.
// This is an unexpected path, but has been seen in some circumstances.
if NotificationService.backgroundSyncService.roomSummary(forRoomId: roomId)?.membership == .join {
notificationTitle = self.messageTitle(for: eventSenderName, in: roomDisplayName)
notificationTitle = eventSenderName
if !(roomSummary?.isDirect ?? false) {
notificationSubTitle = roomDisplayName
}

// If the sender's membership is join and hasn't changed.
if let newContent = MXRoomMemberEventContent(fromJSON: event.content),
Expand Down Expand Up @@ -441,12 +448,18 @@ class NotificationService: UNNotificationServiceExtension {
}

case .sticker:
notificationTitle = self.messageTitle(for: eventSenderName, in: roomDisplayName)
notificationTitle = eventSenderName
if !(roomSummary?.isDirect ?? false) {
notificationSubTitle = roomDisplayName
}
notificationBody = NSString.localizedUserNotificationString(forKey: "STICKER_FROM_USER", arguments: [eventSenderName as Any])

// Reactions are unexpected notification types, but have been seen in some circumstances.
case .reaction:
notificationTitle = self.messageTitle(for: eventSenderName, in: roomDisplayName)
notificationTitle = eventSenderName
if !(roomSummary?.isDirect ?? false) {
notificationSubTitle = roomDisplayName
}
if let reactionKey = event.relatesTo?.key {
// Try to show the reaction key in the notification.
notificationBody = NSString.localizedUserNotificationString(forKey: "REACTION_FROM_USER", arguments: [eventSenderName, reactionKey])
Expand Down Expand Up @@ -479,52 +492,48 @@ class NotificationService: UNNotificationServiceExtension {
MXLog.debug("[NotificationService] notificationContentForEvent: Resetting title and body because app protection is set")
notificationBody = NSString.localizedUserNotificationString(forKey: "MESSAGE_PROTECTED", arguments: [])
notificationTitle = nil
notificationSubTitle = nil
}

guard notificationBody != nil else {
MXLog.debug("[NotificationService] notificationContentForEvent: notificationBody is nil")
onComplete(nil)
return
}

let notificationContent = self.notificationContent(withTitle: notificationTitle,
withSubTitle: notificationSubTitle,
body: notificationBody,
threadIdentifier: threadIdentifier,
userId: currentUserId,
event: event,
pushRule: pushRule,
additionalInfo: additionalUserInfo)

MXLog.debug("[NotificationService] notificationContentForEvent: Calling onComplete.")
onComplete(notificationContent)

if #available(iOS 15.0, *) {
self.makeCommunicationNotification(
forEvent: event,
// TODO: use real room/user avatar
senderImage: INImage.systemImageNamed("person.circle.fill"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was nil would it give the full sized app icon until the real avatar is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I still did not figure out what one should do there. So if nil or this is the better approach

Copy link
Member

@pixlwave pixlwave Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with nil for now as it will look familiar. Having a template image may give the impression that something has broken.

body: notificationBody,
notificationContent: notificationContent,
roomDisplayName: roomDisplayName,
senderName: eventSenderName,
roomSummary: roomSummary,
onComplete: onComplete)
} else {
MXLog.debug("[NotificationService] notificationContentForEvent: Calling onComplete.")
onComplete(notificationContent)
}
case .failure(let error):
MXLog.debug("[NotificationService] notificationContentForEvent: error: \(error)")
onComplete(nil)
}
})
}

/// Returns the default title for message notifications.
/// - Parameters:
/// - eventSenderName: The displayname of the sender.
/// - roomDisplayName: The displayname of the room the message was sent in.
/// - Returns: A string to be used for the notification's title.
private func messageTitle(for eventSenderName: String, in roomDisplayName: String?) -> String {
// Display the room name only if it is different than the sender name
if let roomDisplayName = roomDisplayName, roomDisplayName != eventSenderName {
return NSString.localizedUserNotificationString(forKey: "MSG_FROM_USER_IN_ROOM_TITLE", arguments: [eventSenderName, roomDisplayName])
} else {
return eventSenderName
}
}

private func replyTitle(for eventSenderName: String, in roomDisplayName: String?) -> String {
// Display the room name only if it is different than the sender name
if let roomDisplayName = roomDisplayName, roomDisplayName != eventSenderName {
return NSString.localizedUserNotificationString(forKey: "REPLY_FROM_USER_IN_ROOM_TITLE", arguments: [eventSenderName, roomDisplayName])
} else {
return NSString.localizedUserNotificationString(forKey: "REPLY_FROM_USER_TITLE", arguments: [eventSenderName])
}
private func replyTitle(for eventSenderName: String) -> String {
return NSString.localizedUserNotificationString(forKey: "REPLY_FROM_USER_TITLE", arguments: [eventSenderName])
}

/// Get the context of an event.
Expand Down Expand Up @@ -570,6 +579,7 @@ class NotificationService: UNNotificationServiceExtension {
}

private func notificationContent(withTitle title: String?,
withSubTitle subTitle: String?,
body: String?,
threadIdentifier: String?,
userId: String?,
Expand All @@ -581,6 +591,9 @@ class NotificationService: UNNotificationServiceExtension {
if let title = title {
notificationContent.title = title
}
if let subTitle = subTitle {
notificationContent.subtitle = subTitle
Copy link
Member

@pixlwave pixlwave Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to make the various uses of subtitle all lower case, so that they match the UNNotificationContent property?

Suggested change
notificationContent.subtitle = subTitle
notificationContent.subtitle = subtitle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think I meant to do it, but coding to fast :-)

}
if let body = body {
notificationContent.body = body
}
Expand All @@ -600,6 +613,106 @@ class NotificationService: UNNotificationServiceExtension {
return notificationContent
}

@available(iOS 15, *)
private func makeCommunicationNotification(forEvent event: MXEvent,
senderImage: INImage?,
body notificationBody: String?,
notificationContent: UNNotificationContent,
roomDisplayName: String?,
senderName: String,
roomSummary: MXRoomSummary?,
onComplete: @escaping (UNNotificationContent?) -> Void) {
switch event.eventType {
case .roomMessage:
let intent = self.getMessageIntent(
forEvent: event,
body: notificationBody ?? "",
groupName: roomSummary?.isDirect ?? true ? nil : roomDisplayName,
senderName: senderName,
senderImage: senderImage)
intent.setImage(senderImage, forParameterNamed: \.sender)
do {
// TODO: figure out how to set _mentionsCurrentUser and _replyToCurrentUser in the context
let notificationContent = try notificationContent.updating(from: intent)
MXLog.debug("[NotificationService] makeCommunicationNotification: Calling onComplete.")
onComplete(notificationContent)
} catch {
MXLog.debug("[NotificationService] makeCommunicationNotification: error (roomMessage): \(error)")
onComplete(notificationContent)
}
default:
onComplete(notificationContent)
}
}


@available(iOS 15, *)
private func getMessageIntent(forEvent event: MXEvent,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private func getMessageIntent(forEvent event: MXEvent,
private func messageIntent(forEvent event: MXEvent,

body: String,
groupName: String?,
senderName: String,
senderImage: INImage?) -> INSendMessageIntent {
let intentType = getMessageIntentType(event.content["msgtype"] as? String, event)

let speakableGroupName = groupName.flatMap({ INSpeakableString(spokenPhrase: $0 )})

let sender = INPerson(
personHandle: INPersonHandle(value: event.sender, type: .unknown),
nameComponents: nil,
displayName: senderName,
image: senderImage,
contactIdentifier: nil,
customIdentifier: event.sender,
isMe: false,
suggestionType: .instantMessageAddress
)

let incomingMessageIntent = INSendMessageIntent(
// TODO: check
recipients: [],
outgoingMessageType: intentType,
content: body,
speakableGroupName: speakableGroupName,
conversationIdentifier: event.roomId,
// TODO: check
serviceName: nil,
sender: sender,
attachments: nil
)

let intent = INInteraction(intent: incomingMessageIntent, response: nil)
intent.direction = .incoming

intent.donate(completion: nil)

return incomingMessageIntent
}

@available(iOS 14, *)
private func getMessageIntentType(_ msgType: String?, _ event: MXEvent) -> INOutgoingMessageType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private func getMessageIntentType(_ msgType: String?, _ event: MXEvent) -> INOutgoingMessageType {
private func messageIntentType(for msgType: String?, _ event: MXEvent) -> INOutgoingMessageType {

guard let msgType = msgType else {
return .unknown
}

switch msgType {
case kMXMessageTypeEmote:
fallthrough
case kMXMessageTypeImage:
fallthrough
case kMXMessageTypeVideo:
fallthrough
case kMXMessageTypeFile:
return .unknown
case kMXMessageTypeAudio:
if event.isVoiceMessage() {
return .outgoingMessageAudio
}
return .unknown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for .outgoingMessageAudio say "An audio recording". Could this cover all audio messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, will change it

default:
return .outgoingMessageText
}
}

private func notificationUserInfo(forEvent event: MXEvent,
andUserId userId: String?,
additionalInfo: [AnyHashable: Any]? = nil) -> [AnyHashable: Any] {
Expand Down
2 changes: 1 addition & 1 deletion fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ platform :ios do

before_all do
# Ensure used Xcode version
xcversion(version: "~> 12.1")
xcversion(version: "~> 13.0")
end

#### Public ####
Expand Down