Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Ensure we always show read receipts even with hidden events #3056

Merged
merged 5 commits into from
Jun 4, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Jun 3, 2019

This work fixes up the display side of read receipts so that we ensure a user's read receipt remains somewhere on the timeline even when

  • there are hidden events
  • data is out of sync between the events and receipts

Part of element-hq/element-web#9745
Fixes element-hq/element-web#5761

jryans added 2 commits June 3, 2019 15:05
This changes how we determine read receipts for the entire message panel. We now
calculate read receipts for all events up front, which makes it easier to handle
hidden events by moving their read receipts up to the last shown event for
display purposes.

Part of element-hq/element-web#9745
This adds additional receipt storage to so that we can handle cases where the
receipts and events lists get out of sync. If we ever find a user who previously
had a receipt but momentarily no longer does, we recover their previous receipt
and go with that until we hear something new.

Part of element-hq/element-web#9745
@turt2live
Copy link
Member

turt2live commented Jun 3, 2019

Does this happen to fix element-hq/element-web#5761 (the problem within, not necessarily the solution proposed)?

@jryans
Copy link
Collaborator Author

jryans commented Jun 3, 2019

Does this happen to fix vector-im/riot-web#5761 (the problem within, not necessarily the solution proposed)?

Yes, it looks like it should if I understand that issue correctly... If someone has their read receipt at a hidden event, this change will wind that back up to the last displayed event so that it's always shown.

@turt2live
Copy link
Member

That indeed is the problem, so yay!

@turt2live turt2live self-requested a review June 3, 2019 17:09
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm

There's quite a few for loops in here - how does this behave on a davetest-style account?

src/components/structures/MessagePanel.js Outdated Show resolved Hide resolved
src/components/structures/MessagePanel.js Show resolved Hide resolved
@jryans
Copy link
Collaborator Author

jryans commented Jun 4, 2019

There's quite a few for loops in here - how does this behave on a davetest-style account?

Good question! I don't think account size is the limiting factor, but instead the number of people and events in the current room. Here's some timing info from Matrix HQ:

Test Read receipt calc time per render
Develop 0 - 2 ms
This PR as reviewed 3 - 13 ms

So, that's not great... 😖 Initial profiling suggests it's less about the existence of loops and more about calling _shouldShowEvent, which seems to have some slow paths. I'll optimise further before landing.

@jryans
Copy link
Collaborator Author

jryans commented Jun 4, 2019

After further tweaking, we have:

Test Read receipt calc time per render
Develop 0 - 2 ms
This PR as reviewed 3 - 13 ms
... plus cached hidden events setting value 0 - 4 ms
... plus use room prop 0 - 4 ms

I think that's probably good enough for now. We could optimise further by caching the settings reads in shouldHideEvent if needed.

jryans added 2 commits June 4, 2019 16:01
Settings is too expensive to query in a hot code path, so this caches the value
on the MessagePanel component instead.
For some reason, we were getting the room object for every event during read
receipt processing, even though it has been passed in as a prop already.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read receipts should appear on the last visible event instead of disappearing into the ether
2 participants