diff --git a/Cargo.lock b/Cargo.lock index f1369898..9acb2054 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1026,6 +1026,12 @@ dependencies = [ "syn", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.10.7" @@ -1349,6 +1355,7 @@ name = "fractal" version = "10.0.0-beta" dependencies = [ "ashpd", + "diff", "djb_hash", "futures-channel", "futures-util", diff --git a/Cargo.toml b/Cargo.toml index a80a8e18..a70def55 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ codegen-units = 16 # Please keep dependencies sorted. [dependencies] +diff = "0.1" djb_hash = "0.1" futures-channel = "0.3" futures-util = "0.3" diff --git a/src/prelude.rs b/src/prelude.rs index 506e16c9..845a832f 100644 --- a/src/prelude.rs +++ b/src/prelude.rs @@ -4,7 +4,7 @@ pub(crate) use crate::{ ToastableDialogImpl, }, contrib::CameraExt, - session::model::UserExt, + session::model::{TimelineItemExt, UserExt}, session_list::SessionInfoExt, user_facing_error::UserFacingError, utils::{ diff --git a/src/session/model/room/event/mod.rs b/src/session/model/room/event/mod.rs index 03903a30..38b1a2db 100644 --- a/src/session/model/room/event/mod.rs +++ b/src/session/model/room/event/mod.rs @@ -182,10 +182,6 @@ mod imp { } impl TimelineItemImpl for Event { - fn id(&self) -> String { - format!("Event::{:?}", self.identifier()) - } - fn can_hide_header(&self) -> bool { self.item().content().can_show_header() } @@ -509,9 +505,10 @@ glib::wrapper! { impl Event { /// Create a new `Event` in the given room with the given SDK timeline item. - pub fn new(item: EventTimelineItem, room: &Room) -> Self { + pub fn new(item: EventTimelineItem, room: &Room, timeline_id: &str) -> Self { let obj = glib::Object::builder::() .property("room", room) + .property("timeline-id", timeline_id) .build(); obj.imp().set_item(item); @@ -522,26 +519,14 @@ impl Event { /// Try to update this event with the given SDK timeline item. /// /// Returns `true` if the update succeeded. - pub(crate) fn try_update_with(&self, item: &EventTimelineItem) -> bool { - let imp = self.imp(); + pub(crate) fn try_update_with(&self, item: &EventTimelineItem, timeline_id: &str) -> bool { + let is_same_item = self.timeline_id() == timeline_id; - match &self.identifier() { - TimelineEventItemId::TransactionId(txn_id) - if item.transaction_id().is_some_and(|id| id == txn_id) => - { - imp.set_item(item.clone()); - return true; - } - TimelineEventItemId::EventId(event_id) - if item.event_id().is_some_and(|id| id == event_id) => - { - imp.set_item(item.clone()); - return true; - } - _ => {} + if is_same_item { + self.imp().set_item(item.clone()); } - false + is_same_item } /// The underlying SDK timeline item. diff --git a/src/session/model/room/timeline/mod.rs b/src/session/model/room/timeline/mod.rs index 90cc1ebf..232190b7 100644 --- a/src/session/model/room/timeline/mod.rs +++ b/src/session/model/room/timeline/mod.rs @@ -1,7 +1,11 @@ mod timeline_item; mod virtual_item; -use std::{collections::HashMap, ops::ControlFlow, sync::Arc}; +use std::{ + collections::{HashMap, VecDeque}, + ops::ControlFlow, + sync::Arc, +}; use futures_util::StreamExt; use gtk::{ @@ -11,7 +15,7 @@ use gtk::{ subclass::prelude::*, }; use matrix_sdk_ui::{ - eyeball_im::VectorDiff, + eyeball_im::{Vector, VectorDiff}, timeline::{ default_event_filter, RoomExt, Timeline as SdkTimeline, TimelineEventItemId, TimelineItem as SdkTimelineItem, @@ -28,7 +32,7 @@ use tokio::task::AbortHandle; use tracing::error; pub(crate) use self::{ - timeline_item::{TimelineItem, TimelineItemImpl}, + timeline_item::{TimelineItem, TimelineItemExt, TimelineItemImpl}, virtual_item::{VirtualItem, VirtualItemKind}, }; use super::{Event, Room}; @@ -208,16 +212,16 @@ mod imp { let matrix_timeline = Arc::new(matrix_timeline); self.matrix_timeline .set(matrix_timeline.clone()) - .expect("matrix timeline was uninitialized"); + .expect("matrix timeline is uninitialized"); - let (values, timeline_stream) = matrix_timeline.subscribe().await; + let (values, timeline_stream) = matrix_timeline.subscribe_batched().await; if !values.is_empty() { - self.update(VectorDiff::Append { values }); + self.update_with_single_diff(VectorDiff::Append { values }, &room); } let obj_weak = glib::SendWeakRef::from(self.obj().downgrade()); - let fut = timeline_stream.for_each(move |diff| { + let fut = timeline_stream.for_each(move |diff_list| { let obj_weak = obj_weak.clone(); let room_id = room_id.clone(); async move { @@ -225,7 +229,7 @@ mod imp { ctx.spawn(async move { spawn!(async move { if let Some(obj) = obj_weak.upgrade() { - obj.imp().update(diff); + obj.imp().update_with_diff_list(diff_list); } else { error!( "Could not send timeline diff for room {room_id}: \ @@ -238,7 +242,9 @@ mod imp { }); let diff_handle = spawn_tokio!(fut); - self.diff_handle.set(diff_handle.abort_handle()).unwrap(); + self.diff_handle + .set(diff_handle.abort_handle()) + .expect("handle is uninitialized"); self.watch_read_receipts().await; self.set_state(TimelineState::Ready); @@ -266,15 +272,148 @@ mod imp { self.obj().notify_has_room_create(); } - /// Update this `Timeline` with the given diff. - #[allow(clippy::too_many_lines)] - fn update(&self, diff: VectorDiff>) { + /// Update this timeline with the given diff list. + fn update_with_diff_list(&self, diff_list: Vec>>) { let Some(room) = self.room.upgrade() else { return; }; - let sdk_items = &self.sdk_items; let was_empty = self.is_empty(); + let diff_list = self.minimize_diff_list(diff_list); + + for diff in diff_list { + self.update_with_single_diff(diff, &room); + } + + let obj = self.obj(); + if self.is_empty() != was_empty { + obj.notify_is_empty(); + } + + obj.emit_read_change_trigger(); + } + + /// Attempt to minimize the given list of diffs. + /// + /// This is necessary because the SDK diffs are not always optimized, + /// e.g. an item is removed then re-added, which creates jumps in the + /// room history. + fn minimize_diff_list( + &self, + diff_list: Vec>>, + ) -> Vec>> { + let mut old_items = Vec::with_capacity(self.sdk_items.n_items() as usize); + + // Construct the current state. + for item in self.sdk_items.iter::() { + let Ok(item) = item else { + // The list changed, return early. + return diff_list; + }; + + old_items.push(item.timeline_id()); + } + + // Get the new state by applying the diffs. + let mut value_map = HashMap::with_capacity(diff_list.len()); + let mut new_items = old_items.iter().cloned().collect::>(); + + for diff in &diff_list { + match diff { + VectorDiff::Append { values } => { + new_items.reserve(values.len()); + + for value in values { + let timeline_id = value.unique_id().0.clone(); + value_map.insert(timeline_id.clone(), value.clone()); + new_items.push_back(timeline_id); + } + } + VectorDiff::PushFront { value } => { + let timeline_id = value.unique_id().0.clone(); + value_map.insert(timeline_id.clone(), value.clone()); + new_items.push_front(timeline_id); + } + VectorDiff::PushBack { value } => { + let timeline_id = value.unique_id().0.clone(); + value_map.insert(timeline_id.clone(), value.clone()); + new_items.push_back(timeline_id); + } + VectorDiff::PopFront => { + new_items.pop_front(); + } + VectorDiff::PopBack => { + new_items.pop_back(); + } + VectorDiff::Insert { index, value } => { + let timeline_id = value.unique_id().0.clone(); + value_map.insert(timeline_id.clone(), value.clone()); + new_items.insert(*index, timeline_id); + } + VectorDiff::Set { index, value } => { + let timeline_id = value.unique_id().0.clone(); + value_map.insert(timeline_id.clone(), value.clone()); + + let item = new_items.get_mut(*index).expect("item exists"); + *item = timeline_id; + } + VectorDiff::Remove { index } => { + new_items.remove(*index); + } + VectorDiff::Clear | VectorDiff::Truncate { .. } | VectorDiff::Reset { .. } => { + // Minimizing this will probably make a worse diff, so we return early. + return diff_list; + } + } + } + + let new_items = Vec::from(new_items); + let mut min_diff_list = Vec::with_capacity(diff_list.len()); + let mut index = 0; + let mut n_items = old_items.len(); + + for result in diff::slice(&old_items, &new_items) { + match result { + diff::Result::Left(_) => { + min_diff_list.push(VectorDiff::Remove { index }); + n_items -= 1; + } + diff::Result::Both(timeline_id, _) => { + if let Some(value) = value_map.get(timeline_id).cloned() { + min_diff_list.push(VectorDiff::Set { index, value }); + } + index += 1; + } + diff::Result::Right(timeline_id) => { + let value = value_map + .get(timeline_id) + .expect("value exists for added item") + .clone(); + + if index == n_items { + if let Some(VectorDiff::Append { values }) = min_diff_list.last_mut() { + values.push_back(value); + } else { + min_diff_list.push(VectorDiff::Append { + values: Vector::from([value]), + }); + } + } else { + min_diff_list.push(VectorDiff::Insert { index, value }); + } + + index += 1; + n_items += 1; + } + } + } + + min_diff_list + } + + /// Update this timeline with the given diff. + #[allow(clippy::too_many_lines)] + fn update_with_single_diff(&self, diff: VectorDiff>, room: &Room) { match diff { VectorDiff::Append { values } => { let new_list = values @@ -282,10 +421,10 @@ mod imp { .map(|item| self.create_item(&item)) .collect::>(); - let pos = sdk_items.n_items(); + let pos = self.sdk_items.n_items(); let added = new_list.len() as u32; - sdk_items.extend_from_slice(&new_list); + self.sdk_items.extend_from_slice(&new_list); self.update_items_headers(pos, added.max(1)); // Try to update the latest unread message. @@ -294,14 +433,14 @@ mod imp { ); } VectorDiff::Clear => { - sdk_items.remove_all(); + self.sdk_items.remove_all(); self.event_map.borrow_mut().clear(); self.set_has_room_create(false); } VectorDiff::PushFront { value } => { let item = self.create_item(&value); - sdk_items.insert(0, &item); + self.sdk_items.insert(0, &item); self.update_items_headers(0, 1); // Try to update the latest unread message. @@ -311,8 +450,8 @@ mod imp { } VectorDiff::PushBack { value } => { let item = self.create_item(&value); - let pos = sdk_items.n_items(); - sdk_items.append(&item); + let pos = self.sdk_items.n_items(); + self.sdk_items.append(&item); self.update_items_headers(pos, 1); // Try to update the latest unread message. @@ -321,24 +460,32 @@ mod imp { } } VectorDiff::PopFront => { - let item = sdk_items.item(0).and_downcast().unwrap(); + let item = self + .sdk_items + .item(0) + .and_downcast() + .expect("all items are TimelineItems"); self.remove_item(&item); - sdk_items.remove(0); + self.sdk_items.remove(0); self.update_items_headers(0, 1); } VectorDiff::PopBack => { - let pos = sdk_items.n_items() - 1; - let item = sdk_items.item(pos).and_downcast().unwrap(); + let pos = self.sdk_items.n_items() - 1; + let item = self + .sdk_items + .item(pos) + .and_downcast() + .expect("all items are TimelineItems"); self.remove_item(&item); - sdk_items.remove(pos); + self.sdk_items.remove(pos); } VectorDiff::Insert { index, value } => { let pos = index as u32; let item = self.create_item(&value); - sdk_items.insert(pos, &item); + self.sdk_items.insert(pos, &item); self.update_items_headers(pos, 1); // Try to update the latest unread message. @@ -348,7 +495,11 @@ mod imp { } VectorDiff::Set { index, value } => { let pos = index as u32; - let prev_item = sdk_items.item(pos).and_downcast::().unwrap(); + let prev_item = self + .sdk_items + .item(pos) + .and_downcast::() + .expect("all items are TimelineItems"); let item = if prev_item.try_update_with(&value) { if let Some(event) = prev_item.downcast_ref::() { @@ -364,7 +515,7 @@ mod imp { self.remove_item(&prev_item); let item = self.create_item(&value); - sdk_items.splice(pos, 1, &[item.clone()]); + self.sdk_items.splice(pos, 1, &[item.clone()]); item }; @@ -379,22 +530,31 @@ mod imp { } VectorDiff::Remove { index } => { let pos = index as u32; - let item = sdk_items.item(pos).and_downcast().unwrap(); + let item = self + .sdk_items + .item(pos) + .and_downcast() + .expect("all items are TimelineItems"); self.remove_item(&item); - sdk_items.remove(pos); + self.sdk_items.remove(pos); self.update_items_headers(pos, 1); } VectorDiff::Truncate { length } => { let new_len = length as u32; - let old_len = sdk_items.n_items(); + let old_len = self.sdk_items.n_items(); for pos in new_len..old_len { - let item = sdk_items.item(pos).and_downcast().unwrap(); + let item = self + .sdk_items + .item(pos) + .and_downcast() + .expect("all items are TimelineItems"); self.remove_item(&item); } - sdk_items.splice(new_len, old_len - new_len, &[] as &[glib::Object]); + self.sdk_items + .splice(new_len, old_len - new_len, &[] as &[glib::Object]); } VectorDiff::Reset { values } => { // Reset the state. @@ -406,10 +566,10 @@ mod imp { .map(|item| self.create_item(&item)) .collect::>(); - let removed = sdk_items.n_items(); + let removed = self.sdk_items.n_items(); let added = new_list.len() as u32; - sdk_items.splice(0, removed, &new_list); + self.sdk_items.splice(0, removed, &new_list); self.update_items_headers(0, added.max(1)); // Try to update the latest unread message. @@ -418,13 +578,6 @@ mod imp { ); } } - - let obj = self.obj(); - if self.is_empty() != was_empty { - obj.notify_is_empty(); - } - - obj.emit_read_change_trigger(); } /// Update `nb` items' headers starting at `pos`. @@ -625,7 +778,7 @@ mod imp { let handle = spawn_tokio!(fut); self.read_receipts_changed_handle .set(handle.abort_handle()) - .unwrap(); + .expect("handle is uninitialized"); } } } @@ -738,7 +891,7 @@ impl Timeline { .await }) .await - .unwrap(); + .expect("task was not aborted"); let sdk_items = &self.imp().sdk_items; let count = sdk_items.n_items(); diff --git a/src/session/model/room/timeline/timeline_item.rs b/src/session/model/room/timeline/timeline_item.rs index 6626e52f..0151840b 100644 --- a/src/session/model/room/timeline/timeline_item.rs +++ b/src/session/model/room/timeline/timeline_item.rs @@ -6,14 +6,16 @@ use super::VirtualItem; use crate::session::model::{Event, Room}; mod imp { - use std::{cell::Cell, marker::PhantomData}; + use std::{ + cell::{Cell, RefCell}, + marker::PhantomData, + }; use super::*; #[repr(C)] pub struct TimelineItemClass { parent_class: glib::object::ObjectClass, - pub(super) id: fn(&super::TimelineItem) -> String, pub(super) selectable: fn(&super::TimelineItem) -> bool, pub(super) can_hide_header: fn(&super::TimelineItem) -> bool, pub(super) event_sender_id: fn(&super::TimelineItem) -> Option, @@ -23,11 +25,6 @@ mod imp { type Type = TimelineItem; } - pub(super) fn timeline_item_id(this: &super::TimelineItem) -> String { - let klass = this.class(); - (klass.as_ref().id)(this) - } - pub(super) fn timeline_item_selectable(this: &super::TimelineItem) -> bool { let klass = this.class(); (klass.as_ref().selectable)(this) @@ -46,11 +43,9 @@ mod imp { #[derive(Debug, Default, glib::Properties)] #[properties(wrapper_type = super::TimelineItem)] pub struct TimelineItem { - /// A unique ID for this `TimelineItem`. - /// - /// For debugging purposes. - #[property(get = Self::id)] - id: PhantomData, + /// A unique ID for this `TimelineItem` in the local timeline. + #[property(get, construct_only)] + timeline_id: RefCell, /// Whether this `TimelineItem` is selectable. /// /// Defaults to `false`. @@ -85,13 +80,6 @@ mod imp { impl ObjectImpl for TimelineItem {} impl TimelineItem { - /// A unique ID for this `TimelineItem`. - /// - /// For debugging purposes. - fn id(&self) -> String { - imp::timeline_item_id(&self.obj()) - } - /// Whether this `TimelineItem` is selectable. /// /// Defaults to `false`. @@ -135,9 +123,11 @@ impl TimelineItem { /// /// Constructs the proper child type. pub fn new(item: &SdkTimelineItem, room: &Room) -> Self { + let timeline_id = &item.unique_id().0; + match item.kind() { - TimelineItemKind::Event(event) => Event::new(event.clone(), room).upcast(), - TimelineItemKind::Virtual(item) => VirtualItem::new(item).upcast(), + TimelineItemKind::Event(event) => Event::new(event.clone(), room, timeline_id).upcast(), + TimelineItemKind::Virtual(item) => VirtualItem::with_item(item, timeline_id).upcast(), } } @@ -145,14 +135,16 @@ impl TimelineItem { /// /// Returns `true` if the update succeeded. pub(crate) fn try_update_with(&self, item: &SdkTimelineItem) -> bool { + let timeline_id = &item.unique_id().0; + match item.kind() { TimelineItemKind::Event(new_event) => { if let Some(event) = self.downcast_ref::() { - return event.try_update_with(new_event); + return event.try_update_with(new_event, timeline_id); } } TimelineItemKind::Virtual(_item) => { - // Always invalidate. It shouldn't happen often and updating + // Always invalidate. It should not happen often and updating // those should be unexpensive. } } @@ -168,10 +160,8 @@ impl TimelineItem { /// of `TimelineItemImpl`. #[allow(dead_code)] pub(crate) trait TimelineItemExt: 'static { - /// A unique ID for this `TimelineItem`. - /// - /// For debugging purposes. - fn id(&self) -> String; + /// A unique ID for this `TimelineItem` in the local timeline. + fn timeline_id(&self) -> String; /// Whether this `TimelineItem` is selectable. /// @@ -198,8 +188,8 @@ pub(crate) trait TimelineItemExt: 'static { } impl> TimelineItemExt for O { - fn id(&self) -> String { - self.upcast_ref().id() + fn timeline_id(&self) -> String { + self.upcast_ref().timeline_id() } fn selectable(&self) -> bool { @@ -229,8 +219,6 @@ impl> TimelineItemExt for O { /// Overriding a method from this Trait overrides also its behavior in /// `TimelineItemExt`. pub(crate) trait TimelineItemImpl: ObjectImpl { - fn id(&self) -> String; - fn selectable(&self) -> bool { false } @@ -255,7 +243,6 @@ where let klass = class.as_mut(); - klass.id = id_trampoline::; klass.selectable = selectable_trampoline::; klass.can_hide_header = can_hide_header_trampoline::; klass.event_sender_id = event_sender_id_trampoline::; @@ -263,15 +250,6 @@ where } // Virtual method implementation trampolines. -fn id_trampoline(this: &TimelineItem) -> String -where - T: ObjectSubclass + TimelineItemImpl, - T::Type: IsA, -{ - let this = this.downcast_ref::().unwrap(); - this.imp().id() -} - fn selectable_trampoline(this: &TimelineItem) -> bool where T: ObjectSubclass + TimelineItemImpl, diff --git a/src/session/model/room/timeline/virtual_item.rs b/src/session/model/room/timeline/virtual_item.rs index 134ed4f1..6380b42b 100644 --- a/src/session/model/room/timeline/virtual_item.rs +++ b/src/session/model/room/timeline/virtual_item.rs @@ -67,19 +67,7 @@ mod imp { #[glib::derived_properties] impl ObjectImpl for VirtualItem {} - impl TimelineItemImpl for VirtualItem { - fn id(&self) -> String { - match &**self.kind.borrow() { - VirtualItemKind::Spinner => "VirtualItem::Spinner".to_owned(), - VirtualItemKind::Typing => "VirtualItem::Typing".to_owned(), - VirtualItemKind::TimelineStart => "VirtualItem::TimelineStart".to_owned(), - VirtualItemKind::DayDivider(date) => { - format!("VirtualItem::DayDivider({})", date.format("%F").unwrap()) - } - VirtualItemKind::NewMessages => "VirtualItem::NewMessages".to_owned(), - } - } - } + impl TimelineItemImpl for VirtualItem {} } glib::wrapper! { @@ -91,10 +79,12 @@ glib::wrapper! { impl VirtualItem { /// Create a new `VirtualItem` from a virtual timeline item. - pub fn new(item: &VirtualTimelineItem) -> Self { + pub fn with_item(item: &VirtualTimelineItem, timeline_id: &str) -> Self { match item { - VirtualTimelineItem::DateDivider(ts) => Self::day_divider_with_timestamp(*ts), - VirtualTimelineItem::ReadMarker => Self::new_messages(), + VirtualTimelineItem::DateDivider(ts) => { + Self::day_divider_with_timestamp(*ts, timeline_id) + } + VirtualTimelineItem::ReadMarker => Self::new_messages(timeline_id), } } @@ -102,6 +92,7 @@ impl VirtualItem { pub(crate) fn spinner() -> Self { glib::Object::builder() .property("kind", VirtualItemKind::Spinner.boxed()) + .property("timeline-id", "VirtualItemKind::Spinner") .build() } @@ -109,6 +100,7 @@ impl VirtualItem { pub(crate) fn typing() -> Self { glib::Object::builder() .property("kind", VirtualItemKind::Typing.boxed()) + .property("timeline-id", "VirtualItemKind::Typing") .build() } @@ -116,13 +108,15 @@ impl VirtualItem { pub(crate) fn timeline_start() -> Self { glib::Object::builder() .property("kind", VirtualItemKind::TimelineStart.boxed()) + .property("timeline-id", "VirtualItemKind::TimelineStart") .build() } /// Create a new messages virtual item. - pub(crate) fn new_messages() -> Self { + fn new_messages(timeline_id: &str) -> Self { glib::Object::builder() .property("kind", VirtualItemKind::NewMessages.boxed()) + .property("timeline-id", timeline_id) .build() } @@ -133,13 +127,17 @@ impl VirtualItem { /// current local time. /// /// Panics if an error occurred when accessing the current local time. - pub(crate) fn day_divider_with_timestamp(timestamp: MilliSecondsSinceUnixEpoch) -> Self { + fn day_divider_with_timestamp( + timestamp: MilliSecondsSinceUnixEpoch, + timeline_id: &str, + ) -> Self { let date = glib::DateTime::from_unix_utc(timestamp.as_secs().into()) .or_else(|_| glib::DateTime::now_utc()) .expect("We should be able to get the current time"); glib::Object::builder() .property("kind", VirtualItemKind::DayDivider(date).boxed()) + .property("timeline-id", timeline_id) .build() } }