Merge pull request #5959 from matrix-org/rav/clean_up_shield_state

Remove unused `ShieldStateCode::SentInClear`

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

bindings/matrix-sdk-ffi/CHANGELOG.md

@@ -15,6 +15,11 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- [**breaking**] `LazyTimelineItemProvider::get_shields` no longer returns an
+  an `Option`: the `ShieldState` type contains a `None` variant, so the
+  `Option` was redundant. The `message` field has also been removed: since there
+  was no way to localise the returned string, applications should not be using it.
+  ([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5959))
 - Add `SpaceService::get_space_room` to get a space given its id from the space graph if available.
 [#5944](https://github.com/matrix-org/matrix-rust-sdk/pull/5944)
 - Add `QrCodeData::to_bytes()` to allow generation of a QR code.

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

@@ -21,7 +21,6 @@ use matrix_sdk::{
     attachment::{
         AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo, BaseVideoInfo, Thumbnail,
     },
-    deserialized_responses::{ShieldState as SdkShieldState, ShieldStateCode},
     event_cache::RoomPaginationStatus,
     room::edit::EditedContent as SdkEditedContent,
 };
@@ -33,6 +32,7 @@ use matrix_sdk_ui::timeline::{
     self, AttachmentConfig, AttachmentSource, EventItemOrigin,
     LatestEventValue as UiLatestEventValue, LatestEventValueLocalState,
     MediaUploadProgress as SdkMediaUploadProgress, Profile, TimelineDetails,
+    TimelineEventShieldState as SdkShieldState, TimelineEventShieldStateCode,
     TimelineUniqueId as SdkTimelineUniqueId,
 };
 use mime::Mime;
@@ -980,12 +980,12 @@ impl From<&matrix_sdk_ui::timeline::EventSendState> for EventSendState {
 /// authenticity properties.
 #[derive(uniffi::Enum, Clone)]
 pub enum ShieldState {
-    /// A red shield with a tooltip containing the associated message should be
-    /// presented.
-    Red { code: ShieldStateCode, message: String },
-    /// A grey shield with a tooltip containing the associated message should be
-    /// presented.
-    Grey { code: ShieldStateCode, message: String },
+    /// A red shield with a tooltip containing a message appropriate to the
+    /// associated code should be presented.
+    Red { code: TimelineEventShieldStateCode },
+    /// A grey shield with a tooltip containing a message appropriate to the
+    /// associated code should be presented.
+    Grey { code: TimelineEventShieldStateCode },
     /// No shield should be presented.
     None,
 }
@@ -993,12 +993,8 @@ pub enum ShieldState {
 impl From<SdkShieldState> for ShieldState {
     fn from(value: SdkShieldState) -> Self {
         match value {
-            SdkShieldState::Red { code, message } => {
-                Self::Red { code, message: message.to_owned() }
-            }
-            SdkShieldState::Grey { code, message } => {
-                Self::Grey { code, message: message.to_owned() }
-            }
+            SdkShieldState::Red { code } => Self::Red { code },
+            SdkShieldState::Grey { code } => Self::Grey { code },
             SdkShieldState::None => Self::None,
         }
     }
@@ -1277,8 +1273,8 @@ pub struct LazyTimelineItemProvider(Arc<matrix_sdk_ui::timeline::EventTimelineIt
 #[matrix_sdk_ffi_macros::export]
 impl LazyTimelineItemProvider {
     /// Returns the shields for this event timeline item.
-    fn get_shields(&self, strict: bool) -> Option<ShieldState> {
-        self.0.get_shield(strict).map(Into::into)
+    fn get_shields(&self, strict: bool) -> ShieldState {
+        self.0.get_shield(strict).into()
     }
 
     /// Returns some debug information for this event timeline item.

crates/matrix-sdk-common/CHANGELOG.md

@@ -6,6 +6,13 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased] - ReleaseDate
 
+### Features
+
+- [**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/5959))
+
 ### Bug Fixes
 
 - Fix `TimelineEvent::from_bundled_latest_event` sometimes removing the `session_id` of UTDs. This broken event could later be saved to the event cache and become an unresolvable UTD. ([#5970](https://github.com/matrix-org/matrix-rust-sdk/pull/5970)).

crates/matrix-sdk-common/src/deserialized_responses.rs

@@ -46,7 +46,6 @@ const UNKNOWN_DEVICE: &str = "Encrypted by an unknown or deleted device.";
 const MISMATCHED_SENDER: &str = "\
     The sender of the event does not match the owner of the device \
     that created the Megolm session.";
-pub const SENT_IN_CLEAR: &str = "Not encrypted.";
 
 /// Represents the state of verification for a decrypted message sent by a
 /// device.
@@ -283,8 +282,6 @@ pub enum ShieldStateCode {
     UnsignedDevice,
     /// The sender hasn't been verified by the Client's user.
     UnverifiedIdentity,
-    /// An unencrypted event in an encrypted room.
-    SentInClear,
     /// The sender was previously verified but changed their identity.
     #[serde(alias = "PreviouslyVerified")]
     VerificationViolation,
@@ -2008,7 +2005,6 @@ mod tests {
             assert_json_snapshot!(ShieldStateCode::UnknownDevice);
             assert_json_snapshot!(ShieldStateCode::UnsignedDevice);
             assert_json_snapshot!(ShieldStateCode::UnverifiedIdentity);
-            assert_json_snapshot!(ShieldStateCode::SentInClear);
             assert_json_snapshot!(ShieldStateCode::VerificationViolation);
         });
     }

crates/matrix-sdk-common/src/snapshots/snapshot_test_shield_codes-5.snap

@@ -1,5 +1,5 @@
 ---
 source: crates/matrix-sdk-common/src/deserialized_responses.rs
-expression: "ShieldStateCode::SentInClear"
+expression: "ShieldStateCode::VerificationViolation"
 ---
-"SentInClear"
+"VerificationViolation"

crates/matrix-sdk-ui/CHANGELOG.md

@@ -18,6 +18,10 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- [**breaking**] `EventTimelineItem::get_shield` now returns a new type,
+  `TimelineEventShieldState`, which extends the old `ShieldState` with a code
+  for `SentInClear`, now that the latter has been removed from `ShieldState`.
+  ([#5959](https://github.com/matrix-org/matrix-rust-sdk/pull/5959))
 - Add `SpaceService::get_space_room` to get a space
   given its id from the space graph if available.
   ([#5944](https://github.com/matrix-org/matrix-rust-sdk/pull/5944))

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

@@ -24,7 +24,7 @@ use matrix_sdk::{
     deserialized_responses::{EncryptionInfo, ShieldState},
     send_queue::{SendHandle, SendReactionHandle},
 };
-use matrix_sdk_base::deserialized_responses::{SENT_IN_CLEAR, ShieldStateCode};
+use matrix_sdk_base::deserialized_responses::ShieldStateCode;
 use once_cell::sync::Lazy;
 use ruma::{
     EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedMxcUri, OwnedTransactionId,
@@ -309,30 +309,29 @@ impl EventTimelineItem {
         }
     }
 
-    /// Gets the [`ShieldState`] which can be used to decorate messages in the
-    /// recommended way.
-    pub fn get_shield(&self, strict: bool) -> Option<ShieldState> {
+    /// Gets the [`TimelineEventShieldState`] which can be used to decorate
+    /// messages in the recommended way.
+    pub fn get_shield(&self, strict: bool) -> TimelineEventShieldState {
         if !self.is_room_encrypted || self.is_local_echo() {
-            return None;
+            return TimelineEventShieldState::None;
         }
 
         // An unable-to-decrypt message has no authenticity shield.
         if self.content().is_unable_to_decrypt() {
-            return None;
+            return TimelineEventShieldState::None;
         }
 
         match self.encryption_info() {
             Some(info) => {
                 if strict {
-                    Some(info.verification_state.to_shield_state_strict())
+                    info.verification_state.to_shield_state_strict().into()
                 } else {
-                    Some(info.verification_state.to_shield_state_lax())
+                    info.verification_state.to_shield_state_lax().into()
                 }
             }
-            None => Some(ShieldState::Red {
-                code: ShieldStateCode::SentInClear,
-                message: SENT_IN_CLEAR,
-            }),
+            None => {
+                TimelineEventShieldState::Red { code: TimelineEventShieldStateCode::SentInClear }
+            }
         }
     }
 
@@ -693,3 +692,71 @@ impl ReactionsByKeyBySender {
         None
     }
 }
+
+/// Extends [`ShieldState`] to allow for a `SentInClear` code.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum TimelineEventShieldState {
+    /// A red shield with a tooltip containing a message appropriate to the
+    /// associated code should be presented.
+    Red {
+        /// A machine-readable representation.
+        code: TimelineEventShieldStateCode,
+    },
+    /// A grey shield with a tooltip containing a message appropriate to the
+    /// associated code should be presented.
+    Grey {
+        /// A machine-readable representation.
+        code: TimelineEventShieldStateCode,
+    },
+    /// No shield should be presented.
+    None,
+}
+
+impl From<ShieldState> for TimelineEventShieldState {
+    fn from(value: ShieldState) -> Self {
+        match value {
+            ShieldState::Red { code, message: _ } => {
+                TimelineEventShieldState::Red { code: code.into() }
+            }
+            ShieldState::Grey { code, message: _ } => {
+                TimelineEventShieldState::Grey { code: code.into() }
+            }
+            ShieldState::None => TimelineEventShieldState::None,
+        }
+    }
+}
+
+/// Extends [`ShieldStateCode`] to allow for a `SentInClear` code.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))]
+pub enum TimelineEventShieldStateCode {
+    /// Not enough information available to check the authenticity.
+    AuthenticityNotGuaranteed,
+    /// The sending device isn't yet known by the Client.
+    UnknownDevice,
+    /// The sending device hasn't been verified by the sender.
+    UnsignedDevice,
+    /// The sender hasn't been verified by the Client's user.
+    UnverifiedIdentity,
+    /// The sender was previously verified but changed their identity.
+    VerificationViolation,
+    /// The `sender` field on the event does not match the owner of the device
+    /// that established the Megolm session.
+    MismatchedSender,
+    /// An unencrypted event in an encrypted room.
+    SentInClear,
+}
+
+impl From<ShieldStateCode> for TimelineEventShieldStateCode {
+    fn from(value: ShieldStateCode) -> Self {
+        use TimelineEventShieldStateCode::*;
+        match value {
+            ShieldStateCode::AuthenticityNotGuaranteed => AuthenticityNotGuaranteed,
+            ShieldStateCode::UnknownDevice => UnknownDevice,
+            ShieldStateCode::UnsignedDevice => UnsignedDevice,
+            ShieldStateCode::UnverifiedIdentity => UnverifiedIdentity,
+            ShieldStateCode::VerificationViolation => VerificationViolation,
+            ShieldStateCode::MismatchedSender => MismatchedSender,
+        }
+    }
+}

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

@@ -97,7 +97,8 @@ pub use self::{
         MemberProfileChange, MembershipChange, Message, MsgLikeContent, MsgLikeKind,
         OtherMessageLike, OtherState, PollResult, PollState, Profile, ReactionInfo, ReactionStatus,
         ReactionsByKeyBySender, RoomMembershipChange, RoomPinnedEventsChange, Sticker,
-        ThreadSummary, TimelineDetails, TimelineEventItemId, TimelineItemContent,
+        ThreadSummary, TimelineDetails, TimelineEventItemId, TimelineEventShieldState,
+        TimelineEventShieldStateCode, TimelineItemContent,
     },
     event_type_filter::TimelineEventTypeFilter,
     item::{TimelineItem, TimelineItemKind, TimelineUniqueId},

crates/matrix-sdk-ui/src/timeline/tests/shields.rs

@@ -1,6 +1,5 @@
 use assert_matches::assert_matches;
 use eyeball_im::VectorDiff;
-use matrix_sdk_base::deserialized_responses::{ShieldState, ShieldStateCode};
 use matrix_sdk_test::{ALICE, async_test, event_factory::EventFactory};
 use ruma::{
     event_id,
@@ -17,7 +16,7 @@ use ruma::{
 use stream_assert::{assert_next_matches, assert_pending};
 
 use crate::timeline::{
-    EventSendState,
+    EventSendState, TimelineEventShieldState, TimelineEventShieldStateCode,
     tests::{TestTimeline, TestTimelineBuilder},
 };
 
@@ -31,7 +30,7 @@ async fn test_no_shield_in_unencrypted_room() {
 
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
     let shield = item.as_event().unwrap().get_shield(false);
-    assert!(shield.is_none());
+    assert_eq!(shield, TimelineEventShieldState::None);
 }
 
 #[async_test]
@@ -46,7 +45,7 @@ async fn test_sent_in_clear_shield() {
     let shield = item.as_event().unwrap().get_shield(false);
     assert_eq!(
         shield,
-        Some(ShieldState::Red { code: ShieldStateCode::SentInClear, message: "Not encrypted." })
+        TimelineEventShieldState::Red { code: TimelineEventShieldStateCode::SentInClear }
     );
 }
 
@@ -75,7 +74,7 @@ async fn test_local_sent_in_clear_shield() {
     // available).
     assert!(event_item.is_local_echo());
     let shield = event_item.get_shield(false);
-    assert_eq!(shield, None);
+    assert_eq!(shield, TimelineEventShieldState::None);
 
     {
         // The date divider comes in late.
@@ -96,7 +95,7 @@ async fn test_local_sent_in_clear_shield() {
     // Then the local echo still should not have a shield.
     assert!(event_item.is_local_echo());
     let shield = event_item.get_shield(false);
-    assert_eq!(shield, None);
+    assert_eq!(shield, TimelineEventShieldState::None);
 
     // When the remote echo comes in.
     timeline
@@ -118,7 +117,7 @@ async fn test_local_sent_in_clear_shield() {
     let shield = event_item.get_shield(false);
     assert_eq!(
         shield,
-        Some(ShieldState::Red { code: ShieldStateCode::SentInClear, message: "Not encrypted." })
+        TimelineEventShieldState::Red { code: TimelineEventShieldStateCode::SentInClear }
     );
 
     // Date divider is adjusted.
@@ -168,5 +167,5 @@ async fn test_utd_shield() {
     // Then the message is displayed with no shield
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
     let shield = item.as_event().unwrap().get_shield(false);
-    assert!(shield.is_none());
+    assert_eq!(shield, TimelineEventShieldState::None);
 }

crates/matrix-sdk-ui/tests/integration/timeline/mod.rs

@@ -30,8 +30,8 @@ use matrix_sdk_ui::{
     Timeline,
     timeline::{
         AnyOtherFullStateEventContent, Error, EventSendState, MsgLikeKind, OtherMessageLike,
-        RedactError, RoomExt, TimelineBuilder, TimelineEventItemId, TimelineFocus,
-        TimelineItemContent, VirtualTimelineItem, default_event_filter,
+        RedactError, RoomExt, TimelineBuilder, TimelineEventItemId, TimelineEventShieldState,
+        TimelineFocus, TimelineItemContent, VirtualTimelineItem, default_event_filter,
     },
 };
 use ruma::{
@@ -757,7 +757,7 @@ async fn test_timeline_without_encryption_info() {
     assert_eq!(items.len(), 2);
     assert!(items[0].as_virtual().is_some());
     // No encryption, no shields.
-    assert!(items[1].as_event().unwrap().get_shield(false).is_none());
+    assert_eq!(items[1].as_event().unwrap().get_shield(false), TimelineEventShieldState::None);
 }
 
 #[async_test]
@@ -787,7 +787,7 @@ async fn test_timeline_without_encryption_can_update() {
     assert_eq!(items.len(), 2);
     assert!(items[0].as_virtual().is_some());
     // No encryption, no shields
-    assert!(items[1].as_event().unwrap().get_shield(false).is_none());
+    assert_eq!(items[1].as_event().unwrap().get_shield(false), TimelineEventShieldState::None);
 
     let encryption_event_content = RoomEncryptionEventContent::with_recommended_defaults();
     server
@@ -805,17 +805,17 @@ async fn test_timeline_without_encryption_can_update() {
     // Previous timeline event now has a shield.
     assert_let!(VectorDiff::Set { index, value } = &timeline_updates[0]);
     assert_eq!(*index, 1);
-    assert!(value.as_event().unwrap().get_shield(false).is_some());
+    assert_ne!(value.as_event().unwrap().get_shield(false), TimelineEventShieldState::None);
 
     // Room encryption event is received.
     assert_let!(VectorDiff::PushBack { value } = &timeline_updates[1]);
     assert_let!(TimelineItemContent::OtherState(other_state) = value.as_event().unwrap().content());
     assert_let!(AnyOtherFullStateEventContent::RoomEncryption(_) = other_state.content());
-    assert!(value.as_event().unwrap().get_shield(false).is_some());
+    assert_ne!(value.as_event().unwrap().get_shield(false), TimelineEventShieldState::None);
 
     // New message event is received and has a shield.
     assert_let!(VectorDiff::PushBack { value } = &timeline_updates[2]);
-    assert!(value.as_event().unwrap().get_shield(false).is_some());
+    assert_ne!(value.as_event().unwrap().get_shield(false), TimelineEventShieldState::None);
 
     assert_pending!(stream);
 }