refactor: Make Timeline generic over a Focus trait

[Hywan]

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.

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<Self::Loader>;
    async fn latest_event(&self) -> Option<EventTimelineItem>;
    async fn paginate_backwards(&self, …) -> …;
    async fn paginate_forwards(&self, …) -> …;
    // Probably more. Need to dig into the code in details.
    …
}

and then:

struct Live { … }

struct LiveContext { … }

impl TimelineFocus for Live {
    type Context = LiveContext;
    type Loader = ();

    async fn loader(&self) -> Option<Self::Loader> {
        None
    }

    async fn latest_event(&self) -> Option<EventTimelineItem> { … }
    …
}

Pros of having a trait:

• We immediately know what a timeline can or can't do based on its type: Timeline<Live>, Timeline<PinnedEvent>, Timeline<Thread>…
• 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<Live>, TimelineThread to wrap Timeline<Thread> 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:

  pub struct Timeline {
      inner: Box<Timeline<dyn TimelineFocus>>,
  }

  or something. Didn't try. Just a sketchy idea. I don't know if UniFFI will support that.

Thoughts?

[bnjbvr]

Thanks. On the one hand I agree that the profusion of TimelineFocus / TimelineFocusKind / TimelineFocusData is weird and confusing; it's grown organically according to our needs, and would benefit from getting refactored. Having a better abstraction to represent what's possible in one timeline kind or the other would indeed be great.

On the other hand, not sure a new trait is the best way to achieve this: most methods would be implemented exactly the same way, so apart from the "loader", I'm not sure we'd have some many things that are different from one impl to the other.

Besides, there's another big issue in our current design: we're using the all_remote_events to note all the events the current timeline knows about. For the live timeline, it's all good and well; after all, the live timeline is listening to all the events coming down from the sync, so they're indeed all the remote events.

For the other timelines, though, it's a lie: we're still accumulating metadata from the events loaded in the context of those timelines, and putting them into all_remote_events, but they're not modeled after the live updates from the event cache. As a result, we can't apply the live updates in real-time (because the indices from the event cache updates won't match any of the events backing the non-live timeline), and have to complicate the implementation of handling live events related to a non-live timeline (see last commit in #5060).

This suggests that the "model" powering the timeline items should be different, whether we're on a live timeline, or a non-live timeline. It could be that a live timeline uses AllRemoteEvents, while other timelines have it too in addition to their actual Vec<TimelineEvent>. This might benefit from the abstraction we're talking about in this issue, be it a new trait or something else.