Improvements to state items (#478)

- Remove context menu (and fix long press when there are no items).
- Add membership change tests.
- Fix new/old name incorrect ordering.
- Fix message grouping when it comes before/after a state item from the same user.
- Only show reactions picker for items that can be reacted to.

ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift

@@ -32,11 +32,11 @@ enum RoomScreenComposerMode: Equatable {
 
 enum RoomScreenViewAction {
     case displayRoomDetails
-    case displayEmojiPicker(itemId: String)
     case paginateBackwards
     case itemAppeared(id: String)
     case itemDisappeared(id: String)
     case itemTapped(id: String)
+    case itemDoubleTapped(id: String)
     case linkClicked(url: URL)
     case sendMessage
     case sendReaction(key: String, eventID: String)

ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift

@@ -73,7 +73,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
             }
             .store(in: &cancellables)
         
-        state.contextMenuBuilder = buildContexMenuForItemId(_:)
+        state.contextMenuBuilder = buildContextMenuForItemId(_:)
         
         buildTimelineViews()
         
@@ -104,14 +104,14 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
             await timelineController.processItemDisappearance(id)
         case .itemTapped(let id):
             await itemTapped(with: id)
+        case .itemDoubleTapped(let id):
+            itemDoubleTapped(with: id)
         case .linkClicked(let url):
             MXLog.warning("Link clicked: \(url)")
         case .sendMessage:
             await sendCurrentMessage()
         case .sendReaction(let emoji, let itemId):
             await timelineController.sendReaction(emoji, to: itemId)
-        case .displayEmojiPicker(let itemId):
-            callback?(.displayEmojiPicker(itemId: itemId))
         case .cancelReply:
             state.composerMode = .default
         case .cancelEdit:
@@ -157,6 +157,11 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
         state.showLoading = false
     }
     
+    private func itemDoubleTapped(with itemId: String) {
+        guard let item = state.items.first(where: { $0.id == itemId }), item.isReactable else { return }
+        callback?(.displayEmojiPicker(itemId: itemId))
+    }
+    
     private func buildTimelineViews() {
         let stateItems = timelineController.timelineItems.map { item in
             timelineViewFactory.buildTimelineViewFor(timelineItem: item)
@@ -202,7 +207,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
     
     // MARK: ContextMenus
     
-    private func buildContexMenuForItemId(_ itemId: String) -> TimelineItemContextMenu {
+    private func buildContextMenuForItemId(_ itemId: String) -> TimelineItemContextMenu {
         TimelineItemContextMenu(contextMenuActions: contextMenuActionsForItemId(itemId)) { [weak self] action in
             self?.processContentMenuAction(action, itemId: itemId)
         }
@@ -211,7 +216,13 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
     private func contextMenuActionsForItemId(_ itemId: String) -> TimelineItemContextMenuActions {
         guard let timelineItem = timelineController.timelineItems.first(where: { $0.id == itemId }),
               let item = timelineItem as? EventBasedTimelineItemProtocol else {
-            return .init(actions: [], debugActions: [])
+            // Don't show a context menu for non-event based items.
+            return .empty
+        }
+        
+        if timelineItem is StateRoomTimelineItem {
+            // Don't show a context menu for state events.
+            return .empty
         }
         
         var actions: [TimelineItemContextMenuAction] = [

ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift

@@ -17,8 +17,11 @@
 import SwiftUI
 
 struct TimelineItemContextMenuActions {
+    static var empty: TimelineItemContextMenuActions { .init(actions: [], debugActions: []) }
+    
     let actions: [TimelineItemContextMenuAction]
     let debugActions: [TimelineItemContextMenuAction]
+    var isEmpty: Bool { actions.isEmpty && debugActions.isEmpty }
 }
 
 enum TimelineItemContextMenuAction: Identifiable, Hashable {
@@ -48,13 +51,17 @@ public struct TimelineItemContextMenu: View {
     let contextMenuActions: TimelineItemContextMenuActions
     let callback: (TimelineItemContextMenuAction) -> Void
     
-    @ViewBuilder
     public var body: some View {
-        viewsForActions(contextMenuActions.actions)
-        Menu {
-            viewsForActions(contextMenuActions.debugActions)
-        } label: {
-            Label("Developer", systemImage: "hammer")
+        if contextMenuActions.isEmpty {
+            // When there are no actions make sure then menu isn't shown.
+            EmptyView()
+        } else {
+            viewsForActions(contextMenuActions.actions)
+            Menu {
+                viewsForActions(contextMenuActions.debugActions)
+            } label: {
+                Label("Developer", systemImage: "hammer")
+            }
         }
     }
     

ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift

@@ -211,7 +211,7 @@ class TimelineTableViewController: UIViewController {
                         return .systemAction
                     })
                     .onTapGesture(count: 2) {
-                        coordinator.send(viewAction: .displayEmojiPicker(itemId: timelineItem.id))
+                        coordinator.send(viewAction: .itemDoubleTapped(id: timelineItem.id))
                     }
                     .onTapGesture {
                         coordinator.send(viewAction: .itemTapped(id: timelineItem.id))

ElementX/Sources/Services/Timeline/TimelineItemProxy.swift

@@ -37,6 +37,12 @@ enum TimelineItemProxy {
         guard case let .event(selfEventItemProxy) = self, case let .event(previousEventItemProxy) = previousItemProxy else {
             return false
         }
+        
+        // State events aren't rendered as messages so shouldn't be grouped.
+        if selfEventItemProxy.isRoomState || previousEventItemProxy.isRoomState {
+            return false
+        }
+        
         //  can be improved by adding a date threshold
         return previousEventItemProxy.reactions.isEmpty && selfEventItemProxy.sender == previousEventItemProxy.sender
     }
@@ -83,6 +89,10 @@ struct EventTimelineItemProxy: CustomDebugStringConvertible {
         content.asMessage() != nil
     }
     
+    var isRoomState: Bool {
+        content.kind().isRoomState
+    }
+    
     var content: TimelineItemContent {
         item.content()
     }
@@ -116,3 +126,14 @@ struct EventTimelineItemProxy: CustomDebugStringConvertible {
         item.fmtDebug()
     }
 }
+
+extension TimelineItemContentKind {
+    var isRoomState: Bool {
+        switch self {
+        case .state, .roomMembership:
+            return true
+        default:
+            return false
+        }
+    }
+}

ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift

@@ -87,7 +87,7 @@ struct RoomStateEventStringBuilder {
         switch (displayNameChanged, avatarChanged, memberIsYou) {
         case (true, false, false):
             if let displayName, let previousDisplayName {
-                return ElementL10n.noticeDisplayNameChangedFrom(member, displayName, previousDisplayName)
+                return ElementL10n.noticeDisplayNameChangedFrom(member, previousDisplayName, displayName)
             } else if let displayName {
                 return ElementL10n.noticeDisplayNameSet(member, displayName)
             } else if let previousDisplayName {
@@ -100,7 +100,7 @@ struct RoomStateEventStringBuilder {
             return ElementL10n.noticeAvatarUrlChanged(displayName ?? member)
         case (true, false, true):
             if let displayName, let previousDisplayName {
-                return ElementL10n.noticeDisplayNameChangedFromByYou(displayName, previousDisplayName)
+                return ElementL10n.noticeDisplayNameChangedFromByYou(previousDisplayName, displayName)
             } else if let displayName {
                 return ElementL10n.noticeDisplayNameSetByYou(displayName)
             } else if let previousDisplayName {

ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineViewProvider.swift

@@ -68,6 +68,18 @@ enum RoomTimelineViewProvider: Identifiable, Hashable {
             return item.id
         }
     }
+    
+    /// Whether or not it is possible to send a reaction to this timeline item.
+    var isReactable: Bool {
+        switch self {
+        case .text, .image, .video, .file, .emote, .notice, .sticker:
+            return true
+        case .redacted, .encrypted, .unsupported, .state: // Event based items that aren't reactable
+            return false
+        case .timelineStart, .separator, .readMarker, .paginationIndicator: // Virtual items are never reactable
+            return false
+        }
+    }
 }
 
 extension RoomTimelineViewProvider: View {

UnitTests/Sources/RoomStateEventStringBuilderTests.swift

@@ -0,0 +1,89 @@
+//
+// Copyright 2023 New Vector Ltd
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+@testable import ElementX
+import MatrixRustSDK
+import XCTest
+
+class RoomStateEventStringBuilderTests: XCTestCase {
+    var userID: String!
+    var stringBuilder: RoomStateEventStringBuilder!
+    
+    override func setUp() {
+        userID = "@alice:matrix.org"
+        stringBuilder = RoomStateEventStringBuilder(userID: userID)
+    }
+    
+    func testDisplayNameChanges() {
+        // Changes by you.
+        validateDisplayNameChange(senderID: userID, oldName: "Alice", newName: "Bob",
+                                  expectedString: ElementL10n.noticeDisplayNameChangedFromByYou("Alice", "Bob"))
+        validateDisplayNameChange(senderID: userID, oldName: "Alice", newName: nil,
+                                  expectedString: ElementL10n.noticeDisplayNameRemovedByYou("Alice"))
+        validateDisplayNameChange(senderID: userID, oldName: nil, newName: "Alice",
+                                  expectedString: ElementL10n.noticeDisplayNameSetByYou("Alice"))
+        
+        // Changes by someone else.
+        let senderID = "@bob:matrix.org"
+        validateDisplayNameChange(senderID: senderID, oldName: "Bob", newName: "Alice",
+                                  expectedString: ElementL10n.noticeDisplayNameChangedFrom(senderID, "Bob", "Alice"))
+        validateDisplayNameChange(senderID: senderID, oldName: "Bob", newName: nil,
+                                  expectedString: ElementL10n.noticeDisplayNameRemoved(senderID, "Bob"))
+        validateDisplayNameChange(senderID: senderID, oldName: nil, newName: "Bob",
+                                  expectedString: ElementL10n.noticeDisplayNameSet(senderID, "Bob"))
+    }
+    
+    func validateDisplayNameChange(senderID: String, oldName: String?, newName: String?, expectedString: String) {
+        let sender = TimelineItemSender(id: senderID, displayName: newName)
+        let change = MembershipChange.profileChanged(displayName: newName, prevDisplayName: oldName, avatarUrl: nil, prevAvatarUrl: nil)
+        
+        let string = stringBuilder.buildString(for: change, member: sender.id, sender: sender, isOutgoing: sender.id == userID)
+        
+        XCTAssertEqual(string, expectedString)
+    }
+    
+    func testAvatarChanges() {
+        // Changes by you.
+        validateAvatarChange(senderID: userID, oldAvatarURL: "mxc://1", newAvatarURL: "mxc://2",
+                             expectedString: ElementL10n.noticeAvatarUrlChangedByYou)
+        validateAvatarChange(senderID: userID, oldAvatarURL: "mxc://1", newAvatarURL: nil,
+                             expectedString: ElementL10n.noticeAvatarUrlChangedByYou)
+        validateAvatarChange(senderID: userID, oldAvatarURL: nil, newAvatarURL: "mxc://1",
+                             expectedString: ElementL10n.noticeAvatarUrlChangedByYou)
+        
+        // Changes by someone else.
+        let senderID = "@bob:matrix.org"
+        let senderName = "Bob"
+        validateAvatarChange(senderID: senderID, senderName: senderName, oldAvatarURL: "mxc://1", newAvatarURL: "mxc://2",
+                             expectedString: ElementL10n.noticeAvatarUrlChanged(senderName))
+        validateAvatarChange(senderID: senderID, senderName: senderName, oldAvatarURL: "mxc://1", newAvatarURL: nil,
+                             expectedString: ElementL10n.noticeAvatarUrlChanged(senderName))
+        validateAvatarChange(senderID: senderID, senderName: senderName, oldAvatarURL: nil, newAvatarURL: "mxc://1",
+                             expectedString: ElementL10n.noticeAvatarUrlChanged(senderName))
+    }
+    
+    func validateAvatarChange(senderID: String, senderName: String? = nil,
+                              oldAvatarURL: String?, newAvatarURL: String?,
+                              expectedString: String) {
+        let sender = TimelineItemSender(id: senderID, displayName: senderName)
+        let change = MembershipChange.profileChanged(displayName: senderName, prevDisplayName: senderName,
+                                                     avatarUrl: oldAvatarURL, prevAvatarUrl: newAvatarURL)
+        
+        let string = stringBuilder.buildString(for: change, member: sender.id, sender: sender, isOutgoing: sender.id == userID)
+        
+        XCTAssertEqual(string, expectedString)
+    }
+}