Skip to content

feat: implement and observe MSC4278 config value #5040

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

Merged
merged 8 commits into from
May 19, 2025

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented May 14, 2025

This PR

  • Updates Ruma to use the improved MediaPreviewConfig event type that also supports a Default for the content type
  • Implemented a way to observe the stable and unstable values of the event and return the used one accordingly, if no one is present the default will be used
  • Set the value (will only use unstable type for now)

@Velin92 Velin92 marked this pull request as ready for review May 15, 2025 10:26
@Velin92 Velin92 requested a review from a team as a code owner May 15, 2025 10:26
@Velin92 Velin92 requested review from bnjbvr and removed request for a team May 15, 2025 10:26
@Velin92 Velin92 force-pushed the mauroromito/observe_media_preview_config branch from e4b7fe8 to eb2186a Compare May 15, 2025 10:33
@Velin92 Velin92 changed the title wip: added observation for media preview config feat: implement and observe MSC4278 config value May 15, 2025
@Velin92 Velin92 force-pushed the mauroromito/observe_media_preview_config branch from eb2186a to dddff85 Compare May 15, 2025 10:44
Copy link

codecov bot commented May 15, 2025

Codecov Report

Attention: Patch coverage is 64.70588% with 18 lines in your changes missing coverage. Please review.

Project coverage is 85.82%. Comparing base (ea4c9a4) to head (1c91e27).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/account.rs 62.50% 12 Missing ⚠️
crates/matrix-sdk/src/test_utils/mocks/mod.rs 68.42% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5040      +/-   ##
==========================================
- Coverage   85.85%   85.82%   -0.04%     
==========================================
  Files         325      325              
  Lines       35937    35988      +51     
==========================================
+ Hits        30855    30887      +32     
- Misses       5082     5101      +19     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@poljar poljar requested review from poljar and removed request for bnjbvr May 15, 2025 11:01
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Looks mostly good, there are some missing docs here an there.

It would have been nice if this was spit out into separate commits. There are clear separation points. For example the extending of the mocking server could have been one commit, the functionality in the SDK a second one, and finally the bindings the third one.

But ok, it's not that big of a PR so it's still reviewable without this separation. Just something to keep in mind.

@Velin92 Velin92 force-pushed the mauroromito/observe_media_preview_config branch from dddff85 to 00ab99a Compare May 16, 2025 08:58
@poljar
Copy link
Contributor

poljar commented May 16, 2025

I see you have force pushed to implement the requested changes. Please refrain in the future from doing so, it makes the subsequent review rounds harder.

Our contributing guide explains the preferred review process: https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#review-process.

@Velin92 Velin92 force-pushed the mauroromito/observe_media_preview_config branch 5 times, most recently from 85c05c7 to 602eef4 Compare May 16, 2025 11:08
@Velin92 Velin92 requested a review from poljar May 16, 2025 13:45
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

There's still some docs missing and I just realized that there's going to be a small race.

Looks almost ready to be merged.

I know that I complained about force pushes, but now the CI is broken unless you rebase onto main.

So feel free to do that.

@Velin92 Velin92 force-pushed the mauroromito/observe_media_preview_config branch from 602eef4 to 2e26cf3 Compare May 16, 2025 14:26
@Velin92 Velin92 requested a review from poljar May 16, 2025 14:29
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now.

@poljar poljar merged commit 154f29e into main May 19, 2025
43 checks passed
@poljar poljar deleted the mauroromito/observe_media_preview_config branch May 19, 2025 10:35
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

Successfully merging this pull request may close these issues.

2 participants