fix(sdk): RoomEventCache::event looks inside the store
#4708
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4708 +/- ##
==========================================
+ Coverage 85.96% 85.97% +0.01%
==========================================
Files 290 290
Lines 33995 34065 +70
==========================================
+ Hits 29224 29288 +64
- Misses 4771 4777 +6 ☔ View full report in Codecov by Sentry. |
Hywan
force-pushed
the
feat-sdk-event-cache-event-in-store
branch
from
9c1d4cd to
985807a
Compare
Hywan
force-pushed
the
feat-sdk-event-cache-event-in-store
branch
from
985807a to
ca62b1f
Compare
Hywan
requested review from
bnjbvr
and removed request for
a team and
jmartinesp
crates/matrix-sdk-base/src/event_cache/store/integration_tests.rs
Outdated
Show resolved
Hide resolved
Hywan
force-pushed
the
feat-sdk-event-cache-event-in-store
branch
from
ca62b1f to
78c22d7
Compare
This patch adds the method `find_event` on the `EventCacheStore` trait. It helps to find a single event from the store.
This patch fixes `RoomEventCache::event` to look inside `RoomEvents` but also inside the `EventCacheStore` to look for an event. It ultimately fallbacks to `AllEventsCache` because we can't get rid of it for the moment.
Hywan
force-pushed
the
feat-sdk-event-cache-event-in-store
branch
from
0b3b134 to
5cc427d
Compare
| .await | ||
| .unwrap(); | ||
|
|
||
| // Now let's find out the event. |
There was a problem hiding this comment.
Need to double-check with a native speaker, but I find it weird to have find out here, I'd just say find I suppose.
| // Now let's find out the event. | |
| // Now let's find the event. |
| /// Return an iterator over all items. | ||
| pub fn items(&self) -> impl Iterator<Item = (Position, &Item, &RoomId)> { | ||
| self.items.iter().filter_map(|item_row| { | ||
| if let Either::Item(item) = &item_row.item { | ||
| Some((item_row.position, item, item_row.room_id.as_ref())) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Seems this is unordered_items with an extra return value; can we fuse those two functions?
There was a problem hiding this comment.
oh i found the difference now, unordered_items is keyed by room. Maybe the names could be clearer…
| if let Some(event) = maybe_position_and_event.map(|(_position, event)| event) { | ||
| Some(event) | ||
| } | ||
| // Search in `AllEventsCache` for known events that are not stored. |
There was a problem hiding this comment.
Can you put the comment either above the if / else if chain, or inside the arm that relates to it? I find it super ugly and confusing to have comments break else if chains, because it breaks the flow of reading :|
|
lol i forgot i approved yesterday |
|
Oh damn, sorry!! |
|
opened #4711 with my review suggestions! |
This patch fixes a bug in
RoomEventCache::eventsince #4632 has landed.RoomEventsnow likely contains a partial view of the events, i.e. they are not all loaded from the store to memory.A new
EventCacheStore::find_eventis added to look for an event in the store.RoomEventCache::eventuses it to use the store, with a fallback toAllEventsCachebecause we can't get rid of it right now (see #3886).