From cb3624b612f6024c706407517ad190728d5f3cc9 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 17 Dec 2020 10:42:25 -0300 Subject: [PATCH 1/4] DRY up handling of network failures. --- .../CV/CVComponentState+GroupLink.swift | 6 +----- .../ViewControllers/GroupInviteLinksUI.swift | 12 +++--------- SignalMessaging/groups/GroupsV2Impl.swift | 6 +----- .../groups/GroupsV2Migration.swift | 19 ++++--------------- .../Interactions/OWSLinkPreview.swift | 7 +------ .../src/Network/API/NetworkManager.swift | 15 +++++++++++++++ .../src/Network/API/OWSUpload.swift | 6 +----- .../src/Util/StorageService.swift | 9 ++------- 8 files changed, 28 insertions(+), 52 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/CV/CVComponentState+GroupLink.swift b/Signal/src/ViewControllers/ConversationView/CV/CVComponentState+GroupLink.swift index a80517bd93..bf4b69f73e 100644 --- a/Signal/src/ViewControllers/ConversationView/CV/CVComponentState+GroupLink.swift +++ b/Signal/src/ViewControllers/ConversationView/CV/CVComponentState+GroupLink.swift @@ -157,11 +157,7 @@ extension CVComponentState { touchMessage() }.catch { error in // TODO: Add retry? - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) } return GroupInviteLinkViewModel(url: url, diff --git a/Signal/src/ViewControllers/GroupInviteLinksUI.swift b/Signal/src/ViewControllers/GroupInviteLinksUI.swift index 5b8478e78b..75d32c5e8b 100644 --- a/Signal/src/ViewControllers/GroupInviteLinksUI.swift +++ b/Signal/src/ViewControllers/GroupInviteLinksUI.swift @@ -201,11 +201,9 @@ class GroupInviteLinksActionSheet: ActionSheetController { }.catch { [weak self] error in if case GroupsV2Error.expiredGroupInviteLink = error { self?.applyExpiredGroupInviteLink() - } else if IsNetworkConnectivityFailure(error) { - // TODO: Retry errors? - Logger.warn("Error: \(error)") } else { - owsFailDebug("Error: \(error)") + // TODO: Retry errors? + owsFailDebugUnlessNetworkFailure(error) } } } @@ -218,11 +216,7 @@ class GroupInviteLinksActionSheet: ActionSheetController { self?.applyGroupAvatar(groupAvatar) }.catch { error in // TODO: Add retry? - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) } } diff --git a/SignalMessaging/groups/GroupsV2Impl.swift b/SignalMessaging/groups/GroupsV2Impl.swift index 8242b0aa23..266e9bc688 100644 --- a/SignalMessaging/groups/GroupsV2Impl.swift +++ b/SignalMessaging/groups/GroupsV2Impl.swift @@ -1058,11 +1058,7 @@ public class GroupsV2Impl: NSObject, GroupsV2Swift { }.done { _ in Logger.verbose("Update succeeded.") }.catch { error in - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) } } diff --git a/SignalMessaging/groups/GroupsV2Migration.swift b/SignalMessaging/groups/GroupsV2Migration.swift index cec8f3191b..e2c18e906d 100644 --- a/SignalMessaging/groups/GroupsV2Migration.swift +++ b/SignalMessaging/groups/GroupsV2Migration.swift @@ -225,8 +225,7 @@ public extension GroupsV2Migration { } return ContactDiscoveryTask(phoneNumbers: phoneNumbersWithoutUuids).perform().asVoid() }.recover(on: .global()) { (error: Error) -> Promise in - // Log but otherwise ignore errors in CDS lookup. - owsFailDebug("Error: \(error)") + owsFailDebugUnlessNetworkFailure(error) return Promise.value(()) }.map(on: .global()) { _ in Logger.verbose("") @@ -257,7 +256,7 @@ public extension GroupsV2Migration { } } }.catch(on: .global()) { error in - owsFailDebug("Error: \(error)") + owsFailDebugUnlessNetworkFailure(error) } } } @@ -398,12 +397,7 @@ fileprivate extension GroupsV2Migration { return firstly { discoveryTask.perform().asVoid() }.recover(on: .global()) { error -> Promise in - // Log but ignore errors. - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) return Promise.value(()) } }.then(on: .global()) { () -> Promise in @@ -446,12 +440,7 @@ fileprivate extension GroupsV2Migration { // Do not ignore throttling errors. throw error } - // Log but ignore errors. - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) return Promise.value(()) } } diff --git a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift index af4c202e9b..41e0644370 100644 --- a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift +++ b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift @@ -635,12 +635,7 @@ public class OWSLinkPreviewManager: NSObject { }.map { (avatarData: Data) -> Data? in return avatarData }.recover { (error: Error) -> Promise in - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) return Promise.value(nil) } }.then(on: Self.workQueue) { (imageData: Data?) -> Promise in diff --git a/SignalServiceKit/src/Network/API/NetworkManager.swift b/SignalServiceKit/src/Network/API/NetworkManager.swift index 24587aceee..f07785cee6 100644 --- a/SignalServiceKit/src/Network/API/NetworkManager.swift +++ b/SignalServiceKit/src/Network/API/NetworkManager.swift @@ -157,3 +157,18 @@ public extension Error { HTTPRetryAfterDateForError(self) } } + +// MARK: - + +@inlinable +public func owsFailDebugUnlessNetworkFailure(_ error: Error, + file: String = #file, + function: String = #function, + line: Int = #line) { + if IsNetworkConnectivityFailure(error) { + // Log but otherwise ignore network failures. + Logger.warn("Error: \(error)", file: file, function: function, line: line) + } else { + owsFailDebug("Error: \(error)", file: file, function: function, line: line) + } +} diff --git a/SignalServiceKit/src/Network/API/OWSUpload.swift b/SignalServiceKit/src/Network/API/OWSUpload.swift index a4fed9804b..99e10e5dba 100644 --- a/SignalServiceKit/src/Network/API/OWSUpload.swift +++ b/SignalServiceKit/src/Network/API/OWSUpload.swift @@ -723,11 +723,7 @@ public extension OWSUpload { AppExpiry.shared.setHasAppExpiredAtCurrentVersion() } - if IsNetworkConnectivityFailure(error) { - Logger.warn("Error: \(error)") - } else { - owsFailDebug("Error: \(error)") - } + owsFailDebugUnlessNetworkFailure(error) resolver.reject(error) }) } diff --git a/SignalServiceKit/src/Util/StorageService.swift b/SignalServiceKit/src/Util/StorageService.swift index 2e6afde462..46bf781ef8 100644 --- a/SignalServiceKit/src/Util/StorageService.swift +++ b/SignalServiceKit/src/Util/StorageService.swift @@ -443,13 +443,8 @@ public struct StorageService { return StorageResponse(status: status, data: responseData) }.recover(on: .global()) { (error: Error) -> Promise in - if IsNetworkConnectivityFailure(error) { - throw StorageError.networkError(statusCode: 0, underlyingError: error) - } else { - // This should never happen. - owsFailDebug("Error: \(error)") - throw StorageError.networkError(statusCode: 0, underlyingError: error) - } + owsFailDebugUnlessNetworkFailure(error) + throw StorageError.networkError(statusCode: 0, underlyingError: error) } } } From fc003412cdf3310c2ee4f83714ae9b7aaf2fbed1 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 17 Dec 2020 10:43:50 -0300 Subject: [PATCH 2/4] Improve landing of loads. --- .../ConversationViewController+CVC.swift | 59 +++++++++++-------- ...tionViewController+LastVisibleSortId.swift | 4 +- .../ConversationViewController+Scroll.swift | 3 +- .../ConversationViewController.m | 3 - SignalServiceKit/src/Util/FeatureFlags.swift | 2 +- 5 files changed, 39 insertions(+), 32 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift index 8c7defeb5a..ccf0f906b8 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift @@ -244,6 +244,7 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { self.viewState.scrollContinuityMap = nil self.loadCoordinator.loadDidLand() + self.configureScrollDownButtons() } // The view's first appearance and the first load can race. @@ -329,10 +330,10 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { self.layout.invalidateLayout() } - updateViewToReflectLoad(loadedRenderState: self.renderState) - scrollToInitialPosition(animated: false) + updateViewToReflectLoad(loadedRenderState: self.renderState) + loadCoordinator.enqueueReload() setHasAppliedFirstLoadIfNecessary() @@ -342,11 +343,19 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { private func updateForMinorUpdate(scrollAction: CVScrollAction) { Logger.verbose("") + // If the scroll action is not animated, perform it _before_ + // updateViewToReflectLoad(). + if !scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } + updateViewToReflectLoad(loadedRenderState: self.renderState) loadDidLand() - perform(scrollAction: scrollAction) + if scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } } private func updateWithFirstLoad() { @@ -368,16 +377,16 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { benchSteps.step("2") - updateViewToReflectLoad(loadedRenderState: self.renderState) + scrollToInitialPosition(animated: false) + clearInitialScrollState() benchSteps.step("3") - loadDidLand() + updateViewToReflectLoad(loadedRenderState: self.renderState) benchSteps.step("4") - scrollToInitialPosition(animated: false) - clearInitialScrollState() + loadDidLand() benchSteps.step("5") @@ -396,22 +405,6 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { } viewState.hasAppliedFirstLoad = true clearInitialScrollState() - - guard hasViewWillAppearEverBegun else { - Logger.verbose("Not fading in collection view.") - return - } - - // If we apply a load for the first time after the view - // appeared, we should fade in the collection view. - let useFadeAnimations = false - if useFadeAnimations { - // Fade in the collection view. - collectionView.alpha = 0.05 - UIView.animate(withDuration: 0.025) { - self.collectionView.alpha = 1.0 - } - } } private func updateReloadingAll(renderState: CVRenderState, @@ -426,9 +419,17 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { DispatchQueue.main.async { [weak self] in guard let self = self else { return } + + // If the scroll action is not animated, perform it _before_ + // updateViewToReflectLoad(). + if !scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } self.updateViewToReflectLoad(loadedRenderState: renderState) self.loadDidLand() - self.perform(scrollAction: scrollAction) + if scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } } } @@ -560,11 +561,19 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { return } + // If the scroll action is not animated, perform it _before_ + // updateViewToReflectLoad(). + if !scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } + self.updateViewToReflectLoad(loadedRenderState: renderState) self.loadDidLand() - self.perform(scrollAction: scrollAction) + if scrollAction.isAnimated { + self.perform(scrollAction: scrollAction) + } viewState.scrollActionForUpdate = nil diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+LastVisibleSortId.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+LastVisibleSortId.swift index e598aaf9e8..e17ea36858 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+LastVisibleSortId.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+LastVisibleSortId.swift @@ -16,7 +16,7 @@ extension ConversationViewController { /// The index path of the last item in the collection view's visible rect @objc - var firstVisibleIndexPath: IndexPath? { + public var firstVisibleIndexPath: IndexPath? { // For people looking at this in the future, UICollectionView has a very similar looking // property: -indexPathsForVisibleItems. Why aren't we using that? // @@ -39,7 +39,7 @@ extension ConversationViewController { /// The index path of the last item in the collection view's visible rect @objc - var lastVisibleIndexPath: IndexPath? { + public var lastVisibleIndexPath: IndexPath? { // For people looking at this in the future, UICollectionView has a very similar looking // property: -indexPathsForVisibleItems. Why aren't we using that? // diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift index d0542f3ca5..3a893ffde3 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift @@ -468,7 +468,8 @@ extension ConversationViewController { return isScrolledToBottom } - private var safeDistanceFromBottom: CGFloat { + @objc + public var safeDistanceFromBottom: CGFloat { // This is a bit subtle. // // The _wrong_ way to determine if we're scrolled to the bottom is to diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index c3ad50bc16..e8d3f3a934 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2007,8 +2007,6 @@ typedef enum : NSUInteger { { OWSAssertIsOnMainThread(); - // TODO: If you open CVC with one unread message, the scroll down button - // briefly flashes. if (!self.hasAppearedAndHasAppliedFirstLoad) { self.scrollDownButton.hidden = YES; self.scrollToNextMentionButton.hidden = YES; @@ -2034,7 +2032,6 @@ typedef enum : NSUInteger { if (self.isInPreviewPlatter) { scrollDownIsHidden = YES; scrollToNextMentionIsHidden = YES; - } else if (self.isPresentingMessageActions) { // Content offset calculations get messed up when we're presenting message actions // Don't change button visibility if we're presenting actions diff --git a/SignalServiceKit/src/Util/FeatureFlags.swift b/SignalServiceKit/src/Util/FeatureFlags.swift index bf7469c243..f4552cab9e 100644 --- a/SignalServiceKit/src/Util/FeatureFlags.swift +++ b/SignalServiceKit/src/Util/FeatureFlags.swift @@ -249,7 +249,7 @@ public class DebugFlags: BaseFlags { public static let suppressBackgroundActivity = false @objc - public static let logSQLQueries = build.includes(.dev) + public static let logSQLQueries = false @objc public static let groupsV2IgnoreCapability = false From 5f7196b85057739abaf5330731286124caa89cb7 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 17 Dec 2020 10:46:24 -0300 Subject: [PATCH 3/4] Improve landing of loads. --- .../ConversationView/ConversationViewController+CVC.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift index ccf0f906b8..0b7f4a6934 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+CVC.swift @@ -244,7 +244,6 @@ extension ConversationViewController: CVLoadCoordinatorDelegate { self.viewState.scrollContinuityMap = nil self.loadCoordinator.loadDidLand() - self.configureScrollDownButtons() } // The view's first appearance and the first load can race. From 1b508556d4d712edb95218afda5d06299d852132 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 18 Dec 2020 11:24:40 -0300 Subject: [PATCH 4/4] Respond to CR. --- SignalServiceKit/src/Util/FeatureFlags.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Util/FeatureFlags.swift b/SignalServiceKit/src/Util/FeatureFlags.swift index f4552cab9e..bf7469c243 100644 --- a/SignalServiceKit/src/Util/FeatureFlags.swift +++ b/SignalServiceKit/src/Util/FeatureFlags.swift @@ -249,7 +249,7 @@ public class DebugFlags: BaseFlags { public static let suppressBackgroundActivity = false @objc - public static let logSQLQueries = false + public static let logSQLQueries = build.includes(.dev) @objc public static let groupsV2IgnoreCapability = false