timeline queue: refactor the code a bit #3433
Conversation
A few renamings here and there, making use of `as_variant!` a bit more, adding a few comments,…
bnjbvr
force-pushed
the
bnjbvr/bnjbvrify-the-timeline-sending-q
branch
from
79fc8fc
to
55540f2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3433 +/- ##
==========================================
+ Coverage 83.12% 83.14% +0.01%
==========================================
Files 247 247
Lines 25016 25008 -8
==========================================
- Hits 20795 20793 -2
+ Misses 4221 4215 -6 ☔ View full report in Codecov by Sentry. |
| // reflected in the timeline, so we set all other pending events to | ||
| // cancelled. | ||
| // | ||
| // TODO(bnjbvr): spooky action at a distance here^ |
There was a problem hiding this comment.
While I like the physics reference and don't want you to remove it, maybe it would make sense what the physicist in you meant here? I can guess that it's probably about one item affecting/cancelling other items, it would be nice to write it down.
There was a problem hiding this comment.
Hah fun, I for one didn't know it was a physics reference xD
https://en.wikipedia.org/wiki/Action_at_a_distance_(computer_programming)
In this case, there's tight coupling between this loop and the behavior of the sending queue, which is just working by coincidence (both paths know they interact together that way), and it's not good. Will update the comment.
| error_return!("Retrying state events is not currently supported"); | ||
| | TimelineItemContent::OtherState(_) | ||
| | TimelineItemContent::CallInvite => { | ||
| error_return!("Retrying state events/call invite is not currently supported"); |
There was a problem hiding this comment.
| error_return!("Retrying state events/call invite is not currently supported"); | |
| error_return!("Retrying state events/call invites is not currently supported"); |
| }, | ||
| ) | ||
| .await; | ||
| return; |
There was a problem hiding this comment.
Why would you do a if/return/implicit return instead of a if/else here? I understand the above one to avoid a nested if, this one seems to go a bit too far with the removal of the nesting.
There was a problem hiding this comment.
I do like that the main operation is not under a conditional, and just lives at the end in the main path, but I can revert this one change here.
Part of #3361. This is me understanding the code and starting to refactor it a bit.