mirror of
https://github.com/matrix-org/matrix-rust-sdk.git
synced 2026-05-08 07:56:55 -04:00
32e83a942d
There are problems with `SlidingSyncListRequestGenerator`: * It's part of the module API, * It contains a clone of `SlidingSyncList`. To create a `SlidingSyncListRequestGenerator`, one has to call `SlidingSyncList::request_generator`. It was done in `SlidingSync::stream`. The problem is that it clones `SlidingSyncList`. So theoritically it is possible to create multiple request generators for the _same_ list, and use them to send many requests and to update the _same_ list with multiple responses. This is utterly error-prone and can lead to really complex bugs to discover. Moreover, it's a lot of clones. Cloning a `SlidingSyncListRequestGenerator` isn't cheap as it means cloning a `SlidingSyncList`. Moreover, cloning a `SlidingSyncList` isn't cheap as it means cloning the entire struct. Having `SlidingSyncListRequestGenerator` inside the module API also makes the code of `SlidingSync` more complex (why having to deal with lists and request generators at the same time? we must be very careful to maintain both side by side? what if a list is removed but not its request generator? and so on). So. This patch simplifies all that. First off, it extracts all the fields of `SlidingSyncList` into a `SlidingSyncListInner` struct. Then, `SlidingSyncList` has only one field: `Arc<SlidingSyncListInner>`. Boom, it's now cheap to clone it. Second, `SlidingSyncListRequestGenerator` is only a struct with constructors, but there is no extra methods. `SlidingSyncListRequestGenerator` _no longer_ contains a `SlidingSyncList`, and doesn't no need a list at all to work. It's just values. Third, `SlidingSyncList` holds a `SlidingSyncListRequestGenerator`, and only one. Fourth, `SlidingSyncList` (and `SlidingSyncListInner`) now has methods to handle the internal request generator. The initial `impl Iterator for SlidingSyncListRequestGenerator` becomes a simple `SlidingSyncList::next_request` method. Fifth, previously, the `SlidingSyncList::handle_response` was never called directly by `SlidingSync`. It was called by `SlidingSyncListRequestGenerator`! How confusing! `SlidingSync` called `SlidingSyncListRequestGenerator::handle_response` which was calling `SlidingSyncList::handle_response`. Now, the flow is more natural: `SlidingSync` calls `SlidingSyncList::handle_response` and that's it. Sixth, the `SlidingSyncList::handle_response` is now composed of 2 parts: updating the list itself, and updating the request generator. It was kind of the case before, but onto two different types. It was unclear which types were updating `SlidingSyncList`. For example, `SlidingSyncList::state` was updated by… `SlidingSyncListRequestGenerator`. Now `SlidingSyncList` is responsible to update itself, and no one else. Finally, `SlidingSync` no longer have to deal with `SlidingSyncListRequestGenerator`. All it has is a set of `SlidingSyncList`, and that's it! The tests are still passing, hurray.