feat(ffi): expose a custom timeline builder that can be configured to show only media, images and videos or files and audios #4365
Conversation
stefanceriu
force-pushed
the
stefan/mediaGallery
branch
from
b4dca4e to
e99d0e4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4365 +/- ##
==========================================
+ Coverage 85.09% 85.12% +0.02%
==========================================
Files 280 280
Lines 30547 30547
==========================================
+ Hits 25995 26002 +7
+ Misses 4552 4545 -7 ☔ View full report in Codecov by Sentry. |
bindings/matrix-sdk-ffi/src/room.rs
Outdated
| ImageAndVideo, | ||
| FileAndAudio, |
There was a problem hiding this comment.
A few remarks:
- can you add doc comments for these variants, please?
- it seems a bit arbitrary to join image with video, and file with audio. I suppose these come from ElementX requirements, but we're trying to make the SDK a bit more general (although arguably, the FFI layer is quite adhoc to EX apps). Would it make sense to split those into four variants:
Image,Video,File,Audio, and accept an array of them inmedia_events_timelineinstead? That way, embedders can compose these as they want to, and not have to add new variants in there.
There was a problem hiding this comment.
can you add doc comments
sure thing 👍
Would it make sense to split those into four variants
Yep yep, makes sense to me, I'll make the change
bindings/matrix-sdk-ffi/src/room.rs
Outdated
| ) -> Result<Arc<Timeline>, ClientError> { | ||
| let room = &self.inner; | ||
|
|
||
| let mut builder = matrix_sdk_ui::timeline::Timeline::builder(room); |
There was a problem hiding this comment.
Could we also ask for a timeline internal id prefix in this function's signature, and pass it here? This would help tracking issues in rageshakes, since the internal id prefix is used to distinguish across different timeline instances.
There was a problem hiding this comment.
We may want to add a note that we're not passing a UTD hook here, but that's fine because we should be flagging those in the main timeline anyways.
bindings/matrix-sdk-ffi/src/room.rs
Outdated
| @@ -260,6 +267,33 @@ impl Room { | |||
| Ok(Timeline::new(timeline)) | |||
| } | |||
|
|
|||
| pub async fn media_events_timeline( | |||
There was a problem hiding this comment.
Can you add a rough doc comment, please?
|
Alrighty, I went with exposing all of the possible RoomMessage message types and allowing any combination of them to be passed in as a filter. I'll squash and update the description before merging if everything else is in order 👍 |
…onfigured to only include RoomMessage type events and filter those further based on their message type. Virtual timeline items will still be provided and the `default_event_filter` will be applied before everything else. Instances of these timelines will be used to power the 2 different tabs shown on the new media browser. The client will be responsible for interacting with it similar to a normal timeline and transforming its data into something renderable e.g. section by date separators (which will be made configurable in a follow up PR)
stefanceriu
force-pushed
the
stefan/mediaGallery
branch
from
545fd53 to
7abd774
Compare
Instances of these timelines will be used to power the 2 different tabs shown on the new media browser. The client will be responsible for interacting with it similar to a normal timeline and transforming its data into something renderable e.g. section by date separators (which will be made configurable in a follow up PR)