-
Notifications
You must be signed in to change notification settings - Fork 260
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
send queue: allow editing local echoes #3616
Conversation
248e23a
to
df636eb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3616 +/- ##
==========================================
+ Coverage 84.18% 84.20% +0.02%
==========================================
Files 255 255
Lines 26391 26490 +99
==========================================
+ Hits 22217 22306 +89
- Misses 4174 4184 +10 ☔ View full report in Codecov by Sentry. |
a4a4d02
to
d2255f0
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.
It looks generally good to me. The fact the patches modify both the SendQueue
, the database, and the Timeline
make it a bit difficult to follow, but thanks to your atomic commits, it wasn't that much complicated.
Please rebase your fixup!
patches before merging your PR.
let AnyMessageLikeEventContent::RoomMessage(content) = content else { | ||
// Ideally, we'd support replacing local echoes for a reaction, etc., but | ||
// handling RoomMessage should be sufficient in most cases. Worst | ||
// case, the local echo will be sent Soon™ and we'll get another chance at |
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.
“Soon™” 😄
By the way, I agree with this. It means someone reacts to its own local event, and wants to change that. That's clearly an edge case.
new_content: Arc<RoomMessageEventContentWithoutRelation>, | ||
) -> Result<bool, ClientError> { | ||
let edit_info = item.0.edit_info().map_err(|err| anyhow::anyhow!(err))?; |
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.
ClientError::from(err)
doesn't work (to replace anyhow!
)?
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.
Yes, with a new conversion method. I'll add it. We should do a pass at making better errors at the FFI layer some day.
The first parameter doesn't need to be taken by ownership, reference is sufficient.
It also resets the wedged state, so that the queue will retry to send this event later. This will show useful in the following case: when an event is too big, we can now retry to send it, even if it was blocked, by splitting the event instead of copy/abort/recreate it.
…roduce a more general `Timeline::edit`
c0919b5
to
a3ae048
Compare
a3ae048
to
0155ddf
Compare
Final step of #3361, as a bonus.
Fixes #3361.