[V5] Remove controllerWillChangeChannels and cleanup willChange handling (#3826)

Author: laevandus

Sources/StreamChat/Controllers/ChannelListController/ChannelListController.swift

@@ -86,18 +86,6 @@ public class ChatChannelListController: DataController, DelegateCallable, DataSt
                 $0.controller(self, didChangeChannels: changes)
             }
         }
-
-        observer.onWillChange = { [weak self] in
-            self?.delegateCallback { [weak self] in
-                guard let self = self else {
-                    log.warning("Callback called while self is nil")
-                    return
-                }
-
-                $0.controllerWillChangeChannels(self)
-            }
-        }
-
         return observer
     }()
 
@@ -278,12 +266,6 @@ extension ChatChannelListController {
 
 /// `ChatChannelListController` uses this protocol to communicate changes to its delegate.
 public protocol ChatChannelListControllerDelegate: DataControllerStateDelegate {
-    /// The controller will update the list of observed channels.
-    ///
-    /// - Parameter controller: The controller emitting the change callback.
-    ///
-    func controllerWillChangeChannels(_ controller: ChatChannelListController)
-
     /// The controller changed the list of observed channels.
     ///
     /// - Parameters:
@@ -297,8 +279,6 @@ public protocol ChatChannelListControllerDelegate: DataControllerStateDelegate {
 }
 
 public extension ChatChannelListControllerDelegate {
-    func controllerWillChangeChannels(_ controller: ChatChannelListController) {}
-
     func controller(
         _ controller: ChatChannelListController,
         didChangeChannels changes: [ListChange<ChatChannel>]

Sources/StreamChat/Controllers/DatabaseObserver/BackgroundDatabaseObserver.swift

@@ -6,10 +6,6 @@ import CoreData
 import Foundation
 
 class BackgroundDatabaseObserver<Item: Sendable, DTO: NSManagedObject>: @unchecked Sendable {
-    /// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerWillChangeContent`
-    /// on its delegate.
-    var onWillChange: (@Sendable () -> Void)?
-
     /// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerDidChangeContent`
     /// on its delegate.
     var onDidChange: (@Sendable ([ListChange<Item>]) -> Void)?
@@ -30,24 +26,10 @@ class BackgroundDatabaseObserver<Item: Sendable, DTO: NSManagedObject>: @uncheck
 
     private let queue = DispatchQueue(label: "io.getstream.list-database-observer", qos: .userInitiated)
     private var _items: [Item]?
-    
-    // State handling for supporting will change, because in the callback we should return the previous state.
-    private var _willChangeItems: [Item]?
-    private var _notifyingWillChange = false
 
     /// The items that have been fetched and mapped
     var rawItems: [Item] {
-        // During the onWillChange we swap the results back to the previous state because onWillChange
-        // is dispatched to the main thread and when the main thread handles it, observer has already processed
-        // the database change.
-        if onWillChange != nil {
-            let willChangeState: (active: Bool, cachedItems: [Item]?) = queue.sync { (_notifyingWillChange, _willChangeItems) }
-            if willChangeState.active {
-                return willChangeState.cachedItems ?? []
-            }
-        }
-        
-        var rawItems: [Item]!
+        nonisolated(unsafe) var rawItems: [Item]!
         frc.managedObjectContext.performAndWait {
             // When we already have loaded items, reuse them, otherwise refetch all
             rawItems = _items ?? updateItems(nil)
@@ -98,9 +80,6 @@ class BackgroundDatabaseObserver<Item: Sendable, DTO: NSManagedObject>: @uncheck
             sectionNameKeyPath: nil,
             cacheName: nil
         )
-        changeAggregator.onWillChange = { [weak self] in
-            self?.notifyWillChange()
-        }
         changeAggregator.onDidChange = { [weak self] changes in
             guard let self else { return }
             // Runs on the NSManagedObjectContext's queue, therefore skip performAndWait
@@ -133,33 +112,6 @@ class BackgroundDatabaseObserver<Item: Sendable, DTO: NSManagedObject>: @uncheck
         }
     }
 
-    private func notifyWillChange() {
-        guard let onWillChange = onWillChange else {
-            return
-        }
-        // Will change callback happens on the main thread but by that time the observer
-        // has already updated its local cached state. For allowing to access the previous
-        // state from the will change callback, there is no other way than caching previous state.
-        // This is used by the channel list delegate.
-        
-        // `_items` is mutated by the NSManagedObjectContext's queue, here we are on that queue
-        // so it is safe to read the `_items` state from `self.queue`.
-        queue.sync {
-            _willChangeItems = _items
-        }
-        DispatchQueue.main.async { [weak self] in
-            guard let self else { return }
-            self.queue.async {
-                self._notifyingWillChange = true
-            }
-            onWillChange()
-            self.queue.async {
-                self._willChangeItems = nil
-                self._notifyingWillChange = false
-            }
-        }
-    }
-
     private func notifyDidChange(changes: [ListChange<Item>]) {
         guard let onDidChange = onDidChange else {
             return

Sources/StreamChat/Controllers/DatabaseObserver/ListChange.swift

@@ -144,10 +144,6 @@ class ListChangeAggregator<DTO: NSManagedObject, Item>: NSObject, NSFetchedResul
     /// Used for converting the `DTO`s provided by `FetchResultsController` to the resulting `Item`.
     let itemCreator: (DTO) throws -> Item
 
-    /// Called when the aggregator is about to change the current content. It gets called when the `FetchedResultsController`
-    /// calls `controllerWillChangeContent` on its delegate.
-    var onWillChange: (() -> Void)?
-
     /// Called with the aggregated changes after `FetchResultsController` calls controllerDidChangeContent` on its delegate.
     var onDidChange: (([ListChange<Item>]) -> Void)?
 
@@ -167,7 +163,6 @@ class ListChangeAggregator<DTO: NSManagedObject, Item>: NSObject, NSFetchedResul
     // This should ideally be in the extensions but it's not possible to implement @objc methods in extensions of generic types.
 
     func controllerWillChangeContent(_ controller: NSFetchedResultsController<NSFetchRequestResult>) {
-        onWillChange?()
         currentChanges = []
     }
 

Sources/StreamChatUI/ChatChannelList/ChatChannelListVC.swift

@@ -403,10 +403,6 @@ open class ChatChannelListVC: _ViewController,
 
     // MARK: - ChatChannelListControllerDelegate
 
-    open func controllerWillChangeChannels(_ controller: ChatChannelListController) {
-        collectionView.layoutIfNeeded()
-    }
-
     open func controller(
         _ controller: ChatChannelListController,
         didChangeChannels changes: [ListChange<ChatChannel>]

TestTools/StreamChatTestTools/SpyPattern/QueueAware/ChannelListController_Delegate.swift

@@ -8,19 +8,13 @@ import Foundation
 // A concrete `ChannelListControllerDelegate` implementation allowing capturing the delegate calls
 final class ChannelListController_Delegate: QueueAwareDelegate, ChatChannelListControllerDelegate {
     @Atomic var state: DataController.State?
-    @Atomic var willChangeChannels_called = false
     @Atomic var didChangeChannels_changes: [ListChange<ChatChannel>]?
 
     func controller(_ controller: DataController, didChangeState state: DataController.State) {
         self.state = state
         validateQueue()
     }
 
-    func controllerWillChangeChannels(_ controller: ChatChannelListController) {
-        willChangeChannels_called = true
-        validateQueue()
-    }
-
     func controller(
         _ controller: ChatChannelListController,
         didChangeChannels changes: [ListChange<ChatChannel>]

Tests/StreamChatTests/Controllers/ChannelListController/ChannelListController_Tests.swift

@@ -651,38 +651,26 @@ final class ChannelListController_Tests: XCTestCase {
         let channel = try XCTUnwrap(client.databaseContainer.viewContext.channel(cid: cid)).asModel()
 
         AssertAsync {
-            Assert.willBeTrue(delegate.willChangeChannels_called)
             Assert.willBeEqual(delegate.didChangeChannels_changes, [.insert(channel, index: [0, 0])])
         }
     }
 
-    @MainActor func test_willAndDidCallbacks_areCalledInCorrectOrder() throws {
+    @MainActor func test_didCallbacks_areCalledInCorrectOrder() throws {
         class Delegate: ChatChannelListControllerDelegate {
             let cid: ChannelId
 
-            var willChangeCallbackCalled = false
             var didChangeCallbackCalled = false
 
             init(cid: ChannelId) {
                 self.cid = cid
             }
 
-            func controllerWillChangeChannels(_ controller: ChatChannelListController) {
-                // Check the new channel is NOT in reported channels yet
-                XCTAssertFalse(controller.channels.contains { $0.cid == cid })
-                // Assert the "did" callback hasn't been called yet
-                XCTAssertFalse(didChangeCallbackCalled)
-                willChangeCallbackCalled = true
-            }
-
             func controller(
                 _ controller: ChatChannelListController,
                 didChangeChannels changes: [ListChange<ChatChannel>]
             ) {
                 // Check the new channel is in reported channels
                 XCTAssertTrue(controller.channels.contains { $0.cid == cid })
-                // Assert the "will" callback has been called
-                XCTAssertTrue(willChangeCallbackCalled)
                 didChangeCallbackCalled = true
             }
         }
@@ -699,7 +687,6 @@ final class ChannelListController_Tests: XCTestCase {
         }
 
         AssertAsync {
-            Assert.willBeTrue(delegate.willChangeCallbackCalled)
             Assert.willBeTrue(delegate.didChangeCallbackCalled)
         }
     }

Tests/StreamChatTests/Controllers/MessageController/MessageController_Tests.swift

@@ -905,9 +905,8 @@ final class MessageController_Tests: XCTestCase {
 
     func test_deleteMessageForMe_propagatesError() {
         // Simulate `deleteMessageForMe` call and catch the completion
-        var completionError: Error?
-        controller.deleteMessageForMe { [callbackQueueID] in
-            AssertTestQueue(withId: callbackQueueID)
+        nonisolated(unsafe) var completionError: Error?
+        controller.deleteMessageForMe {
             completionError = $0
         }
 
@@ -921,9 +920,8 @@ final class MessageController_Tests: XCTestCase {
 
     func test_deleteMessageForMe_propagatesNilError() {
         // Simulate `deleteMessageForMe` call and catch the completion
-        var completionCalled = false
-        controller.deleteMessageForMe { [callbackQueueID] in
-            AssertTestQueue(withId: callbackQueueID)
+        nonisolated(unsafe) var completionCalled = false
+        controller.deleteMessageForMe {
             XCTAssertNil($0)
             completionCalled = true
         }

Tests/StreamChatTests/ListChangeAggregator_Tests.swift

@@ -29,16 +29,6 @@ final class ListChangeAggregator_Tests: XCTestCase {
         super.tearDown()
     }
 
-    func test_onWillChange_isCalled() {
-        // Set up aggregator callback
-        var callbackCalled = false
-        aggregator.onWillChange = { callbackCalled = true }
-
-        // Simulate FRC starts updating
-        aggregator.controllerWillChangeContent(fakeController)
-        XCTAssertTrue(callbackCalled)
-    }
-
     func test_addingItems() {
         // Set up aggregator callback
         var result: [ListChange<String>]?