From 9f865cf5c943a7a48792c5f9482abc95adfa7db2 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Mon, 23 Jan 2023 10:01:45 +0000 Subject: [PATCH] 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. --- .../Screens/RoomScreen/RoomScreenModels.swift | 2 +- .../RoomScreen/RoomScreenViewModel.swift | 21 +++-- .../View/TimelineItemContextMenu.swift | 19 ++-- .../View/TimelineTableViewController.swift | 2 +- .../Services/Timeline/TimelineItemProxy.swift | 21 +++++ .../RoomStateEventStringBuilder.swift | 4 +- .../RoomTimelineViewProvider.swift | 12 +++ .../RoomStateEventStringBuilderTests.swift | 89 +++++++++++++++++++ 8 files changed, 155 insertions(+), 15 deletions(-) create mode 100644 UnitTests/Sources/RoomStateEventStringBuilderTests.swift diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index 30b4f178b..736ec5cc2 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/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) diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index 3f4ed4fc9..55ab399c5 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/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] = [ diff --git a/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift b/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift index 09837cbfe..20bd1662d 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift +++ b/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") + } } } diff --git a/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift b/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift index d2bb68b0c..b4d58f873 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift +++ b/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)) diff --git a/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift b/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift index 6d45d64c7..93d5ae4e1 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItemProxy.swift +++ b/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 + } + } +} diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift index cce6f3886..21e1fc06f 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomStateEventStringBuilder.swift +++ b/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 { diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineViewProvider.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineViewProvider.swift index a73509cd9..7d56b428d 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineViewProvider.swift +++ b/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 { diff --git a/UnitTests/Sources/RoomStateEventStringBuilderTests.swift b/UnitTests/Sources/RoomStateEventStringBuilderTests.swift new file mode 100644 index 000000000..f15996362 --- /dev/null +++ b/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) + } +}