Skip to content
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.

Sign up for GitHub

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

feat(ffi): generate formatted captions for send_* media fns #4261

Merged
merged 2 commits into from
Nov 14, 2024
Merged

feat(ffi): generate formatted captions for send_* media fns #4261

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e75bdd
feat(ffi): generate formatted captions for `send_*` media fns
jmartinesp Nov 13, 2024
5d77f32
refactor(sdk): move `formatted_caption_from` to the SDK, rename it
jmartinesp Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions bindings/matrix-sdk-ffi/src/timeline/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ use ruma::{
},
receipt::ReceiptThread,
room::message::{
ForwardThread, LocationMessageEventContent, MessageType,
RoomMessageEventContentWithoutRelation,
FormattedBody as RumaFormattedBody, ForwardThread, LocationMessageEventContent,
MessageType, RoomMessageEventContentWithoutRelation,
},
AnyMessageLikeEventContent,
},
Expand Down Expand Up @@ -289,6 +289,7 @@ impl Timeline {
progress_watcher: Option<Box<dyn ProgressWatcher>>,
use_send_queue: bool,
) -> Arc<SendAttachmentJoinHandle> {
let formatted_caption = formatted_caption_from(&caption, &formatted_caption);
SendAttachmentJoinHandle::new(RUNTIME.spawn(async move {
let base_image_info = BaseImageInfo::try_from(&image_info)
.map_err(|_| RoomError::InvalidAttachmentData)?;
Expand All @@ -297,7 +298,7 @@ impl Timeline {
let attachment_config = build_thumbnail_info(thumbnail_url, image_info.thumbnail_info)?
.info(attachment_info)
.caption(caption)
.formatted_caption(formatted_caption.map(Into::into));
.formatted_caption(formatted_caption);

self.send_attachment(
url,
Expand All @@ -321,6 +322,7 @@ impl Timeline {
progress_watcher: Option<Box<dyn ProgressWatcher>>,
use_send_queue: bool,
) -> Arc<SendAttachmentJoinHandle> {
let formatted_caption = formatted_caption_from(&caption, &formatted_caption);
SendAttachmentJoinHandle::new(RUNTIME.spawn(async move {
let base_video_info: BaseVideoInfo = BaseVideoInfo::try_from(&video_info)
.map_err(|_| RoomError::InvalidAttachmentData)?;
Expand Down Expand Up @@ -351,6 +353,7 @@ impl Timeline {
progress_watcher: Option<Box<dyn ProgressWatcher>>,
use_send_queue: bool,
) -> Arc<SendAttachmentJoinHandle> {
let formatted_caption = formatted_caption_from(&caption, &formatted_caption);
SendAttachmentJoinHandle::new(RUNTIME.spawn(async move {
let base_audio_info: BaseAudioInfo = BaseAudioInfo::try_from(&audio_info)
.map_err(|_| RoomError::InvalidAttachmentData)?;
Expand Down Expand Up @@ -383,6 +386,7 @@ impl Timeline {
progress_watcher: Option<Box<dyn ProgressWatcher>>,
use_send_queue: bool,
) -> Arc<SendAttachmentJoinHandle> {
let formatted_caption = formatted_caption_from(&caption, &formatted_caption);
SendAttachmentJoinHandle::new(RUNTIME.spawn(async move {
let base_audio_info: BaseAudioInfo = BaseAudioInfo::try_from(&audio_info)
.map_err(|_| RoomError::InvalidAttachmentData)?;
Expand Down Expand Up @@ -710,6 +714,24 @@ impl Timeline {
}
}

/// Given a pair of optional `caption` and `formatted_caption` parameters,
/// return a formatted caption:
///
/// - If a `formatted_caption` exists, return it.
/// - If it doesn't exist but there is a `caption`, parse it as markdown and
/// return the result.
/// - Return `None` if there are no `caption` or `formatted_caption` parameters.
fn formatted_caption_from(
caption: &Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use Option<&str> and have the caller use .deref() here

formatted_caption: &Option<FormattedBody>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't pass formatted_caption by ref, since it must be consumed by this function; avoids a clone() below

) -> Option<RumaFormattedBody> {
match (&caption, formatted_caption) {
(None, None) => None,
(Some(body), None) => RumaFormattedBody::markdown(body),
(_, Some(formatted_body)) => Some(formatted_body.clone().into()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would invert those two branches, otherwise a body and a formatted_body will result in the body overriding the formatted body 🤪

…This needs a test 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would invert those two branches, otherwise a body and a formatted_body will result in the body overriding the formatted body 🤪

I don't mind, but can't the branch that overrides only happen if there is no formatted body (body -> Some, formatted_body -> None)? Maybe I misunderstood how this pattern matching of tuples work.

…This needs a test 😁

I guess I can move this into the SDK crate then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right, sorry I misread that. That would then be clearer as a series of if let some return, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter much but I find the match much clearer, every single branch is neatly presented to you and the compiler ensures that all the branches are handled.

A series of if let Some return let's you skip branches and then I need to spend more time investigating which branches are skipped if any.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter much but I find the match much clearer

I just changed it 😭 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, though I don't really think you need to change it back. I'm just saying that using a match like that is completely fine and even preferred by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found it more expressive. Since it states all 3 possible causes and the states of the variables for those. Anyway, it's not a big deal as you said.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was definitely confused and misread it, I do find the new code much much clearer since it's in-your-face at the beginning that the formatted body is preferred over anything else, so YMMV, heh :)

}
}

/// A handle to perform actions onto a local echo.
#[derive(uniffi::Object)]
pub struct SendHandle {
Expand Down
Loading