From e5faaeb2f2622e0d740b6ef836ce1d0b107e6743 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 5 Jan 2022 17:36:12 -0800 Subject: [PATCH 1/2] Fix `Global::gt` and rename it to `changed_since` A false negative return value of `gt` was preventing guests' multibuffers from syncing correctly. Co-Authored-By: Nathan Sobo --- crates/clock/src/clock.rs | 24 +++++++++--------------- crates/editor/src/multi_buffer.rs | 22 +++++++++++++++++++++- crates/language/src/buffer.rs | 7 ++++--- crates/text/src/text.rs | 14 +++++++------- 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/crates/clock/src/clock.rs b/crates/clock/src/clock.rs index 2632aecce5..888889871f 100644 --- a/crates/clock/src/clock.rs +++ b/crates/clock/src/clock.rs @@ -178,7 +178,7 @@ impl Global { } } - pub fn ge(&self, other: &Self) -> bool { + pub fn observed_all(&self, other: &Self) -> bool { let mut lhs = self.0.iter(); let mut rhs = other.0.iter(); loop { @@ -196,22 +196,16 @@ impl Global { } } - pub fn gt(&self, other: &Self) -> bool { - let mut lhs = self.0.iter(); - let mut rhs = other.0.iter(); - loop { - if let Some(left) = lhs.next() { - if let Some(right) = rhs.next() { - if left <= right { - return false; - } - } else { - return true; - } - } else { - return rhs.next().is_none(); + pub fn changed_since(&self, other: &Self) -> bool { + if self.0.len() > other.0.len() { + return true; + } + for (left, right) in self.0.iter().zip(other.0.iter()) { + if left > right { + return true; } } + false } pub fn iter<'a>(&'a self) -> impl 'a + Iterator { diff --git a/crates/editor/src/multi_buffer.rs b/crates/editor/src/multi_buffer.rs index d382bd1abe..7e48264221 100644 --- a/crates/editor/src/multi_buffer.rs +++ b/crates/editor/src/multi_buffer.rs @@ -804,7 +804,7 @@ impl MultiBuffer { let selections_update_count = buffer.selections_update_count(); let diagnostics_update_count = buffer.diagnostics_update_count(); - let buffer_edited = version.gt(&buffer_state.last_version); + let buffer_edited = version.changed_since(&buffer_state.last_version); let buffer_reparsed = parse_count > buffer_state.last_parse_count; let buffer_selections_updated = selections_update_count > buffer_state.last_selections_update_count; @@ -2116,6 +2116,26 @@ mod tests { ); } + #[gpui::test] + fn test_remote_multibuffer(cx: &mut MutableAppContext) { + let host_buffer = cx.add_model(|cx| Buffer::new(0, "a", cx)); + let guest_buffer = cx.add_model(|cx| { + let message = host_buffer.read(cx).to_proto(); + Buffer::from_proto(1, message, None, cx).unwrap() + }); + let multibuffer = cx.add_model(|cx| MultiBuffer::singleton(guest_buffer.clone(), cx)); + let snapshot = multibuffer.read(cx).snapshot(cx); + assert_eq!(snapshot.text(), "a"); + + guest_buffer.update(cx, |buffer, cx| buffer.edit([1..1], "b", cx)); + let snapshot = multibuffer.read(cx).snapshot(cx); + assert_eq!(snapshot.text(), "ab"); + + guest_buffer.update(cx, |buffer, cx| buffer.edit([2..2], "c", cx)); + let snapshot = multibuffer.read(cx).snapshot(cx); + assert_eq!(snapshot.text(), "abc"); + } + #[gpui::test] fn test_excerpt_buffer(cx: &mut MutableAppContext) { let buffer_1 = cx.add_model(|cx| Buffer::new(0, sample_text(6, 6, 'a'), cx)); diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 22bb46d175..39e95a99ac 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -725,7 +725,8 @@ impl Buffer { let grammar_changed = this .grammar() .map_or(true, |curr_grammar| !Arc::ptr_eq(&grammar, curr_grammar)); - let parse_again = this.version.gt(&parsed_version) || grammar_changed; + let parse_again = + this.version.changed_since(&parsed_version) || grammar_changed; this.parsing_in_background = false; this.did_finish_parsing(new_tree, parsed_version, cx); @@ -1095,12 +1096,12 @@ impl Buffer { } pub fn is_dirty(&self) -> bool { - !self.saved_version.ge(&self.version) + !self.saved_version.observed_all(&self.version) || self.file.as_ref().map_or(false, |file| file.is_deleted()) } pub fn has_conflict(&self) -> bool { - !self.saved_version.ge(&self.version) + !self.saved_version.observed_all(&self.version) && self .file .as_ref() diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 9db7591f22..4f80fa9281 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -1125,8 +1125,8 @@ impl Buffer { false } else { match op { - Operation::Edit(edit) => self.version.ge(&edit.version), - Operation::Undo { undo, .. } => self.version.ge(&undo.version), + Operation::Edit(edit) => self.version.observed_all(&edit.version), + Operation::Undo { undo, .. } => self.version.observed_all(&undo.version), } } } @@ -1648,10 +1648,10 @@ impl BufferSnapshot { let fragments_cursor = if *since == self.version { None } else { - Some( - self.fragments - .filter(move |summary| !since.ge(&summary.max_version), &None), - ) + Some(self.fragments.filter( + move |summary| !since.observed_all(&summary.max_version), + &None, + )) }; let mut cursor = self .fragments @@ -2025,7 +2025,7 @@ impl<'a> sum_tree::Dimension<'a, FragmentSummary> for VersionedFullOffset { fn add_summary(&mut self, summary: &'a FragmentSummary, cx: &Option) { if let Self::Offset(offset) = self { let version = cx.as_ref().unwrap(); - if version.ge(&summary.max_insertion_version) { + if version.observed_all(&summary.max_insertion_version) { *offset += summary.text.visible + summary.text.deleted; } else if version.observed_any(&summary.min_insertion_version) { *self = Self::Invalid; From f9f75e98f8dddc4bdf654e8996d6b4c71b546e4b Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 5 Jan 2022 17:58:24 -0800 Subject: [PATCH 2/2] Fix Locator::from_index Enhance language::tests::test_random_collaborators so that it checks buffer invariants. Co-Authored-By: Nathan Sobo --- crates/language/src/tests.rs | 4 ++++ crates/text/src/locator.rs | 2 +- crates/text/src/tests.rs | 33 ----------------------------- crates/text/src/text.rs | 41 ++++++++++++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 34 deletions(-) diff --git a/crates/language/src/tests.rs b/crates/language/src/tests.rs index fefbbe04f9..937d0d16c2 100644 --- a/crates/language/src/tests.rs +++ b/crates/language/src/tests.rs @@ -941,6 +941,10 @@ fn test_random_collaboration(cx: &mut MutableAppContext, mut rng: StdRng) { now += Duration::from_millis(rng.gen_range(0..=200)); buffers.extend(new_buffer); + for buffer in &buffers { + buffer.read(cx).check_invariants(); + } + if mutation_count == 0 && network.is_idle() { break; } diff --git a/crates/text/src/locator.rs b/crates/text/src/locator.rs index ddd3663e73..cb1e78153c 100644 --- a/crates/text/src/locator.rs +++ b/crates/text/src/locator.rs @@ -20,7 +20,7 @@ impl Locator { } pub fn from_index(ix: usize, count: usize) -> Self { - let id = ((ix as u128 * u64::MAX as u128) / count as u128) as u64; + let id = (1 + ix as u64) * (u64::MAX / (count as u64 + 2)); Self(smallvec![id]) } diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index e1ffc928c0..586be6f9a2 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -602,36 +602,3 @@ fn test_random_concurrent_edits(mut rng: StdRng) { buffer.check_invariants(); } } - -impl Buffer { - fn check_invariants(&self) { - // Ensure every fragment is ordered by locator in the fragment tree and corresponds - // to an insertion fragment in the insertions tree. - let mut prev_fragment_id = Locator::min(); - for fragment in self.snapshot.fragments.items(&None) { - assert!(fragment.id > prev_fragment_id); - prev_fragment_id = fragment.id.clone(); - - let insertion_fragment = self - .snapshot - .insertions - .get( - &InsertionFragmentKey { - timestamp: fragment.insertion_timestamp.local(), - split_offset: fragment.insertion_offset, - }, - &(), - ) - .unwrap(); - assert_eq!(insertion_fragment.fragment_id, fragment.id); - } - - let mut cursor = self.snapshot.fragments.cursor::>(); - for insertion_fragment in self.snapshot.insertions.cursor::<()>() { - cursor.seek(&Some(&insertion_fragment.fragment_id), Bias::Left, &None); - let fragment = cursor.item().unwrap(); - assert_eq!(insertion_fragment.fragment_id, fragment.id); - assert_eq!(insertion_fragment.split_offset, fragment.insertion_offset); - } - } -} diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 4f80fa9281..108863dd48 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -1244,6 +1244,47 @@ impl Buffer { #[cfg(any(test, feature = "test-support"))] impl Buffer { + pub fn check_invariants(&self) { + // Ensure every fragment is ordered by locator in the fragment tree and corresponds + // to an insertion fragment in the insertions tree. + let mut prev_fragment_id = Locator::min(); + for fragment in self.snapshot.fragments.items(&None) { + assert!(fragment.id > prev_fragment_id); + prev_fragment_id = fragment.id.clone(); + + let insertion_fragment = self + .snapshot + .insertions + .get( + &InsertionFragmentKey { + timestamp: fragment.insertion_timestamp.local(), + split_offset: fragment.insertion_offset, + }, + &(), + ) + .unwrap(); + assert_eq!(insertion_fragment.fragment_id, fragment.id); + } + + let mut cursor = self.snapshot.fragments.cursor::>(); + for insertion_fragment in self.snapshot.insertions.cursor::<()>() { + cursor.seek(&Some(&insertion_fragment.fragment_id), Bias::Left, &None); + let fragment = cursor.item().unwrap(); + assert_eq!(insertion_fragment.fragment_id, fragment.id); + assert_eq!(insertion_fragment.split_offset, fragment.insertion_offset); + } + + let fragment_summary = self.snapshot.fragments.summary(); + assert_eq!( + fragment_summary.text.visible, + self.snapshot.visible_text.len() + ); + assert_eq!( + fragment_summary.text.deleted, + self.snapshot.deleted_text.len() + ); + } + pub fn set_group_interval(&mut self, group_interval: Duration) { self.history.group_interval = group_interval; }