-
Notifications
You must be signed in to change notification settings - Fork 289
refactor(timeline): replace TimelineEventKind
with TimelineAction
#4655
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4655 +/- ##
==========================================
+ Coverage 85.85% 85.89% +0.04%
==========================================
Files 325 325
Lines 35855 35871 +16
==========================================
+ Hits 30784 30813 +29
+ Misses 5071 5058 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This information can be accessed via the `TimelineMetadata::own_user_id` field, which is instantiated only once.
Once again, this information is available in the `TimelineMetadata`.
The intent is that the `TimelineItemContent` can be constructed in a single place, avoiding the need to create it in multiple places.
…and state events
…entKind::AddItem` items
…for non-items This variant will be used to cause side-effects to existing timeline items, because the event they relate to is for an aggregation (edit/reaction/etc.). This is used here for reactions.
…ll edits/responses/ends
1234cc7
to
0071fef
Compare
TimelineEventKind
TimelineEventKind
with TimelineAction
0071fef
to
1011700
Compare
…and add comments
…ems for local events
1011700
to
802049f
Compare
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.
That's a very nice refactoring. I've enjoyed reading the patches, well done. Everything is clear, and I appreciate the new state of this particular flow, it's a clear improvement. It also gently paves the road for thread support.
Thank you!
This centralizes (and simplifies, IMO) the creation of new timeline items vs handling of aggregations. Before, this was done partly when creating the (type previously known as)
TimelineEventKind
and handling it later in theTimelineEventHandler
. Now, the event is analyzed once, and analyzing the event results in three outcomes:TimelineAction::AddItem
TimelineAction::HandleAggregation
That way, it should be much simpler to handle aggregation-only events in non-live timelines, which will be handy for thread timelines.
Part of #4869.