From 15627e45127034335abb55a9b9e39f964f9e3edb Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:40:43 +0100 Subject: [PATCH] Minor quicklook fixes. (#6112) * Only show a spinner when opening the media viewer is slow * Keep a sent local echo's page in the media viewer * Fix the caption visibility. --------- Co-authored-by: Stefan Ceriu Co-authored-by: Matthew Hodgson --- .../TimelineMediaPreviewDataSource.swift | 22 ++++++-- .../View/TimelineMediaPreviewController.swift | 56 +++++++++++++++++-- .../Screens/Timeline/TimelineViewModel.swift | 9 ++- .../TimelineMediaPreviewDataSourceTests.swift | 35 ++++++++++++ 4 files changed, 112 insertions(+), 10 deletions(-) diff --git a/ElementX/Sources/Screens/FilePreviewScreen/TimelineMediaPreviewDataSource.swift b/ElementX/Sources/Screens/FilePreviewScreen/TimelineMediaPreviewDataSource.swift index 4fd3160df..d2ef74049 100644 --- a/ElementX/Sources/Screens/FilePreviewScreen/TimelineMediaPreviewDataSource.swift +++ b/ElementX/Sources/Screens/FilePreviewScreen/TimelineMediaPreviewDataSource.swift @@ -112,8 +112,9 @@ class TimelineMediaPreviewDataSource: NSObject, QLPreviewControllerDataSource { .flatMap { $0.previewableMedia(allowedGalleryItemTypes: allowedGalleryItemTypes) } .map { newItem in // If an item already exists use that instead to preserve the file handle, download error etc. - if let oldItem = previewItems.first(where: { $0.id == newItem.id }) { + if let oldItem = previewItems.first(where: { $0.id == newItem.id || $0.isLocalEcho(of: newItem) }) { oldItem.content = newItem.content + oldItem.id = newItem.id // Make sure a local echo's transaction ID → event ID. return oldItem } @@ -237,9 +238,22 @@ enum TimelineMediaPreviewItem: Equatable { var downloadError: Error? - /// A stable identifier that's unique per preview item — including individual gallery - /// attachments that would otherwise share their parent event's ID. - let id: MediaPreviewItemID + /// A stable identifier that's unique per preview item, even for gallery attachments that would + /// otherwise share their parent event's ID. Not stable for local echoes however, those will + /// transition from transaction ID to event ID once sent. + fileprivate(set) var id: MediaPreviewItemID + + /// Whether this is the local echo that `sentItem` is the remote echo of. Sending swaps the + /// transaction ID for an event ID but the unique ID remains stable. + fileprivate func isLocalEcho(of sentItem: Media) -> Bool { + guard timelineItem.id.transactionID != nil, sentItem.timelineItem.id.eventID != nil, + timelineItem.id.uniqueID == sentItem.timelineItem.id.uniqueID else { return false } + switch (content, sentItem.content) { + case (.timelineItem, .timelineItem): return true + case (.galleryItem(_, let item), .galleryItem(_, let sentGalleryItem)): return item.id.mediaIndex == sentGalleryItem.id.mediaIndex + default: return false + } + } init(timelineItem: EventBasedMessageTimelineItemProtocol) { content = .timelineItem(timelineItem) diff --git a/ElementX/Sources/Screens/FilePreviewScreen/View/TimelineMediaPreviewController.swift b/ElementX/Sources/Screens/FilePreviewScreen/View/TimelineMediaPreviewController.swift index e9240438b..91fdabe04 100644 --- a/ElementX/Sources/Screens/FilePreviewScreen/View/TimelineMediaPreviewController.swift +++ b/ElementX/Sources/Screens/FilePreviewScreen/View/TimelineMediaPreviewController.swift @@ -73,6 +73,11 @@ class TimelineMediaPreviewController: QLPreviewController { super.init(nibName: nil, bundle: nil) + headerHostingController.rootView.onMoveToWindow = { [weak self] in + guard #available(iOS 26, *) else { return } + self?.updateCaptionVisibility() + } + view.addSubview(captionView) // Constraints added later as the toolbar isn't available yet. @@ -128,11 +133,9 @@ class TimelineMediaPreviewController: QLPreviewController { super.viewWillLayoutSubviews() if let bottomBarItemsContainer { - // Using the toolbar's visibility doesn't work so check its frame. - captionView.isHidden = if #available(iOS 26, *) { - navigationBar?.topItem?.leftBarButtonItem?.frame(in: view) == nil - } else { - bottomBarItemsContainer.frame.minY >= view.frame.maxY + if #unavailable(iOS 26) { + // Using the toolbar's visibility doesn't work so check its frame. + captionView.isHidden = bottomBarItemsContainer.frame.minY >= view.frame.maxY } if captionView.constraints.isEmpty { @@ -176,6 +179,15 @@ class TimelineMediaPreviewController: QLPreviewController { barButtonTimer = nil } + @available(iOS 26, *) + private func updateCaptionVisibility() { + // The caption should be hidden alongside the header. + let isHeaderHidden = headerHostingController.view.window == nil + if captionView.isHidden != isHeaderHidden { + captionView.isHidden = isHeaderHidden + } + } + private func updateBarButtons() { guard let topItem = navigationBar?.topItem else { return } @@ -306,11 +318,20 @@ class TimelineMediaPreviewController: QLPreviewController { private struct HeaderView: View { @ObservedObject var context: TimelineMediaPreviewViewModel.Context + /// Called whenever the header is added or removed from a window. + var onMoveToWindow: () -> Void = { } + private var currentItem: TimelineMediaPreviewItem { context.viewState.currentItem } var body: some View { + content + .background(WindowTrackingView(onMoveToWindow: onMoveToWindow)) + } + + @ViewBuilder + private var content: some View { if let mediaItem = currentItem.mediaItem { VStack(spacing: 0) { Text(mediaItem.sender.displayName ?? mediaItem.sender.id) @@ -475,6 +496,31 @@ private struct DownloadIndicatorView: View { // MARK: - Helpers +/// An invisible view that reports when it is added to or removed from a window. +private struct WindowTrackingView: UIViewRepresentable { + let onMoveToWindow: () -> Void + + func makeUIView(context: Context) -> TrackingView { + let view = TrackingView() + view.isUserInteractionEnabled = false + view.onMoveToWindow = onMoveToWindow + return view + } + + func updateUIView(_ uiView: TrackingView, context: Context) { + uiView.onMoveToWindow = onMoveToWindow + } + + class TrackingView: UIView { + var onMoveToWindow: (() -> Void)? + + override func didMoveToWindow() { + super.didMoveToWindow() + onMoveToWindow?() + } + } +} + private extension UIView { func firstScrollView() -> UIScrollView? { for view in subviews { diff --git a/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift b/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift index 933b0be95..b9b370a8a 100644 --- a/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift +++ b/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift @@ -708,8 +708,15 @@ class TimelineViewModel: TimelineViewModelType, TimelineViewModelProtocol { } private func handleMediaTapped(with itemID: TimelineItemIdentifier, galleryIndex: Int? = nil) async { - state.showLoading = true + // Building the media timeline takes ~100ms from the event cache, however its possible that + // a focussed timeline may hit /context so we need to show a spinner when it's actually slow. + let spinner = Task { + try? await Task.sleep(for: .milliseconds(300)) + guard !Task.isCancelled else { return } + state.showLoading = true + } let action = await timelineInteractionHandler.processItemTap(itemID) + spinner.cancel() switch action { case .displayMediaPreview(let item, let timelineViewModelKind): diff --git a/UnitTests/Sources/TimelineMediaPreviewDataSourceTests.swift b/UnitTests/Sources/TimelineMediaPreviewDataSourceTests.swift index 6a1871691..f31b5e085 100644 --- a/UnitTests/Sources/TimelineMediaPreviewDataSourceTests.swift +++ b/UnitTests/Sources/TimelineMediaPreviewDataSourceTests.swift @@ -343,6 +343,41 @@ struct TimelineMediaPreviewDataSourceTests { .filter { !($0 is GalleryRoomTimelineItem) } // Galleries are previewed through their own data source. } + @Test + func sentLocalEchoKeepsItsPage() async throws { + // Given a viewer opened on an upload in flight: a local echo with a transaction ID. + let uniqueID = TimelineItemIdentifier.UniqueID(UUID().uuidString) + func makeUpload(id: TimelineItemIdentifier.EventOrTransactionID) -> ImageRoomTimelineItem { + ImageRoomTimelineItem(id: .event(uniqueID: uniqueID, eventOrTransactionID: id), + timestamp: .mock, + isOutgoing: true, + isEditable: false, + canBeRepliedTo: false, + sender: .init(id: "Alice"), + content: .init(filename: "upload.jpg", imageInfo: .mockImage, thumbnailInfo: nil, blurhash: nil)) + } + let localEcho = makeUpload(id: .transactionID("t1")) + var items = newChunk() + items.append(localEcho) + let dataSource = TimelineMediaPreviewDataSource(itemViewStates: items.map { RoomTimelineItemViewState(item: $0, groupStyle: .single) }, + initialItem: localEcho, + paginationState: .initial) + let page = try #require(dataSource.currentItem.mediaItem) + #expect(page.id == .timelineItem(.transactionID("t1"))) + + // When the upload completes and the timeline swaps the echo's transaction ID for its event ID. + items[items.count - 1] = makeUpload(id: .eventID("$e1")) + let deferred = deferFailure(dataSource.previewItemsPaginationPublisher, timeout: .seconds(1)) { _ in true } + dataSource.updatePreviewItems(itemViewStates: items.map { RoomTimelineItemViewState(item: $0, groupStyle: .single) }) + try await deferred.fulfill() + + // Then the same page carries on under the new ID (its file came from the local media cache), nothing rebuilt. + #expect(dataSource.currentItem.mediaItem === page) + #expect(page.id == .timelineItem(.eventID("$e1"))) + #expect(dataSource.previewItems.map(\.id).last == .timelineItem(.eventID("$e1"))) + #expect(dataSource.previewItems.count == items.count) + } + @discardableResult private func assertInitialDataSource() throws -> TimelineMediaPreviewDataSource { // Given a data source built with the initial items.