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

jmartinesp · 2024-11-13T15:35:24Z

At the moment we could only pass existing caption and formatted_caption parameters, assuming they were either plain text for caption or already parsed for formatted_caption. With these changes, the caption parameter is now treated as MD and transformed to HTML which will be used as formatted_caption if none is provided.

Bonus screenshot:

Public API changes documented in changelogs (optional)

Signed-off-by:

Changelog: For `Timeline::send_*` fns, treat the passed `caption` parameter as markdown and use the HTML generated from it as the `formatted_caption` if there is none.

codecov · 2024-11-13T15:52:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.91%. Comparing base (aca83fb) to head (5d77f32).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4261      +/-   ##
==========================================
- Coverage   84.92%   84.91%   -0.01%     
==========================================
  Files         274      274              
  Lines       29805    29805              
==========================================
- Hits        25311    25309       -2     
- Misses       4494     4496       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-11-13T16:17:03Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+/// - Return `None` if there are no `caption` or `formatted_caption` parameters.
+fn formatted_caption_from(
+    caption: &Option<String>,
+    formatted_caption: &Option<FormattedBody>,


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

bnjbvr · 2024-11-13T16:17:16Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+///   return the result.
+/// - Return `None` if there are no `caption` or `formatted_caption` parameters.
+fn formatted_caption_from(
+    caption: &Option<String>,


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

bnjbvr · 2024-11-13T16:18:00Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+        (Some(body), None) => RumaFormattedBody::markdown(body),
+        (_, Some(formatted_body)) => Some(formatted_body.clone().into()),


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 😁

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.

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

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.

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

I just changed it 😭 .

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.

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.

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 :)

jmartinesp · 2024-11-14T08:45:20Z

It seems like, if I enable the markdown feature as default for the main SDK crate I also need to add it for testing for it to work.

bnjbvr

Thanks for the tests!

bnjbvr · 2024-11-14T08:48:42Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+        (Some(body), None) => RumaFormattedBody::markdown(body),
+        (_, Some(formatted_body)) => Some(formatted_body.clone().into()),


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 :)

bnjbvr · 2024-11-14T08:49:43Z

crates/matrix-sdk/src/utils.rs

+/// - If a `formatted_body` exists, return it.
+/// - If it doesn't exist but there is a `body`, parse it as markdown and return
+///   the result.
+/// - Return `None` if there are no `body` or `formatted_body` parameters.


Think this comment could be more concise: "return the formatted body if available, or interpret the body as markdown, if provided."

bnjbvr · 2024-11-14T08:50:17Z

crates/matrix-sdk/src/utils.rs

+    } else if let Some(body) = body {
+        FormattedBody::markdown(body)
+    } else {
+        None
+    }


Suggested change

} else if let Some(body) = body {

FormattedBody::markdown(body)

} else {

None

}

} else {

body.map(FormattedBody::markdown)

}

Add the `markdown` feature to the SDK crate, otherwise we can't use `FormattedBody::markdown`. Refactor the pattern matching into an if, add tests to check its behaviour.

jmartinesp requested a review from a team as a code owner November 13, 2024 15:35

jmartinesp requested review from poljar and removed request for a team November 13, 2024 15:35

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

6e75bdd

Changelog: For `Timeline::send_*` fns, treat the passed `caption` parameter as markdown and use the HTML generated from it as the `formatted_caption` if there is none.

jmartinesp force-pushed the feat/parse-md-for-sending-media-captions branch from b695229 to 6e75bdd Compare November 13, 2024 15:37

jmartinesp mentioned this pull request Nov 13, 2024

Use formatted captions for images and video element-hq/element-x-android#3864

Merged

10 tasks

bnjbvr requested changes Nov 13, 2024

View reviewed changes

bnjbvr removed the request for review from poljar November 13, 2024 16:18

bnjbvr approved these changes Nov 14, 2024

View reviewed changes

jmartinesp force-pushed the feat/parse-md-for-sending-media-captions branch 2 times, most recently from f6c6fc1 to f76d83d Compare November 14, 2024 09:14

refactor(sdk): move formatted_caption_from to the SDK, rename it

5d77f32

Add the `markdown` feature to the SDK crate, otherwise we can't use `FormattedBody::markdown`. Refactor the pattern matching into an if, add tests to check its behaviour.

jmartinesp force-pushed the feat/parse-md-for-sending-media-captions branch from f76d83d to 5d77f32 Compare November 14, 2024 09:23

jmartinesp enabled auto-merge (rebase) November 14, 2024 09:36

jmartinesp merged commit d614878 into main Nov 14, 2024
40 checks passed

jmartinesp deleted the feat/parse-md-for-sending-media-captions branch November 14, 2024 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmartinesp commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

bnjbvr Nov 13, 2024

bnjbvr Nov 13, 2024

bnjbvr Nov 13, 2024

jmartinesp Nov 13, 2024

bnjbvr Nov 13, 2024

poljar Nov 14, 2024

jmartinesp Nov 14, 2024

poljar Nov 14, 2024

jmartinesp Nov 14, 2024

bnjbvr Nov 14, 2024

jmartinesp commented Nov 14, 2024

bnjbvr left a comment

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

		(Some(body), None) => RumaFormattedBody::markdown(body),
		(_, Some(formatted_body)) => Some(formatted_body.clone().into()),

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

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

Conversation

jmartinesp commented Nov 13, 2024 • edited Loading

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Nov 14, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

jmartinesp commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading