-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
event cache: internalize handling of the account data #3276
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3276 +/- ##
==========================================
- Coverage 83.61% 83.60% -0.01%
==========================================
Files 238 238
Lines 24585 24571 -14
==========================================
- Hits 20556 20542 -14
Misses 4029 4029 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and I prefer a more-specific API in general terms. I don't think I understand well enough to comment on this exact choice of API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work. However, I'm not sure about this changes. The idea is to remove RoomEventCacheUpdate
to replace it by a single Stream<Item = Vec<VectorDiff<SyncTimelineEvent>>>
, and I don't know how this feature could fit in the next API.
The timeline will always need to listen to those account data changes, or at least the "read marker has been updated" event, so:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a discussion about the possible evolution of this API in private, I'm approving this PR :-). Thanks!
(By moving handling of the fully read marker into the event cache itself.)
003ff4c
to
9885dfe
Compare
This is a relatively simple change: this moves handling the fully read marker for a given room into the event cache, so listeners can react to a specific event for that, instead of handling themselves all the account data changes.
Not sure if that's a better API than just returning all the account data changes, so I would love feedback on this. At least it's sufficient for the UI timeline use case, and so it seems sufficient right now (and we could make it different later).
Part of #3058.