Remove unused `ShieldStateCode::SentInClear` #5959

richvdh · 2025-12-12T15:05:47Z

VerificationState::to_shield_state_{strict,lax} return a type ShieldState, whose inner type ShieldStateCode currently includes a variant SentInClear for "unencrypted event". This is very misleading, because those functions never actually set that variant.

Rather, there is a separate method in matrix-sdk-ui, EventTimelineItem::get_shield, which uses the same type, but does set that variant where appropriate.

As a user of matrix-sdk-common, without the matrix-sdk-ui layer, this is dangerously misleading: it gives the impression that we are checking for unencrypted events, when in fact we are not.

The solution seems to be to use different types for the different levels of the stack.

While we're at it, we fix up some of the confusion of methods that return an Option of an enum type which itself has a None variant.

Since this is returned within an `Option`, the `None` variant is pointless

The `ShieldState` enum has a `None` variant, so we don't need an `Option` on top of it.

Separate the shield types between common and UI, so that we can change common without breaking UI

codspeed-hq · 2025-12-12T15:28:53Z

CodSpeed Performance Report

Merging #5959 will not alter performance

_{Comparing rav/clean_up_shield_state (12f4e94) with main (3a63838)}

Summary

✅ 50 untouched

codecov · 2025-12-12T16:59:12Z

Codecov Report

❌ Patch coverage is 26.92308% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.49%. Comparing base (ce65317) to head (e871001).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rates/matrix-sdk-ui/src/timeline/event_item/mod.rs	26.92%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5959      +/-   ##
==========================================
- Coverage   88.51%   88.49%   -0.02%     
==========================================
  Files         363      363              
  Lines      103469   103486      +17     
  Branches   103469   103486      +17     
==========================================
+ Hits        91582    91584       +2     
- Misses       7533     7547      +14     
- Partials     4354     4355       +1

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

richvdh · 2025-12-12T17:12:11Z

This will need complement-crypto updates, but maybe someone could give it a review for sanity before I invest time in that.

stefanceriu

Makes sense to me 👍 I'll leave it to you to deal with my comments in whatever way you see fit.

stefanceriu · 2025-12-15T07:13:08Z

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

+        /// A machine-readable representation.
+        code: TimelineEventShieldStateCode,
+        /// A human readable description.
+        message: &'static str,


Seeing as we're on the UI level I'm questioning how useful the message actually is without localization support. I'd say the code is good enough for both debugging and localization purposes and we can entirely drop the message.

Yup. To be honest my goal here was to make the code slightly less confusing without having to spend too long digging into (or changing) the way that element-x-android or element-x-ios use the shield state. If neither app uses the message, then of course I'm happy to drop it, but I'm reluctant to start refactoring the higher-level code.

Yup okay, makes sense 👍

crates/matrix-sdk-ui/CHANGELOG.md

stefanceriu · 2025-12-15T07:21:06Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

 #[matrix_sdk_ffi_macros::export]
 impl LazyTimelineItemProvider {
    /// Returns the shields for this event timeline item.
    fn get_shields(&self, strict: bool) -> Option<ShieldState> {


Why did you decide to keep the option on the FFI layer but the non-optional None variant on the UI level? 🤔

yeah, good question. Again it comes down to not wanting to have to change the application layers too much. Removing the Option in favour or the None variant felt like a cleaner solution, so I went with that for matrix-sdk-ui, but it felt like a more invasive change than just removing the None variant.

As a general rule we tend to be very lax on accepting breaking ffi changes. It's the price to pay to move things along without tripping on ceremony and we're fine with it.

stefanceriu · 2025-12-15T07:29:08Z

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

+impl From<ShieldState> for TimelineEventShieldState {
+    fn from(value: ShieldState) -> Self {
+        match value {
+            ShieldState::Red { code, message } => {


Random point: I've said this before and I'll keep saying it every time I see these things: Red and Gray have no intrinsic meaning and depending on the culture you come from might mean the exact opposite (e.g. China). Naming them after a color does by no means guarantee that they will actually be colored that way in the UI (being in the UI crate makes it worse). I would much rather see them call for what they are BigProblem, LegacyProblem, NoProblem or whatever. Thanks for coming to my TED Talk.

yup, absolutely. It's also worth noting that we don't actually use shields in the UI for this (rather, padlocks, /!\ symbols, anything other than a shield), so the name is crap twice over.

But again, I didn't really want to do a big refactor here.

omg, didn't even think of that 🤦

stefanceriu · 2025-12-15T07:31:45Z

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

+/// Extends [`ShieldStateCode`] to allow for a `SentInClear` code.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))]
+pub enum TimelineEventShieldStateCode {


I quite like how errors derive their display implementation, I wonder if there's anything like that for normal enums. Then again, any reason why TimelineEventShieldStateCode can't be an actual thiserror::Error?

I'm not entirely following you. Do we need to implement display here? What don't you like about what's there today?

Ah sorry, this was coming as an addition to removing the msg. We have very well definied messages in the documentatioe here and also a good way of exposing them to the final user through that macro. I figured it fits better.

richvdh · 2025-12-12T17:13:33Z

crates/matrix-sdk-common/CHANGELOG.md

+- [**breaking**] `ShieldStateCode` no longer includes
+  `SentInClear`. `VeificationState::to_shield_state_{lax,strict}` never
+  returned that code, ans so having it in the enum was somewhat misleading.
+  ([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5936))


Suggested change

([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5936))

([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5959))

richvdh · 2025-12-12T17:13:47Z

crates/matrix-sdk-ui/CHANGELOG.md

+- [**breaking**] `EventTimelineItem::get_shield` now returns a new type,
+  `TimelineEventShieldState`, which extends the old `ShieldState` with a code
+  for `SentInCliear`, now that the latter has been removed from `ShieldState`.
+  ([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5936))


Suggested change

([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5936))

([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5959))

Co-authored-by: Stefan Ceriu <[email protected]> Signed-off-by: Richard van der Hoff <[email protected]>

richvdh added 3 commits December 12, 2025 14:38

bindings: remove redundant ShieldState::None

94e3aaf

Since this is returned within an `Option`, the `None` variant is pointless

TimelineEventItem::get_shield: stop returning Option

8ff9b95

The `ShieldState` enum has a `None` variant, so we don't need an `Option` on top of it.

UI: new type for EventTimelineItem::get_shield

3c32872

Separate the shield types between common and UI, so that we can change common without breaking UI

richvdh force-pushed the rav/clean_up_shield_state branch from 239eebb to 1344cb5 Compare December 12, 2025 15:06

common: remove now-unused ShieldStateCode::SentInClear

e871001

richvdh force-pushed the rav/clean_up_shield_state branch from 1344cb5 to e871001 Compare December 12, 2025 16:39

richvdh marked this pull request as ready for review December 12, 2025 17:11

richvdh requested a review from a team as a code owner December 12, 2025 17:11

richvdh requested review from stefanceriu and removed request for a team December 12, 2025 17:11

stefanceriu approved these changes Dec 15, 2025

View reviewed changes

richvdh commented Dec 15, 2025

View reviewed changes

Update crates/matrix-sdk-ui/CHANGELOG.md

12f4e94

Co-authored-by: Stefan Ceriu <[email protected]> Signed-off-by: Richard van der Hoff <[email protected]>

	([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5936))
	([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5959))

Remove unused ShieldStateCode::SentInClear #5959

Are you sure you want to change the base?

Remove unused ShieldStateCode::SentInClear #5959

Uh oh!

Conversation

richvdh commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5959 will not alter performance

Summary

Uh oh!

codecov bot commented Dec 12, 2025

Codecov Report

Uh oh!

richvdh commented Dec 12, 2025

Uh oh!

stefanceriu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Remove unused `ShieldStateCode::SentInClear` #5959

Remove unused `ShieldStateCode::SentInClear` #5959

richvdh commented Dec 12, 2025 •

edited

Loading

codspeed-hq bot commented Dec 12, 2025 •

edited

Loading