From fb27efee10d7bd7af940589aacdbaaaa6d03ff8d Mon Sep 17 00:00:00 2001 From: Mauro <34335419+Velin92@users.noreply.github.com> Date: Wed, 8 Mar 2023 18:50:06 +0100 Subject: [PATCH] Cached avatar support (from Rust) (#633) * loading a cached avatar at first, and then actually doing the request so that the avatar can still be displayed without connection * improved the code * fix + changelog * policy on how to fetch the avatar url added * code improvements * addressed al PR comments * improved the error handling * improved the async task code * fixed a typo * fixed another typo --- Dangerfile.swift | 3 +- .../xcshareddata/swiftpm/Package.resolved | 4 +- .../HomeScreen/HomeScreenViewModel.swift | 10 +++-- .../Settings/SettingsScreenViewModel.swift | 8 ++-- .../Sources/Services/Client/ClientProxy.swift | 44 ++++++++++++++----- .../Services/Client/ClientProxyProtocol.swift | 9 ++-- .../Services/Client/MockClientProxy.swift | 8 ++-- .../MediaProvider/MediaLoaderTests.swift | 4 +- changelog.d/550.change | 1 + project.yml | 2 +- 10 files changed, 62 insertions(+), 31 deletions(-) create mode 100644 changelog.d/550.change diff --git a/Dangerfile.swift b/Dangerfile.swift index d6dc10358..d53cdbcda 100644 --- a/Dangerfile.swift +++ b/Dangerfile.swift @@ -49,7 +49,8 @@ let allowList = ["stefanceriu", "manuroe", "gileluard", "phlniji", - "aringenbach"] + "aringenbach", + "Velin92"] let requiresSignOff = !allowList.contains(where: { $0.caseInsensitiveCompare(danger.github.pullRequest.user.login) == .orderedSame diff --git a/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index d97927b30..009c17692 100644 --- a/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -95,8 +95,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/matrix-org/matrix-rust-components-swift", "state" : { - "revision" : "32645162ea39badc586d610d2ada6bbc52f398ba", - "version" : "1.0.43-alpha" + "revision" : "8d7ace5140d0f024b9a03939cdff29c837dc5645", + "version" : "1.0.45-alpha" } }, { diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift index 0f2289e94..915ef3529 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift @@ -30,7 +30,7 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol var callback: ((HomeScreenViewModelAction) -> Void)? - // swiftlint:disable:next function_body_length cyclomatic_complexity + // swiftlint:disable:next function_body_length init(userSession: UserSessionProtocol, attributedStringBuilder: AttributedStringBuilderProtocol) { self.userSession = userSession self.attributedStringBuilder = attributedStringBuilder @@ -54,6 +54,10 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol } } .store(in: &cancellables) + + userSession.clientProxy.avatarURLPublisher + .weakAssign(to: \.state.userAvatarURL, on: self) + .store(in: &cancellables) guard let visibleRoomsSummaryProvider, let allRoomsSummaryProvider else { MXLog.error("Room summary provider unavailable") @@ -91,9 +95,7 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol // Delay user profile detail loading until after the initial room list loads if roomListMode == .rooms { Task { - if case let .success(url) = await userSession.clientProxy.loadUserAvatarURL() { - self.state.userAvatarURL = url - } + await userSession.clientProxy.loadUserAvatarURL() } Task { diff --git a/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift b/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift index 38aad95ac..862b39c20 100644 --- a/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift +++ b/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift @@ -43,11 +43,13 @@ class SettingsScreenViewModel: SettingsScreenViewModelType, SettingsScreenViewMo ServiceLocator.shared.settings.$timelineStyle .weakAssign(to: \.state.bindings.timelineStyle, on: self) .store(in: &cancellables) + + userSession.clientProxy.avatarURLPublisher + .weakAssign(to: \.state.userAvatarURL, on: self) + .store(in: &cancellables) Task { - if case let .success(userAvatarURL) = await userSession.clientProxy.loadUserAvatarURL() { - state.userAvatarURL = userAvatarURL - } + await userSession.clientProxy.loadUserAvatarURL() } Task { diff --git a/ElementX/Sources/Services/Client/ClientProxy.swift b/ElementX/Sources/Services/Client/ClientProxy.swift index 944844b95..88c407bcc 100644 --- a/ElementX/Sources/Services/Client/ClientProxy.swift +++ b/ElementX/Sources/Services/Client/ClientProxy.swift @@ -58,6 +58,15 @@ class ClientProxy: ClientProxyProtocol { var allRoomsSlidingSyncView: SlidingSyncList? var allRoomsSummaryProvider: RoomSummaryProviderProtocol? + + private var loadCachedAvatarURLTask: Task? + private let avatarURLSubject = CurrentValueSubject(nil) + var avatarURLPublisher: AnyPublisher { + avatarURLSubject + .removeDuplicates() + .receive(on: DispatchQueue.main) + .eraseToAnyPublisher() + } private var cancellables = Set() private var visibleRoomsViewProxyStateObservationToken: AnyCancellable? @@ -82,6 +91,8 @@ class ClientProxy: ClientProxyProtocol { client.setDelegate(delegate: WeakClientProxyWrapper(clientProxy: self)) configureSlidingSync() + + loadUserAvatarURLFromCache() } var userID: String { @@ -147,6 +158,7 @@ class ClientProxy: ClientProxyProtocol { func loadUserDisplayName() async -> Result { await Task.dispatch(on: clientQueue) { do { + self.loadCachedAvatarURLTask?.cancel() let displayName = try self.client.displayName() return .success(displayName) } catch { @@ -154,24 +166,19 @@ class ClientProxy: ClientProxyProtocol { } } } - - func loadUserAvatarURL() async -> Result { + + func loadUserAvatarURL() async { await Task.dispatch(on: clientQueue) { do { let urlString = try self.client.avatarUrl() - - guard let url = URL(string: urlString) else { - MXLog.error("Invalid avatar URL string: \(String(describing: urlString))") - return .failure(.failedRetrievingAvatarURL) - } - - return .success(url) + self.avatarURLSubject.value = urlString.flatMap(URL.init) } catch { - return .failure(.failedRetrievingAvatarURL) + MXLog.error("Failed fetching the user avatar url: \(error)") + return } } } - + func accountDataEvent(type: String) async -> Result where Content: Decodable { await Task.dispatch(on: clientQueue) { .failure(.failedRetrievingAccountData) @@ -232,6 +239,21 @@ class ClientProxy: ClientProxyProtocol { } // MARK: Private + + private func loadUserAvatarURLFromCache() { + loadCachedAvatarURLTask = Task { + let urlString = await Task.dispatch(on: clientQueue) { + do { + return try self.client.cachedAvatarUrl() + } catch { + MXLog.error("Failed to look for the avatar url in the cache: \(error)") + return nil + } + } + guard !Task.isCancelled else { return } + self.avatarURLSubject.value = urlString.flatMap(URL.init) + } + } private func restartSync() { MXLog.info("Restarting sync") diff --git a/ElementX/Sources/Services/Client/ClientProxyProtocol.swift b/ElementX/Sources/Services/Client/ClientProxyProtocol.swift index 96709289d..a31f82db9 100644 --- a/ElementX/Sources/Services/Client/ClientProxyProtocol.swift +++ b/ElementX/Sources/Services/Client/ClientProxyProtocol.swift @@ -24,7 +24,6 @@ enum ClientProxyCallback { } enum ClientProxyError: Error { - case failedRetrievingAvatarURL case failedRetrievingDisplayName case failedRetrievingAccountData case failedSettingAccountData @@ -72,6 +71,8 @@ protocol ClientProxyProtocol: AnyObject, MediaLoaderProtocol { var homeserver: String { get } + var avatarURLPublisher: AnyPublisher { get } + var restorationToken: RestorationToken? { get } var visibleRoomsSummaryProvider: RoomSummaryProviderProtocol? { get } @@ -85,9 +86,9 @@ protocol ClientProxyProtocol: AnyObject, MediaLoaderProtocol { func roomForIdentifier(_ identifier: String) async -> RoomProxyProtocol? func loadUserDisplayName() async -> Result - - func loadUserAvatarURL() async -> Result - + + func loadUserAvatarURL() async + func accountDataEvent(type: String) async -> Result func setAccountData(content: Content, type: String) async -> Result diff --git a/ElementX/Sources/Services/Client/MockClientProxy.swift b/ElementX/Sources/Services/Client/MockClientProxy.swift index dfb911ff9..8039e2a08 100644 --- a/ElementX/Sources/Services/Client/MockClientProxy.swift +++ b/ElementX/Sources/Services/Client/MockClientProxy.swift @@ -29,11 +29,15 @@ class MockClientProxy: ClientProxyProtocol { var visibleRoomsSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider() var allRoomsSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider() + + var avatarURLPublisher: AnyPublisher { Empty().eraseToAnyPublisher() } internal init(userID: String, roomSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider()) { self.userID = userID visibleRoomsSummaryProvider = roomSummaryProvider } + + func loadUserAvatarURL() async { } func startSync() { } @@ -56,10 +60,6 @@ class MockClientProxy: ClientProxyProtocol { .success("User display name") } - func loadUserAvatarURL() async -> Result { - .failure(.failedRetrievingAvatarURL) - } - func accountDataEvent(type: String) async -> Result where Content: Decodable { .failure(.failedRetrievingAccountData) } diff --git a/UnitTests/Sources/MediaProvider/MediaLoaderTests.swift b/UnitTests/Sources/MediaProvider/MediaLoaderTests.swift index 6e3af9aeb..89822d3a0 100644 --- a/UnitTests/Sources/MediaProvider/MediaLoaderTests.swift +++ b/UnitTests/Sources/MediaProvider/MediaLoaderTests.swift @@ -83,7 +83,9 @@ private class MockMediaLoadingClient: ClientProtocol { func setDisplayName(name: String) throws { } - func avatarUrl() throws -> String { fatalError() } + func avatarUrl() throws -> String? { fatalError() } + + func cachedAvatarUrl() throws -> String? { fatalError() } func deviceId() throws -> String { fatalError() } diff --git a/changelog.d/550.change b/changelog.d/550.change new file mode 100644 index 000000000..f79ab9c59 --- /dev/null +++ b/changelog.d/550.change @@ -0,0 +1 @@ +Avatar url is now cached on the rust side. \ No newline at end of file diff --git a/project.yml b/project.yml index b7a5fc815..2b072bc03 100644 --- a/project.yml +++ b/project.yml @@ -40,7 +40,7 @@ include: packages: MatrixRustSDK: url: https://github.com/matrix-org/matrix-rust-components-swift - exactVersion: 1.0.43-alpha + exactVersion: 1.0.45-alpha # path: ../matrix-rust-sdk DesignKit: path: DesignKit