From 6c3301ded0fa27fc4989c83c91cf713cd7a44d4f Mon Sep 17 00:00:00 2001 From: Andrei Zisu Date: Tue, 5 Dec 2023 23:48:03 +0100 Subject: [PATCH 1/5] Implement summaries list diffing --- app/src/models/folder_conversations_list.rs | 129 ++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/app/src/models/folder_conversations_list.rs b/app/src/models/folder_conversations_list.rs index 685487c..e149348 100644 --- a/app/src/models/folder_conversations_list.rs +++ b/app/src/models/folder_conversations_list.rs @@ -174,3 +174,132 @@ pub mod row_data { } } } + +#[derive(PartialEq, Debug)] +enum MismatchState { + None, + Started { + old_summaries_index_start: usize, + new_summaries_index_start: usize, + }, + Ended { + old_summaries_index_start: usize, + new_summaries_index_start: usize, + new_summaries_index_end: usize, + }, +} + +fn diff_summaries(old_summaries: &Vec, new_summaries: &Vec) -> Vec<(usize, usize, usize)> { + //@TODO at the moment handling only newly added items + let mut new_summaries_cursor = 0; + + let mut mismatch_state_machine = MismatchState::None; + + let mut diff = Vec::new(); + + for (old_summaries_index, old_item) in old_summaries.iter().enumerate() { + for (new_summaries_index, new_item) in new_summaries.iter().enumerate().skip(new_summaries_cursor) { + if new_item.message_id == old_item.message_id { + new_summaries_cursor = new_summaries_index + 1; + + if let MismatchState::Started { + old_summaries_index_start, + new_summaries_index_start, + } = mismatch_state_machine + { + mismatch_state_machine = MismatchState::Ended { + old_summaries_index_start, + new_summaries_index_start, + new_summaries_index_end: new_summaries_index, + }; + } + break; + } else if MismatchState::None == mismatch_state_machine { + mismatch_state_machine = MismatchState::Started { + old_summaries_index_start: old_summaries_index, + new_summaries_index_start: new_summaries_index, + }; + } + } + + if let MismatchState::Ended { + old_summaries_index_start, + new_summaries_index_start, + new_summaries_index_end, + } = mismatch_state_machine + { + let new_change = (old_summaries_index_start, 0, new_summaries_index_end - new_summaries_index_start); + diff.push(new_change); + + mismatch_state_machine = MismatchState::None; + } + } + + if new_summaries_cursor < new_summaries.len() { + diff.push((old_summaries.len(), 0, new_summaries.len() - new_summaries_cursor)); + } + + diff +} + +#[cfg(test)] +pub mod test { + use chrono::NaiveDateTime; + + use crate::models::MessageSummary; + + use super::*; + + fn create_message_summary(id: i32) -> models::MessageSummary { + models::MessageSummary { + id, + message_id: format!("id_{}", id).to_string(), + subject: format!("subject {}", id).to_string(), + from: format!("from {}", id).to_string(), + time_received: chrono::offset::Utc::now().naive_utc(), + } + } + + #[test] + fn test_diff_summaries() { + let old_summaries = vec![create_message_summary(0), create_message_summary(1)]; + + let mut new_summaries = old_summaries.clone(); + + assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![]); + + let mut new_summaries = old_summaries.clone(); + + new_summaries.insert(1, create_message_summary(4)); + new_summaries.insert(2, create_message_summary(5)); + + assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![(1, 0, 2)]); + + let mut new_summaries = old_summaries.clone(); + + new_summaries.insert(1, create_message_summary(4)); + new_summaries.insert(2, create_message_summary(5)); + + new_summaries.push(create_message_summary(6)); + new_summaries.push(create_message_summary(7)); + + assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![(1, 0, 2), (2, 0, 2)]); + + let old_summaries = vec![create_message_summary(0), create_message_summary(1)]; + + let mut new_summaries = old_summaries.clone(); + new_summaries.insert(0, create_message_summary(2)); + new_summaries.insert(1, create_message_summary(3)); + + new_summaries.insert(3, create_message_summary(4)); + new_summaries.insert(4, create_message_summary(5)); + + new_summaries.push(create_message_summary(6)); + new_summaries.push(create_message_summary(7)); + + assert_eq!( + diff_summaries(&old_summaries, &new_summaries), + vec![(0, 0, 2), (1, 0, 2), (2, 0, 2)] + ); + } +} From 56a1f43b8df3711e70dbf6f098378b0d9f562002 Mon Sep 17 00:00:00 2001 From: Andrei Zisu Date: Thu, 7 Dec 2023 00:32:43 +0100 Subject: [PATCH 2/5] Adjust the diff position for item change signal --- app/src/models/folder_conversations_list.rs | 200 ++++++++++++++++++-- 1 file changed, 187 insertions(+), 13 deletions(-) diff --git a/app/src/models/folder_conversations_list.rs b/app/src/models/folder_conversations_list.rs index e149348..2ddfdc4 100644 --- a/app/src/models/folder_conversations_list.rs +++ b/app/src/models/folder_conversations_list.rs @@ -189,7 +189,14 @@ enum MismatchState { }, } -fn diff_summaries(old_summaries: &Vec, new_summaries: &Vec) -> Vec<(usize, usize, usize)> { +#[derive(PartialEq, Debug)] +pub struct SummaryListsDiff { + pub position: u32, + pub removed: u32, + pub added: u32, +} + +fn diff_summary_lists(old_summaries: &Vec, new_summaries: &Vec) -> Vec { //@TODO at the moment handling only newly added items let mut new_summaries_cursor = 0; @@ -228,7 +235,12 @@ fn diff_summaries(old_summaries: &Vec, new_summaries: &V new_summaries_index_end, } = mismatch_state_machine { - let new_change = (old_summaries_index_start, 0, new_summaries_index_end - new_summaries_index_start); + let new_change = SummaryListsDiff { + position: old_summaries_index_start as u32, + removed: 0, + added: (new_summaries_index_end - new_summaries_index_start) as u32, + }; + diff.push(new_change); mismatch_state_machine = MismatchState::None; @@ -236,18 +248,37 @@ fn diff_summaries(old_summaries: &Vec, new_summaries: &V } if new_summaries_cursor < new_summaries.len() { - diff.push((old_summaries.len(), 0, new_summaries.len() - new_summaries_cursor)); + diff.push(SummaryListsDiff { + position: old_summaries.len() as u32, + removed: 0, + added: (new_summaries.len() - new_summaries_cursor) as u32, + }); } diff } -#[cfg(test)] -pub mod test { - use chrono::NaiveDateTime; +fn adjust_diffs_for_items_changed_notification(diffs: Vec) -> Vec { + let mut changes = Vec::new(); + + let mut accumulator: i32 = 0; - use crate::models::MessageSummary; + for diff in diffs { + changes.push(SummaryListsDiff { + position: diff.position + accumulator as u32, + removed: diff.removed, + added: diff.added, + }); + + accumulator -= diff.removed as i32; + accumulator += diff.added as i32; + } + changes +} + +#[cfg(test)] +pub mod test { use super::*; fn create_message_summary(id: i32) -> models::MessageSummary { @@ -261,19 +292,26 @@ pub mod test { } #[test] - fn test_diff_summaries() { + fn test_diff_summary_lists() { let old_summaries = vec![create_message_summary(0), create_message_summary(1)]; let mut new_summaries = old_summaries.clone(); - assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![]); + assert_eq!(diff_summary_lists(&old_summaries, &new_summaries), vec![]); let mut new_summaries = old_summaries.clone(); new_summaries.insert(1, create_message_summary(4)); new_summaries.insert(2, create_message_summary(5)); - assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![(1, 0, 2)]); + assert_eq!( + diff_summary_lists(&old_summaries, &new_summaries), + vec![SummaryListsDiff { + position: 1, + removed: 0, + added: 2 + }] + ); let mut new_summaries = old_summaries.clone(); @@ -283,7 +321,21 @@ pub mod test { new_summaries.push(create_message_summary(6)); new_summaries.push(create_message_summary(7)); - assert_eq!(diff_summaries(&old_summaries, &new_summaries), vec![(1, 0, 2), (2, 0, 2)]); + assert_eq!( + diff_summary_lists(&old_summaries, &new_summaries), + vec![ + SummaryListsDiff { + position: 1, + removed: 0, + added: 2 + }, + SummaryListsDiff { + position: 2, + removed: 0, + added: 2 + } + ] + ); let old_summaries = vec![create_message_summary(0), create_message_summary(1)]; @@ -298,8 +350,130 @@ pub mod test { new_summaries.push(create_message_summary(7)); assert_eq!( - diff_summaries(&old_summaries, &new_summaries), - vec![(0, 0, 2), (1, 0, 2), (2, 0, 2)] + diff_summary_lists(&old_summaries, &new_summaries), + vec![ + SummaryListsDiff { + position: 0, + removed: 0, + added: 2 + }, + SummaryListsDiff { + position: 1, + removed: 0, + added: 2 + }, + SummaryListsDiff { + position: 2, + removed: 0, + added: 2 + } + ] + ); + } + + #[test] + fn test_adjust_diffs_for_items_changed_notification() { + let diffs = vec![SummaryListsDiff { + position: 1, + removed: 0, + added: 2, + }]; + + assert_eq!( + adjust_diffs_for_items_changed_notification(diffs), + vec![SummaryListsDiff { + position: 1, + removed: 0, + added: 2, + }] + ); + + let diffs = vec![ + SummaryListsDiff { + position: 0, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 1, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 2, + removed: 0, + added: 2, + }, + ]; + + assert_eq!( + adjust_diffs_for_items_changed_notification(diffs), + vec![ + SummaryListsDiff { + position: 0, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 3, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 6, + removed: 0, + added: 2, + }, + ] + ); + + let diffs = vec![ + SummaryListsDiff { + position: 0, + removed: 1, + added: 2, + }, + SummaryListsDiff { + position: 1, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 2, + removed: 3, + added: 2, + }, + SummaryListsDiff { + position: 3, + removed: 0, + added: 1, + }, + ]; + + assert_eq!( + adjust_diffs_for_items_changed_notification(diffs), + vec![ + SummaryListsDiff { + position: 0, + removed: 1, + added: 2, + }, + SummaryListsDiff { + position: 2, + removed: 0, + added: 2, + }, + SummaryListsDiff { + position: 6, + removed: 3, + added: 2, + }, + SummaryListsDiff { + position: 6, + removed: 0, + added: 1, + }, + ] ); } } From 1571b4e332652158775702c1b667e265d9d61035 Mon Sep 17 00:00:00 2001 From: Andrei Zisu Date: Thu, 7 Dec 2023 00:34:46 +0100 Subject: [PATCH 3/5] Improve handling of change loading --- app/src/models/folder_conversations_list.rs | 64 +++++++++++++-------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/app/src/models/folder_conversations_list.rs b/app/src/models/folder_conversations_list.rs index 2ddfdc4..34ab616 100644 --- a/app/src/models/folder_conversations_list.rs +++ b/app/src/models/folder_conversations_list.rs @@ -80,44 +80,62 @@ pub mod model { self_.store.replace(Some(store)); } + fn load_summaries(&self) -> Option> { + let self_ = imp::FolderModel::from_obj(self); + + if let Some(currently_loaded_folder) = self_.currently_loaded_folder.borrow().as_ref() { + let new_summaries = self_ + .store + .borrow() + .as_ref() + .unwrap() + .get_message_summaries_for_folder(currently_loaded_folder) + .expect("Unable to get message summary"); + + return Some(new_summaries); + } + + None + } + pub fn load_folder(self, folder: models::Folder) { let self_ = imp::FolderModel::from_obj(&self); self_.currently_loaded_folder.replace(Some(folder)); - self.update_list(); + let previous_count = self_.n_items(); + + let new_summaries = self.load_summaries(); + + self_.summaries.replace(new_summaries); + + let new_count = self_.n_items(); + + self.items_changed(0, previous_count, new_count); } - fn update_list(&self) { + pub fn handle_new_messages_for_folder(&self, folder: &models::Folder) { let self_ = imp::FolderModel::from_obj(self); if let Some(currently_loaded_folder) = self_.currently_loaded_folder.borrow().as_ref() { - let previous_count = self_.n_items(); + if currently_loaded_folder.folder_name == folder.folder_name && currently_loaded_folder.identity_id == folder.identity_id { + let self_ = imp::FolderModel::from_obj(self); - self_.summaries.replace(Some( - self_ - .store - .borrow() - .as_ref() - .unwrap() - .get_message_summaries_for_folder(currently_loaded_folder) - .expect("Unable to get message summary"), - )); + let new_summaries = self.load_summaries(); - let new_count = self_.n_items(); + // The only case in which it can be None is when a folder is not selected, but we already checked + // above if it is selected, so it's okay to unwrap. + let new_summaries = new_summaries.unwrap(); - self.items_changed(0, previous_count, new_count); - } - } + let diffs = diff_summary_lists(&self_.summaries.borrow().as_ref().unwrap(), &new_summaries); - pub fn handle_new_messages_for_folder(self, folder: &models::Folder) { - let self_ = imp::FolderModel::from_obj(&self); + let changes = adjust_diffs_for_items_changed_notification(diffs); - if let Some(currently_loaded_folder) = self_.currently_loaded_folder.borrow().as_ref() { - if currently_loaded_folder.folder_name == folder.folder_name && currently_loaded_folder.identity_id == folder.identity_id { - // This creates ugly flicker and loss of selection. Should - // be fixed with issue #227 - // self.update_list(); + self_.summaries.replace(Some(new_summaries)); + + for change in changes { + self.items_changed(change.position, change.removed, change.added); + } } } } From dc6bca38c07822c463757bf08969b97433ebb344 Mon Sep 17 00:00:00 2001 From: Andrei Zisu Date: Thu, 7 Dec 2023 23:39:13 +0100 Subject: [PATCH 4/5] Move the reload to a separate signal --- app/src/models/folder_conversations_list.rs | 22 ++++++++++++--------- app/src/ui/window/dynamic_list_view.rs | 13 ++++++++++-- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/src/models/folder_conversations_list.rs b/app/src/models/folder_conversations_list.rs index 34ab616..74ebd31 100644 --- a/app/src/models/folder_conversations_list.rs +++ b/app/src/models/folder_conversations_list.rs @@ -5,9 +5,13 @@ use gtk::prelude::*; use glib::subclass::prelude::*; use gtk::subclass::prelude::*; +use gtk::glib::subclass::Signal; + use std::cell::RefCell; use std::rc::Rc; +use once_cell::sync::Lazy; + use crate::models; use crate::services; @@ -15,6 +19,7 @@ pub mod model { use super::*; use row_data::ConversationRowData; mod imp { + use super::*; #[derive(Debug)] @@ -41,7 +46,12 @@ pub mod model { } } } - impl ObjectImpl for FolderModel {} + impl ObjectImpl for FolderModel { + fn signals() -> &'static [Signal] { + static SIGNALS: Lazy> = Lazy::new(|| vec![Signal::builder("folder-loaded").build()]); + SIGNALS.as_ref() + } + } impl ListModelImpl for FolderModel { fn item_type(&self) -> glib::Type { ConversationRowData::static_type() @@ -103,15 +113,9 @@ pub mod model { self_.currently_loaded_folder.replace(Some(folder)); - let previous_count = self_.n_items(); - - let new_summaries = self.load_summaries(); - - self_.summaries.replace(new_summaries); - - let new_count = self_.n_items(); + self_.summaries.replace(self.load_summaries()); - self.items_changed(0, previous_count, new_count); + self_.obj().emit_by_name::<()>("folder-loaded", &[]); } pub fn handle_new_messages_for_folder(&self, folder: &models::Folder) { diff --git a/app/src/ui/window/dynamic_list_view.rs b/app/src/ui/window/dynamic_list_view.rs index 736da4c..2e6f858 100644 --- a/app/src/ui/window/dynamic_list_view.rs +++ b/app/src/ui/window/dynamic_list_view.rs @@ -103,12 +103,13 @@ mod imp { } pub fn set_conversations_list_model(&self, conversations_list_model: FolderModel) { - let obj = self.obj().clone(); let first_item_clone = self.first_item.clone(); let last_item_clone = self.last_item.clone(); let vertical_adjustment_clone = self.vertical_adjustment.clone(); - conversations_list_model.connect_items_changed(move |_, _, _, _| { + let obj = self.obj().clone(); + + conversations_list_model.connect_local("folder-loaded", false, move |_| { // Currently there is no ability to refresh folder contents while they are // loaded, so I'm hijacking this to signify loading a new folder. Will have to // change in the future. @@ -128,6 +129,14 @@ mod imp { *last_item_clone.borrow_mut() = 0; obj.queue_allocate(); + + None + }); + + let obj = self.obj().clone(); + + conversations_list_model.connect_items_changed(move |_, _, _, _| { + obj.queue_allocate(); }); *self.conversations_list_model.borrow_mut() = Some(conversations_list_model); From 3a9999aa03a9f17e8168b043cb01c76a03bb0b7e Mon Sep 17 00:00:00 2001 From: Andrei Zisu Date: Thu, 7 Dec 2023 23:42:44 +0100 Subject: [PATCH 5/5] Simplify summaries loading --- app/src/models/folder_conversations_list.rs | 34 ++++++++------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/app/src/models/folder_conversations_list.rs b/app/src/models/folder_conversations_list.rs index 74ebd31..e9e9c53 100644 --- a/app/src/models/folder_conversations_list.rs +++ b/app/src/models/folder_conversations_list.rs @@ -90,30 +90,24 @@ pub mod model { self_.store.replace(Some(store)); } - fn load_summaries(&self) -> Option> { - let self_ = imp::FolderModel::from_obj(self); - - if let Some(currently_loaded_folder) = self_.currently_loaded_folder.borrow().as_ref() { - let new_summaries = self_ - .store - .borrow() - .as_ref() - .unwrap() - .get_message_summaries_for_folder(currently_loaded_folder) - .expect("Unable to get message summary"); - - return Some(new_summaries); - } + fn load_summaries(&self, folder: &models::Folder) -> Vec { + let self_ = imp::FolderModel::from_obj(&self); - None + return self_ + .store + .borrow() + .as_ref() + .unwrap() + .get_message_summaries_for_folder(folder) + .expect("Unable to get message summary"); } pub fn load_folder(self, folder: models::Folder) { let self_ = imp::FolderModel::from_obj(&self); - self_.currently_loaded_folder.replace(Some(folder)); + self_.summaries.replace(Some(self.load_summaries(&folder))); - self_.summaries.replace(self.load_summaries()); + self_.currently_loaded_folder.replace(Some(folder)); self_.obj().emit_by_name::<()>("folder-loaded", &[]); } @@ -125,11 +119,7 @@ pub mod model { if currently_loaded_folder.folder_name == folder.folder_name && currently_loaded_folder.identity_id == folder.identity_id { let self_ = imp::FolderModel::from_obj(self); - let new_summaries = self.load_summaries(); - - // The only case in which it can be None is when a folder is not selected, but we already checked - // above if it is selected, so it's okay to unwrap. - let new_summaries = new_summaries.unwrap(); + let new_summaries = self.load_summaries(currently_loaded_folder); let diffs = diff_summary_lists(&self_.summaries.borrow().as_ref().unwrap(), &new_summaries);