From 6850633a52e6a5aa536afc803a8e144ef577a3e9 Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Wed, 1 Feb 2023 10:14:40 +0100 Subject: [PATCH] Async-await refactor --- .../MXNotificationSettingsService.swift | 70 +++++----- .../MockNotificationSettingsService.swift | 8 +- .../NotificationSettingsServiceType.swift | 13 +- .../NotificationSettingsViewModelTests.swift | 122 ++++++------------ .../View/NotificationSettings.swift | 4 +- .../NotificationSettingsViewModel.swift | 82 ++++++------ 6 files changed, 127 insertions(+), 172 deletions(-) diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Service/MatrixSDK/MXNotificationSettingsService.swift b/RiotSwiftUI/Modules/Settings/Notifications/Service/MatrixSDK/MXNotificationSettingsService.swift index f5e4c457a..6ccd54c1a 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Service/MatrixSDK/MXNotificationSettingsService.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Service/MatrixSDK/MXNotificationSettingsService.swift @@ -44,7 +44,9 @@ class MXNotificationSettingsService: NotificationSettingsServiceType { // Observe future updates to content rules rulesUpdated - .compactMap { _ in self.session.notificationCenter.rules.global.content as? [MXPushRule] } + .compactMap { [weak self] _ in + self?.session.notificationCenter.rules.global.content as? [MXPushRule] + } .assign(to: &$contentRules) // Set initial value of rules @@ -53,7 +55,9 @@ class MXNotificationSettingsService: NotificationSettingsServiceType { } // Observe future updates to rules rulesUpdated - .compactMap { _ in self.session.notificationCenter.flatRules as? [MXPushRule] } + .compactMap { [weak self] _ in + self?.session.notificationCenter.flatRules as? [MXPushRule] + } .assign(to: &$rules) } @@ -72,52 +76,50 @@ class MXNotificationSettingsService: NotificationSettingsServiceType { func updatePushRuleActions(for ruleId: String, enabled: Bool, - actions: NotificationActions?, - completion: ((Result) -> Void)?) { + actions: NotificationActions?) async throws { guard let rule = session.notificationCenter.rule(byId: ruleId) else { - completion?(.success) return } guard let actions = actions else { - enableRule(rule: rule, enabled: enabled, completion: completion) + try await session.notificationCenter.enableRule(pushRule: rule, isEnabled: enabled) return } // Updating the actions before enabling the rule allows the homeserver to triggers just one sync update - session.notificationCenter.updatePushRuleActions(ruleId, + try await session.notificationCenter.updatePushRuleActions(ruleId, kind: rule.kind, notify: actions.notify, soundName: actions.sound, - highlight: actions.highlight) { [weak self] error in - switch error.result { - case .success: - self?.enableRule(rule: rule, enabled: enabled, completion: completion) - case .failure: - completion?(error.result) + highlight: actions.highlight) + + try await session.notificationCenter.enableRule(pushRule: rule, isEnabled: enabled) + } +} + +private extension MXNotificationCenter { + func enableRule(pushRule: MXPushRule, isEnabled: Bool) async throws { + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in + enableRule(pushRule, isEnabled: isEnabled) { error in + if let error = error { + continuation.resume(with: .failure(error)) + } else { + continuation.resume() + } + } + } + } + + func updatePushRuleActions(ruleId: String, kind: __MXPushRuleKind, notify: Bool, soundName: String, highlight: Bool) async throws { + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in + updatePushRuleActions(ruleId, kind: kind, notify: notify, soundName: soundName, highlight: highlight) { error in + if let error = error { + continuation.resume(with: .failure(error)) + } else { + continuation.resume() + } } } } } - -private extension MXNotificationSettingsService { - func enableRule(rule: MXPushRule, enabled: Bool, completion: ((Result) -> Void)?) { - session.notificationCenter.enableRule(rule, isEnabled: enabled) { error in - completion?(error.result) - } - } -} - -private extension Result where Success == Void { - static var success: Self { - .success(()) - } -} - -private extension Optional where Wrapped == Error { - var result: Result { - map { .failure($0) } ?? .success - } -} - diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Service/Mock/MockNotificationSettingsService.swift b/RiotSwiftUI/Modules/Settings/Notifications/Service/Mock/MockNotificationSettingsService.swift index 7d74f5288..ea4bd640c 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Service/Mock/MockNotificationSettingsService.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Service/Mock/MockNotificationSettingsService.swift @@ -44,15 +44,11 @@ class MockNotificationSettingsService: NotificationSettingsServiceType, Observab keywords.remove(keyword) } - func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?, completion: ((Result) -> Void)?) { + func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?) async throws { guard let ruleIndex = rules.firstIndex(where: { $0.ruleId == ruleId }) else { - completion?(.success(())) return } - rules[ruleIndex] = MockNotificationPushRule(ruleId: ruleId, - enabled: enabled, - actions: actions) - completion?(.success(())) + rules[ruleIndex] = MockNotificationPushRule(ruleId: ruleId, enabled: enabled, actions: actions) } } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Service/NotificationSettingsServiceType.swift b/RiotSwiftUI/Modules/Settings/Notifications/Service/NotificationSettingsServiceType.swift index 9e6419d23..5b06dfb6d 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Service/NotificationSettingsServiceType.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Service/NotificationSettingsServiceType.swift @@ -40,16 +40,5 @@ protocol NotificationSettingsServiceType { /// - ruleId: The id of the rule. /// - enabled: Whether the rule should be enabled or disabled. /// - actions: The actions to update with. - /// - completion: The completion of the operation. - func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?, completion: ((Result) -> Void)?) -} - -extension NotificationSettingsServiceType { - func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?) async throws { - try await withCheckedThrowingContinuation { continuation in - updatePushRuleActions(for: ruleId, enabled: enabled, actions: actions) { result in - continuation.resume(with: result) - } - } - } + func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?) async throws } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift index ea698d9e0..a24e28733 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift @@ -32,113 +32,71 @@ final class NotificationSettingsViewModelTests: XCTestCase { XCTAssertTrue(viewModel.viewState.selectionState.values.allSatisfy { $0 }) } - func testUpdateRule() throws { + func testUpdateRule() async { viewModel = .init(notificationSettingsService: notificationService, ruleIds: .default) notificationService.rules = [MockNotificationPushRule].default - viewModel.update(ruleID: .encrypted, isChecked: false) + await viewModel.update(ruleID: .encrypted, isChecked: false) XCTAssertEqual(viewModel.viewState.selectionState.count, 4) XCTAssertEqual(viewModel.viewState.selectionState[.encrypted], false) } - func testUpdateOneToOneRuleAlsoUpdatesPollRules() { - let expectation = expectation(description: #function) + func testUpdateOneToOneRuleAlsoUpdatesPollRules() async { setupWithPollRules() - viewModel.update(ruleID: .oneToOneRoom, isChecked: false) { result in - guard case .success = result else { - XCTFail() - return - } - - XCTAssertEqual(self.viewModel.viewState.selectionState.count, 8) - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOneRoom], false) - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOnePollStart], false) - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOnePollEnd], false) - - // unrelated poll rules stay the same - XCTAssertEqual(self.viewModel.viewState.selectionState[.allOtherMessages], true) - XCTAssertEqual(self.viewModel.viewState.selectionState[.pollStart], true) - XCTAssertEqual(self.viewModel.viewState.selectionState[.pollEnd], true) - - expectation.fulfill() - } + await viewModel.update(ruleID: .oneToOneRoom, isChecked: false) + + XCTAssertEqual(viewModel.viewState.selectionState.count, 8) + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOneRoom], false) + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOnePollStart], false) + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOnePollEnd], false) - waitForExpectations(timeout: 1.0) + // unrelated poll rules stay the same + XCTAssertEqual(viewModel.viewState.selectionState[.allOtherMessages], true) + XCTAssertEqual(viewModel.viewState.selectionState[.pollStart], true) + XCTAssertEqual(viewModel.viewState.selectionState[.pollEnd], true) } - func testUpdateMessageRuleAlsoUpdatesPollRules() { - let expectation = expectation(description: #function) + func testUpdateMessageRuleAlsoUpdatesPollRules() async { setupWithPollRules() - viewModel.update(ruleID: .allOtherMessages, isChecked: false) { result in - guard case .success = result else { - XCTFail() - return - } - - XCTAssertEqual(self.viewModel.viewState.selectionState.count, 8) - XCTAssertEqual(self.viewModel.viewState.selectionState[.allOtherMessages], false) - XCTAssertEqual(self.viewModel.viewState.selectionState[.pollStart], false) - XCTAssertEqual(self.viewModel.viewState.selectionState[.pollEnd], false) - - // unrelated poll rules stay the same - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOneRoom], true) - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOnePollStart], true) - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOnePollEnd], true) - - expectation.fulfill() - } + await viewModel.update(ruleID: .allOtherMessages, isChecked: false) + XCTAssertEqual(viewModel.viewState.selectionState.count, 8) + XCTAssertEqual(viewModel.viewState.selectionState[.allOtherMessages], false) + XCTAssertEqual(viewModel.viewState.selectionState[.pollStart], false) + XCTAssertEqual(viewModel.viewState.selectionState[.pollEnd], false) - waitForExpectations(timeout: 1.0) + // unrelated poll rules stay the same + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOneRoom], true) + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOnePollStart], true) + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOnePollEnd], true) } - func testMismatchingRulesAreHandled() { - let expectation = expectation(description: #function) + func testMismatchingRulesAreHandled() async { setupWithPollRules() - viewModel.update(ruleID: .allOtherMessages, isChecked: false) { result in - guard case .success = result else { - XCTFail() - return - } - - // simulating a "mismatch" on the poll started rule - self.viewModel.update(ruleID: .pollStart, isChecked: true) - - XCTAssertEqual(self.viewModel.viewState.selectionState.count, 8) - - // The other messages rule ui flag should match the loudest related poll rule - XCTAssertEqual(self.viewModel.viewState.selectionState[.allOtherMessages], true) - - expectation.fulfill() - } + await viewModel.update(ruleID: .allOtherMessages, isChecked: false) - waitForExpectations(timeout: 1.0) + // simulating a "mismatch" on the poll started rule + await viewModel.update(ruleID: .pollStart, isChecked: true) + + XCTAssertEqual(viewModel.viewState.selectionState.count, 8) + + // The other messages rule ui flag should match the loudest related poll rule + XCTAssertEqual(viewModel.viewState.selectionState[.allOtherMessages], true) } - func testMismatchingOneToOneRulesAreHandled() { - let expectation = expectation(description: #function) + func testMismatchingOneToOneRulesAreHandled() async { setupWithPollRules() - viewModel.update(ruleID: .oneToOneRoom, isChecked: false) { result in - guard case .success = result else { - XCTFail() - return - } - - // simulating a "mismatch" on the one to one poll started rule - self.viewModel.update(ruleID: .oneToOnePollStart, isChecked: true) - - XCTAssertEqual(self.viewModel.viewState.selectionState.count, 8) - - // The one to one room rule ui flag should match the loudest related poll rule - XCTAssertEqual(self.viewModel.viewState.selectionState[.oneToOneRoom], true) - - expectation.fulfill() - } + await viewModel.update(ruleID: .oneToOneRoom, isChecked: false) + // simulating a "mismatch" on the one to one poll started rule + await viewModel.update(ruleID: .oneToOnePollStart, isChecked: true) - waitForExpectations(timeout: 1.0) + XCTAssertEqual(viewModel.viewState.selectionState.count, 8) + + // The one to one room rule ui flag should match the loudest related poll rule + XCTAssertEqual(viewModel.viewState.selectionState[.oneToOneRoom], true) } } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift index 407be6828..4d1d3ee04 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift @@ -33,7 +33,9 @@ struct NotificationSettings: View { ForEach(viewModel.viewState.ruleIds) { ruleId in let checked = viewModel.viewState.selectionState[ruleId] ?? false FormPickerItem(title: ruleId.title, selected: checked) { - viewModel.update(ruleID: ruleId, isChecked: !checked) + Task { + await viewModel.update(ruleID: ruleId, isChecked: !checked) + } } } } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift index 208b8d2bb..8d60ee163 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift @@ -49,7 +49,9 @@ final class NotificationSettingsViewModel: NotificationSettingsViewModelType, Ob // Observe when the rules are updated, to subsequently update the state of the settings. notificationSettingsService.rulesPublisher - .sink(receiveValue: rulesUpdated(newRules:)) + .sink { [weak self] newRules in + self?.rulesUpdated(newRules: newRules) + } .store(in: &cancellables) // Only observe keywords if the current settings view displays it. @@ -88,7 +90,9 @@ final class NotificationSettingsViewModel: NotificationSettingsViewModelType, Ob // Keyword rules were updates, check if we need to update the setting. keywordsRules .map { $0.contains { $0.enabled } } - .sink(receiveValue: keywordRuleUpdated(anyEnabled:)) + .sink { [weak self] in + self?.keywordRuleUpdated(anyEnabled: $0) + } .store(in: &cancellables) // Update the viewState with the final keywords to be displayed. @@ -105,30 +109,29 @@ final class NotificationSettingsViewModel: NotificationSettingsViewModelType, Ob // MARK: - Public - func update(ruleID: NotificationPushRuleId, isChecked: Bool, completion: ((Result) -> Void)? = nil) { + @MainActor + func update(ruleID: NotificationPushRuleId, isChecked: Bool) async { let index = NotificationIndex.index(when: isChecked) let standardActions = ruleID.standardActions(for: index) let enabled = standardActions != .disabled switch ruleID { case .keywords: // Keywords is handled differently to other settings - updateKeywords(isChecked: isChecked) + await updateKeywords(isChecked: isChecked) case .oneToOneRoom, .allOtherMessages: - updatePushAction( + await updatePushAction( id: ruleID, enabled: enabled, standardActions: standardActions, - then: ruleID.syncedRules, - completion: completion + then: ruleID.syncedRules ) default: - notificationSettingsService.updatePushRuleActions( + try? await notificationSettingsService.updatePushRuleActions( for: ruleID.rawValue, enabled: enabled, - actions: standardActions.actions, - completion: completion + actions: standardActions.actions ) } } @@ -149,58 +152,63 @@ final class NotificationSettingsViewModel: NotificationSettingsViewModelType, Ob // MARK: - Private private extension NotificationSettingsViewModel { - func updateKeywords(isChecked: Bool) { + @MainActor + func updateKeywords(isChecked: Bool) async { guard !keywordsOrdered.isEmpty else { viewState.selectionState[.keywords]?.toggle() return } + // Get the static definition and update the actions and enabled state for every keyword. let index = NotificationIndex.index(when: isChecked) let standardActions = NotificationPushRuleId.keywords.standardActions(for: index) let enabled = standardActions != .disabled + let keywordsToUpdate = keywordsOrdered - keywordsOrdered.forEach { keyword in - notificationSettingsService.updatePushRuleActions( - for: keyword, - enabled: enabled, - actions: standardActions.actions, - completion: nil - ) + await withThrowingTaskGroup(of: Void.self) { group in + for keyword in keywordsToUpdate { + group.addTask { + try await self.notificationSettingsService.updatePushRuleActions( + for: keyword, + enabled: enabled, + actions: standardActions.actions + ) + } + } } } func updatePushAction(id: NotificationPushRuleId, enabled: Bool, standardActions: NotificationStandardActions, - then rules: [NotificationPushRuleId], - completion: ((Result) -> Void)?) { - viewState.saving = true + then rules: [NotificationPushRuleId]) async { + await MainActor.run { + viewState.saving = true + } - Task { - do { - try await notificationSettingsService.updatePushRuleActions(for: id.rawValue, enabled: enabled, actions: standardActions.actions) - - try await withThrowingTaskGroup(of: Void.self) { group in - for ruleId in rules { - group.addTask { - try await self.notificationSettingsService.updatePushRuleActions(for: ruleId.rawValue, enabled: enabled, actions: standardActions.actions) - } + do { + // update the 'parent rule' first + try await notificationSettingsService.updatePushRuleActions(for: id.rawValue, enabled: enabled, actions: standardActions.actions) + + // synchronize all the 'children rules' with the parent rule + try await withThrowingTaskGroup(of: Void.self) { group in + for ruleId in rules { + group.addTask { + try await self.notificationSettingsService.updatePushRuleActions(for: ruleId.rawValue, enabled: enabled, actions: standardActions.actions) } - - try await group.waitForAll() - await completeUpdate(completion: completion, result: .success(())) } - } catch { - await completeUpdate(completion: completion, result: .failure(error)) + try await group.waitForAll() } + await completeUpdate() + } catch { + await completeUpdate() } } @MainActor - func completeUpdate(completion: ((Result) -> Void)?, result: Result) { + func completeUpdate() { #warning("Handle error here in the next ticket") viewState.saving = false - completion?(result) } func rulesUpdated(newRules: [NotificationPushRuleType]) {