matrix-org / matrix-org/matrix-rust-sdk

refactor: Make `Timeline` generic over a `Focus` trait

Open
#5,053 1 comment 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.