Fix: Update the read marker position even if it is not displayed #7461
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #7461 +/- ##
===========================================
- Coverage 12.30% 12.29% -0.01%
===========================================
Files 1645 1645
Lines 163052 163135 +83
Branches 66936 66982 +46
===========================================
+ Hits 20059 20064 +5
- Misses 142344 142421 +77
- Partials 649 650 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
@giomfo I've updated this PR to reflect your comments. |
| @@ -7946,6 +7955,9 @@ -(void)reloadRoomWihtEventId:(NSString *)eventId | |||
|
|
|||
| // Give the data source ownership to the room view controller. | |||
| self.hasRoomDataSourceOwnership = YES; | |||
|
|
|||
| // Force the read marker update if needed (this | |||
nimau
force-pushed
the
nimau/7420-readmarker
branch
from
5c18086
to
dc18e22
Compare
nimau
requested review from
a team and
stefanceriu
and removed request for
a team
| contentBottomOffsetY = _bubblesTableView.contentSize.height; | ||
| } | ||
| // Be a bit less retrictive, consider visible an event at the bottom even if is partially hidden. | ||
| contentBottomOffsetY += 8; |
There was a problem hiding this comment.
This 8 is a bit magical, maybe you should move it to a constant at the top of the file
There was a problem hiding this comment.
Ok, I moved this value to a new constant: kCellVisibilityMinimumHeight
| - (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); |
There was a problem hiding this comment.
Do we need any special handling for when the keyboard is displayed or better yet do we need to tweak the ContentInsetAdjustmentBehavior?
There was a problem hiding this comment.
I’m not sure. I used the same mechanism as the one used to find the event to acknowledge because the read marker was updated in the same process as the acknowledgement.
| cell = visibleCells[index]; | ||
|
|
||
| // Check whether the cell is actually visible | ||
| if (cell && (cell.frame.origin.y < contentBottomOffsetY)) |
There was a problem hiding this comment.
This code would be a lot cleaner with early bail outs e.g.:
if (!cell || cell.frame.origin.y > contentBottomOffsetY) {
continue
}
| while (componentIndex --) | ||
| { | ||
| component = bubbleComponents[componentIndex]; | ||
| if (![cell isKindOfClass:MXKRoomBubbleTableViewCell.class]) |
There was a problem hiding this comment.
Can we combine this check with the previous isKindOfClass:MXKTableViewCell.class and maybe use the casted version from then on? 🤔
There was a problem hiding this comment.
Done, it should be much easier to understand now.
|
Kudos, SonarCloud Quality Gate passed!
|
This PR fixes #7420
The current implementation only updates the read marker if we are able to display the read marker view and we have identified some cases where the read marker is not updated because its view cannot be displayed:
Also, relatesTo events can be problematic as they can cause the user to jump backwards in the timeline, even if the current read marker timestamp is more recent than the related event.
To fix these issues:
Pull Request Checklist