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

MatrixRTC: Fix different devices from the same user overwriting the room info state event. #3972

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Sep 10, 2024

This PR is best reviewed with the background from: #3998
The main change in this PR is the transition from using a UserId as the index for call member events in the room info to using the newly introduced CallMemberStateKey that is a combination of UserId and device id.

This is important since otherwise the room state could only store one event per user (but a user can join with multiple devices)

It also adds tests for session membership events (which is the new state event format for call member state events we need for reliable state events)

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@toger5
Copy link
Contributor Author

toger5 commented Sep 12, 2024

The commit history does not conform to guidelines. I suspect a rebase will be required anyways which contains the switch to the ruma/ruma.

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.27%. Comparing base (f576c72) to head (77b416c).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-base/src/rooms/mod.rs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3972   +/-   ##
=======================================
  Coverage   84.27%   84.27%           
=======================================
  Files         266      266           
  Lines       28336    28337    +1     
=======================================
+ Hits        23880    23881    +1     
  Misses       4456     4456           

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

@toger5 toger5 force-pushed the toger5/use-ruma-member-event-state-key branch 2 times, most recently from 3f20c3b to c97cde0 Compare September 16, 2024 12:34
@toger5 toger5 marked this pull request as ready for review September 16, 2024 12:49
@toger5 toger5 requested a review from a team as a code owner September 16, 2024 12:49
@toger5 toger5 requested review from poljar and removed request for a team September 16, 2024 12:49
@toger5 toger5 changed the title MatrixRTC: Use new call member state key stype from ruma and fix different devices from the same user overwriting the room info state event. MatrixRTC: Fix different devices from the same user overwriting the room info state event. Sep 16, 2024
@toger5 toger5 requested a review from Hywan September 16, 2024 12:56
@toger5 toger5 force-pushed the toger5/use-ruma-member-event-state-key branch from c97cde0 to 2602849 Compare September 16, 2024 12:58
@toger5
Copy link
Contributor Author

toger5 commented Sep 16, 2024

Tarpaulin is failing. test_stale_local_echo_time_abort_edit (fails)

Is this sth that I can fix?

@bnjbvr
Copy link
Member

bnjbvr commented Sep 16, 2024

Tarpaulin is failing. test_stale_local_echo_time_abort_edit (fails)

Is this sth that I can fix?

No, intermittent failure. Do you have the permissions to restart it by yourself in the CI interface? (I'll restart it now for you)

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

It looks good, thanks for these changes!

Tiny feedback.

crates/matrix-sdk-base/src/rooms/normal.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/normal.rs Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/normal.rs Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/normal.rs Outdated Show resolved Hide resolved
@toger5 toger5 force-pushed the toger5/use-ruma-member-event-state-key branch 2 times, most recently from e2fc856 to 79c93ac Compare September 16, 2024 19:51
@poljar poljar removed their request for review September 17, 2024 07:49
@toger5 toger5 force-pushed the toger5/use-ruma-member-event-state-key branch from 79c93ac to 77b416c Compare September 17, 2024 10:15
@toger5
Copy link
Contributor Author

toger5 commented Sep 17, 2024

LegacyMembershipDataInit should then definitly also require a DeviceId. This cannot be done in this PR but I will add the change to another PR.
(#3972 (comment))

The ruma PR that changes this got merged, so I updated this PR to use the updated LegacyMembershipDataInit.

Copy link
Member

@Hywan Hywan 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 to me, thanks for the PR on Ruma!

@Hywan Hywan merged commit 8119697 into main Sep 17, 2024
40 checks passed
@Hywan Hywan deleted the toger5/use-ruma-member-event-state-key branch September 17, 2024 15:02
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