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.

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

fix(media): Make sure that local URIs only try to get media from the cache and ignore requested dimensions #4329

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

zecakeh
Copy link
Collaborator

@zecakeh zecakeh commented Nov 26, 2024

Avoids a tricky API where the users need to make exactly the right media request to get the local file in cache, and avoids to make a request over the network when we know that the file should only be found locally.

cc @bnjbvr, I don't think this is exactly what you had in mind but it does the job.

…its key

Using a `MediaFormat::Thumbnail(_)` with the thumbnail's dimensions
means that we request
"a server-generated thumbnail of the client-generated thumbnail with the
same dimensions as the client-generated thumbnail".

A simpler way of getting the same result is to get the client-generated
thumbnail directly, so we can just use `MediaFormat::File`.

This simplifies the data passed around in the send queue

Signed-off-by: Kévin Commaille <[email protected]>
…cache and ignore requested dimensions

Signed-off-by: Kévin Commaille <[email protected]>
@zecakeh zecakeh requested a review from a team as a code owner November 26, 2024 16:06
@zecakeh zecakeh requested review from Hywan and removed request for a team November 26, 2024 16:06
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 89.52381% with 11 lines in your changes missing coverage. Please review.

Project coverage is 85.09%. Comparing base (75d7d07) to head (f77bff8).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
...tes/matrix-sdk-base/src/store/migration_helpers.rs 75.86% 7 Missing ⚠️
crates/matrix-sdk/src/send_queue.rs 71.42% 2 Missing ⚠️
crates/matrix-sdk-sqlite/src/state_store.rs 94.11% 1 Missing ⚠️
crates/matrix-sdk/src/media.rs 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4329      +/-   ##
==========================================
+ Coverage   85.05%   85.09%   +0.04%     
==========================================
  Files         275      275              
  Lines       30309    30385      +76     
==========================================
+ Hits        25780    25857      +77     
+ Misses       4529     4528       -1     

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

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Thanks; I haven't looked into it too much, but since this changes the format of the DependentRequestKins, it needs a migration (that can be at least a plain removal of pending dependent events). I'll take a deeper look later.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Nov 27, 2024

Thanks; I haven't looked into it too much, but since this changes the format of the DependentRequestKins, it needs a migration (that can be at least a plain removal of pending dependent events). I'll take a deeper look later.

Oh you are right, I forgot about that. Looking more into it, it seems complicated to migrate the cached media though. We would need the list of migrated URIs from the StateStore and then remove or migrate the media in the EventCacheStore. We would not need to migrate the cached media if we could access it via its MXC URI alone with a new API, like you suggested. So it seems to be the simplest solution and I am going to work on that.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Nov 27, 2024

Migrations added.

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