From 27b4fd78811ebda5472d6f13c8e6dfeb95ded833 Mon Sep 17 00:00:00 2001 From: Val Lorentz Date: Sun, 12 Nov 2023 10:52:13 +0100 Subject: [PATCH] List space children in canonical order --- Cargo.toml | 1 + src/buffers/mod.rs | 89 ++++++++++++++++++++++++++++++++++++++------- src/buffers/room.rs | 39 +++++++++++++------- 3 files changed, 102 insertions(+), 27 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4749a77..7010736 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ memuse = "0.2.1" nonempty = { version = "0.8.1", features = ["serialize"] } signal-hook = "0.3.17" smallvec = "1.11.1" +sorted-vec = "0.8.3" # Matrix eyeball = "0.8.7" # data structures observer returned by matrix-sdk-ui diff --git a/src/buffers/mod.rs b/src/buffers/mod.rs index 3ee8556..6742228 100644 --- a/src/buffers/mod.rs +++ b/src/buffers/mod.rs @@ -14,13 +14,17 @@ * along with this program. If not, see . */ -use crate::widgets::Prerender; +use std::cmp::Ordering; +use std::collections::{HashMap, HashSet}; + use futures::stream::FuturesUnordered; use futures::StreamExt; use matrix_sdk::async_trait; use nonempty::NonEmpty; use ratatui::text::Text; -use std::collections::{HashMap, HashSet}; +use sorted_vec::SortedVec; + +use crate::widgets::Prerender; mod log; pub use log::LogBuffer; @@ -35,6 +39,47 @@ pub enum BufferId { Room(matrix_sdk::ruma::OwnedRoomId), } +/// Values should follow the ["Ordering of children within a space" algorithm](https://spec.matrix.org/v1.8/client-server-api/#ordering-of-children-within-a-space). +#[derive(Clone, Debug, Default, Eq, PartialEq)] +pub struct BufferSortKey { + /// Explicit `order` key in the `m.space.child` event + explicit_order: Option, + origin_server_ts: Option, +} + +impl Ord for BufferSortKey { + fn cmp(&self, other: &Self) -> Ordering { + match (self.explicit_order.as_ref(), other.explicit_order.as_ref()) { + (Some(e1), Some(e2)) => e1.cmp(&e2), + (Some(_), None) => Ordering::Less, // children without 'order' go to the end + (None, Some(_)) => Ordering::Greater, // ditto + (None, None) => Ordering::Equal, + } + // Should we apply the same algo here? The algo from the Matrix spec does not apply + // because it does not consider children with m.space.parent. + // I'd say yes, so that all children declared by the parent are first, then all children + // not explicitly recognized by the parent; instead of having the latter in the middle + // of the list if only some children of the parent have an explicit "order" key. + .then( + match ( + self.origin_server_ts.as_ref(), + other.origin_server_ts.as_ref(), + ) { + (Some(ts1), Some(ts2)) => ts1.cmp(&ts2), + (Some(_), None) => Ordering::Less, + (None, Some(_)) => Ordering::Greater, + (None, None) => Ordering::Equal, + }, + ) + } +} + +impl PartialOrd for BufferSortKey { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + #[derive(Debug, Clone)] pub enum BufferItemContent<'buf> { Text(Text<'buf>), @@ -57,7 +102,7 @@ pub trait Buffer: Send + Sync + memuse::DynamicUsage { fn parent(&self) -> Option { None } - fn children(&self) -> Option> { + fn children(&self) -> Option> { None } /// Returns if there are any updates to apply. @@ -79,7 +124,7 @@ pub trait Buffer: Send + Sync + memuse::DynamicUsage { pub struct Buffers { buffers: Vec>, parents: HashMap>, - children: HashMap>, + children: HashMap>, /// Set of buffers already placed after a parent space, so other spaces should not /// steal them, even if they are also their child (or it would cause buffers to move /// every time we re-sort the buffer list) @@ -127,7 +172,7 @@ impl Buffers { &self.parents } - pub fn children(&self) -> &HashMap> { + pub fn children(&self) -> &HashMap> { &self.children } @@ -148,15 +193,20 @@ impl Buffers { for child in children.iter() { self .parents - .entry(child.clone()) + .entry(child.1.clone()) .or_insert_with(HashSet::new) .insert(new_buffer_id.clone()); } - self + let all_children = self .children .entry(new_buffer_id.clone()) - .or_insert_with(HashSet::new) - .extend(children.into_iter()); + .or_insert_with(SortedVec::new); + // all_children.extend() uses .insert() which always does a bisect search, + // while .push is O(1) when the element is at the end (which is usually what + // happens here). + for child in children.into_vec().into_iter() { + all_children.push(child); + } }, None => {}, }; @@ -167,11 +217,17 @@ impl Buffers { .entry(new_buffer_id.clone()) .or_insert_with(HashSet::new) .insert(parent.clone()); - self + let siblings = self .children .entry(parent.clone()) - .or_insert_with(HashSet::new) - .insert(new_buffer_id.clone()); + .or_insert_with(SortedVec::new); + if siblings + .iter() + .find(|(_key, buffer_id)| *buffer_id == new_buffer_id) + .is_none() + { + siblings.push((BufferSortKey::default(), new_buffer_id.clone())); + } Some(parent) }, None => self @@ -182,7 +238,7 @@ impl Buffers { let children = self .children .entry(new_buffer_id.clone()) - .or_insert_with(HashSet::new); + .or_insert_with(SortedVec::new); match parent.and_then(|parent_id| self.buffers.iter().position(|buf| buf.id() == parent_id)) { None => self.buffers.push(new_buffer), @@ -235,7 +291,12 @@ impl Buffers { let mut child_buffers = Vec::new(); std::mem::swap(&mut all_buffers, &mut self.buffers); for buf in all_buffers.into_iter() { - if children.contains(&buf.id()) && !self.attached_to_parent.contains(&buf.id()) { + if children + .iter() + .find(|(_key, buf_id)| *buf_id == buf.id()) + .is_some() + && !self.attached_to_parent.contains(&buf.id()) + { self.attached_to_parent.insert(buf.id()); child_buffers.push(buf); } else { diff --git a/src/buffers/room.rs b/src/buffers/room.rs index f6fdccd..2324b0a 100644 --- a/src/buffers/room.rs +++ b/src/buffers/room.rs @@ -40,9 +40,10 @@ use matrix_sdk_ui::timeline::{ use memuse::DynamicUsage; use ratatui::text::Text; use smallvec::SmallVec; +use sorted_vec::SortedVec; use tokio::sync::oneshot; -use super::{Buffer, BufferId, BufferItem, BufferItemContent}; +use super::{Buffer, BufferId, BufferItem, BufferItemContent, BufferSortKey}; use crate::widgets::Prerender; /// Like [`BufferItemContent`] but owned. @@ -441,16 +442,18 @@ impl Buffer for RoomBuffer { .and_then(|roominfo| roominfo.parent.as_ref()) .map(|parent| BufferId::Room(parent.to_owned())) } - fn children(&self) -> Option> { + fn children(&self) -> Option> { self .computed_roominfo .as_ref() .and_then(|roominfo| roominfo.children.as_ref()) - .map(|children| { - children + .map(|children: &SortedVec<_>| { + let children = children .iter() - .map(|child| BufferId::Room(child.to_owned())) - .collect() + .map(|(sort_key, room_id)| (sort_key.clone(), BufferId::Room(room_id.to_owned()))) + .collect(); + // This is safe because the map above preserves order + unsafe { SortedVec::from_sorted(children) } }) } @@ -588,23 +591,33 @@ async fn compute_room_info( .await { Ok(child_events) => { - Some( + Some(SortedVec::from_unsorted( child_events .into_iter() // Extract state key (ie. the child's id) and sender .flat_map(|parent_event| { match parent_event.deserialize() { - Ok(SyncOrStrippedState::Sync(SyncStateEvent::Original(e))) => { - Some(e.state_key.to_owned()) - }, + Ok(SyncOrStrippedState::Sync(SyncStateEvent::Original(e))) => Some(( + BufferSortKey { + explicit_order: e.content.order, + origin_server_ts: Some(e.origin_server_ts), + }, + e.state_key.to_owned(), + )), Ok(SyncOrStrippedState::Sync(SyncStateEvent::Redacted(_))) => None, - Ok(SyncOrStrippedState::Stripped(e)) => Some(e.state_key.to_owned()), + Ok(SyncOrStrippedState::Stripped(e)) => Some(( + BufferSortKey { + explicit_order: None, + origin_server_ts: None, // Why don't stripped events have origin_server_ts!? + }, + e.state_key.to_owned(), + )), Err(_) => None, // Ignore deserialization errors } }) .collect(), - ) + )) }, Err(e) => { tracing::error!("Failed to get child rooms of {}: {:?}", room.room_id(), e); @@ -649,7 +662,7 @@ async fn compute_room_info( struct ComputedRoomInfo { display_name: Option, parent: Option, - children: Option>, + children: Option>, roominfo_hash: u64, }