From 9eab2019a8e32a371cccb0f355379698325f043e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Wed, 7 Oct 2026 00:17:14 -0400 Subject: [PATCH] =?UTF-8?q?Panics=20=C2=B7=20P22=20Collaboration:=20marks?= =?UTF-8?q?=20outside=20their=20text=20stay=20out=20under=20the=20owner's?= =?UTF-8?q?=20index;=20accept=20answers=20None;=20zero=20reactions=20and?= =?UTF-8?q?=20stale=20masked=20offsets=20hold?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- examples/gallery/pages/collab/talk.rs | 12 ++++- src/collab/annotate.rs | 51 +++++++++++++++--- src/collab/peers.rs | 5 ++ src/collab/reactions.rs | 10 +++- src/collab/sidebar.rs | 5 ++ src/collab/tests.rs | 71 ++++++++++++++++++++++++- src/collab/track.rs | 76 ++++++++++++++++++++------- src/forms/text/edit.rs | 17 +++++- tasks/panics.md | 3 +- 9 files changed, 215 insertions(+), 35 deletions(-) diff --git a/examples/gallery/pages/collab/talk.rs b/examples/gallery/pages/collab/talk.rs index bb7c718..e463c4e 100644 --- a/examples/gallery/pages/collab/talk.rs +++ b/examples/gallery/pages/collab/talk.rs @@ -261,7 +261,10 @@ const BASE: &str = "A lift blends color toward white. Half a lift sits midway be fn applied(base: &str, suggestions: Vec) -> String { let (mut text, mut left) = (base.to_string(), suggestions); while !left.is_empty() { - (text, left) = accept(&text, &left, 0); + let Some(taken) = accept(&text, &left, 0) else { + break; + }; + (text, left) = taken; } text } @@ -306,11 +309,16 @@ pub fn changes(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> { let pending = suggested(&base_text, field.read(cx).text(), &you()); match decision { Decision::Accept(ix) => { - set(&decided, accept(&base_text, &pending, ix).0, cx) + if let Some((text, _)) = accept(&base_text, &pending, ix) { + set(&decided, text, cx) + } } Decision::AcceptAll => set(&decided, applied(&base_text, pending), cx), Decision::Reject(ix) => { let mut rest = pending; + if ix >= rest.len() { + return; + } rest.remove(ix); let text = applied(&base_text, rest); field.update(cx, |input, cx| input.set_text(text, cx)); diff --git a/src/collab/annotate.rs b/src/collab/annotate.rs index 84d9d4d..db68c71 100644 --- a/src/collab/annotate.rs +++ b/src/collab/annotate.rs @@ -26,6 +26,7 @@ pub struct AnnotatedText { id: ElementId, text: SharedString, annotations: Rc>, + kept: Rc>, picked: Option, on_pick: Option, } @@ -38,15 +39,25 @@ impl AnnotatedText { annotations: impl IntoIterator, ) -> Self { let text = text.into(); - let annotations: Vec = annotations.into_iter().collect(); + let (kept, annotations): (Vec, Vec) = annotations + .into_iter() + .enumerate() + .filter(|(_, annotation)| { + let inside = text.get(annotation.range.clone()).is_some(); + if !inside { + log::error!( + "annotated text: {:?} lies outside the text; left out", + annotation.range + ); + } + inside + }) + .unzip(); for annotation in &annotations { - let range = &annotation.range; assert!( - range.start < range.end - && range.end <= text.len() - && text.is_char_boundary(range.start) - && text.is_char_boundary(range.end), - "annotation {range:?} lies outside the text" + !annotation.range.is_empty(), + "annotation {:?} is empty", + annotation.range ); } assert!( @@ -59,6 +70,7 @@ impl AnnotatedText { id: id.into(), text, annotations: Rc::new(annotations), + kept: Rc::new(kept), picked: None, on_pick: None, } @@ -78,6 +90,9 @@ impl AnnotatedText { impl RenderOnce for AnnotatedText { fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement { + if let Some(picked) = self.picked.filter(|picked| !self.kept.contains(picked)) { + log::error!("annotated text: annotation {picked} is not shown; none picked"); + } let thickness = cx.theme().underline_thickness(); let highlights: Vec<(Range, HighlightStyle)> = self .annotations @@ -85,7 +100,7 @@ impl RenderOnce for AnnotatedText { .enumerate() .map(|(ix, annotation)| { let color = annotation.author.color(cx); - let wash = if self.picked == Some(ix) { + let wash = if self.picked == Some(self.kept[ix]) { PICKED } else { WASH @@ -123,6 +138,7 @@ impl RenderOnce for AnnotatedText { }); match self.on_pick { Some(pick) => text.on_click(ranges, move |ix, window, cx| { + let ix = self.kept[ix]; log::info!("annotated text: picked {ix}"); pick(ix, window, cx) }), @@ -130,3 +146,22 @@ impl RenderOnce for AnnotatedText { } } } + +#[cfg(test)] +mod tests { + use super::{AnnotatedText, Annotation}; + use crate::collab::Peer; + + #[test] + fn an_annotation_outside_the_text_is_left_out() { + let note = |range| Annotation { + range, + author: Peer::new("ada", "Ada", 0), + note: None, + }; + let text = AnnotatedText::new("text", "éabc", [note(1..2), note(2..3)]); + assert_eq!(text.annotations.len(), 1); + assert_eq!(text.annotations[0].range, 2..3); + assert_eq!(*text.kept, [1], "a pick names the owner's index"); + } +} diff --git a/src/collab/peers.rs b/src/collab/peers.rs index 6126435..7c79388 100644 --- a/src/collab/peers.rs +++ b/src/collab/peers.rs @@ -102,6 +102,11 @@ impl PresenceAvatars { impl RenderOnce for PresenceAvatars { fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement { + if let Some(following) = &self.following + && !self.peers.iter().any(|peer| &peer.key == following) + { + log::error!("presence: {following} left; following none"); + } let size = AvatarSize::Sm; let rest = self.peers.len().saturating_sub(self.max); let shown: Vec = self diff --git a/src/collab/reactions.rs b/src/collab/reactions.rs index e197720..41e89ba 100644 --- a/src/collab/reactions.rs +++ b/src/collab/reactions.rs @@ -33,7 +33,10 @@ pub fn toggled(reactions: &[Reaction], emoji: &str) -> Vec { .position(|reaction| reaction.emoji.as_ref() == emoji) { Some(ix) if next[ix].mine => { - next[ix].count -= 1; + if next[ix].count == 0 { + log::error!("reactions: {emoji} counts none yet is yours; it comes off"); + } + next[ix].count = next[ix].count.saturating_sub(1); next[ix].mine = false; if next[ix].count == 0 { next.remove(ix); @@ -191,6 +194,11 @@ impl RenderOnce for Reactions { mod tests { use super::*; + #[test] + fn yours_at_no_count_comes_off() { + assert_eq!(toggled(&[reaction("👍", 0, true)], "👍"), []); + } + fn reaction(emoji: &str, count: usize, mine: bool) -> Reaction { Reaction { emoji: emoji.to_string().into(), diff --git a/src/collab/sidebar.rs b/src/collab/sidebar.rs index 375ddc4..df49819 100644 --- a/src/collab/sidebar.rs +++ b/src/collab/sidebar.rs @@ -118,6 +118,11 @@ impl RenderOnce for CommentSidebar { }) }) }); + if let Some(active) = &self.active + && !self.threads.iter().any(|thread| &thread.key == active) + { + log::error!("comment sidebar: thread {active} is gone; none active"); + } let shown: Vec = self .threads .into_iter() diff --git a/src/collab/tests.rs b/src/collab/tests.rs index b2b9fc8..72bcc9b 100644 --- a/src/collab/tests.rs +++ b/src/collab/tests.rs @@ -3,11 +3,11 @@ use gpui::{ TestAppContext, VisualTestContext, Window, div, }; -use super::{FollowMode, Peer, Reply}; +use super::{Decision, FollowMode, Peer, Reply, Suggestion, TrackChanges}; use crate::{ forms, forms::{Input, TextInput}, - theme::Theme, + theme::{ActiveTheme, ControlSize, Theme}, }; /// A field outside a followed view and one inside it, with what they asked: replies sent, follows stopped. @@ -145,3 +145,70 @@ fn without_resolving_one_list_takes_keyboard_picks(cx: &mut TestAppContext) { let picked = board.read_with(cx, |board, _| board.picked.clone()); assert_eq!(picked, vec![gpui::SharedString::from("a")]); } + +/// Two suggestions whose first falls inside a character once the text changes, and the decisions asked. +struct Changing { + changed: bool, + decided: Vec, +} + +impl Render for Changing { + fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { + let owner = cx.entity(); + let suggestion = |range, text: &str| Suggestion { + author: Peer::new("ada", "Ada", 0), + range, + text: text.to_string().into(), + }; + let (base, second) = match self.changed { + false => ("abc", 1..2), + true => ("éabc", 2..3), + }; + let pending = [suggestion(0..1, "X"), suggestion(second, "B")]; + div() + .size_full() + .child( + TrackChanges::new("track", base, pending).on_decide(move |decision, _, cx| { + owner.update(cx, |owner, _| owner.decided.push(decision)) + }), + ) + } +} + +#[gpui::test] +fn a_press_on_a_suggestion_left_out_decides_nothing(cx: &mut TestAppContext) { + cx.update(Theme::init); + let (view, cx) = cx.add_window_view(|_, _| Changing { + changed: false, + decided: Vec::new(), + }); + cx.run_until_parked(); + let row = cx + .debug_bounds("suggestion 0") + .expect("the first suggestion"); + let at = cx.update(|window, cx| { + let button = cx + .theme() + .control_height(ControlSize::Sm) + .to_pixels(window.rem_size()); + let gap = gpui::rems(0.5).to_pixels(window.rem_size()); + gpui::point(row.right() - button - gap - button / 2.0, row.center().y) + }); + cx.simulate_click(at, gpui::Modifiers::none()); + assert_eq!( + view.read_with(cx, |view, _| view.decided.clone()), + [Decision::Accept(0)] + ); + cx.simulate_mouse_down(at, gpui::MouseButton::Left, gpui::Modifiers::none()); + view.update(cx, |view, cx| { + view.changed = true; + cx.notify(); + }); + cx.run_until_parked(); + cx.simulate_mouse_up(at, gpui::MouseButton::Left, gpui::Modifiers::none()); + cx.run_until_parked(); + assert_eq!( + view.read_with(cx, |view, _| view.decided.clone()), + [Decision::Accept(0)] + ); +} diff --git a/src/collab/track.rs b/src/collab/track.rs index 00a7b8d..b0a639c 100644 --- a/src/collab/track.rs +++ b/src/collab/track.rs @@ -158,9 +158,23 @@ pub fn suggested(base: &str, edited: &str, author: &Peer) -> Vec { found } -/// `base` with suggestion `ix` taken, and the others moved to match. -pub fn accept(base: &str, suggestions: &[Suggestion], ix: usize) -> (String, Vec) { - let taken = &suggestions[ix]; +/// `base` with suggestion `ix` taken, and the others moved to match; none when `ix` or its range left `base`. +pub fn accept( + base: &str, + suggestions: &[Suggestion], + ix: usize, +) -> Option<(String, Vec)> { + let Some(taken) = suggestions.get(ix) else { + log::error!("track changes: no suggestion {ix} of {}", suggestions.len()); + return None; + }; + if base.get(taken.range.clone()).is_none() { + log::error!( + "track changes: suggestion {:?} lies outside the text", + taken.range + ); + return None; + } let text = format!( "{}{}{}", &base[..taken.range.start], @@ -181,7 +195,7 @@ pub fn accept(base: &str, suggestions: &[Suggestion], ix: usize) -> (String, Vec suggestion }) .collect(); - (text, rest) + Some((text, rest)) } /// What a suggestion does, in a line: replace, add or delete, with the words. @@ -206,6 +220,7 @@ pub struct TrackChanges { id: ElementId, base: SharedString, suggestions: Vec, + kept: Vec, on_decide: Option, } @@ -217,15 +232,22 @@ impl TrackChanges { suggestions: impl IntoIterator, ) -> Self { let base = base.into(); - let suggestions: Vec = suggestions.into_iter().collect(); + let (kept, suggestions): (Vec, Vec) = suggestions + .into_iter() + .enumerate() + .filter(|(_, suggestion)| { + let inside = base.get(suggestion.range.clone()).is_some(); + if !inside { + log::error!( + "track changes: {:?} lies outside the text; left out", + suggestion.range + ); + } + inside + }) + .unzip(); for suggestion in &suggestions { let range = &suggestion.range; - assert!( - range.end <= base.len() - && base.is_char_boundary(range.start) - && base.is_char_boundary(range.end), - "suggestion {range:?} lies outside the text" - ); assert!( !(range.is_empty() && suggestion.text.is_empty()), "suggestion at {range:?} changes nothing" @@ -241,6 +263,7 @@ impl TrackChanges { id: id.into(), base, suggestions, + kept, on_decide: None, } } @@ -297,8 +320,10 @@ impl RenderOnce for TrackChanges { }; let pending = !self.suggestions.is_empty() && self.on_decide.is_some(); let rows = self.suggestions.iter().enumerate().map(|(ix, suggestion)| { - let id = |what: &str| (self.id.clone(), format!("{what}-{ix}")); + let at = self.kept[ix]; + let id = |what: &str| (self.id.clone(), format!("{what}-{at}")); div() + .debug_selector(move || format!("suggestion {at}")) .flex() .items_center() .gap_2() @@ -327,14 +352,14 @@ impl RenderOnce for TrackChanges { .variant(ButtonVariant::Ghost) .size(ControlSize::Sm) .tooltip("Accept") - .on_click(decide(Decision::Accept(ix))), + .on_click(decide(Decision::Accept(at))), ) .child( IconButton::new(id("reject"), IconName::X) .variant(ButtonVariant::Ghost) .size(ControlSize::Sm) .tooltip("Reject") - .on_click(decide(Decision::Reject(ix))), + .on_click(decide(Decision::Reject(at))), ) }) }); @@ -430,9 +455,9 @@ mod tests { found, [suggestion(2..6, "tint"), suggestion(14..14, "every ")] ); - let (text, rest) = accept("A lift blends color toward white.", &found, 0); + let (text, rest) = accept("A lift blends color toward white.", &found, 0).unwrap(); assert_eq!( - accept(&text, &rest, 0).0, + accept(&text, &rest, 0).unwrap().0, "A tint blends every color toward white." ); } @@ -441,16 +466,29 @@ mod tests { fn accepting_one_moves_the_rest() { let base = "A lift blends color."; let all = [suggestion(2..6, "tint"), suggestion(14..14, "every ")]; - let (text, rest) = accept(base, &all, 0); + let (text, rest) = accept(base, &all, 0).unwrap(); assert_eq!(text, "A tint blends color."); assert_eq!( rest, [suggestion(14..14, "every ")], "same length, same place" ); - let (text, rest) = accept(base, &[suggestion(2..6, "shade of"), all[1].clone()], 0); + let (text, rest) = + accept(base, &[suggestion(2..6, "shade of"), all[1].clone()], 0).unwrap(); assert_eq!(text, "A shade of blends color."); - let (text, _) = accept(&text, &rest, 0); + let (text, _) = accept(&text, &rest, 0).unwrap(); assert_eq!(text, "A shade of blends every color."); } + + #[test] + fn a_suggestion_gone_or_outside_the_text_is_not_taken() { + let base = "A lift blends."; + let inside = suggestion(2..6, "tint"); + assert_eq!(accept(base, std::slice::from_ref(&inside), 1), None); + assert_eq!(accept("A", std::slice::from_ref(&inside), 0), None); + let stale = suggestion(0..40, ""); + let track = TrackChanges::new("track", base, [stale, inside.clone()]); + assert_eq!(track.suggestions, [inside]); + assert_eq!(track.kept, [1], "decisions name the owner's index"); + } } diff --git a/src/forms/text/edit.rs b/src/forms/text/edit.rs index 7cc1181..27ddaa2 100644 --- a/src/forms/text/edit.rs +++ b/src/forms/text/edit.rs @@ -69,9 +69,12 @@ pub(crate) fn from_utf16(text: &str, offset: usize) -> usize { text.len() } -/// Display offset of a content offset when each grapheme shows as `bullet`. +/// Display offset of a content offset when each grapheme shows as `bullet`; an offset the text lost clamps into it. pub(crate) fn masked_offset(text: &str, offset: usize, bullet: char) -> usize { - text[..offset].graphemes(true).count() * bullet.len_utf8() + text[..text.floor_char_boundary(offset)] + .graphemes(true) + .count() + * bullet.len_utf8() } /// Content offset of a display offset in masked text. @@ -182,6 +185,16 @@ mod tests { assert_eq!(masked_offset(text, 6, '•'), 9); assert_eq!(unmasked_offset(text, 9, '•'), 6); assert_eq!(unmasked_offset(text, 12, '•'), text.len()); + assert_eq!( + masked_offset("a", 2, '•'), + 3, + "an offset past the text clamps" + ); + assert_eq!( + masked_offset(text, 4, '•'), + 6, + "inside a character, before it" + ); } #[test] diff --git a/tasks/panics.md b/tasks/panics.md index 0d076d8..ef6011e 100644 --- a/tasks/panics.md +++ b/tasks/panics.md @@ -36,7 +36,7 @@ Each item reads every site in its folders, fixes what the rules name, updates ca - [x] P19 Git — `src/git` - [x] P20 Debug — `src/debug` - [x] P21 Documents — `src/documents` -- [ ] P22 Collaboration — `src/collab` +- [x] P22 Collaboration — `src/collab` - [ ] P23 AI Chat — `src/chat` - [ ] P24 Agent — `src/agent` - [ ] P25 Generative — `src/generative` @@ -87,3 +87,4 @@ One line per item: what changed, what stayed and why, review rounds. - P19: a blame that lags its code draws every line, bare where no owner fits; `git::resolve` returns `Option` for a conflict gone from the text. Kept: diff groups, merge regions, similar's pinned resolutions. Review: 2 rounds. - P20: a hex selection off the bytes and a stack frame past the stack select none; a flame focus keeps what the profile holds; the timeline's wheel reports only finite, positive ranges from finite factors; a dump's last byte must have an address; flame paints log. Kept: disassembly patterns, `zoomed`'s contract. Review: 3 rounds. - P21: block editor listeners painted for a block, field, cell or kind since gone log and change nothing, a dropped picture drag leaves undo alone; a visual block press or edit over text the owner replaced is dropped; nested images parse; a version or find hit past its list selects none. Review: 4 rounds. +- P22: annotations and suggestions outside their text log and stay out, picks and decisions keeping the owner's index; `accept` returns `Option`; a gone pick, active thread or followed peer logs; yours at no count comes off; a masked field clamps a stale offset, so remote cursors and selections draw. Kept: a thread's comments, presence max, one list's order, empty marks, constant role and mode tables. Review: 3 rounds.