sdk-ui: make room encryption optional to create a timeline #4021

jmartinesp · 2024-09-19T08:37:58Z

Instead of forcing the room encryption to be known when the timeline is created and failing if it's not known, take the latest room encryption info as a base value and update it when processing timeline events.

At the time of writing this PR, the encryption info is only used to decide whether shields should be calculated for timeline items or not.

Fixes #3850 (I hope).

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-09-19T08:52:36Z

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.25%. Comparing base (794dbb3) to head (4182ec2).

Files with missing lines	Patch %	Lines
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	92.85%	1 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4021      +/-   ##
==========================================
- Coverage   84.26%   84.25%   -0.01%     
==========================================
  Files         266      266              
  Lines       28349    28391      +42     
==========================================
+ Hits        23888    23921      +33     
- Misses       4461     4470       +9

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

poljar

Left some suggestions and questions. Please feel free to use whitespace more lavishly.

poljar · 2024-09-20T10:54:51Z

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

+    /// [`matrix_sdk_base::RoomInfo`] and applies the new value to the
+    /// existing timeline items.
+    pub async fn handle_encryption_state_changes(&self) {
+        let mut room_info = self.room_data_provider.room_info();


Suggested change

let mut room_info = self.room_data_provider.room_info();

let mut room_info = self.room_data_provider.room_info();

poljar · 2024-09-20T10:55:55Z

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

@@ -345,6 +345,29 @@ impl<P: RoomDataProvider> TimelineController<P> {
        }
    }

+    /// Listens to encryption state changes for the room in
+    /// [`matrix_sdk_base::RoomInfo`] and applies the new value to the


I think that this could e a bit clearer, the function also reloads shield states so we might want to call this out as well.

Technically this function isn't recalculating the shield states: that is created on the fly every time we call the fn EventTimelineItem::get_shield, based on both is_room_encrypted and the remote event's encryption info.

poljar · 2024-09-20T10:56:05Z

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

+    pub async fn handle_encryption_state_changes(&self) {
+        let mut room_info = self.room_data_provider.room_info();
+        while let Some(info) = room_info.next().await {
+            let mut changed = false;


Suggested change

let mut changed = false;

let mut changed = false;

poljar · 2024-09-20T10:56:38Z

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

+                    changed = true;
+                    *is_room_encrypted = Some(is_encrypted_now);
+                }
+            }


Suggested change

}

}

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

poljar · 2024-09-20T11:08:35Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

@@ -295,6 +300,19 @@ impl TimelineState {
        result
    }

+    pub(super) fn reload_shields(&mut self) {
+        let Ok(is_room_encrypted) = self.meta.is_room_encrypted.read() else {


We generally unwrap() if a lock is poisoned, it's the only place where we allow unwraps.

poljar · 2024-09-20T11:10:36Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+            return;
+        };
+
+        if let Some(is_encrypted) = *is_room_encrypted {


Don't we need to do this only if it's Some() as well as true? On the other hand, the only place where we call this is in a place where we know the value of this and if it'll be true, why don't we just unconditionally call replace_all_events_encryption()?

poljar · 2024-09-20T11:10:55Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+
+    fn replace_all_events_encryption(&mut self, is_encrypted: bool) {
+        for idx in 0..self.items.len() {
+            let item = &self.items[idx];


Suggested change

let item = &self.items[idx];

let item = &self.items[idx];

poljar · 2024-09-20T11:12:43Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

@@ -720,6 +738,18 @@ impl TimelineStateTransaction<'_> {
    fn adjust_day_dividers(&mut self, mut adjuster: DayDividerAdjuster) {
        adjuster.run(&mut self.items, &mut self.meta);
    }
+
+    fn replace_all_events_encryption(&mut self, is_encrypted: bool) {


That's probably a really poor name for this method, at least I can't really explain from the name what this method does.

Nor is it quite clear that this method does enough, it's just setting a flag on the event that the room is encrypted? How will this trigger shield replacements?

I can add some docs for it and maybe find a new name, but it's kind of difficult, since all it does is replace the is_room_encrypted value for each timeline item and create a VectorDiff::Set for each item so this update is emitted.

poljar · 2024-09-20T11:13:00Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+            let item = &self.items[idx];
+            if let Some(event) = item.as_event() {
+                let mut cloned_event = event.clone();
+                cloned_event.is_room_encrypted = Some(is_encrypted);


Suggested change

cloned_event.is_room_encrypted = Some(is_encrypted);

cloned_event.is_room_encrypted = Some(is_encrypted);

jmartinesp · 2024-09-20T11:58:17Z

I tried following your suggestions in 71a8e24 🤞 .

poljar

Left a small nit, I think that this is fine now.

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

Instead of forcing the room encryption to be known when the timeline is created and failing if it's not known, take the latest room encryption info as a base value and update it when processing timeline events. At the time of writing this commit, the encryption info is only used to decide whether shields should be calculated for timeline items or not.

jmartinesp requested a review from a team as a code owner September 19, 2024 08:37

jmartinesp requested review from poljar and removed request for a team September 19, 2024 08:37

jmartinesp force-pushed the misc/optional-encryption-in-timeline branch from 0bddcfd to f12c24f Compare September 19, 2024 16:20

poljar requested changes Sep 20, 2024

View reviewed changes

jmartinesp requested a review from poljar September 20, 2024 11:58

jmartinesp mentioned this pull request Sep 21, 2024

Tapping alternating rooms can lead to ElX displaying a "Join Room" screen element-hq/element-x-ios#3270

Open

poljar approved these changes Sep 23, 2024

View reviewed changes

crates/matrix-sdk-ui/src/timeline/controller/mod.rs Show resolved Hide resolved

jmartinesp force-pushed the misc/optional-encryption-in-timeline branch from cc9bcbf to e44efb4 Compare September 23, 2024 12:02

jmartinesp force-pushed the misc/optional-encryption-in-timeline branch from e44efb4 to 4182ec2 Compare September 23, 2024 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk-ui: make room encryption optional to create a timeline #4021

sdk-ui: make room encryption optional to create a timeline #4021

jmartinesp commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

poljar left a comment

poljar Sep 20, 2024

poljar Sep 20, 2024

jmartinesp Sep 20, 2024

poljar Sep 20, 2024

poljar Sep 20, 2024

poljar Sep 20, 2024

poljar Sep 20, 2024

poljar Sep 20, 2024

poljar Sep 20, 2024

jmartinesp Sep 20, 2024

poljar Sep 20, 2024

jmartinesp commented Sep 20, 2024

poljar left a comment

	let mut room_info = self.room_data_provider.room_info();
	let mut room_info = self.room_data_provider.room_info();

	cloned_event.is_room_encrypted = Some(is_encrypted);
	cloned_event.is_room_encrypted = Some(is_encrypted);

sdk-ui: make room encryption optional to create a timeline #4021

Are you sure you want to change the base?

sdk-ui: make room encryption optional to create a timeline #4021

Conversation

jmartinesp commented Sep 19, 2024 • edited Loading

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Sep 20, 2024

poljar left a comment

Choose a reason for hiding this comment

jmartinesp commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading