From 7d38c9066ccebb2e9d4e42bb1f43dab65cde80b8 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 21 Aug 2024 16:54:22 +1000 Subject: [PATCH] Reworked unobserved change handling to try to resolve crash (regenerate changeset) --- .../Conversations/ConversationViewModel.swift | 35 +++++++---------- Session/Home/HomeViewModel.swift | 39 +++++++++---------- .../MediaGalleryViewModel.swift | 28 +++++++------ .../Types/PagedDatabaseObserver.swift | 24 ++++++------ 4 files changed, 58 insertions(+), 68 deletions(-) diff --git a/Session/Conversations/ConversationViewModel.swift b/Session/Conversations/ConversationViewModel.swift index e86a9352e..abdeb5844 100644 --- a/Session/Conversations/ConversationViewModel.swift +++ b/Session/Conversations/ConversationViewModel.swift @@ -264,7 +264,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { private var lastInteractionIdMarkedAsRead: Int64? = nil private var lastInteractionTimestampMsMarkedAsRead: Int64 = 0 - public private(set) var unobservedInteractionDataChanges: ([SectionModel], StagedChangeset<[SectionModel]>)? + public private(set) var unobservedInteractionDataChanges: [SectionModel]? public private(set) var interactionData: [SectionModel] = [] public private(set) var reactionExpandedInteractionIds: Set = [] public private(set) var pagedDataObserver: PagedDatabaseObserver? @@ -273,14 +273,15 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { didSet { // When starting to observe interaction changes we want to trigger a UI update just in case the // data was changed while we weren't observing - if let changes: ([SectionModel], StagedChangeset<[SectionModel]>) = self.unobservedInteractionDataChanges { - let performChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())? = onInteractionChange - - switch Thread.isMainThread { - case true: performChange?(changes.0, changes.1) - case false: DispatchQueue.main.async { performChange?(changes.0, changes.1) } - } - + if let changes: [SectionModel] = self.unobservedInteractionDataChanges { + PagedData.processAndTriggerUpdates( + updatedData: changes, + currentDataRetriever: { [weak self] in self?.interactionData }, + onDataChangeRetriever: { [weak self] in self?.onInteractionChange }, + onUnobservedDataChange: { [weak self] updatedData in + self?.unobservedInteractionDataChanges = updatedData + } + ) self.unobservedInteractionDataChanges = nil } } @@ -424,11 +425,8 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { ), currentDataRetriever: { self?.interactionData }, onDataChangeRetriever: { self?.onInteractionChange }, - onUnobservedDataChange: { updatedData, changeset in - self?.unobservedInteractionDataChanges = (changeset.isEmpty ? - nil : - (updatedData, changeset) - ) + onUnobservedDataChange: { updatedData in + self?.unobservedInteractionDataChanges = updatedData } ) } @@ -688,7 +686,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { guard let currentPageInfo: PagedData.PageInfo = self.pagedDataObserver?.pageInfo.wrappedValue else { return } /// **MUST** have the same logic as in the 'PagedDataObserver.onChangeUnsorted' above - let currentData: [SectionModel] = (unobservedInteractionDataChanges?.0 ?? interactionData) + let currentData: [SectionModel] = (unobservedInteractionDataChanges ?? interactionData) PagedData.processAndTriggerUpdates( updatedData: process( @@ -699,11 +697,8 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { ), currentDataRetriever: { [weak self] in self?.interactionData }, onDataChangeRetriever: { [weak self] in self?.onInteractionChange }, - onUnobservedDataChange: { [weak self] updatedData, changeset in - self?.unobservedInteractionDataChanges = (changeset.isEmpty ? - nil : - (updatedData, changeset) - ) + onUnobservedDataChange: { [weak self] updatedData in + self?.unobservedInteractionDataChanges = updatedData } ) } diff --git a/Session/Home/HomeViewModel.swift b/Session/Home/HomeViewModel.swift index 806c7ea1b..47696560a 100644 --- a/Session/Home/HomeViewModel.swift +++ b/Session/Home/HomeViewModel.swift @@ -211,11 +211,8 @@ public class HomeViewModel { updatedData: self?.process(data: updatedData, for: updatedPageInfo), currentDataRetriever: { self?.threadData }, onDataChangeRetriever: { self?.onThreadChange }, - onUnobservedDataChange: { updatedData, changeset in - self?.unobservedThreadDataChanges = (changeset.isEmpty ? - nil : - (updatedData, changeset) - ) + onUnobservedDataChange: { updatedData in + self?.unobservedThreadDataChanges = updatedData } ) @@ -280,7 +277,7 @@ public class HomeViewModel { else { return } /// **MUST** have the same logic as in the 'PagedDataObserver.onChangeUnsorted' above - let currentData: [SectionModel] = (self.unobservedThreadDataChanges?.0 ?? self.threadData) + let currentData: [SectionModel] = (self.unobservedThreadDataChanges ?? self.threadData) let updatedThreadData: [SectionModel] = self.process( data: (currentData.first(where: { $0.model == .threads })?.elements ?? []), for: currentPageInfo @@ -288,13 +285,10 @@ public class HomeViewModel { PagedData.processAndTriggerUpdates( updatedData: updatedThreadData, - currentDataRetriever: { [weak self] in (self?.unobservedThreadDataChanges?.0 ?? self?.threadData) }, + currentDataRetriever: { [weak self] in (self?.unobservedThreadDataChanges ?? self?.threadData) }, onDataChangeRetriever: { [weak self] in self?.onThreadChange }, - onUnobservedDataChange: { [weak self] updatedData, changeset in - self?.unobservedThreadDataChanges = (changeset.isEmpty ? - nil : - (updatedData, changeset) - ) + onUnobservedDataChange: { [weak self] updatedData in + self?.unobservedThreadDataChanges = updatedData } ) } @@ -302,22 +296,25 @@ public class HomeViewModel { // MARK: - Thread Data private var hasReceivedInitialThreadData: Bool = false - public private(set) var unobservedThreadDataChanges: ([SectionModel], StagedChangeset<[SectionModel]>)? + public private(set) var unobservedThreadDataChanges: [SectionModel]? public private(set) var threadData: [SectionModel] = [] public private(set) var pagedDataObserver: PagedDatabaseObserver? public var onThreadChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())? { didSet { + guard onThreadChange != nil else { return } + // When starting to observe interaction changes we want to trigger a UI update just in case the // data was changed while we weren't observing - if let changes: ([SectionModel], StagedChangeset<[SectionModel]>) = self.unobservedThreadDataChanges { - let performChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())? = onThreadChange - - switch Thread.isMainThread { - case true: performChange?(changes.0, changes.1) - case false: DispatchQueue.main.async { performChange?(changes.0, changes.1) } - } - + if let changes: [SectionModel] = self.unobservedThreadDataChanges { + PagedData.processAndTriggerUpdates( + updatedData: changes, + currentDataRetriever: { [weak self] in self?.threadData }, + onDataChangeRetriever: { [weak self] in self?.onThreadChange }, + onUnobservedDataChange: { [weak self] updatedData in + self?.unobservedThreadDataChanges = updatedData + } + ) self.unobservedThreadDataChanges = nil } } diff --git a/Session/Media Viewing & Editing/MediaGalleryViewModel.swift b/Session/Media Viewing & Editing/MediaGalleryViewModel.swift index ee5c78035..8f6baedfa 100644 --- a/Session/Media Viewing & Editing/MediaGalleryViewModel.swift +++ b/Session/Media Viewing & Editing/MediaGalleryViewModel.swift @@ -43,20 +43,21 @@ public class MediaGalleryViewModel { public private(set) var pagedDataObserver: PagedDatabaseObserver? /// This value is the current state of a gallery view - private var unobservedGalleryDataChanges: ([SectionModel], StagedChangeset<[SectionModel]>)? + private var unobservedGalleryDataChanges: [SectionModel]? public private(set) var galleryData: [SectionModel] = [] public var onGalleryChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())? { didSet { // When starting to observe interaction changes we want to trigger a UI update just in case the // data was changed while we weren't observing - if let changes: ([SectionModel], StagedChangeset<[SectionModel]>) = self.unobservedGalleryDataChanges { - let performChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())? = onGalleryChange - - switch Thread.isMainThread { - case true: performChange?(changes.0, changes.1) - case false: DispatchQueue.main.async { performChange?(changes.0, changes.1) } - } - + if let changes: [SectionModel] = self.unobservedGalleryDataChanges { + PagedData.processAndTriggerUpdates( + updatedData: changes, + currentDataRetriever: { [weak self] in self?.galleryData }, + onDataChangeRetriever: { [weak self] in self?.onGalleryChange }, + onUnobservedDataChange: { [weak self] updatedData in + self?.unobservedGalleryDataChanges = updatedData + } + ) self.unobservedGalleryDataChanges = nil } } @@ -104,11 +105,8 @@ public class MediaGalleryViewModel { updatedData: self?.process(data: updatedData, for: updatedPageInfo), currentDataRetriever: { self?.galleryData }, onDataChangeRetriever: { self?.onGalleryChange }, - onUnobservedDataChange: { updatedData, changeset in - self?.unobservedGalleryDataChanges = (changeset.isEmpty ? - nil : - (updatedData, changeset) - ) + onUnobservedDataChange: { updatedData in + self?.unobservedGalleryDataChanges = updatedData } ) } @@ -132,7 +130,7 @@ public class MediaGalleryViewModel { // we don't want to mess with the initial view controller behaviour) guard !performInitialQuerySync else { loadInitialData() - updateGalleryData(self.unobservedGalleryDataChanges?.0 ?? []) + updateGalleryData(self.unobservedGalleryDataChanges ?? []) return } diff --git a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift index 0856fc338..89d9f81ca 100644 --- a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift +++ b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift @@ -1017,10 +1017,21 @@ public enum PagedData { updatedData: [SectionModel]?, currentDataRetriever: @escaping (() -> [SectionModel]?), onDataChangeRetriever: @escaping (() -> (([SectionModel], StagedChangeset<[SectionModel]>) -> ())?), - onUnobservedDataChange: @escaping (([SectionModel], StagedChangeset<[SectionModel]>) -> Void) + onUnobservedDataChange: @escaping (([SectionModel]) -> Void) ) { guard let updatedData: [SectionModel] = updatedData else { return } + /// If we don't have a callback then store the changes to be sent back through this function if we ever start + /// observing again (when we have the callback it needs to do the data updating as it's tied to UI updates + /// and can cause crashes if not updated in the correct order) + /// + /// **Note:** We do this even if the 'changeset' is empty because if this change reverts a previous change we + /// need to ensure the `onUnobservedDataChange` gets cleared so it doesn't end up in an invalid state + guard let onDataChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = onDataChangeRetriever() else { + onUnobservedDataChange(updatedData) + return + } + // Note: While it would be nice to generate the changeset on a background thread it introduces // a multi-threading issue where a data change can come in while the table is processing multiple // updates resulting in the data being in a partially updated state (which makes the subsequent @@ -1033,17 +1044,6 @@ public enum PagedData { target: updatedData ) - /// If we have the callback then trigger it, otherwise just store the changes to be sent to the callback if we ever - /// start observing again (when we have the callback it needs to do the data updating as it's tied to UI updates - /// and can cause crashes if not updated in the correct order) - /// - /// **Note:** We do this even if the 'changeset' is empty because if this change reverts a previous change we - /// need to ensure the `onUnobservedDataChange` gets cleared so it doesn't end up in an invalid state - guard let onDataChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = onDataChangeRetriever() else { - onUnobservedDataChange(updatedData, changeset) - return - } - // No need to do anything if there were no changes guard !changeset.isEmpty else { return }