feat(timeline): utilize the cache and include common relations when focusing on an event without context #5858
Conversation
CodSpeed Performance ReportMerging #5858 will not alter performanceComparing Summary
|
Johennes
force-pushed
the
johannes/focus-no-context
branch
2 times, most recently
from
1b0b967 to
fa1bcaf
Compare
…ocusing on an event without context Signed-off-by: Johannes Marbach <[email protected]>
Johennes
force-pushed
the
johannes/focus-no-context
branch
from
fa1bcaf to
93dc44a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5858 +/- ##
==========================================
- Coverage 88.65% 88.64% -0.01%
==========================================
Files 362 362
Lines 103146 103163 +17
Branches 103146 103163 +17
==========================================
+ Hits 91440 91446 +6
- Misses 7458 7469 +11
Partials 4248 4248 ☔ View full report in Codecov by Sentry. |
Hywan
requested review from
Hywan and
stefanceriu
and removed request for
andybalaam
| let events = if *num_context_events == 0 { | ||
| // If no context is requested, try to load the event from the cache first and | ||
| // include common relations such as reactions and edits. | ||
| let request_config = Some(RequestConfig::default().retry_limit(3)); | ||
| let relations_filter = | ||
| Some(vec![RelationType::Annotation, RelationType::Replacement]); | ||
|
|
||
| // Load the event from the cache or, failing that, the server. | ||
| match self | ||
| .room_data_provider | ||
| .load_event_with_relations(event_id, request_config, relations_filter) | ||
| .await | ||
| { | ||
| Ok((event, related_events)) => { | ||
| let mut events = vec![event]; | ||
| events.extend(related_events); | ||
| events | ||
| } | ||
| Err(err) => { | ||
| warn!("error when loading focussed event: {err}"); | ||
| vec![] | ||
| } | ||
| } | ||
| } else { | ||
| // Start a /context request to load the focussed event and surrounding events. | ||
| let start_from_result = event_paginator | ||
| .start_from(event_id, (*num_context_events).into()) | ||
| .await | ||
| .map_err(PaginationError::Paginator)?; | ||
| start_from_result.events | ||
| }; |
There was a problem hiding this comment.
A potential issue I'm now seeing is that the if branch will leave the paginator in initial state meaning you cannot use it to extend the timeline later. This isn't really an issue for the intended event details use case but might still be problematic. Maybe this is further reason to extend TimelineFocus::PinnedEvents rather than TimelineFocus::Event? We could just add a different loader type that loads a single event only based on a specified ID.
There was a problem hiding this comment.
Is it related to my feedback about falling back to /context if the if branch fails?
There was a problem hiding this comment.
I think partially. If the if branch does not fail, we'd still leave the paginator in initial state with the current implementation though. This might be ok, however, since pagination doesn't seem like a likely use case when loading no context at all.
Hywan
left a comment
Hywan
left a comment
There was a problem hiding this comment.
Thanks for the PR. Can you add a test when num_context_events = 0 to test the new code please?
| let events = if *num_context_events == 0 { | ||
| // If no context is requested, try to load the event from the cache first and | ||
| // include common relations such as reactions and edits. | ||
| let request_config = Some(RequestConfig::default().retry_limit(3)); | ||
| let relations_filter = | ||
| Some(vec![RelationType::Annotation, RelationType::Replacement]); | ||
|
|
||
| // Load the event from the cache or, failing that, the server. | ||
| match self | ||
| .room_data_provider | ||
| .load_event_with_relations(event_id, request_config, relations_filter) | ||
| .await | ||
| { | ||
| Ok((event, related_events)) => { | ||
| let mut events = vec![event]; | ||
| events.extend(related_events); | ||
| events | ||
| } | ||
| Err(err) => { | ||
| warn!("error when loading focussed event: {err}"); | ||
| vec![] | ||
| } | ||
| } | ||
| } else { | ||
| // Start a /context request to load the focussed event and surrounding events. | ||
| let start_from_result = event_paginator | ||
| .start_from(event_id, (*num_context_events).into()) | ||
| .await | ||
| .map_err(PaginationError::Paginator)?; | ||
| start_from_result.events | ||
| }; |
There was a problem hiding this comment.
Is it related to my feedback about falling back to /context if the if branch fails?
… when focusing on an event without context Switch to error and fallback to /context when loading single event fails Signed-off-by: Johannes Marbach <[email protected]>
… when focusing on an event without context Add tests with num_context_events = 0 Signed-off-by: Johannes Marbach <[email protected]>
… when focusing on an event without context Use different event ID to work around spurious test failure Signed-off-by: Johannes Marbach <[email protected]>
2 participants
At the moment,
TimelineController::init_focusdoesn't use the event cache at all in theTimelineFocus::Eventarm. I believe this is because the cache currently cannot load surrounding context for an event. However, when no context is requested, meaningnum_context_events = 0, this result in a potentially superfluous/contextrequest.This pull request, differentiates this special case and attempts to load the event from the cache before falling back to the server.
Additionally, common relations (reactions & edits) are included, if possible. This is similar to the behavior in the
TimelineFocus::PinnedEventsarm and allows using focused timelines to power event details views.Open questions:
Given the similarity to the
TimelineFocus::PinnedEventsbehavior, I'm not sure if it might be better to generalizeTimelineFocus::PinnedEventsto support both cases.It would be great if
TimelineFocus::Eventwould make use of the event cache regardless of whether context was requested or not. I'm not sure if that's feasibly possible though.Public API changes documented in changelogs (optional)