Merge pull request #7461 from vector-im/nimau/7420-readmarker

Fix: Update the read marker position even if it is not displayed

Riot/Modules/Room/MXKRoomViewController.m

@@ -44,6 +44,9 @@
 
 #import "MXKPreviewViewController.h"
 
+// Constant used to determine whether an event is visible at the bottom of the tableview, based on its visible height
+static const CGFloat kCellVisibilityMinimumHeight = 8.0;
+
 @interface MXKRoomViewController () <MXKPreviewViewControllerDelegate>
 {
     /**
@@ -2419,7 +2422,7 @@
             contentBottomOffsetY = _bubblesTableView.contentSize.height;
         }
         // Be a bit less retrictive, consider visible an event at the bottom even if is partially hidden.
-        contentBottomOffsetY += 8;
+        contentBottomOffsetY += kCellVisibilityMinimumHeight;
         
         // Reset the current event id
         currentEventIdAtTableBottom = nil;
@@ -2489,24 +2492,26 @@
                     if (acknowledge && self.isEventsAcknowledgementEnabled)
                     {
                         // Indicate to the homeserver that the user has read this event.
-                        
-                        // Check whether the read marker must be updated.
-                        BOOL updateReadMarker = _updateRoomReadMarker;
-                        if (updateReadMarker && roomDataSource.room.accountData.readMarkerEventId)
-                        {
-                            MXEvent *currentReadMarkerEvent = [roomDataSource.mxSession.store eventWithEventId:roomDataSource.room.accountData.readMarkerEventId inRoom:roomDataSource.roomId];
-                            if (!currentReadMarkerEvent)
-                            {
-                                currentReadMarkerEvent = [roomDataSource eventWithEventId:roomDataSource.room.accountData.readMarkerEventId];
-                            }
-                            
-                            // Update the read marker only if the current event is available, and the new event is posterior to it.
-                            updateReadMarker = (currentReadMarkerEvent && (currentReadMarkerEvent.originServerTs <= component.event.originServerTs));
-                        }
-                        
                         if (self.navigationController.viewControllers.lastObject == self)
                         {
-                            [roomDataSource.room acknowledgeEvent:component.event andUpdateReadMarker:updateReadMarker];
+                            // Check if the selected event is eligible to be the new read marker position too
+                            if (!bubbleData.collapsed && [self eligibleForReadMarkerUpdate:component.event])
+                            {
+                                BOOL updateRoomReadMarker = _updateRoomReadMarker && [self isEventPosteriorToCurrentReadMarker:component.event];
+                                // Acknowledge this event and update the read marker if needed
+                                [roomDataSource.room acknowledgeEvent:component.event andUpdateReadMarker:updateRoomReadMarker];
+                            }
+                            else
+                            {
+                                // Acknowledge only this event. The read marker is handled separately
+                                [roomDataSource.room acknowledgeEvent:component.event andUpdateReadMarker:NO];
+
+                                if (_updateRoomReadMarker)
+                                {
+                                    // Try to find the best event for the new read marker position
+                                    [self updateReadMarkerEvent];
+                                }
+                            }
                         }
                     }
                     break;
@@ -2517,6 +2522,118 @@
     }
 }
 
+- (BOOL)eligibleForReadMarkerUpdate:(MXEvent *)event {
+    // Prevent the readmarker to be placed on a relatesTo or a redaction event
+    if (event.relatesTo || event.redacts)
+    {
+        return NO;
+    }
+    
+    return YES;
+}
+
+- (BOOL)isEventPosteriorToCurrentReadMarker:(MXEvent *)event {
+    if (roomDataSource.room.accountData.readMarkerEventId)
+    {
+        MXEvent *currentReadMarkerEvent = [roomDataSource.mxSession.store eventWithEventId:roomDataSource.room.accountData.readMarkerEventId inRoom:roomDataSource.roomId];
+        if (!currentReadMarkerEvent)
+        {
+            currentReadMarkerEvent = [roomDataSource eventWithEventId:roomDataSource.room.accountData.readMarkerEventId];
+        }
+        
+        // Update the read marker only if the current event is available, and the new event is posterior to it.
+        return currentReadMarkerEvent && (currentReadMarkerEvent.originServerTs <= event.originServerTs);
+    }
+    return YES;
+}
+
+/// Try to update the read marker by looking for an eligible event displayed at the bottom of the tableview
+- (void)updateReadMarkerEvent
+{
+    // Compute the content offset corresponding to the line displayed at the table bottom (just above the toolbar).
+    CGFloat contentBottomOffsetY = _bubblesTableView.contentOffset.y + (_bubblesTableView.frame.size.height - _bubblesTableView.adjustedContentInset.bottom);
+    if (contentBottomOffsetY > _bubblesTableView.contentSize.height)
+    {
+        contentBottomOffsetY = _bubblesTableView.contentSize.height;
+    }
+    // Be a bit less retrictive, consider visible an event at the bottom even if is partially hidden.
+    contentBottomOffsetY += kCellVisibilityMinimumHeight;
+    
+    // Consider the visible cells (starting by those displayed at the bottom)
+    NSArray *visibleCells = [_bubblesTableView visibleCells];
+    NSInteger index = visibleCells.count;
+    UITableViewCell *cell;
+    while (index--)
+    {
+        cell = visibleCells[index];
+        
+        // Check whether the cell is actually visible
+        if (!cell || cell.frame.origin.y > contentBottomOffsetY)
+        {
+            continue;
+        }
+
+        if (![cell isKindOfClass:MXKRoomBubbleTableViewCell.class])
+        {
+            continue;
+        }
+        MXKRoomBubbleTableViewCell *roomBubbleTableViewCell = (MXKRoomBubbleTableViewCell *)cell;
+        MXKRoomBubbleCellData *bubbleData = roomBubbleTableViewCell.bubbleData;
+        if (!bubbleData)
+        {
+            continue;
+        }
+        
+        // Prevent to place the read marker on a collapsed cell
+        if (bubbleData.collapsed)
+        {
+            continue;
+        }
+        
+        // Check which bubble component is displayed at the bottom.
+        // For that update each component position.
+        [bubbleData prepareBubbleComponentsPosition];
+        
+        NSArray *bubbleComponents = bubbleData.bubbleComponents;
+        NSInteger componentIndex = bubbleComponents.count;
+        
+        CGFloat bottomPositionY = cell.frame.size.height;
+        
+        MXKRoomBubbleComponent *component;
+        
+        while (componentIndex --)
+        {
+            component = bubbleComponents[componentIndex];
+            
+            // Prevent the read marker to be placed on an unsupported event (e.g. redactions, reactions, ...)
+            if (![self eligibleForReadMarkerUpdate:component.event])
+            {
+                continue;
+            }
+            
+            // Check whether the bottom part of the component is visible.
+            CGFloat pos = cell.frame.origin.y + bottomPositionY;
+            if (pos <= contentBottomOffsetY)
+            {
+                // We found the component
+                // Check whether the read marker must be updated.
+                if ([self isEventPosteriorToCurrentReadMarker:component.event])
+                {
+                    // Move the read marker to this event
+                    [roomDataSource.room moveReadMarkerToEventId:component.event.eventId];
+                }
+                
+                return;
+            }
+            
+            // Prepare the bottom position for the next component
+            bottomPositionY = roomBubbleTableViewCell.msgTextViewTopConstraint.constant + component.position.y;
+        }
+        
+        // else we consider the previous cell.
+    }
+}
+
 #pragma mark - MXKDataSourceDelegate
 
 - (Class<MXKCellRendering>)cellViewClassForCellData:(MXKCellData*)cellData

Riot/Modules/Room/RoomViewController.m

@@ -5323,7 +5323,7 @@ static CGSize kThreadListBarButtonItemImageSize;
         [self dismissKeyboard];
         NSString *eventId = self.roomDataSource.room.accountData.readMarkerEventId;
         NSString *threadId = self.roomDataSource.threadId;
-        [self reloadRoomWihtEventId:eventId threadId:threadId];
+        [self reloadRoomWihtEventId:eventId threadId:threadId forceUpdateRoomMarker:YES];
     }
     else if (sender == self.resetReadMarkerButton)
     {
@@ -6612,8 +6612,12 @@ static CGSize kThreadListBarButtonItemImageSize;
     // Check whether the read marker exists and has not been rendered yet.
     if (self.roomDataSource.isLive && !self.roomDataSource.isPeeking && self.roomDataSource.showReadMarker && self.roomDataSource.room.accountData.readMarkerEventId)
     {
-        UITableViewCell *cell = [self.bubblesTableView visibleCells].firstObject;
-        if ([cell isKindOfClass:MXKRoomBubbleTableViewCell.class] && ![cell isKindOfClass:MXKRoomEmptyBubbleTableViewCell.class])
+        NSPredicate *predicate = [NSPredicate predicateWithBlock:^BOOL(id evaluatedObject, NSDictionary *bindings) {
+            return [evaluatedObject isKindOfClass:MXKRoomBubbleTableViewCell.class];
+        }];
+        NSArray *visibleCells = [[self.bubblesTableView visibleCells] filteredArrayUsingPredicate:predicate];
+        UITableViewCell *cell = visibleCells.firstObject;
+        if (cell)
         {
             MXKRoomBubbleTableViewCell *roomBubbleTableViewCell = (MXKRoomBubbleTableViewCell*)cell;
             // Check whether the read marker is inside the first displayed cell.
@@ -6640,6 +6644,9 @@ static CGSize kThreadListBarButtonItemImageSize;
                 else
                 {
                     self.jumpToLastUnreadBannerContainer.hidden = YES;
+                    
+                    // Force the read marker position in order to not depend on the read marker animation (https://github.com/vector-im/element-ios/issues/7420)
+                    self.updateRoomReadMarker = YES;
                 }
             }
         }
@@ -7919,15 +7926,17 @@ static CGSize kThreadListBarButtonItemImageSize;
         [[AppDelegate theDelegate] showRoomWithParameters:parameters];
     }
 }
+
 - (void)roomInfoCoordinatorBridgePresenter:(RoomInfoCoordinatorBridgePresenter *)coordinator
                        viewEventInTimeline:(MXEvent *)event
 {
     [self.navigationController popToViewController:self animated:true];
-    [self reloadRoomWihtEventId:event.eventId threadId:event.threadId];
+    [self reloadRoomWihtEventId:event.eventId threadId:event.threadId forceUpdateRoomMarker:NO];
 }
 
 -(void)reloadRoomWihtEventId:(NSString *)eventId
                     threadId:(NSString *)threadId
+       forceUpdateRoomMarker:(BOOL)forceUpdateRoomMarker
 {
     // Jump to the last unread event by using a temporary room data source initialized with the last unread event id.
     MXWeakify(self);
@@ -7946,6 +7955,9 @@ static CGSize kThreadListBarButtonItemImageSize;
         
         // Give the data source ownership to the room view controller.
         self.hasRoomDataSourceOwnership = YES;
+        
+        // Force the read marker update if needed (e.g if we jumped on the last unread message using the banner).
+        self.updateRoomReadMarker |= forceUpdateRoomMarker;
     }];
 }
 

changelog.d/7420.bugfix

@@ -0,0 +1 @@
+Update the read marker position even if it is not displayed