matrix-org / matrix-org/matrix-rust-sdk
refactor: Make `Timeline` generic over a `Focus` trait
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 2.3k
- Forks
- 500
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 106
Description
Hej hej,
Here is a proposal. `Timeline` so far holds the logic for different focuses, i.e. modes/types. We have the following focuses defined by the `TimelineFocus` enum:
- `Live`, focus on live events, it's all real-time,
- `Event`, focus on a specific event, like with a permalink,
- `Thread`, focus on a specific thread root,
- `PinnedEvent`, only focus on pinned events.
The problem is that we are matching over this `TimelineFocus` enum in several places in the code. It's not fantastic to test and to maintain. We have a couple of `is_live` or `is_pinned_events` variables for example, which is really not great. Or we have different pagination behaviours based on the focus. Then we also have `TimelineFocusData` and `TimelineFocusKind` (which is like `TimelineFocus` or `TimelineFocusData` but without any data). Well, it becomes confusing.
The proposal is to make `Timeline` generic over a trait that represents a focus, which will isolate the different behaviours. Basically, `TimelineFocus` would become a trait.
```rust
trait TimelineFocus {
// Replace `TimelineFocusData`. It could also be named `Data`, but it's a bit too generic.
type Context;
// Replace the loader we have in most focuses.
// `TimelineLoader` is likely to be another trait.
type Loader: TimelineLoader;
async fn loader(&self) -> Option;
async fn latest_event(&self) -> Option;
async fn paginate_backwards(&self, …) -> …;
async fn paginate_forwards(&self, …) -> …;
// Probably more. Need to dig into the code in details.
…
}
```
and then:
```rust
struct Live { … }
struct LiveContext { … }
impl TimelineFocus for Live {
type Context = LiveContext;
type Loader = ();
async fn loader(&self) -> Option {
None
}
async fn latest_event(&self) -> Option { … }
…
}
```
Pros of having a trait:
- We immediately know what a timeline can or can't do based on its type: `Timeline`, `Timeline`, `Timeline`…
- The different behaviours are coded in standalone types: `Live`, `PinnedEvent`, `Thread`… which make them easier to test! We are sure to not miss a particular edge case or combination that could be hard to trigger with a regular `Timeline` in front.
- It's more easily extensible if other focuses are added (the last one being added is `Thread` for example).
Cons of having a trait:
- We need a good abstraction and it can be difficult to find sometimes (but it's for the better I believe).
- On the FFI side, we would need a `TimelineLive` type to wrap `Timeline`, `TimelineThread` to wrap `Timeline` and so on probably. It's likely to imply code duplication for the same operations. A macro can help though. We _may_ be able to solve this by using a:
```rust
pub struct Timeline {
inner: Box>,
}
```
or something. Didn't try. Just a sketchy idea. I don't know if UniFFI will support that.
Thoughts?
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by tracing TimelineFocus, TimelineFocusData, TimelineFocusKind, and the existing Timeline implementations to map their matching and pagination behavior. Then inspect the FFI and UniFFI boundary, including the proposed Timeline and TimelineLoader shapes; done would require a settled abstraction covering the existing focuses and their exposed operations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- api
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100