From 8d66572cc95d7e36a59b1919413b7081c243b31d Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Thu, 25 Apr 2024 14:53:18 +0300 Subject: [PATCH] Fixes #2706 - Use disambiguated display names for timeline item senders and state event bodies. (#2739) --- .../Style/TimelineItemBubbledStylerView.swift | 18 ++++-- .../Sources/Services/Client/ClientProxy.swift | 5 +- .../RoomSummary/RoomEventStringBuilder.swift | 31 +++++----- .../Services/Timeline/TimelineItemProxy.swift | 4 +- .../Timeline/TimelineItemSender.swift | 12 +++- .../RoomStateEventStringBuilder.swift | 60 +++++++++++-------- .../RoomTimelineItemFactory.swift | 4 +- 7 files changed, 84 insertions(+), 50 deletions(-) diff --git a/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineItemBubbledStylerView.swift b/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineItemBubbledStylerView.swift index b1845348d..c35129a17 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineItemBubbledStylerView.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineItemBubbledStylerView.swift @@ -78,11 +78,19 @@ struct TimelineItemBubbledStylerView: View { if shouldShowSenderDetails { HStack(alignment: .top, spacing: 4) { TimelineSenderAvatarView(timelineItem: timelineItem) - Text(timelineItem.sender.displayName ?? timelineItem.sender.id) - .font(.compound.bodySMSemibold) - .foregroundColor(.compound.decorativeColor(for: timelineItem.sender.id).text) - .lineLimit(1) - .scaledPadding(.vertical, 3) + HStack(alignment: .center, spacing: 4) { + Text(timelineItem.sender.displayName ?? timelineItem.sender.id) + .font(.compound.bodySMSemibold) + .foregroundColor(.compound.decorativeColor(for: timelineItem.sender.id).text) + + if timelineItem.sender.displayName != nil, timelineItem.sender.isDisplayNameAmbiguous { + Text(timelineItem.sender.id) + .font(.compound.bodyXS) + .foregroundColor(.compound.textSecondary) + } + } + .lineLimit(1) + .scaledPadding(.vertical, 3) } // sender info are read inside the `TimelineAccessibilityModifier` .accessibilityHidden(true) diff --git a/ElementX/Sources/Services/Client/ClientProxy.swift b/ElementX/Sources/Services/Client/ClientProxy.swift index 9a20c9dc0..c5c93263c 100644 --- a/ElementX/Sources/Services/Client/ClientProxy.swift +++ b/ElementX/Sources/Services/Client/ClientProxy.swift @@ -712,8 +712,9 @@ class ClientProxy: ClientProxyProtocol { let roomMessageEventStringBuilder = RoomMessageEventStringBuilder(attributedStringBuilder: AttributedStringBuilder(cacheKey: "roomList", mentionBuilder: PlainMentionBuilder())) - let eventStringBuilder = RoomEventStringBuilder(stateEventStringBuilder: RoomStateEventStringBuilder(userID: userID), - messageEventStringBuilder: roomMessageEventStringBuilder) + let eventStringBuilder = RoomEventStringBuilder(stateEventStringBuilder: RoomStateEventStringBuilder(userID: userID, shouldDisambiguateDisplayNames: false), + messageEventStringBuilder: roomMessageEventStringBuilder, + shouldDisambiguateDisplayNames: false) roomSummaryProvider = RoomSummaryProvider(roomListService: roomListService, eventStringBuilder: eventStringBuilder, diff --git a/ElementX/Sources/Services/Room/RoomSummary/RoomEventStringBuilder.swift b/ElementX/Sources/Services/Room/RoomSummary/RoomEventStringBuilder.swift index ebafd9bba..113ef433b 100644 --- a/ElementX/Sources/Services/Room/RoomSummary/RoomEventStringBuilder.swift +++ b/ElementX/Sources/Services/Room/RoomSummary/RoomEventStringBuilder.swift @@ -18,36 +18,35 @@ import Foundation import MatrixRustSDK struct RoomEventStringBuilder { - private let stateEventStringBuilder: RoomStateEventStringBuilder - private let messageEventStringBuilder: RoomMessageEventStringBuilder - - init(stateEventStringBuilder: RoomStateEventStringBuilder, messageEventStringBuilder: RoomMessageEventStringBuilder) { - self.stateEventStringBuilder = stateEventStringBuilder - self.messageEventStringBuilder = messageEventStringBuilder - } + let stateEventStringBuilder: RoomStateEventStringBuilder + let messageEventStringBuilder: RoomMessageEventStringBuilder + let shouldDisambiguateDisplayNames: Bool func buildAttributedString(for eventItemProxy: EventTimelineItemProxy) -> AttributedString? { let sender = eventItemProxy.sender let isOutgoing = eventItemProxy.isOwn - - let senderDisplayName = sender.displayName ?? sender.id + let displayName = if shouldDisambiguateDisplayNames { + sender.disambiguatedDisplayName ?? sender.id + } else { + sender.displayName ?? sender.id + } switch eventItemProxy.content.kind() { case .unableToDecrypt: - return prefix(L10n.commonDecryptionError, with: senderDisplayName) + return prefix(L10n.commonDecryptionError, with: displayName) case .redactedMessage: - return prefix(L10n.commonMessageRemoved, with: senderDisplayName) + return prefix(L10n.commonMessageRemoved, with: displayName) case .sticker: - return prefix(L10n.commonSticker, with: senderDisplayName) + return prefix(L10n.commonSticker, with: displayName) case .failedToParseMessageLike, .failedToParseState: - return prefix(L10n.commonUnsupportedEvent, with: senderDisplayName) + return prefix(L10n.commonUnsupportedEvent, with: displayName) case .message: guard let messageContent = eventItemProxy.content.asMessage() else { fatalError("Invalid message timeline item: \(eventItemProxy)") } let messageType = messageContent.msgtype() - return messageEventStringBuilder.buildAttributedString(for: messageType, senderDisplayName: senderDisplayName, prefixWithSenderName: true) + return messageEventStringBuilder.buildAttributedString(for: messageType, senderDisplayName: displayName, prefixWithSenderName: true) case .state(_, let state): return stateEventStringBuilder .buildString(for: state, sender: sender, isOutgoing: isOutgoing) @@ -66,9 +65,9 @@ struct RoomEventStringBuilder { memberIsYou: isOutgoing) .map(AttributedString.init) case .poll(let question, _, _, _, _, _, _): - return prefix(L10n.commonPollSummary(question), with: senderDisplayName) + return prefix(L10n.commonPollSummary(question), with: displayName) case .callInvite: - return prefix(L10n.commonCallInvite, with: senderDisplayName) + return prefix(L10n.commonCallInvite, with: displayName) } } diff --git a/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift b/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift index b4e355114..b07557e48 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift @@ -100,13 +100,15 @@ class EventTimelineItemProxy { let profile = item.senderProfile() switch profile { - case let .ready(displayName, _, avatarUrl): + case let .ready(displayName, isDisplayNameAmbiguous, avatarUrl): return .init(id: item.sender(), displayName: displayName, + isDisplayNameAmbiguous: isDisplayNameAmbiguous, avatarURL: avatarUrl.flatMap(URL.init(string:))) default: return .init(id: item.sender(), displayName: nil, + isDisplayNameAmbiguous: false, avatarURL: nil) } }() diff --git a/ElementX/Sources/Services/Timeline/TimelineItemSender.swift b/ElementX/Sources/Services/Timeline/TimelineItemSender.swift index d7321d0a1..3bb75980b 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItemSender.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItemSender.swift @@ -21,11 +21,21 @@ struct TimelineItemSender: Identifiable, Hashable { let id: String let displayName: String? + let isDisplayNameAmbiguous: Bool let avatarURL: URL? - init(id: String, displayName: String? = nil, avatarURL: URL? = nil) { + init(id: String, displayName: String? = nil, isDisplayNameAmbiguous: Bool = false, avatarURL: URL? = nil) { self.id = id self.displayName = displayName + self.isDisplayNameAmbiguous = isDisplayNameAmbiguous self.avatarURL = avatarURL } + + var disambiguatedDisplayName: String? { + guard let displayName else { + return nil + } + + return isDisplayNameAmbiguous ? "\(displayName) (\(id))" : displayName + } } diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift index cf480eab2..c2f77d09d 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift @@ -19,55 +19,63 @@ import UIKit struct RoomStateEventStringBuilder { let userID: String + var shouldDisambiguateDisplayNames = true - func buildString(for change: MembershipChange?, member: String, sender: TimelineItemSender, isOutgoing: Bool) -> String? { + func buildString(for change: MembershipChange?, + member: String, + sender: TimelineItemSender, + isOutgoing: Bool) -> String? { guard let change else { MXLog.verbose("Filtering timeline item for membership change that is nil") return nil } - let senderName = sender.displayName ?? sender.id let senderIsYou = isOutgoing let memberIsYou = member == userID + let displayName = if shouldDisambiguateDisplayNames { + sender.disambiguatedDisplayName ?? sender.id + } else { + sender.displayName ?? sender.id + } switch change { case .joined: - return memberIsYou ? L10n.stateEventRoomJoinByYou : L10n.stateEventRoomJoin(member) + return memberIsYou ? L10n.stateEventRoomJoinByYou : L10n.stateEventRoomJoin(displayName) case .left: - return memberIsYou ? L10n.stateEventRoomLeaveByYou : L10n.stateEventRoomLeave(member) + return memberIsYou ? L10n.stateEventRoomLeaveByYou : L10n.stateEventRoomLeave(displayName) case .banned, .kickedAndBanned: - return senderIsYou ? L10n.stateEventRoomBanByYou(member) : L10n.stateEventRoomBan(senderName, member) + return senderIsYou ? L10n.stateEventRoomBanByYou(member) : L10n.stateEventRoomBan(displayName, member) case .unbanned: - return senderIsYou ? L10n.stateEventRoomUnbanByYou(member) : L10n.stateEventRoomUnban(senderName, member) + return senderIsYou ? L10n.stateEventRoomUnbanByYou(member) : L10n.stateEventRoomUnban(displayName, member) case .kicked: - return senderIsYou ? L10n.stateEventRoomRemoveByYou(member) : L10n.stateEventRoomRemove(senderName, member) + return senderIsYou ? L10n.stateEventRoomRemoveByYou(member) : L10n.stateEventRoomRemove(displayName, member) case .invited: if senderIsYou { return L10n.stateEventRoomInviteByYou(member) } else if memberIsYou { - return L10n.stateEventRoomInviteYou(senderName) + return L10n.stateEventRoomInviteYou(displayName) } else { - return L10n.stateEventRoomInvite(senderName, member) + return L10n.stateEventRoomInvite(displayName, member) } case .invitationAccepted: - return memberIsYou ? L10n.stateEventRoomInviteAcceptedByYou : L10n.stateEventRoomInviteAccepted(member) + return memberIsYou ? L10n.stateEventRoomInviteAcceptedByYou : L10n.stateEventRoomInviteAccepted(displayName) case .invitationRejected: - return memberIsYou ? L10n.stateEventRoomRejectByYou : L10n.stateEventRoomReject(member) + return memberIsYou ? L10n.stateEventRoomRejectByYou : L10n.stateEventRoomReject(displayName) case .invitationRevoked: - return senderIsYou ? L10n.stateEventRoomThirdPartyRevokedInviteByYou(member) : L10n.stateEventRoomThirdPartyRevokedInvite(sender, member) + return senderIsYou ? L10n.stateEventRoomThirdPartyRevokedInviteByYou(member) : L10n.stateEventRoomThirdPartyRevokedInvite(displayName, member) case .knocked: return memberIsYou ? L10n.stateEventRoomKnockByYou : L10n.stateEventRoomKnock(member) case .knockAccepted: - return senderIsYou ? L10n.stateEventRoomKnockAcceptedByYou(senderName) : L10n.stateEventRoomKnockAccepted(senderName, member) + return senderIsYou ? L10n.stateEventRoomKnockAcceptedByYou(displayName) : L10n.stateEventRoomKnockAccepted(displayName, member) case .knockRetracted: return memberIsYou ? L10n.stateEventRoomKnockRetractedByYou : L10n.stateEventRoomKnockRetracted(member) case .knockDenied: if senderIsYou { return L10n.stateEventRoomKnockDeniedByYou(member) } else if memberIsYou { - return L10n.stateEventRoomKnockDeniedYou(senderName) + return L10n.stateEventRoomKnockDeniedYou(displayName) } else { - return L10n.stateEventRoomKnockDenied(senderName, member) + return L10n.stateEventRoomKnockDenied(displayName, member) } case .none, .error, .notImplemented: // Not useful information for the user. MXLog.verbose("Filtering timeline item for membership change: \(change)") @@ -122,30 +130,34 @@ struct RoomStateEventStringBuilder { } func buildString(for state: OtherState, sender: TimelineItemSender, isOutgoing: Bool) -> String? { - let senderName = sender.displayName ?? sender.id + let displayName = if shouldDisambiguateDisplayNames { + sender.disambiguatedDisplayName ?? sender.id + } else { + sender.displayName ?? sender.id + } switch state { case .roomAvatar(let url): switch (url, isOutgoing) { case (.some, false): - return L10n.stateEventRoomAvatarChanged(senderName) + return L10n.stateEventRoomAvatarChanged(displayName) case (nil, false): - return L10n.stateEventRoomAvatarRemoved(senderName) + return L10n.stateEventRoomAvatarRemoved(displayName) case (.some, true): return L10n.stateEventRoomAvatarChangedByYou case (nil, true): return L10n.stateEventRoomAvatarRemovedByYou } case .roomCreate: - return isOutgoing ? L10n.stateEventRoomCreatedByYou : L10n.stateEventRoomCreated(senderName) + return isOutgoing ? L10n.stateEventRoomCreatedByYou : L10n.stateEventRoomCreated(displayName) case .roomEncryption: return L10n.commonEncryptionEnabled case .roomName(let name): switch (name, isOutgoing) { case (.some(let name), false): - return L10n.stateEventRoomNameChanged(senderName, name) + return L10n.stateEventRoomNameChanged(displayName, name) case (nil, false): - return L10n.stateEventRoomNameRemoved(senderName) + return L10n.stateEventRoomNameRemoved(displayName) case (.some(let name), true): return L10n.stateEventRoomNameChangedByYou(name) case (nil, true): @@ -160,14 +172,14 @@ struct RoomStateEventStringBuilder { if isOutgoing { return L10n.stateEventRoomThirdPartyInviteByYou(displayName) } else { - return L10n.stateEventRoomThirdPartyInvite(senderName, displayName) + return L10n.stateEventRoomThirdPartyInvite(displayName, displayName) } case .roomTopic(let topic): switch (topic, isOutgoing) { case (.some(let topic), false): - return L10n.stateEventRoomTopicChanged(senderName, topic) + return L10n.stateEventRoomTopicChanged(displayName, topic) case (nil, false): - return L10n.stateEventRoomTopicRemoved(senderName) + return L10n.stateEventRoomTopicRemoved(displayName) case (.some(let name), true): return L10n.stateEventRoomTopicChangedByYou(name) case (nil, true): diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift index caeba9b43..893000b44 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift @@ -626,13 +626,15 @@ struct RoomTimelineItemFactory: RoomTimelineItemFactoryProtocol { case let .ready(timelineItem, senderID, senderProfile): let sender: TimelineItemSender switch senderProfile { - case let .ready(displayName, _, avatarUrl): + case let .ready(displayName, isDisplayNameAmbiguous, avatarUrl): sender = TimelineItemSender(id: senderID, displayName: displayName, + isDisplayNameAmbiguous: isDisplayNameAmbiguous, avatarURL: avatarUrl.flatMap(URL.init(string:))) default: sender = TimelineItemSender(id: senderID, displayName: nil, + isDisplayNameAmbiguous: false, avatarURL: nil) }