element-hq / element-x-ios

Next generation Matrix client for iOS built with SwiftUI on top of matrix-rust-sdk.
https://element.io/labs/element-x
GNU Affero General Public License v3.0
409 stars 95 forks source link

Fix bridged notifications displaying the username twice #2479

Open swiftbird07 opened 7 months ago

swiftbird07 commented 7 months ago

Is your feature request related to a problem? Please describe. When using e.g. mautrix-whatsapp bridge to bridge conversations from WhatsApp to Matrix, the "@whatsappbot" is always in the conversation of me and the other person, even on 1:1 chats. This results in Elements notifications of bridged messages to look like this:

Mark (WA) Mark (WA) Hey there!

because Element thinks this is an actual n:n room where it needs to display the room and user together with the new message notification; however since the message is actual a 1:1 chat between just Mark and me and the WhatsApp-bridge names the room like the person you are chatting with it results in the above not so pretty notification.

Describe the solution you'd like I think the easiest solution to this would be to just add a line before sending the notification event that checks if the "room name" is the same as the "user name" and if so omit the room name from the notification.

Describe alternatives you've considered I mean the bridge author could somehow try to not make the bridge bot join bridged conversations, but I don't think this is technical possible.

swiftbird07 commented 7 months ago

I looked at the code and (with my admittedly limited knowledge of iOS app development) I came up with a change that should in theory fix the issue.

Change line 49 in the computed property "isDM" of the NotificationItemProxyProtocol extension: https://github.com/element-hq/element-x-ios/blob/382b9a69c303f7ef6692a64897d9cffa338c7e13/ElementX/Sources/Services/Notification/Proxy/NotificationItemProxyProtocol.swift#L47-L50

To:

 extension NotificationItemProxyProtocol { 
     var isDM: Bool { 
                return (isRoomDirect && roomJoinedMembers <= 2) || (roomDisplayName == senderDisplayName)
     } 

This should then return true if room and sender have the same display name so that the notification is treated like a DM.

Velin92 commented 7 months ago

I looked at the code and (with my admittedly limited knowledge of iOS app development) I came up with a change that should in theory fix the issue.

Change line 49 in the computed property "isDM" of the NotificationItemProxyProtocol extension:

https://github.com/element-hq/element-x-ios/blob/382b9a69c303f7ef6692a64897d9cffa338c7e13/ElementX/Sources/Services/Notification/Proxy/NotificationItemProxyProtocol.swift#L47-L50

To:

 extension NotificationItemProxyProtocol { 
     var isDM: Bool { 
                return (isRoomDirect && roomJoinedMembers <= 2) || (roomDisplayName == senderDisplayName)
     } 

This should then return true if room and sender have the same display name so that the notification is treated like a DM.

Sadly we can't accept this code/solution, since this would mean that a user changing the name of a group to reflect a name of the member would mess up how the notification is displayed, we need to determine the DM condition based on valid data that can't be directly tampered. Comparing the sender the display would not be a valid source of truth.

swiftbird07 commented 7 months ago

Having a source of truth is tricky as there is not really an indication that tells the client that this is 100% a 1:1 conversation.

On the other hand it would be possible to check if the 3rd person in the "group" of 3 persons is called "@.+bot@matrix.server" or if that is also too generic than also check if the display name of that "user" contains the string "bridge bot".

Together with the other checks (roomDisplayName == senderDisplayName) this would reduce the implied false positive rate to about 0% (as no sane person would name a group as a contact name PLUS have exactly 3 members in that group PLUS one of the members happen to have "bridge bot" in the name.

If this doesn't work the only other solution would be to implement a client setting (default off) that an user can enable like "Enable bridged conversation detection" which would then enable my first proposed check.