Skip to content

feat(bindings): added APIs to get the media preview config from the store #5062

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 7 commits into from
May 21, 2025

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented May 19, 2025

The reason behind this PR, is to extend #5040
While the observable value is very useful in the context of the main app, it may be problematic in the context of parallel processes that aim to aid the main app process, like for example the Notification Service Extension on iOS, which is treated as a different process, and has some very harsh memory contraints, and a limited lifetime, which would make the observable values not quite useful.

Also while we do have a get_media_preview_config_event_content that is used by the observable API, this sends a request to the server to check for the current account data value of such event, and in the context of the NSE which already waits for a sync, we probably do not want to slow it down with another request to the server (not mentioning that sending a request for each notification might be also problematic for the servers).
For the NSE scenario I think getting whatever is currently in the store would be fine.

@Velin92 Velin92 requested a review from a team as a code owner May 19, 2025 15:58
@Velin92 Velin92 requested review from bnjbvr and poljar and removed request for a team and bnjbvr May 19, 2025 15:58
@Velin92
Copy link
Member Author

Velin92 commented May 19, 2025

@poljar I wonder if we shouldn't actually use also in the observable API the currently stored value, instead of doing a request to the server... WDYT?

Copy link

codecov bot commented May 19, 2025

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.75%. Comparing base (d462683) to head (f8ae8fb).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/account.rs 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5062      +/-   ##
==========================================
- Coverage   85.81%   85.75%   -0.06%     
==========================================
  Files         325      325              
  Lines       35922    35932      +10     
==========================================
- Hits        30826    30815      -11     
- Misses       5096     5117      +21     

☔ 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
Copy link
Contributor

poljar commented May 20, 2025

@poljar I wonder if we shouldn't actually use also in the observable API the currently stored value, instead of doing a request to the server... WDYT?

As mentioned here: #5062 (comment). Yes we can do that.

@Velin92 Velin92 requested a review from zecakeh May 20, 2025 16:14
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 good, let's just rename the methods a bit.

@Velin92 Velin92 requested a review from poljar May 21, 2025 09:50
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, looking good.

@poljar poljar merged commit 91815ab into main May 21, 2025
42 checks passed
@poljar poljar deleted the mauroromito/get_stored_media_preview_config branch May 21, 2025 10:15
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.

3 participants