timeline: make use of the sending queue #3475
Conversation
bnjbvr
force-pushed
the
bnjbvr/send-q-in-timeline
branch
from
0d79bec to
125ffe4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3475 +/- ##
==========================================
+ Coverage 83.71% 83.80% +0.09%
==========================================
Files 255 254 -1
Lines 25773 25729 -44
==========================================
- Hits 21575 21562 -13
+ Misses 4198 4167 -31 ☔ View full report in Codecov by Sentry. |
|
Complement-Crypto is unhappy likely due to the same reason as #3261 |
bnjbvr
force-pushed
the
bnjbvr/send-q-in-timeline
branch
2 times, most recently
from
e70e0e1 to
545e54d
Compare
bnjbvr
force-pushed
the
bnjbvr/send-q-in-timeline
branch
from
0941b60 to
eca6f3e
Compare
|
(Note: will need a rebase after #3496 lands) |
Can we delete |
What should we do if we want to redact an item that's not loaded in any timeline? For example redacting a poll in which you have the pollStartEventID but the real event is somewhere in unloaded past history. |
No, Room::redact should still exist for SDK users not using the Timeline. My comment above is specific to embedders making use of the Timeline, and who don't want to have to remember |
There was a problem hiding this comment.
Marking the PR as “Request changes” because of #3475 (comment), I think it's important.
bnjbvr
force-pushed
the
bnjbvr/send-q-in-timeline
branch
from
eca6f3e to
d846a58
Compare
Technically, the test already passed before the change in the builder, because `TimelineEventHandler::handle_event` already filters out local events on non-live timelines, but it's a waste of resources to even spawn the local echo listener task in that case, so let's not do it.
…cal echo The test relied on the fact that sending an event from a given timeline is not observable from another timeline. Indeed, it sent a message using a first timeline object, then constructed a second timeline object and expected only the remote event to be in there. Now, the sending queue is shared across all instances of a Room, thus all instances of a Timeline, and the second timeline can see the local echo for the message sent by the first timeline. The "fix" is thus in the test structure itself: when waiting for the remote echo to be there, check that the timeline item doesn't pertain to a local echo, i.e. is a remote echo.
It was used after a previous local echo couldn't be sent (i.e. the sending queue failed to send it). However, it was slightly incorrect to mark those as cancelled, since those local echoes would still have corresponding items in the sending queue, and would be retried as soon as the sending queue was reenabled and could send events. Instead, this is removed: it means that previously cancelled events would be in the NotSentYet state, which is correct. (At the limit, we could even get rid of the SendingFailed variant, since the sending queue will automatically retry as soon as possible.)
Also reunify two methods that did the same thing, with slightly different semantics, and test the one that wasn't tested at all. Note that `is_editable()` was already exposed to the FFI and used in EX apps.
bnjbvr
force-pushed
the
bnjbvr/send-q-in-timeline
branch
from
d846a58 to
c0c5e5a
Compare
This is part 2 of #3465, extracted to make review easier. This integrates the new sending queue with the timeline, exposes FFI methods, and adds some way to quickly test the new queuing mechanism in
multiverse.Note to embedders: to allow cancelling a local echo, embedders should use
Timeline::redact()instead ofRoom::redact(). This requires anEventTimelineItem, which can be retrieved usingget_event_timeline_item_with_event_id(~for remote echoes) orget_event_timeline_item_with_transaction_id(for local echoes).Part of #3361.