From e4736b12bbece83df7beaa37f7de5c84b4e66af8 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Mon, 23 Sep 2024 13:00:40 +0100 Subject: [PATCH] A bunch of random tweaks. (#3317) - Fix warning on single case enum. - Remove unused subjects. - Fix a lint warning. - Remove unused success value. - Fix warning about position of try await. - Add a note about the common tracing configuration. - Show an indicator when resolving a send failure fails. - Make sure the whole row is clickable in the GlobalSearchScreen. --- .../RoomFlowCoordinator.swift | 3 ++- .../Other/Logging/TracingConfiguration.swift | 4 +-- .../Screens/CallScreen/CallScreenModels.swift | 2 +- .../CallScreen/CallScreenViewModel.swift | 2 +- .../Screens/CallScreen/View/CallScreen.swift | 4 +-- .../View/GlobalSearchScreen.swift | 1 + ...fiedUserSendFailureScreenCoordinator.swift | 4 ++- ...rifiedUserSendFailureScreenViewModel.swift | 25 ++++++++++++++++--- ...ResolveVerifiedUserSendFailureScreen.swift | 6 +++-- .../Screens/Timeline/TimelineViewModel.swift | 6 ++--- .../Services/Timeline/TimelineProxy.swift | 6 ++--- ...dUserSendFailureScreenViewModelTests.swift | 18 +++++-------- .../Sources/RoomScreenViewModelTests.swift | 2 -- 13 files changed, 48 insertions(+), 35 deletions(-) diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 918fc155a..599315283 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -1366,7 +1366,8 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { private func presentResolveSendFailure(failure: TimelineItemSendFailure.VerifiedUser, itemID: TimelineItemIdentifier) { let coordinator = ResolveVerifiedUserSendFailureScreenCoordinator(parameters: .init(failure: failure, itemID: itemID, - roomProxy: roomProxy)) + roomProxy: roomProxy, + userIndicatorController: userIndicatorController)) coordinator.actionsPublisher.sink { [weak self] action in guard let self else { return } diff --git a/ElementX/Sources/Other/Logging/TracingConfiguration.swift b/ElementX/Sources/Other/Logging/TracingConfiguration.swift index 60e6c1b0e..3ff5250de 100644 --- a/ElementX/Sources/Other/Logging/TracingConfiguration.swift +++ b/ElementX/Sources/Other/Logging/TracingConfiguration.swift @@ -67,7 +67,7 @@ struct TracingConfiguration { } static let targets: OrderedDictionary = [ - .common: .info, + .common: .info, // Never set this lower than info - 3rd-party crates may start logging user data. .elementx: .info, .hyper: .warn, .matrix_sdk_ffi: .info, @@ -106,7 +106,7 @@ struct TracingConfiguration { let overrides = Self.targets.keys.reduce(into: [Target: LogLevel]()) { partialResult, target in // Keep the defaults here - let ignoredTargets: [Target] = [.common, + let ignoredTargets: [Target] = [.common, // Never remove common from the ignored targets (see above for more info). .hyper, .matrix_sdk_ffi, .matrix_sdk_oidc, diff --git a/ElementX/Sources/Screens/CallScreen/CallScreenModels.swift b/ElementX/Sources/Screens/CallScreen/CallScreenModels.swift index 30a77891d..bc312e49a 100644 --- a/ElementX/Sources/Screens/CallScreen/CallScreenModels.swift +++ b/ElementX/Sources/Screens/CallScreen/CallScreenModels.swift @@ -28,7 +28,7 @@ struct CallScreenViewState: BindableState { struct Bindings { var javaScriptMessageHandler: ((Any) -> Void)? var javaScriptEvaluator: ((String) async throws -> Any)? - var requestPictureInPictureHandler: (() async -> Result)? + var requestPictureInPictureHandler: (() async -> Result)? var alertInfo: AlertInfo? } diff --git a/ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift b/ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift index 335b56acf..abbc817bb 100644 --- a/ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift +++ b/ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift @@ -189,7 +189,7 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol } switch await requestPictureInPictureHandler() { - case .success(let controller): + case .success: actionsSubject.send(.pictureInPictureStarted) case .failure: actionsSubject.send(.dismiss) diff --git a/ElementX/Sources/Screens/CallScreen/View/CallScreen.swift b/ElementX/Sources/Screens/CallScreen/View/CallScreen.swift index 11467ed48..97e224ed9 100644 --- a/ElementX/Sources/Screens/CallScreen/View/CallScreen.swift +++ b/ElementX/Sources/Screens/CallScreen/View/CallScreen.swift @@ -200,7 +200,7 @@ private struct CallView: UIViewRepresentable { // MARK: - Picture in Picture - func requestPictureInPicture() async -> Result { + func requestPictureInPicture() async -> Result { guard let pictureInPictureController, pictureInPictureController.isPictureInPicturePossible, case .success(true) = await webViewCanEnterPictureInPicture() else { @@ -208,7 +208,7 @@ private struct CallView: UIViewRepresentable { } pictureInPictureController.startPictureInPicture() - return .success(pictureInPictureController) + return .success(()) } func stopPictureInPicture() { diff --git a/ElementX/Sources/Screens/GlobalSearchScreen/View/GlobalSearchScreen.swift b/ElementX/Sources/Screens/GlobalSearchScreen/View/GlobalSearchScreen.swift index ef478ddf2..25fbd5a2c 100644 --- a/ElementX/Sources/Screens/GlobalSearchScreen/View/GlobalSearchScreen.swift +++ b/ElementX/Sources/Screens/GlobalSearchScreen/View/GlobalSearchScreen.swift @@ -23,6 +23,7 @@ struct GlobalSearchScreen: View { GlobalSearchScreenListRow(room: room, context: context) .listRowBackground(backgroundColor(for: room)) .listRowInsets(.init()) + .contentShape(.rect) .onTapGesture { context.send(viewAction: .select(roomID: room.id)) } diff --git a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenCoordinator.swift b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenCoordinator.swift index d1c697b13..633138332 100644 --- a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenCoordinator.swift +++ b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenCoordinator.swift @@ -21,6 +21,7 @@ struct ResolveVerifiedUserSendFailureScreenCoordinatorParameters { let failure: TimelineItemSendFailure.VerifiedUser let itemID: TimelineItemIdentifier let roomProxy: JoinedRoomProxyProtocol + let userIndicatorController: UserIndicatorControllerProtocol } enum ResolveVerifiedUserSendFailureScreenCoordinatorAction { @@ -43,7 +44,8 @@ final class ResolveVerifiedUserSendFailureScreenCoordinator: CoordinatorProtocol viewModel = ResolveVerifiedUserSendFailureScreenViewModel(failure: parameters.failure, itemID: parameters.itemID, - roomProxy: parameters.roomProxy) + roomProxy: parameters.roomProxy, + userIndicatorController: parameters.userIndicatorController) } func start() { diff --git a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenViewModel.swift b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenViewModel.swift index 90868cddb..58b60a5aa 100644 --- a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenViewModel.swift +++ b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/ResolveVerifiedUserSendFailureScreenViewModel.swift @@ -26,12 +26,17 @@ class ResolveVerifiedUserSendFailureScreenViewModel: ResolveVerifiedUserSendFail private let roomProxy: JoinedRoomProxyProtocol private var members: [String: RoomMemberProxyProtocol] + private let userIndicatorController: UserIndicatorControllerProtocol + private let actionsSubject: PassthroughSubject = .init() var actionsPublisher: AnyPublisher { actionsSubject.eraseToAnyPublisher() } - init(failure: TimelineItemSendFailure.VerifiedUser, itemID: TimelineItemIdentifier, roomProxy: JoinedRoomProxyProtocol) { + init(failure: TimelineItemSendFailure.VerifiedUser, + itemID: TimelineItemIdentifier, + roomProxy: JoinedRoomProxyProtocol, + userIndicatorController: UserIndicatorControllerProtocol) { iterator = switch failure { case .hasUnsignedDevice(let devices): UnsignedDeviceFailureIterator(devices: devices) case .changedIdentity(let users): ChangedIdentityFailureIterator(users: users) @@ -40,6 +45,7 @@ class ResolveVerifiedUserSendFailureScreenViewModel: ResolveVerifiedUserSendFail self.failure = failure self.itemID = itemID self.roomProxy = roomProxy + self.userIndicatorController = userIndicatorController members = Dictionary(uniqueKeysWithValues: roomProxy.membersPublisher.value.map { ($0.userID, $0) }) @@ -77,7 +83,8 @@ class ResolveVerifiedUserSendFailureScreenViewModel: ResolveVerifiedUserSendFail } if case let .failure(error) = result { - #warning("Show the error?") + MXLog.error("Failed to resolve send failure: \(error)") + showFailureIndicator() return } @@ -94,10 +101,20 @@ class ResolveVerifiedUserSendFailureScreenViewModel: ResolveVerifiedUserSendFail switch await roomProxy.resend(itemID: itemID) { case .success: actionsSubject.send(.dismiss) - case .failure(let failure): - #warning("Show the error?") + case .failure(let error): + MXLog.error("Failed to resend message: \(error)") + showFailureIndicator() } } + + private static let failureIndicatorIdentifier = "\(ResolveVerifiedUserSendFailureScreenViewModel.self)-Failure" + + private func showFailureIndicator() { + ServiceLocator.shared.userIndicatorController.submitIndicator(UserIndicator(id: Self.failureIndicatorIdentifier, + type: .toast, + title: L10n.errorUnknown, + iconName: "xmark")) + } } // MARK: - Iterators diff --git a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/View/ResolveVerifiedUserSendFailureScreen.swift b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/View/ResolveVerifiedUserSendFailureScreen.swift index 4ac7666d4..2d2df80a6 100644 --- a/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/View/ResolveVerifiedUserSendFailureScreen.swift +++ b/ElementX/Sources/Screens/ResolveVerifiedUserSendFailureScreen/View/ResolveVerifiedUserSendFailureScreen.swift @@ -95,14 +95,16 @@ struct ResolveVerifiedUserSendFailureScreen_Previews: PreviewProvider, TestableP static func makeViewModel(failure: TimelineItemSendFailure.VerifiedUser) -> ResolveVerifiedUserSendFailureScreenViewModel { ResolveVerifiedUserSendFailureScreenViewModel(failure: failure, itemID: .random, - roomProxy: JoinedRoomProxyMock(.init())) + roomProxy: JoinedRoomProxyMock(.init()), + userIndicatorController: UserIndicatorControllerMock()) } } struct ResolveVerifiedUserSendFailureScreenSheet_Previews: PreviewProvider { static let viewModel = ResolveVerifiedUserSendFailureScreenViewModel(failure: .changedIdentity(users: ["@alice:matrix.org"]), itemID: .random, - roomProxy: JoinedRoomProxyMock(.init())) + roomProxy: JoinedRoomProxyMock(.init()), + userIndicatorController: UserIndicatorControllerMock()) static var previews: some View { Text("Hello") diff --git a/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift b/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift index c0557862b..45081efd5 100644 --- a/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift +++ b/ElementX/Sources/Screens/Timeline/TimelineViewModel.swift @@ -565,10 +565,8 @@ class TimelineViewModel: TimelineViewModelType, TimelineViewModelProtocol { } private func slashCommand(message: String) -> SlashCommand? { - for command in SlashCommand.allCases { - if message.starts(with: command.rawValue) { - return command - } + for command in SlashCommand.allCases where message.starts(with: command.rawValue) { + return command } return nil } diff --git a/ElementX/Sources/Services/Timeline/TimelineProxy.swift b/ElementX/Sources/Services/Timeline/TimelineProxy.swift index 3865ea1bb..a3bee43fd 100644 --- a/ElementX/Sources/Services/Timeline/TimelineProxy.swift +++ b/ElementX/Sources/Services/Timeline/TimelineProxy.swift @@ -130,9 +130,9 @@ final class TimelineProxy: TimelineProxyProtocol { subject.send(.paginating) do { - let timelineEndReached = try await switch direction { - case .backwards: timeline.paginateBackwards(numEvents: requestSize) - case .forwards: timeline.focusedPaginateForwards(numEvents: requestSize) + let timelineEndReached = switch direction { + case .backwards: try await timeline.paginateBackwards(numEvents: requestSize) + case .forwards: try await timeline.focusedPaginateForwards(numEvents: requestSize) } MXLog.info("Finished paginating \(direction.rawValue)") diff --git a/UnitTests/Sources/ResolveVerifiedUserSendFailureScreenViewModelTests.swift b/UnitTests/Sources/ResolveVerifiedUserSendFailureScreenViewModelTests.swift index cc3186c9f..91fc8e427 100644 --- a/UnitTests/Sources/ResolveVerifiedUserSendFailureScreenViewModelTests.swift +++ b/UnitTests/Sources/ResolveVerifiedUserSendFailureScreenViewModelTests.swift @@ -60,7 +60,10 @@ class ResolveVerifiedUserSendFailureScreenViewModelTests: XCTestCase { // MARK: Helpers private func makeViewModel(with failure: TimelineItemSendFailure.VerifiedUser) -> ResolveVerifiedUserSendFailureScreenViewModel { - ResolveVerifiedUserSendFailureScreenViewModel(failure: failure, itemID: .random, roomProxy: roomProxy) + ResolveVerifiedUserSendFailureScreenViewModel(failure: failure, + itemID: .random, + roomProxy: roomProxy, + userIndicatorController: UserIndicatorControllerMock()) } private func verifyResolving(userIDs: [String], assertStrings: Bool = true) async throws { @@ -73,7 +76,7 @@ class ResolveVerifiedUserSendFailureScreenViewModelTests: XCTestCase { } // When resolving the first failure. - let deferredFailure = deferFailure(viewModel.actionsPublisher, timeout: 1) { $0.isDismiss } + let deferredFailure = deferFailure(viewModel.actionsPublisher, timeout: 1) { $0 == .dismiss } context.send(viewAction: .resolveAndResend) // Then the sheet should remain open for the next failure. @@ -88,7 +91,7 @@ class ResolveVerifiedUserSendFailureScreenViewModelTests: XCTestCase { } // When resolving the final failure. - let deferred = deferFulfillment(viewModel.actionsPublisher) { $0.isDismiss } + let deferred = deferFulfillment(viewModel.actionsPublisher) { $0 == .dismiss } context.send(viewAction: .resolveAndResend) // Then the sheet should be dismissed. @@ -110,12 +113,3 @@ class ResolveVerifiedUserSendFailureScreenViewModelTests: XCTestCase { XCTAssertTrue(context.viewState.subtitle.contains(displayName)) } } - -private extension ResolveVerifiedUserSendFailureScreenViewModelAction { - var isDismiss: Bool { - switch self { - case .dismiss: true - default: false - } - } -} diff --git a/UnitTests/Sources/RoomScreenViewModelTests.swift b/UnitTests/Sources/RoomScreenViewModelTests.swift index b4c4f4a1c..a7998b4dc 100644 --- a/UnitTests/Sources/RoomScreenViewModelTests.swift +++ b/UnitTests/Sources/RoomScreenViewModelTests.swift @@ -102,8 +102,6 @@ class RoomScreenViewModelTests: XCTestCase { func testPinnedEventsBannerSelection() async throws { ServiceLocator.shared.settings.pinningEnabled = true - let timelineSubject = PassthroughSubject() - let updateSubject = PassthroughSubject() let roomProxyMock = JoinedRoomProxyMock(.init()) // setup a way to inject the mock of the pinned events timeline let pinnedTimelineMock = TimelineProxyMock()