From 452a4c12b817a84940a79946b60efdae3584d472 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Thu, 2 May 2024 17:51:38 +0100 Subject: [PATCH] Permalink polish (#2771) * Fix unbalanced padding. * Animate the scroll to an event when it is already loaded. * Perform the timeline clearance immediately before replacing the items. Hides the empty timeline that was briefly shown. * Make switching timeline more resilient. --- .../Screens/RoomScreen/RoomScreenModels.swift | 27 ++++++++++++--- .../RoomScreen/RoomScreenViewModel.swift | 32 +++++++++-------- .../Screens/RoomScreen/View/RoomScreen.swift | 2 +- .../HighlightedTimelineItemModifier.swift | 29 ++++++++++++++-- .../TimelineTableViewController.swift | 31 ++++++++++------- .../View/Timeline/TimelineView.swift | 11 +++--- .../Fixtures/RoomTimelineItemFixtures.swift | 21 ++++++++---- .../MockRoomTimelineController.swift | 4 +-- .../RoomTimelineController.swift | 34 +++++++++---------- .../RoomTimelineControllerProtocol.swift | 2 +- ...ghtedTimelineItemModifier-iPad-en-GB.1.png | 3 -- ...TimelineItemModifier-iPad-en-GB.Layout.png | 3 ++ ...htedTimelineItemModifier-iPad-pseudo.1.png | 3 -- ...imelineItemModifier-iPad-pseudo.Layout.png | 3 ++ ...TimelineItemModifier-iPhone-15-en-GB.1.png | 3 -- ...ineItemModifier-iPhone-15-en-GB.Layout.png | 3 ++ ...imelineItemModifier-iPhone-15-pseudo.1.png | 3 -- ...neItemModifier-iPhone-15-pseudo.Layout.png | 3 ++ ...hlight-0-iPad-10th-generation-en-GB.UI.png | 4 +-- ...omLayoutHighlight-0-iPhone-15-en-GB.UI.png | 4 +-- .../Sources/RoomScreenViewModelTests.swift | 23 +++++-------- changelog.d/2757.change | 1 + changelog.d/2764.bugfix | 1 + 23 files changed, 152 insertions(+), 98 deletions(-) delete mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.1.png create mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.Layout.png delete mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.1.png create mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.Layout.png delete mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.1.png create mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.Layout.png delete mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.1.png create mode 100644 PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.Layout.png create mode 100644 changelog.d/2757.change create mode 100644 changelog.d/2764.bugfix diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index 32a5afb5d..87da91e6b 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift @@ -115,6 +115,8 @@ enum RoomScreenViewAction { case focusLive /// The timeline scrolled to reveal the focussed item. case scrolledToFocussedItem + /// The table view has loaded the first items for a new timeline. + case hasSwitchedTimeline } enum RoomScreenComposerAction { @@ -227,10 +229,27 @@ struct TimelineViewState { var isLive = true var paginationState = PaginationState.default - /// The ID of the focussed event navigated to via a permalink. - var focussedEventID: String? { didSet { focussedEventNeedsDisplay = focussedEventID != nil } } - /// Whether the timeline should scroll to `focussedEventID` once its item has been built and added to the timeline. - var focussedEventNeedsDisplay: Bool + /// The room is in the process of loading items from a new timeline (switching to/from a detached timeline). + var isSwitchingTimelines = false + + struct FocussedEvent: Equatable { + enum Appearance { + /// The event should be shown using an animated scroll. + case animated + /// The event should be shown immediately, without any animation. + case immediate + /// The event has already been shown. + case hasAppeared + } + + /// The ID of the event. + let eventID: String + /// How the event should be shown, or whether it has already appeared. + var appearance: Appearance + } + + /// A focussed event that was navigated to via a permalink. + var focussedEvent: FocussedEvent? // These can be removed when we have full swiftUI and moved as @State values in the view var scrollToBottomPublisher = PassthroughSubject() diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index b8273e7c2..0e62c2924 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift @@ -88,8 +88,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol roomAvatarURL: roomProxy.avatarURL, timelineStyle: appSettings.timelineStyle, isEncryptedOneToOneRoom: roomProxy.isEncryptedOneToOneRoom, - timelineViewState: TimelineViewState(focussedEventID: focussedEventID, - focussedEventNeedsDisplay: focussedEventID != nil), + timelineViewState: TimelineViewState(focussedEvent: focussedEventID.map { .init(eventID: $0, appearance: .immediate) }), ownUserID: roomProxy.ownUserID, hasOngoingCall: roomProxy.hasOngoingCall, bindings: .init(reactionsCollapsed: [:])), @@ -119,7 +118,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol return self.roomScreenInteractionHandler.audioPlayerState(for: itemID) } - buildTimelineViews() + buildTimelineViews(timelineItems: timelineController.timelineItems) updateMembers(roomProxy.membersPublisher.value) @@ -192,9 +191,11 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol focusLive() case .scrolledToFocussedItem: Task { // Use a Task to mutate view state after the current view update. - state.timelineViewState.focussedEventNeedsDisplay = false + state.timelineViewState.focussedEvent?.appearance = .hasAppeared hideFocusLoadingIndicator() } + case .hasSwitchedTimeline: + Task { state.timelineViewState.isSwitchingTimelines = false } } } @@ -230,7 +231,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol func focusOnEvent(eventID: String) async { if state.timelineViewState.hasLoadedItem(with: eventID) { - state.timelineViewState.focussedEventID = eventID + state.timelineViewState.focussedEvent = .init(eventID: eventID, appearance: .animated) return } @@ -239,7 +240,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol switch await timelineController.focusOnEvent(eventID, timelineSize: Constants.detachedTimelineSize) { case .success: - state.timelineViewState.focussedEventID = eventID + state.timelineViewState.focussedEvent = .init(eventID: eventID, appearance: .immediate) case .failure(let error): MXLog.error("Failed to focus on event \(eventID)") @@ -255,7 +256,6 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol private func focusLive() { timelineController.focusLive() - state.timelineViewState.focussedEventID = nil } private func attach(_ attachment: ComposerAttachmentType) { @@ -336,8 +336,8 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol guard let self else { return } switch callback { - case .updatedTimelineItems: - buildTimelineViews() + case .updatedTimelineItems(let updatedItems, let isSwitchingTimelines): + buildTimelineViews(timelineItems: updatedItems, isSwitchingTimelines: isSwitchingTimelines) case .paginationState(let paginationState): if state.timelineViewState.paginationState != paginationState { state.timelineViewState.paginationState = paginationState @@ -347,8 +347,8 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol state.timelineViewState.isLive = isLive // Remove the event highlight *only* when transitioning from non-live to live. - if isLive, state.timelineViewState.focussedEventID != nil { - state.timelineViewState.focussedEventID = nil + if isLive, state.timelineViewState.focussedEvent != nil { + state.timelineViewState.focussedEvent = nil } } } @@ -568,10 +568,10 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol // MARK: - Timeline Item Building - private func buildTimelineViews() { + private func buildTimelineViews(timelineItems: [RoomTimelineItemProtocol], isSwitchingTimelines: Bool = false) { var timelineItemsDictionary = OrderedDictionary() - timelineController.timelineItems.filter { $0 is RedactedRoomTimelineItem }.forEach { timelineItem in + timelineItems.filter { $0 is RedactedRoomTimelineItem }.forEach { timelineItem in // Stops the audio player when a voice message is redacted. guard let playerState = mediaPlayerProvider.playerState(for: .timelineItemIdentifier(timelineItem.id)) else { return @@ -583,7 +583,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol } } - let itemsGroupedByTimelineDisplayStyle = timelineController.timelineItems.chunked { current, next in + let itemsGroupedByTimelineDisplayStyle = timelineItems.chunked { current, next in canGroupItem(timelineItem: current, with: next) } @@ -614,6 +614,10 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol } } + if isSwitchingTimelines { + state.timelineViewState.isSwitchingTimelines = true + } + state.timelineViewState.itemsDictionary = timelineItemsDictionary } diff --git a/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift b/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift index f30fd516e..77a71e91f 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift @@ -96,7 +96,7 @@ struct RoomScreen: View { .id(context.viewState.roomID) .environmentObject(context) .environment(\.timelineStyle, context.viewState.timelineStyle) - .environment(\.focussedEventID, context.viewState.timelineViewState.focussedEventID) + .environment(\.focussedEventID, context.viewState.timelineViewState.focussedEvent?.eventID) .overlay(alignment: .bottomTrailing) { scrollToBottomButton } diff --git a/ElementX/Sources/Screens/RoomScreen/View/Timeline/HighlightedTimelineItemModifier.swift b/ElementX/Sources/Screens/RoomScreen/View/Timeline/HighlightedTimelineItemModifier.swift index e393883ca..75d125d97 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Timeline/HighlightedTimelineItemModifier.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Timeline/HighlightedTimelineItemModifier.swift @@ -28,6 +28,7 @@ private struct HighlightedTimelineItemModifier: ViewModifier { func body(content: Content) -> some View { content + .padding(.top, isHighlighted ? 1 : 0) .background { if isHighlighted { VStack(spacing: 0) { @@ -47,7 +48,9 @@ private struct HighlightedTimelineItemModifier: ViewModifier { } } -// swiftlint:disable line_length +// MARK: - Previews + +// swiftlint:disable line_length blanket_disable_command struct HighlightedTimelineItemModifier_Previews: PreviewProvider, TestablePreview { static var previews: some View { ScrollView { @@ -67,6 +70,7 @@ struct HighlightedTimelineItemModifier_Previews: PreviewProvider, TestablePrevie .highlightedTimelineItem(true) } } + .previewDisplayName("Layout") } struct Bubble: View { @@ -85,4 +89,25 @@ struct HighlightedTimelineItemModifier_Previews: PreviewProvider, TestablePrevie } } -// swiftlint:enable line_length +/// A preview that allows quick testing of the highlight appearance across various timeline scenarios. +struct HighlightedTimelineItemTimeline_Previews: PreviewProvider { + static let focussedEventID = "RoomTimelineItemFixtures.default.5" + static let viewModel = RoomScreenViewModel(roomProxy: RoomProxyMock(with: .init(name: "Preview room")), + focussedEventID: focussedEventID, + timelineController: MockRoomTimelineController(), + mediaProvider: MockMediaProvider(), + mediaPlayerProvider: MediaPlayerProviderMock(), + voiceMessageMediaManager: VoiceMessageMediaManagerMock(), + userIndicatorController: ServiceLocator.shared.userIndicatorController, + appMediator: AppMediatorMock.default, + appSettings: ServiceLocator.shared.settings, + analyticsService: ServiceLocator.shared.analytics, + notificationCenter: NotificationCenterMock()) + + static var previews: some View { + NavigationStack { + RoomScreen(context: viewModel.context, composerToolbar: ComposerToolbar.mock()) + } + .previewDisplayName("Timeline") + } +} diff --git a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift index c92aabdc1..8628390eb 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift @@ -66,7 +66,7 @@ class TimelineTableViewController: UIViewController { } applySnapshot() - + if timelineItemsDictionary.isEmpty { paginatePublisher.send() } @@ -104,15 +104,14 @@ class TimelineTableViewController: UIViewController { } } - /// The ID of the focussed event if navigating to an event permalink within the room. - var focussedEventID: String? - - /// Whether the timeline should scroll to `focussedEventID` when that item is added to the data source. - /// This is necessary as the focussed event can be set before the timeline builder has built its item. - var focussedEventNeedsDisplay = false { + /// Whether the table view is about to load items from a new timeline or not. + var isSwitchingTimelines = false + + /// The focussed event if navigating to an event permalink within the room. + var focussedEvent: TimelineViewState.FocussedEvent? { didSet { - guard focussedEventNeedsDisplay, let focussedEventID else { return } - scrollToItem(eventID: focussedEventID, animated: false) + guard let focussedEvent, focussedEvent.appearance != .hasAppeared else { return } + scrollToItem(eventID: focussedEvent.eventID, animated: focussedEvent.appearance == .animated) } } @@ -300,7 +299,7 @@ class TimelineTableViewController: UIViewController { let newestItemIdentifier = snapshot.mainItemIdentifiers.first let currentNewestItemIdentifier = currentSnapshot.mainItemIdentifiers.first let newestItemIDChanged = snapshot.numberOfMainItems > 0 && currentSnapshot.numberOfMainItems > 0 && newestItemIdentifier != currentNewestItemIdentifier - let animated = isLive && newestItemIDChanged + let animated = isLive && !isSwitchingTimelines && newestItemIDChanged let layout: Layout? = if !isLive, newestItemIDChanged { snapshotLayout() @@ -310,10 +309,16 @@ class TimelineTableViewController: UIViewController { dataSource.apply(snapshot, animatingDifferences: animated) - if let focussedEventID, focussedEventNeedsDisplay { - scrollToItem(eventID: focussedEventID, animated: false) + if let focussedEvent, focussedEvent.appearance != .hasAppeared { + scrollToItem(eventID: focussedEvent.eventID, animated: focussedEvent.appearance == .animated) } else if let layout { restoreLayout(layout) + } else if isSwitchingTimelines { + scrollToNewestItem(animated: false) + } + + if isSwitchingTimelines { + coordinator.send(viewAction: .hasSwitchedTimeline) } } @@ -335,7 +340,7 @@ class TimelineTableViewController: UIViewController { /// Scrolls to the item with the corresponding event ID if loaded in the timeline. private func scrollToItem(eventID: String, animated: Bool) { - if let kvPair = timelineItemsDictionary.first(where: { $0.value.identifier.eventID == focussedEventID }), + if let kvPair = timelineItemsDictionary.first(where: { $0.value.identifier.eventID == eventID }), let indexPath = dataSource?.indexPath(for: kvPair.key) { tableView.scrollToRow(at: indexPath, at: .middle, animated: animated) coordinator.send(viewAction: .scrolledToFocussedItem) diff --git a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineView.swift b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineView.swift index 1ac01f864..28be93d3a 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineView.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineView.swift @@ -50,6 +50,10 @@ struct TimelineView: UIViewControllerRepresentable { /// Updates the specified table view's properties from the current view state. func update(tableViewController: TimelineTableViewController, timelineStyle: TimelineStyle) { + if tableViewController.isSwitchingTimelines != context.viewState.timelineViewState.isSwitchingTimelines { + // Must come before timelineItemsDictionary in order to disable animations. + tableViewController.isSwitchingTimelines = context.viewState.timelineViewState.isSwitchingTimelines + } if tableViewController.timelineStyle != timelineStyle { tableViewController.timelineStyle = timelineStyle } @@ -62,11 +66,8 @@ struct TimelineView: UIViewControllerRepresentable { if tableViewController.isLive != context.viewState.timelineViewState.isLive { tableViewController.isLive = context.viewState.timelineViewState.isLive } - if tableViewController.focussedEventID != context.viewState.timelineViewState.focussedEventID { - tableViewController.focussedEventID = context.viewState.timelineViewState.focussedEventID - } - if tableViewController.focussedEventNeedsDisplay != context.viewState.timelineViewState.focussedEventNeedsDisplay { - tableViewController.focussedEventNeedsDisplay = context.viewState.timelineViewState.focussedEventNeedsDisplay + if tableViewController.focussedEvent != context.viewState.timelineViewState.focussedEvent { + tableViewController.focussedEvent = context.viewState.timelineViewState.focussedEvent } if tableViewController.typingMembers.members != context.viewState.typingMembers { diff --git a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift index 641fb6459..0d763b6ca 100644 --- a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift +++ b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift @@ -20,7 +20,8 @@ enum RoomTimelineItemFixtures { /// The default timeline items used in Xcode previews etc. static var `default`: [RoomTimelineItemProtocol] = [ SeparatorRoomTimelineItem(id: .init(timelineID: "Yesterday"), text: "Yesterday"), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: ".RoomTimelineItemFixtures.default.0", + eventID: "RoomTimelineItemFixtures.default.0"), timestamp: "10:10 AM", isOutgoing: false, isEditable: false, @@ -29,7 +30,8 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Jacob"), content: .init(body: "That looks so good!"), properties: RoomTimelineItemProperties(isEdited: true)), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.1", + eventID: "RoomTimelineItemFixtures.default.1"), timestamp: "10:11 AM", isOutgoing: false, isEditable: false, @@ -40,7 +42,8 @@ enum RoomTimelineItemFixtures { properties: RoomTimelineItemProperties(reactions: [ AggregatedReaction(accountOwnerID: "me", key: "🙌", senders: [ReactionSender(id: "me", timestamp: Date())]) ])), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.2", + eventID: "RoomTimelineItemFixtures.default.2"), timestamp: "10:11 AM", isOutgoing: false, isEditable: false, @@ -59,7 +62,8 @@ enum RoomTimelineItemFixtures { ]) ])), SeparatorRoomTimelineItem(id: .init(timelineID: "Today"), text: "Today"), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.3", + eventID: "RoomTimelineItemFixtures.default.3"), timestamp: "5 PM", isOutgoing: false, isEditable: false, @@ -68,7 +72,8 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Helena"), content: .init(body: "Wow, cool. Ok, lets go the usual place tomorrow?! Is that too soon? Here’s the menu, let me know what you want it’s on me!"), properties: RoomTimelineItemProperties(orderedReadReceipts: [ReadReceipt(userID: "alice", formattedTimestamp: nil)])), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.4", + eventID: "RoomTimelineItemFixtures.default.4"), timestamp: "5 PM", isOutgoing: true, isEditable: true, @@ -76,7 +81,8 @@ enum RoomTimelineItemFixtures { isThreaded: false, sender: .init(id: "", displayName: "Bob"), content: .init(body: "And John's speech was amazing!")), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.5", + eventID: "RoomTimelineItemFixtures.default.5"), timestamp: "5 PM", isOutgoing: true, isEditable: true, @@ -89,7 +95,8 @@ enum RoomTimelineItemFixtures { ReadReceipt(userID: "bob", formattedTimestamp: nil), ReadReceipt(userID: "charlie", formattedTimestamp: nil), ReadReceipt(userID: "dan", formattedTimestamp: nil)])), - TextRoomTimelineItem(id: .random, + TextRoomTimelineItem(id: .init(timelineID: "RoomTimelineItemFixtures.default.6", + eventID: "RoomTimelineItemFixtures.default.6"), timestamp: "5 PM", isOutgoing: false, isEditable: false, diff --git a/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift b/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift index 66df8f70b..51ed918f4 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift +++ b/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift @@ -165,7 +165,7 @@ class MockRoomTimelineController: RoomTimelineControllerProtocol { let incomingItem = incomingItems.removeFirst() timelineItems.append(incomingItem) - callbacks.send(.updatedTimelineItems) + callbacks.send(.updatedTimelineItems(timelineItems: timelineItems, isSwitchingTimelines: false)) try client?.send(.success) } @@ -181,7 +181,7 @@ class MockRoomTimelineController: RoomTimelineControllerProtocol { let newItems = backPaginationResponses.removeFirst() timelineItems.insert(contentsOf: newItems, at: 0) - callbacks.send(.updatedTimelineItems) + callbacks.send(.updatedTimelineItems(timelineItems: timelineItems, isSwitchingTimelines: false)) try client?.send(.success) } diff --git a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift index 8152ed449..b46d6c01a 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift +++ b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift @@ -33,7 +33,7 @@ class RoomTimelineController: RoomTimelineControllerProtocol { private var activeTimeline: TimelineProxyProtocol private var activeTimelineProvider: RoomTimelineProviderProtocol { didSet { - configureActiveTimelineProvider(clearExistingItems: true) + configureActiveTimelineProvider() } } @@ -272,31 +272,26 @@ class RoomTimelineController: RoomTimelineControllerProtocol { /// The cancellable used to update the timeline items. private var updateTimelineItemsCancellable: AnyCancellable? + /// The controller is switching the `activeTimelineProvider`. + private var isSwitchingTimelines = false /// Configures the controller to listen to `activeTimeline` for events. /// - Parameter clearExistingItems: Whether or not to clear any existing items before loading the timeline's contents. - private func configureActiveTimelineProvider(clearExistingItems: Bool = false) { + private func configureActiveTimelineProvider() { + updateTimelineItemsCancellable = nil + + isSwitchingTimelines = true + + // Inform the world that the initial items are loading from the store + callbacks.send(.paginationState(.init(backward: .paginating, forward: .paginating))) + callbacks.send(.isLive(activeTimelineProvider.isLive)) + updateTimelineItemsCancellable = activeTimelineProvider .updatePublisher .receive(on: serialDispatchQueue) .sink { [weak self] items, paginationState in self?.updateTimelineItems(itemProxies: items, paginationState: paginationState) } - - // Inform the world that the initial items are loading from the store - callbacks.send(.paginationState(.init(backward: .paginating, forward: .paginating))) - callbacks.send(.isLive(activeTimelineProvider.isLive)) - - if clearExistingItems { - // Transition through empty to prevent animations. - timelineItems.removeAll() - callbacks.send(.updatedTimelineItems) - } - - serialDispatchQueue.async { [activeTimelineProvider] in - self.updateTimelineItems(itemProxies: activeTimelineProvider.itemProxies, paginationState: activeTimelineProvider.paginationState) - self.callbacks.send(.paginationState(.init(backward: .idle, forward: .idle))) - } } @objc private func contentSizeCategoryDidChange() { @@ -309,6 +304,9 @@ class RoomTimelineController: RoomTimelineControllerProtocol { private func updateTimelineItems(itemProxies: [TimelineItemProxy], paginationState: PaginationState) { var newTimelineItems = [RoomTimelineItemProtocol]() + let isNewTimeline = isSwitchingTimelines + isSwitchingTimelines = false + let collapsibleChunks = itemProxies.groupBy { isItemCollapsible($0) } for (index, collapsibleChunk) in collapsibleChunks.enumerated() { @@ -362,7 +360,7 @@ class RoomTimelineController: RoomTimelineControllerProtocol { timelineItems = newTimelineItems } - callbacks.send(.updatedTimelineItems) + callbacks.send(.updatedTimelineItems(timelineItems: newTimelineItems, isSwitchingTimelines: isNewTimeline)) callbacks.send(.paginationState(paginationState)) } diff --git a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift index 779950f1e..d8f109c69 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift +++ b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift @@ -19,7 +19,7 @@ import Foundation import UIKit enum RoomTimelineControllerCallback { - case updatedTimelineItems + case updatedTimelineItems(timelineItems: [RoomTimelineItemProtocol], isSwitchingTimelines: Bool) case paginationState(PaginationState) case isLive(Bool) } diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.1.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.1.png deleted file mode 100644 index 552ef2fc5..000000000 --- a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.1.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:94a132c8800c74bc3f9e9299323c06166a4a351fa9f54ba1a3fdc680086a277d -size 417744 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.Layout.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.Layout.png new file mode 100644 index 000000000..8ff8ad79f --- /dev/null +++ b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-en-GB.Layout.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d79d725cd4976368ba4e3e0e374d4f48e2b2e7c60e28c90e8e1ca92fc4038a80 +size 426581 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.1.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.1.png deleted file mode 100644 index 552ef2fc5..000000000 --- a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.1.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:94a132c8800c74bc3f9e9299323c06166a4a351fa9f54ba1a3fdc680086a277d -size 417744 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.Layout.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.Layout.png new file mode 100644 index 000000000..8ff8ad79f --- /dev/null +++ b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPad-pseudo.Layout.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d79d725cd4976368ba4e3e0e374d4f48e2b2e7c60e28c90e8e1ca92fc4038a80 +size 426581 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.1.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.1.png deleted file mode 100644 index 39f5eaf28..000000000 --- a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.1.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:8a3a0c17c3a5069ac1e80799e4830bc7f8ae593e0d36b83b2b094fc9f90abfa0 -size 342251 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.Layout.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.Layout.png new file mode 100644 index 000000000..906366f53 --- /dev/null +++ b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-en-GB.Layout.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:1d8789df893dd7c2390ff8ecf6446b066cad2801dff8faac5b70bea6afffb040 +size 343065 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.1.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.1.png deleted file mode 100644 index 39f5eaf28..000000000 --- a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.1.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:8a3a0c17c3a5069ac1e80799e4830bc7f8ae593e0d36b83b2b094fc9f90abfa0 -size 342251 diff --git a/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.Layout.png b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.Layout.png new file mode 100644 index 000000000..906366f53 --- /dev/null +++ b/PreviewTests/__Snapshots__/PreviewTests/test_highlightedTimelineItemModifier-iPhone-15-pseudo.Layout.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:1d8789df893dd7c2390ff8ecf6446b066cad2801dff8faac5b70bea6afffb040 +size 343065 diff --git a/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPad-10th-generation-en-GB.UI.png b/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPad-10th-generation-en-GB.UI.png index bc0d72f1b..c8b762520 100644 --- a/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPad-10th-generation-en-GB.UI.png +++ b/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPad-10th-generation-en-GB.UI.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:85ccfdd71e9fa2e59b65b9731c8e0135e7e44349d7f3debdb5afcff0262576d0 -size 233181 +oid sha256:3f8f4642b8e6371079ca3e81ea55820264951dd7cbab8997eeb0b9087113360b +size 233502 diff --git a/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPhone-15-en-GB.UI.png b/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPhone-15-en-GB.UI.png index b39baa20f..e90f1e586 100644 --- a/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPhone-15-en-GB.UI.png +++ b/UITests/Sources/__Snapshots__/Application/roomLayoutHighlight-0-iPhone-15-en-GB.UI.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:79e3b444b2682f3697c1f27c898be7a3ed50d230c60d5c0fd33bfe9125056802 -size 331437 +oid sha256:1143957bde8bb8ff04948e2c3bbb3be3d22b8e65bfc6d9eeb550a498537b509a +size 331822 diff --git a/UnitTests/Sources/RoomScreenViewModelTests.swift b/UnitTests/Sources/RoomScreenViewModelTests.swift index b69ea06b4..772a7a8c0 100644 --- a/UnitTests/Sources/RoomScreenViewModelTests.swift +++ b/UnitTests/Sources/RoomScreenViewModelTests.swift @@ -171,8 +171,7 @@ class RoomScreenViewModelTests: XCTestCase { let viewModel = makeViewModel(timelineController: timelineController) XCTAssertEqual(timelineController.focusOnEventCallCount, 0) XCTAssertTrue(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEventID) - XCTAssertFalse(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEvent) // When focussing on an item that isn't loaded. let deferred = deferFulfillment(viewModel.context.$viewState) { !$0.timelineViewState.isLive } @@ -182,8 +181,7 @@ class RoomScreenViewModelTests: XCTestCase { // Then a new timeline should be loaded and the room focussed on that event. XCTAssertEqual(timelineController.focusOnEventCallCount, 1) XCTAssertFalse(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEventID, "t4") - XCTAssertTrue(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEvent, .init(eventID: "t4", appearance: .immediate)) } func testFocusLoadedItem() async throws { @@ -197,8 +195,7 @@ class RoomScreenViewModelTests: XCTestCase { let viewModel = makeViewModel(timelineController: timelineController) XCTAssertEqual(timelineController.focusOnEventCallCount, 0) XCTAssertTrue(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEventID) - XCTAssertFalse(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEvent) // When focussing on a loaded item. let deferred = deferFailure(viewModel.context.$viewState, timeout: 1) { !$0.timelineViewState.isLive } @@ -208,8 +205,7 @@ class RoomScreenViewModelTests: XCTestCase { // Then the timeline should remain live and the item should be focussed. XCTAssertEqual(timelineController.focusOnEventCallCount, 0) XCTAssertTrue(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEventID, "t1") - XCTAssertTrue(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEvent, .init(eventID: "t1", appearance: .animated)) } func testFocusLive() async throws { @@ -228,8 +224,7 @@ class RoomScreenViewModelTests: XCTestCase { XCTAssertEqual(timelineController.focusLiveCallCount, 0) XCTAssertFalse(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEventID, "t4") - XCTAssertTrue(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEvent, .init(eventID: "t4", appearance: .immediate)) // When switching back to a live timeline. deferred = deferFulfillment(viewModel.context.$viewState) { $0.timelineViewState.isLive } @@ -239,16 +234,14 @@ class RoomScreenViewModelTests: XCTestCase { // Then the timeline should switch back to being live and the event focus should be removed. XCTAssertEqual(timelineController.focusLiveCallCount, 1) XCTAssertTrue(viewModel.context.viewState.timelineViewState.isLive) - XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEventID) - XCTAssertFalse(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertNil(viewModel.context.viewState.timelineViewState.focussedEvent) } func testInitialFocusViewState() async throws { let timelineController = MockRoomTimelineController() let viewModel = makeViewModel(focussedEventID: "t10", timelineController: timelineController) - XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEventID, "t10") - XCTAssertTrue(viewModel.context.viewState.timelineViewState.focussedEventNeedsDisplay) + XCTAssertEqual(viewModel.context.viewState.timelineViewState.focussedEvent, .init(eventID: "t10", appearance: .immediate)) } // MARK: - Sending @@ -370,7 +363,7 @@ class RoomScreenViewModelTests: XCTestCase { // When a new message is received and marked as read. let newMessage = TextRoomTimelineItem(eventID: "t4") timelineController.timelineItems.append(newMessage) - timelineController.callbacks.send(.updatedTimelineItems) + timelineController.callbacks.send(.updatedTimelineItems(timelineItems: timelineController.timelineItems, isSwitchingTimelines: false)) try await Task.sleep(for: .milliseconds(100)) viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(newMessage.id)) diff --git a/changelog.d/2757.change b/changelog.d/2757.change new file mode 100644 index 000000000..5a49d98ce --- /dev/null +++ b/changelog.d/2757.change @@ -0,0 +1 @@ +Use an animation when scrolling to a nearby timeline item. \ No newline at end of file diff --git a/changelog.d/2764.bugfix b/changelog.d/2764.bugfix new file mode 100644 index 000000000..805f0b56d --- /dev/null +++ b/changelog.d/2764.bugfix @@ -0,0 +1 @@ +Add missing padding on the permalink highlight. \ No newline at end of file