Skip to content

ffi: refactor timeline creation and don't maintain timeline instances alive #4718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
5 tasks
bnjbvr opened this issue Feb 25, 2025 · 1 comment
Open
5 tasks

Comments

@bnjbvr
Copy link
Member

bnjbvr commented Feb 25, 2025

There are multiple ways to create a timeline as of now, in the SDK-UI and the FFI layer, and some of these ways even involve holding onto a live timeline instance.

This is problematic, because we now have a mechanism to automatically make opening a timeline instance super fast, but for this, we need to make sure that there's no alive timeline in the background, otherwise it won't trigger.

So as to avoid this, and also simplify the FFI APIs to create a timeline, we should get rid of those cached timeline instances. I've spotted a few places:

  • the FFI Room may hold onto a live timeline instance, when it's created from RoomListItem::full_room. This is used only to retrieve that timeline, or mark a room as read; in this case, we can plainly create a throwaway timeline and do it immediately
  • the FFI RoomListItem may hold onto a live time instance. In fact, if you want to get the Room from the RoomListItem::full_room, you'll need the latter's timeline to be initialized. This is a bit strange, because the room list item is likely transient, and holding a live timeline item into it doesn't quite make sense, as the timeline's refcount will decrease as soon as the room list item is gone, likely leading to the timeline being dropped. Also, you might want to get a handle to the underlying Room without manipulating its timeline, and the timeline MUST be initialized so as to get the room. The pattern of initializing the timeline of a RoomListItem is a bit contrived, involving several steps (checking if it's been initialized before, and initializing it only in that case).

Instead of these two, it might be possible to only use Room::timeline_with_configuration, provided that it implements as much as RoomListItem::init_timeline. For that to happen, we need the following:

  • make sure that we have access to the UTD hook in Room::timeline_with_configuration.
    • either by providing it as an argument (bleh), or by stashing it in one FFI structure and reusing it there, or by having a UTD hook in the SDK's client.
  • make sure that we can filter on event type, in addition to message types, in Room::timeline_with_configuration.
  • Then, the room_list_service::room::RoomInner shouldn't make use of its inner timeline; it's only used in a direct getter, or to compute the latest room event, but it's not working as intended, since local echoes aren't properly displayed in the room list. This non-working feature can be removed, in favor of [meta] Latest Event API, the missing one #4112 instead.
  • Creating a timeline Room::timeline_with_configuration should fill the room's event cache with the sliding sync queue, until we enable the persistent storage for realz for everyone forever (aka do what is done in default_room_timeline_builder()). Unfortunately, this requires access to the underlying sliding sync, but there might be ways to make it happen some place else.
  • Once we've done those steps, we should be isofunctional, and could get rid of RoomListItem::init_timeline, as well as the stored timeline in the underlying SDK UI data structure, if not the entirety of room_list_service::room::Room.
@bnjbvr bnjbvr self-assigned this Feb 25, 2025
@bnjbvr
Copy link
Member Author

bnjbvr commented Feb 26, 2025

This might have to wait for the persisted event cache storage to land first, otherwise this could create timelines without force-feeding the event cache, if persistence is disabled.

@bnjbvr bnjbvr removed their assignment Feb 26, 2025
stefanceriu added a commit that referenced this issue May 19, 2025
… timeline

As per the plan defined in #4718:

```
the room_list_service::room::RoomInner shouldn't make use of its inner timeline;
it's only used in a direct getter, or to compute the latest room event, but it's not working
as intended, since local echoes aren't properly displayed in the room list.
This non-working feature can be removed, in favor of #4112
```
stefanceriu added a commit that referenced this issue May 21, 2025
… timeline

As per the plan defined in #4718:

```
the room_list_service::room::RoomInner shouldn't make use of its inner timeline;
it's only used in a direct getter, or to compute the latest room event, but it's not working
as intended, since local echoes aren't properly displayed in the room list.
This non-working feature can be removed, in favor of #4112
```
stefanceriu added a commit that referenced this issue May 21, 2025
… timeline

As per the plan defined in #4718:

```
the room_list_service::room::RoomInner shouldn't make use of its inner timeline;
it's only used in a direct getter, or to compute the latest room event, but it's not working
as intended, since local echoes aren't properly displayed in the room list.
This non-working feature can be removed, in favor of #4112
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant