From ee89f8846ff17fcdbb3194c36ea72b84bea02364 Mon Sep 17 00:00:00 2001 From: Mauro <34335419+Velin92@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:51:46 +0200 Subject: [PATCH] Fix for the media browser showing also images and videos in file mode (#5853) * fix for the bug that makes the images appear in the file list of the media browser * pr suggestions --- ElementX.xcodeproj/project.pbxproj | 4 ++ .../MediaEventsTimelineScreenModels.swift | 4 +- .../MediaEventsTimelineScreenViewModel.swift | 37 ++++++---- .../View/MediaEventsTimelineScreen.swift | 29 ++++---- ...iaEventsTimelineScreenViewModelTests.swift | 67 +++++++++++++++++++ 5 files changed, 111 insertions(+), 30 deletions(-) create mode 100644 UnitTests/Sources/MediaEventsTimelineScreenViewModelTests.swift diff --git a/ElementX.xcodeproj/project.pbxproj b/ElementX.xcodeproj/project.pbxproj index 9dfbe7f0c..63866f62b 100644 --- a/ElementX.xcodeproj/project.pbxproj +++ b/ElementX.xcodeproj/project.pbxproj @@ -686,6 +686,7 @@ 6FD8053301C5FEFA82D2F246 /* URLComponents.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2BFDCA5A09EE70BC17F2EFA7 /* URLComponents.swift */; }; 70394ECD2DCC70741538620D /* AccessibilityIdentifiers.swift in Sources */ = {isa = PBXBuildFile; fileRef = 04BB8DDE245ED86C489BA983 /* AccessibilityIdentifiers.swift */; }; 706289B086B0A6B0C211763F /* UITestsSignalling.swift in Sources */ = {isa = PBXBuildFile; fileRef = B7F0192CE2F891141A25B49F /* UITestsSignalling.swift */; }; + 7070E5CA0095CBEFAA7DE466 /* MediaEventsTimelineScreenViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F505E3D79018B6EFF953F647 /* MediaEventsTimelineScreenViewModelTests.swift */; }; 707E49BE07E8EB8A13C0EB1E /* SessionVerificationScreen.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5FACD034DB52525A3CEF2BDF /* SessionVerificationScreen.swift */; }; 708FC3184CCED825F0A36273 /* RoomThreadListServiceProxy.swift in Sources */ = {isa = PBXBuildFile; fileRef = CCE06F4A71FD46C9D8CD432E /* RoomThreadListServiceProxy.swift */; }; 709A9B52FC26B4CB86B8B020 /* EncryptedRoomTimelineView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2F06F70B9C433BAD4BC6B9F5 /* EncryptedRoomTimelineView.swift */; }; @@ -3067,6 +3068,7 @@ F46E441BA50705E6CEC89FE0 /* RoomSummaryProviderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RoomSummaryProviderTests.swift; sourceTree = ""; }; F48A2FA6814F824ABB4C07F3 /* RoomCallControlsToolbar.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RoomCallControlsToolbar.swift; sourceTree = ""; }; F4CEB4590CCF70F0E3C0B171 /* GeneratedAccessibilityTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GeneratedAccessibilityTests.swift; sourceTree = ""; }; + F505E3D79018B6EFF953F647 /* MediaEventsTimelineScreenViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MediaEventsTimelineScreenViewModelTests.swift; sourceTree = ""; }; F506C6ADB1E1DA6638078E11 /* UITests.xctest */ = {isa = PBXFileReference; includeInIndex = 0; lastKnownFileType = wrapper.cfbundle; path = UITests.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; F51D674A5B5F1FE1B878E20F /* nb */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = nb; path = nb.lproj/Localizable.strings; sourceTree = ""; }; F57C8022B8A871A1DCD1750A /* UserIndicatorToastView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserIndicatorToastView.swift; sourceTree = ""; }; @@ -5038,6 +5040,7 @@ 1D262A26713C18BB70C82CA5 /* MapTilerURLBuilderTests.swift */, 2216751513D9DC0A855D657C /* MatrixClientWellKnownTests.swift */, F31F59030205A6F65B057E1A /* MatrixEntityRegexTests.swift */, + F505E3D79018B6EFF953F647 /* MediaEventsTimelineScreenViewModelTests.swift */, 2D7A2C4A3A74F0D2FFE9356A /* MediaPlayerProviderTests.swift */, AE203026B9AD3DB412439866 /* MediaUploadingPreprocessorTests.swift */, 03FABD73FD8086EFAB699F42 /* MediaUploadPreviewScreenViewModelTests.swift */, @@ -8027,6 +8030,7 @@ 3BEBDCB42BABFA3B456FECA7 /* MapTilerURLBuilderTests.swift in Sources */, 60A02A6FCE475B7F0A070836 /* MatrixClientWellKnownTests.swift in Sources */, 2E43A3D221BE9587BC19C3F1 /* MatrixEntityRegexTests.swift in Sources */, + 7070E5CA0095CBEFAA7DE466 /* MediaEventsTimelineScreenViewModelTests.swift in Sources */, 4B978C09567387EF4366BD7A /* MediaLoaderTests.swift in Sources */, 3582056513A384F110EC8274 /* MediaPlayerProviderTests.swift in Sources */, 167D00CAA13FAFB822298021 /* MediaProviderTests.swift in Sources */, diff --git a/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenModels.swift b/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenModels.swift index de6706d5f..6528c3332 100644 --- a/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenModels.swift +++ b/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenModels.swift @@ -28,6 +28,7 @@ struct MediaEventsTimelineScreenViewState: BindableState { var isBackPaginating = false var shouldShowEmptyState = false + var screenMode: MediaEventsTimelineScreenMode var groups = [MediaEventsTimelineGroup]() var activeTimelineContext: TimelineViewModel.Context @@ -36,13 +37,12 @@ struct MediaEventsTimelineScreenViewState: BindableState { } struct MediaEventsTimelineScreenViewStateBindings { - var screenMode: MediaEventsTimelineScreenMode var mediaPreviewViewModel: TimelineMediaPreviewViewModel? var mediaPreviewSheetViewModel: TimelineMediaPreviewViewModel? } enum MediaEventsTimelineScreenViewAction { - case changedScreenMode + case changeScreenMode(MediaEventsTimelineScreenMode) case oldestItemDidAppear case oldestItemDidDisappear case tappedItem(item: RoomTimelineItemViewState) diff --git a/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenViewModel.swift b/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenViewModel.swift index 5d4b169ca..00a2509ce 100644 --- a/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenViewModel.swift +++ b/ElementX/Sources/Screens/MediaEventsTimelineScreen/MediaEventsTimelineScreenViewModel.swift @@ -21,7 +21,7 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType private var isOldestItemVisible = false private var activeTimelineViewModel: TimelineViewModelProtocol { - switch state.bindings.screenMode { + switch state.screenMode { case .media: mediaTimelineViewModel case .files: @@ -53,10 +53,13 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType case .files: filesTimelineViewModel.context } - super.init(initialViewState: .init(activeTimelineContext: activeTimelineContext, bindings: .init(screenMode: initialScreenMode)), mediaProvider: mediaProvider) + super.init(initialViewState: .init(screenMode: initialScreenMode, + activeTimelineContext: activeTimelineContext, + bindings: .init()), + mediaProvider: mediaProvider) mediaTimelineViewModel.context.$viewState.sink { [weak self] timelineViewState in - guard let self, state.bindings.screenMode == .media else { + guard let self, state.screenMode == .media else { return } @@ -81,7 +84,7 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType .store(in: &cancellables) filesTimelineViewModel.context.$viewState.sink { [weak self] timelineViewState in - guard let self, state.bindings.screenMode == .files else { + guard let self, state.screenMode == .files else { return } @@ -114,13 +117,8 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType MXLog.info("View model: received view action: \(viewAction)") switch viewAction { - case .changedScreenMode: - switch state.bindings.screenMode { - case .media: state.activeTimelineContext = mediaTimelineViewModel.context - case .files: state.activeTimelineContext = filesTimelineViewModel.context - } - - updateWithTimelineViewState(activeTimelineViewModel.context.viewState) + case .changeScreenMode(let screenMode): + changeScreenMode(to: screenMode) case .oldestItemDidAppear: isOldestItemVisible = true backPaginateIfNecessary(backPaginationState: activeTimelineViewModel.context.viewState.timelineState.paginationState.backward) @@ -140,6 +138,19 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType // MARK: - Private + private func changeScreenMode(to screenMode: MediaEventsTimelineScreenMode) { + guard screenMode != state.screenMode else { return } + + state.screenMode = screenMode + + switch screenMode { + case .media: state.activeTimelineContext = mediaTimelineViewModel.context + case .files: state.activeTimelineContext = filesTimelineViewModel.context + } + + updateWithTimelineViewState(activeTimelineViewModel.context.viewState) + } + private func displayMediaPreviewSheet(for item: EventBasedMessageTimelineItemProtocol) { let sheetModel = TimelineMediaPreviewViewModel(initialItem: item, timelineViewModel: activeTimelineViewModel, @@ -172,9 +183,9 @@ class MediaEventsTimelineScreenViewModel: MediaEventsTimelineScreenViewModelType timelineViewState.timelineState.itemViewStates.filter { itemViewState in switch itemViewState.type { case .image, .video: - state.bindings.screenMode == .media + state.screenMode == .media case .audio, .file, .voice: - state.bindings.screenMode == .files + state.screenMode == .files case .separator: true default: diff --git a/ElementX/Sources/Screens/MediaEventsTimelineScreen/View/MediaEventsTimelineScreen.swift b/ElementX/Sources/Screens/MediaEventsTimelineScreen/View/MediaEventsTimelineScreen.swift index 6ec019d8f..50c19e5c7 100644 --- a/ElementX/Sources/Screens/MediaEventsTimelineScreen/View/MediaEventsTimelineScreen.swift +++ b/ElementX/Sources/Screens/MediaEventsTimelineScreen/View/MediaEventsTimelineScreen.swift @@ -22,9 +22,6 @@ struct MediaEventsTimelineScreen: View { .toolbar { toolbar } .environmentObject(context.viewState.activeTimelineContext) .environment(\.timelineContext, context.viewState.activeTimelineContext) - .onChange(of: context.screenMode) { _, _ in - context.send(viewAction: .changedScreenMode) - } .timelineMediaPreview(viewModel: $context.mediaPreviewViewModel) .sheet(item: $context.mediaPreviewSheetViewModel) { sheet in if case let .media(media) = sheet.state.currentItem { @@ -59,7 +56,7 @@ struct MediaEventsTimelineScreen: View { private var scrollView: some View { ScrollView { Group { - switch context.viewState.bindings.screenMode { + switch context.viewState.screenMode { case .media: mediaContent case .files: @@ -82,7 +79,7 @@ struct MediaEventsTimelineScreen: View { Button { tappedItem(item) } label: { - viewForTimelineItem(item) + viewForTimelineItem(item, screenMode: .media) .scaleEffect(CGSize(width: -1, height: -1)) } .accessibleLongPress(named: L10n.actionOpenContextMenu) { @@ -111,11 +108,11 @@ struct MediaEventsTimelineScreen: View { Button { tappedItem(item) } label: { - viewForTimelineItem(item) + viewForTimelineItem(item, screenMode: .files) .scaleEffect(CGSize(width: 1, height: -1)) } .accessibilityRepresentation { - viewForTimelineItem(item) + viewForTimelineItem(item, screenMode: .files) } .accessibleLongPress(named: L10n.actionOpenContextMenu) { context.send(viewAction: .longPressedItem(item: item)) @@ -153,18 +150,19 @@ struct MediaEventsTimelineScreen: View { } } + /// The mode check makes sure that stale items from the other mode's layout are never rendered. @ViewBuilder - private func viewForTimelineItem(_ item: RoomTimelineItemViewState) -> some View { + private func viewForTimelineItem(_ item: RoomTimelineItemViewState, screenMode: MediaEventsTimelineScreenMode) -> some View { switch item.type { - case .image(let timelineItem): + case .image(let timelineItem) where screenMode == .media: ImageMediaEventsTimelineView(timelineItem: timelineItem) - case .video(let timelineItem): + case .video(let timelineItem) where screenMode == .media: VideoMediaEventsTimelineView(timelineItem: timelineItem) - case .file(let timelineItem): + case .file(let timelineItem) where screenMode == .files: FileMediaEventsTimelineView(timelineItem: timelineItem) - case .audio(let timelineItem): + case .audio(let timelineItem) where screenMode == .files: AudioMediaEventsTimelineView(timelineItem: timelineItem) - case .voice(let timelineItem): + case .voice(let timelineItem) where screenMode == .files: let defaultPlayerState = AudioPlayerState(id: .timelineItemIdentifier(timelineItem.id), title: L10n.commonVoiceMessage, duration: 0) let playerState = context.viewState.activeTimelineContext.viewState.audioPlayerStateProvider?(timelineItem.id) ?? defaultPlayerState VoiceMessageMediaEventsTimelineView(timelineItem: timelineItem, playerState: playerState) @@ -176,7 +174,7 @@ struct MediaEventsTimelineScreen: View { private var emptyState: some View { FullscreenDialog(topPadding: UIConstants.iconTopPaddingToNavigationBar, background: .gradient) { VStack(spacing: 16) { - switch context.screenMode { + switch context.viewState.screenMode { case .media: emptyMedia case .files: @@ -239,7 +237,8 @@ struct MediaEventsTimelineScreen: View { } private var screenModePicker: some View { - Picker("", selection: $context.screenMode) { + Picker("", selection: Binding(get: { context.viewState.screenMode }, + set: { context.send(viewAction: .changeScreenMode($0)) })) { Text(L10n.screenMediaBrowserListModeMedia) .padding() .tag(MediaEventsTimelineScreenMode.media) diff --git a/UnitTests/Sources/MediaEventsTimelineScreenViewModelTests.swift b/UnitTests/Sources/MediaEventsTimelineScreenViewModelTests.swift new file mode 100644 index 000000000..85d36f206 --- /dev/null +++ b/UnitTests/Sources/MediaEventsTimelineScreenViewModelTests.swift @@ -0,0 +1,67 @@ +// +// Copyright 2026 Element Creations Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. +// Please see LICENSE files in the repository root for full details. +// + +@testable import ElementX +import Testing + +@MainActor +struct MediaEventsTimelineScreenViewModelTests { + @Test + func changingScreenModeSwitchesTimelines() { + let mediaTimelineViewModel = makeTimelineViewModel() + let filesTimelineViewModel = makeTimelineViewModel() + + let viewModel = MediaEventsTimelineScreenViewModel(mediaTimelineViewModel: mediaTimelineViewModel, + filesTimelineViewModel: filesTimelineViewModel, + mediaProvider: MediaProviderMock(.init()), + userIndicatorController: UserIndicatorControllerMock(), + appMediator: AppMediatorMock(.init())) + let context = viewModel.context + + // Initially the media timeline is active, showing only images and videos. + #expect(context.viewState.screenMode == .media) + #expect(context.viewState.activeTimelineContext === mediaTimelineViewModel.context) + #expect(allItemsMatchScreenMode(.media, groups: context.viewState.groups)) + + // When switching the mode, the files timeline becomes active, + // showing only files, audio and voice messages. + context.send(viewAction: .changeScreenMode(.files)) + + #expect(context.viewState.screenMode == .files) + #expect(context.viewState.activeTimelineContext === filesTimelineViewModel.context) + #expect(allItemsMatchScreenMode(.files, groups: context.viewState.groups)) + + // And switching back restores the media timeline. + context.send(viewAction: .changeScreenMode(.media)) + + #expect(context.viewState.screenMode == .media) + #expect(context.viewState.activeTimelineContext === mediaTimelineViewModel.context) + #expect(allItemsMatchScreenMode(.media, groups: context.viewState.groups)) + } + + // MARK: - Helpers + + // Both timelines are given the same mixed chunk of items so that the mode based filtering is what's under test. + private func makeTimelineViewModel() -> TimelineViewModel { + let timelineController = TimelineControllerMock(.init(timelineKind: .media(.mediaFilesScreen), + timelineItems: [TimelineFixtures.separator] + TimelineFixtures.mediaChunk)) + return TimelineViewModel.mock(timelineKind: .media(.mediaFilesScreen), timelineController: timelineController) + } + + private func allItemsMatchScreenMode(_ screenMode: MediaEventsTimelineScreenMode, groups: [MediaEventsTimelineGroup]) -> Bool { + let items = groups.flatMap(\.items) + guard !items.isEmpty else { return false } + + return items.allSatisfy { item in + switch item.type { + case .image, .video: screenMode == .media + case .audio, .file, .voice: screenMode == .files + default: false + } + } + } +}