Widget driver: Distinguish room state and timeline events

[robintown]

This is an implementation of an update to MSC2762. It changes the widget driver to split state updates out into an update_state action and use the send_event action exclusively for forwarding timeline events. Accordingly, read_events now always reads from /messages, never the state store.

⚠️ Do not merge yet: I do not want to merge this until a new version of Element Call is nearing release which would be compatible with the update_state action. However, I would like this to be reviewed. Once approved I will go back and prepare the Element Call changes.

For https://github.com/element-hq/voip-internal/issues/289

[codecov]

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 85.83%. Comparing base (69b8878) to head (1166012).

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/widget/machine/mod.rs	85.04%	16 Missing ⚠️
crates/matrix-sdk/src/widget/matrix.rs	85.18%	4 Missing ⚠️
crates/matrix-sdk/src/widget/machine/driver_req.rs	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4947      +/-   ##
==========================================
+ Coverage   85.81%   85.83%   +0.01%     
==========================================
  Files         325      325              
  Lines       35909    35981      +72     
==========================================
+ Hits        30817    30883      +66     
- Misses       5092     5098       +6     

☔ 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.

[bnjbvr]

I've only looked at the first three commits, a first batch of comments; the change in the second commit is likely wrong.

[bnjbvr]

A few more remarks. It took me lot of time to read the widget-related code, as it's poorly documented, so let's try to improve the situation here: for each new function/type/trait you're introduced, or that you've touched in this PR, can you add a doc comment, please?

Would you also consider breaking the PR into smaller chunks, or smaller commits? The last one has quite the "draw the rest of the freaking howl" vibe :) and without a commit message or extra guidance where to start reading from, it's a bit overwhelming to review…

[edit] to precise the above request to split: I think it'd be nice also to isolate renamings from other changes, because renaming commits are trivial to review, and the last commit seems to include a few of them, which makes it unnecessarily harder to review. If code has been just moved and not changed, that's also a nice candidate for a separate commit, that doesn't require any review either. Overall, this kind of practices makes it easier for your code to be reviewed, and should give faster review turnaround :)

[bnjbvr]

(Moar review comments, now that I'm mostly understanding the flow between Machine/Widget/Driver)

[bnjbvr]

This will badly break the data format, and I suppose that's the reason why you're asking for a coordinated release; is there a chance we could support both the previous and new formats, instead?

[robintown]

This struct is internal, right?

The reason I'm asking for a coordinated release is that after these changes, the SDK will no longer be compatible with widgets that don't understand the update_state to-widget action. It's possible to maintain compatibility with older API versions, as this was done in matrix-widget-api for instance (matrix-org/matrix-widget-api#125 matrix-org/matrix-widget-api#126) but the VoIP team doesn't have resources right now to do the same for matrix-rust-sdk. The widget API has no stability guarantees currently, so we would like to simply coordinate this update with Element Call and Element X.

[poljar]

It is internal. Your explanation sounds reasonable to me.

One suggestion, you might want to consider adding a snapshot test for this if the serialization format needs to be or become stable.

This will raise an alarm if someone accidentally changes the format.

It's pretty easy to add them:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/lib.rs

Lines 1110 to 1115 in 3461b13

	#[test]
	fn snapshot_decryption_settings() {
	assert_json_snapshot!(DecryptionSettings {
	sender_device_trust_requirement: TrustRequirement::Untrusted,
	});
	}

[poljar]

Alright, I think most of this is good. The one thing that I'm objecting to mostly is the complexity of the capability negotiation.

The biggest issue is the naked unwrap in the mentioned function.

[poljar]

It is internal. Your explanation sounds reasonable to me.

One suggestion, you might want to consider adding a snapshot test for this if the serialization format needs to be or become stable.

This will raise an alarm if someone accidentally changes the format.

It's pretty easy to add them:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/lib.rs

Lines 1110 to 1115 in 3461b13

	#[test]
	fn snapshot_decryption_settings() {
	assert_json_snapshot!(DecryptionSettings {
	sender_device_trust_requirement: TrustRequirement::Untrusted,
	});
	}

On holiday.

[poljar]

Ok, I think there's still a bit that can be improved as part of this PR.

I think after this we're good. Please don't use force pushes once a review has started. It makes it quite hard to see the changes between the review rounds.

If you'd like to keep the history clean you can use fixup commits and rebase/autosquash once the PR has been approved.

More info about our review process can be found in the contributing guide: https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#review-process.

[robintown]

Please don't use force pushes once a review has started. It makes it quite hard to see the changes between the review rounds. If you'd like to keep the history clean you can use fixup commits and rebase/autosquash once the PR has been approved.

Noted. I had to resolve some merge conflicts so I haven't used fixup commits for this most recent round of changes, but if you would prefer I absorb them into the previous commits once this is approved, I could do that. (Just means I have to resolve the conflicts a second time, unless you know of another way.)

[poljar]

Please don't use force pushes once a review has started. It makes it quite hard to see the changes between the review rounds. If you'd like to keep the history clean you can use fixup commits and rebase/autosquash once the PR has been approved.

Noted. I had to resolve some merge conflicts so I haven't used fixup commits for this most recent round of changes, but if you would prefer I absorb them into the previous commits once this is approved, I could do that. (Just means I have to resolve the conflicts a second time, unless you know of another way.)

It's fine, I'll take a look tomorrow.

[poljar]

Alright, I think that this is fine.

You can now get rid of the fixup commits and rebase, or if you don't want to spend time on this we can squash merge as well.

[robintown]

Thanks a bunch for your thorough reviews! As a reminder, this still needs to wait on some Element Call release candidate stuff before it's ready for merging.