Merge pull request #789 from Johennes/feature/collapse-one-by-one

Fix collapsing of separately processed events
matrix-org · Mar 24, 2021 · 13ed03e · 13ed03e
2 parents 4f67ebb + eef7124
commit 13ed03e
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 2 deletions.
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -9,7 +9,7 @@ Changes to be released in next version
  * MXKRoomDataSource: Introduce secondaryRoomId and secondaryRoomEventTypes.
 
 🐛 Bugfix
- * 
+ * Fix collapsing of separately processed events
 
 ⚠️ API Changes
  * 

diff --git a/MatrixKit/Models/Room/MXKRoomDataSource.m b/MatrixKit/Models/Room/MXKRoomDataSource.m
@@ -3018,6 +3018,12 @@ - (void)processQueuedEvents:(void (^)(NSUInteger addedHistoryCellNb, NSUInteger
 
                                             // The new cell must have the collapsed state as the series
                                             bubbleData.collapsed = tailBubbleData.collapsed;
+
+                                            // If the start of the collapsible series stems from an event in a different processing
+                                            // batch, we need to track it here so that we can update the summary string later
+                                            if (![collapsingCellDataSeriess containsObject:self->collapsableSeriesAtEnd]) {
+                                                [collapsingCellDataSeriess addObject:self->collapsableSeriesAtEnd];
+                                            }
                                         }
                                         else
                                         {
@@ -3298,7 +3304,7 @@ - (void)processQueuedEvents:(void (^)(NSUInteger addedHistoryCellNb, NSUInteger
                 // Check if all cells of self.bubbles belongs to a single collapse series.
                 // In this case, collapsableSeriesAtStart and collapsableSeriesAtEnd must be equal
                 // in order to handle next forward or backward pagination.
-                if (self->collapsableSeriesAtStart == self->bubbles.firstObject)
+                if (self->collapsableSeriesAtStart && self->collapsableSeriesAtStart == self->bubbles.firstObject)
                 {
                     // Find the tail
                     id<MXKRoomBubbleCellDataStoring> tailBubbleData = self->collapsableSeriesAtStart;

diff --git a/MatrixKitTests/MXKRoomDataSource+Tests.h b/MatrixKitTests/MXKRoomDataSource+Tests.h
@@ -21,4 +21,7 @@
 - (NSArray<id<MXKRoomBubbleCellDataStoring>> *)getBubbles;
 - (void)replaceBubbles:(NSArray<id<MXKRoomBubbleCellDataStoring>> *)newBubbles;
 
+- (void)queueEventForProcessing:(MXEvent*)event withRoomState:(MXRoomState*)roomState direction:(MXTimelineDirection)direction;
+- (void)processQueuedEvents:(void (^)(NSUInteger addedHistoryCellNb, NSUInteger addedLiveCellNb))onComplete;
+
 @end
diff --git a/MatrixKitTests/MXKRoomDataSourceTests.swift b/MatrixKitTests/MXKRoomDataSourceTests.swift
@@ -21,6 +21,8 @@ import XCTest
 
 class MXKRoomDataSourceTests: XCTestCase {
 
+    // MARK: - Destruction tests
+
     func testDestroyRemovesAllBubbles() {
         let dataSource = StubMXKRoomDataSource()
         dataSource.destroy()
@@ -36,8 +38,39 @@ class MXKRoomDataSourceTests: XCTestCase {
         XCTAssertNil(last)
     }
 
+    // MARK: - Collapsing tests
+
+    func testCollapseBubblesWhenProcessingTogether() throws {
+        let dataSource = try FakeMXKRoomDataSource.make()
+        try dataSource.queueEvent1()
+        try dataSource.queueEvent2()
+        awaitEventProcessing(for: dataSource)
+        dataSource.verifyCollapsedEvents(2)
+    }
+
+    func testCollapseBubblesWhenProcessingAlone() throws {
+        let dataSource = try FakeMXKRoomDataSource.make()
+        try dataSource.queueEvent1()
+        awaitEventProcessing(for: dataSource)
+        try dataSource.queueEvent2()
+        awaitEventProcessing(for: dataSource)
+        dataSource.verifyCollapsedEvents(2)
+    }
+
+    private func awaitEventProcessing(for dataSource: MXKRoomDataSource) {
+        let e = expectation(description: "The wai-ai-ting is the hardest part")
+        dataSource.processQueuedEvents { _, _ in
+            e.fulfill()
+        }
+        waitForExpectations(timeout: 2) { error in
+            XCTAssertNil(error)
+        }
+    }
+
 }
 
+// MARK: - Test doubles
+
 private final class StubMXKRoomDataSource: MXKRoomDataSource {
 
     override init() {
@@ -56,3 +89,67 @@ private final class StubMXKRoomDataSource: MXKRoomDataSource {
     }
 
 }
+
+private final class FakeMXKRoomDataSource: MXKRoomDataSource {
+
+    class func make() throws -> FakeMXKRoomDataSource {
+        let dataSource = try XCTUnwrap(FakeMXKRoomDataSource(roomId: "!foofoofoofoofoofoo:matrix.org", andMatrixSession: nil))
+        dataSource.registerCellDataClass(CollapsibleBubbleCellData.self, forCellIdentifier: kMXKRoomBubbleCellDataIdentifier)
+        dataSource.eventFormatter = CountingEventFormatter(matrixSession: nil)
+        return dataSource
+    }
+
+    override var state: MXKDataSourceState {
+        MXKDataSourceStateReady
+    }
+
+    override var roomState: MXRoomState! {
+        nil
+    }
+
+    func queueEvent1() throws {
+        try queueEvent(json: #"{"sender":"@alice:matrix.org","content":{"displayname":"bob","membership":"invite"},"origin_server_ts":1616488993287,"state_key":"@bob:matrix.org","room_id":"!foofoofoofoofoofoo:matrix.org","event_id":"$lGK3budX5w009ErtQwE9ZFhwyUUAV9DqEN5yb2fI4Do","type":"m.room.member","unsigned":{"age":1204610,"prev_sender":"@alice:matrix.org","prev_content":{"membership":"leave"},"replaces_state":"$9mQ6RtscXqHCxWqOElI-eP_kwpkuPd2Czm3UHviGoyE"}}"#)
+    }
+
+    func queueEvent2() throws {
+        try queueEvent(json: #"{"sender":"@alice:matrix.org","content":{"displayname":"john","membership":"invite"},"origin_server_ts":1616488967295,"state_key":"@john:matrix.org","room_id":"!foofoofoofoofoofoo:matrix.org","event_id":"$-00slfAluxVTP2VWytgDThTmh3nLd0WJD6gzBo2scJM","type":"m.room.member","unsigned":{"age":1712006,"prev_sender":"@alice:matrix.org","prev_content":{"membership":"leave"},"replaces_state":"$NRNkCMKeKK5NtTfWkMfTlMr5Ygw60Q2CQYnJNkbzyrs"}}"#)
+    }
+
+    private func queueEvent(json: String) throws {
+        let data = try XCTUnwrap(json.data(using: .utf8))
+        let dict = try XCTUnwrap((try JSONSerialization.jsonObject(with: data, options: [])) as? [AnyHashable: Any])
+        let event = MXEvent(fromJSON: dict)
+        queueEvent(forProcessing: event, with: nil, direction: __MXTimelineDirectionForwards)
+    }
+
+    func verifyCollapsedEvents(_ number: Int) {
+        let message = getBubbles()?.first?.collapsedAttributedTextMessage.string
+        XCTAssertEqual(message, "\(number)")
+    }
+
+}
+
+private final class CollapsibleBubbleCellData: MXKRoomBubbleCellData {
+
+    override init() {
+        super.init()
+    }
+
+    required init!(event: MXEvent!, andRoomState roomState: MXRoomState!, andRoomDataSource roomDataSource: MXKRoomDataSource!) {
+        super.init(event: event, andRoomState: roomState, andRoomDataSource: roomDataSource)
+        collapsable = true
+    }
+
+    override func collapse(with cellData: MXKRoomBubbleCellDataStoring!) -> Bool {
+        true
+    }
+
+}
+
+private final class CountingEventFormatter: MXKEventFormatter {
+
+    override func attributedString(from events: [MXEvent]!, with roomState: MXRoomState!, error: UnsafeMutablePointer<MXKEventFormatterError>!) -> NSAttributedString! {
+        NSAttributedString(string: "\(events.count)")
+    }
+
+}