From 0ad5c3fe900c5037e0d533f3dd9bd105ce9f49b3 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Fri, 6 Nov 2026 21:27:31 +0700 Subject: [PATCH] feat: add combobox filter --- AGENTS.md | 4 +- crates/core/src/clipboard.rs | 59 ++++++++------ crates/core/src/combobox.rs | 42 +++++++--- crates/core/src/input.rs | 12 ++- crates/core/src/toast.rs | 17 ++-- crates/core/tests/combobox.rs | 141 ++++++++++++++++++++++++++++++---- docs/CHECKLIST.md | 6 +- examples/gallery/src/main.rs | 40 ++++++++++ 8 files changed, 256 insertions(+), 65 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 71c14dd..caa81c1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,8 +57,8 @@ GPUI is a UI framework which also provides primitives for state and concurrency - All use of entities and UI rendering occurs on a single foreground thread. - `cx.spawn(async move |cx| ...)` runs on the foreground thread; from a `Context` it looks like `cx.spawn(async move |this, cx| ...)`. - `cx.background_spawn(async move { ... })` runs work on other threads. -- Both return a `Task`; dropping it cancels the work. Prevent cancellation by awaiting, detaching deliberately, or storing the task in a field. -- Prefer keeping a task in a field over `.detach()` when the work belongs to a view or store; a detached task outlives its owner. A finished `Task` held in a container is not reaped — drop finished handles or hold a single `Option>`. +- Both return a `Task`; dropping it cancels the work. Prevent cancellation by awaiting or storing the task in a field. +- Every task tied to a component must be stored on it: `tasks: Vec>` or `task: Option>`. Never `.detach()` a component's task — a detached task outlives its owner. gpui cleans finished tasks up automatically, so just push; no retain or prune logic. ## Rendering and state diff --git a/crates/core/src/clipboard.rs b/crates/core/src/clipboard.rs index 988ed98..2cae3ac 100644 --- a/crates/core/src/clipboard.rs +++ b/crates/core/src/clipboard.rs @@ -5,7 +5,8 @@ use std::time::Duration; use gpui::prelude::FluentBuilder as _; use gpui::{ - AnyView, App, ClipboardItem, ElementId, Entity, IntoElement, RenderOnce, SharedString, Window, + AnyView, App, ClipboardItem, ElementId, Entity, IntoElement, RenderOnce, SharedString, Task, + Window, }; use gpui_native_icons::IconRole; use gpui_native_theme::Size; @@ -92,11 +93,22 @@ impl Sizable for ClipboardButton { } } +/// The transient copied marker keyed per button. +struct CopiedState { + generation: u64, + tasks: Vec>, +} + impl RenderOnce for ClipboardButton { fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement { - let copied: Entity> = - window.use_keyed_state((self.id.clone(), "clipboard-copied"), cx, |_, _| None); - let is_copied = copied.read(cx).is_some(); + let copied: Entity = + window.use_keyed_state((self.id.clone(), "clipboard-copied"), cx, |_, _| { + CopiedState { + generation: 0, + tasks: Vec::new(), + } + }); + let is_copied = copied.read(cx).generation != 0; let selector = if is_copied { "clipboard-copied" @@ -126,30 +138,29 @@ impl RenderOnce for ClipboardButton { cx.write_to_clipboard(ClipboardItem::new_string(value.to_string())); let generation = copied.update(cx, |state, cx| { - let generation = state.map_or(1, |generation| generation.wrapping_add(1)); - *state = Some(generation); + state.generation += 1; cx.notify(); - generation + state.generation }); - // Clear the copied state after the duration expires - window - .spawn(cx, { - let copied = copied.clone(); - async move |cx| { - cx.background_executor().timer(COPIED_DURATION).await; - cx.update(|_, cx| { - copied.update(cx, |state, cx| { - if *state == Some(generation) { - *state = None; - cx.notify(); - } - }) - }) - .ok(); - } + let weak = copied.downgrade(); + let task = window.spawn(cx, async move |cx| { + cx.background_executor().timer(COPIED_DURATION).await; + cx.update(|_, cx| { + weak.update(cx, |state, cx| { + if state.generation == generation { + state.generation = 0; + cx.notify(); + } + }) + .ok(); }) - .detach(); + .ok(); + }); + + copied.update(cx, |state, _| { + state.tasks.push(task); + }); // Invoke the callback after copying if let Some(on_copied) = &on_copied { diff --git a/crates/core/src/combobox.rs b/crates/core/src/combobox.rs index 3ed5f89..bbf50d6 100644 --- a/crates/core/src/combobox.rs +++ b/crates/core/src/combobox.rs @@ -12,8 +12,8 @@ use gpui::prelude::FluentBuilder as _; use gpui::{ App, Bounds, Context, ElementId, Entity, EventEmitter, InteractiveElement as _, IntoElement, KeyBinding, MouseButton, MouseDownEvent, ParentElement as _, Pixels, Render, Role, - ScrollHandle, SharedString, StatefulInteractiveElement as _, Styled as _, Subscription, Window, - actions, canvas, deferred, div, px, + ScrollHandle, SharedString, StatefulInteractiveElement as _, Styled as _, Subscription, Task, + Window, actions, canvas, deferred, div, px, }; use gpui_native_icons::{IconRole, icon}; use gpui_native_theme::{ActiveTheme as _, Size}; @@ -59,12 +59,12 @@ pub enum UnmatchedText { /// An option for a [`Combobox`]: a value and the label the filter matches on. pub type ComboboxOption = (T, SharedString); -/// Reports the value whenever it changes — including when typing clears it. +/// Reports the value whenever it changes; pub type ComboboxHandler = Rc, &mut Window, &mut App)>; /// Receives unmatched text when the field loses focus in create mode. pub type ComboboxCreateHandler = Rc; -/// A filter over an option: the query, the value, the label. -type Filter = Rc bool>; +/// Decides which options survive a query: the query, the value, the label. +pub type ComboboxFilter = Rc bool>; /// Builder for a combobox. Pass it to [`ComboboxState::new`]. pub struct Combobox @@ -78,6 +78,7 @@ where placeholder: SharedString, on_change: Option>, on_create: Option, + filter: Option>, unmatched: UnmatchedText, size: Size, } @@ -99,6 +100,7 @@ where placeholder: SharedString::from("Select..."), on_change: None, on_create: None, + filter: None, unmatched: UnmatchedText::default(), size: Size::default(), } @@ -138,6 +140,15 @@ where self } + /// Sets the matcher that decides which options survive a query. + pub fn filter(mut self, filter: F) -> Self + where + F: Fn(&str, &T, &SharedString) -> bool + 'static, + { + self.filter = Some(Rc::new(filter)); + self + } + /// Leaves unmatched text in the field on blur. pub fn keep_unmatched_text(mut self) -> Self { self.unmatched = UnmatchedText::Keep; @@ -175,9 +186,11 @@ where highlight: Option, on_change: Option>, on_create: Option, - filter: Filter, + filter: ComboboxFilter, unmatched: UnmatchedText, size: Size, + /// Handler reports in flight; held so they die with the state. + tasks: Vec>, /// Keeps the query-event subscription tied to the state's lifetime. _subscription: Subscription, } @@ -230,11 +243,12 @@ where highlight: None, on_change: combobox.on_change, on_create: combobox.on_create, - filter: Rc::new(|query, _value, label| { - label.to_lowercase().contains(&query.to_lowercase()) + filter: combobox.filter.unwrap_or_else(|| { + Rc::new(|query, _value, label| label.to_lowercase().contains(&query.to_lowercase())) }), unmatched: combobox.unmatched, size: combobox.size, + tasks: Vec::new(), _subscription: subscription, } } @@ -313,7 +327,11 @@ where } self.selected = value.clone(); if let Some(on_change) = self.on_change.clone() { - on_change(value, window, cx); + let value = value.clone(); + let task = window.spawn(cx, async move |cx| { + cx.update(|window, cx| on_change(value, window, cx)).ok(); + }); + self.tasks.push(task); } cx.emit(ComboboxChanged); cx.notify(); @@ -367,7 +385,11 @@ where if !text.is_empty() && let Some(on_create) = self.on_create.clone() { - on_create(SharedString::from(text), window, cx); + let text = SharedString::from(text); + let task = window.spawn(cx, async move |cx| { + cx.update(|window, cx| on_create(text, window, cx)).ok(); + }); + self.tasks.push(task); } } } diff --git a/crates/core/src/input.rs b/crates/core/src/input.rs index 9a5a192..3b4269f 100644 --- a/crates/core/src/input.rs +++ b/crates/core/src/input.rs @@ -11,7 +11,7 @@ use gpui::{ DispatchPhase, ElementId, Entity, EntityInputHandler, EventEmitter, Font, HitboxBehavior, InteractiveElement as _, IntoElement, KeyBinding, KeyDownEvent, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, ParentElement as _, Pixels, RenderOnce, Role, ScrollDelta, - ScrollEvent, SharedString, StatefulInteractiveElement as _, Styled as _, Subscription, + ScrollEvent, SharedString, StatefulInteractiveElement as _, Styled as _, Subscription, Task, TextAlign, TextRun, UTF16Selection, Window, WrappedLine, actions, canvas, div, fill, point, px, }; use gpui_native_icons::IconRole; @@ -483,6 +483,8 @@ struct CaretBlink { visible: bool, active: bool, paused: bool, + /// Pending blink timers; held so they die with the controller. + tasks: Vec>, } impl CaretBlink { @@ -493,6 +495,7 @@ impl CaretBlink { visible: true, active: false, paused: false, + tasks: Vec::new(), } } @@ -550,7 +553,7 @@ impl CaretBlink { fn schedule_pause(&mut self, cx: &mut Context) { let generation = self.generation; let interval = self.interval; - cx.spawn(async move |this, cx| { + let task = cx.spawn(async move |this, cx| { cx.background_executor().timer(interval).await; this.update(cx, |this, cx| { if this.generation == generation { @@ -558,8 +561,9 @@ impl CaretBlink { this.tick(cx); } }) - }) - .detach(); + .ok(); + }); + self.tasks.push(task); } } diff --git a/crates/core/src/toast.rs b/crates/core/src/toast.rs index 2020093..ff0595c 100644 --- a/crates/core/src/toast.rs +++ b/crates/core/src/toast.rs @@ -7,7 +7,7 @@ use gpui::prelude::FluentBuilder as _; use gpui::{ Animation, AnimationExt as _, App, Context, ElementId, Entity, InteractiveElement as _, IntoElement, ParentElement as _, RenderOnce, SharedString, StatefulInteractiveElement as _, - Styled as _, Window, deferred, div, ease_out_quint, px, + Styled as _, Task, Window, deferred, div, ease_out_quint, px, }; use gpui_native_icons::{IconRole, icon}; use gpui_native_theme::ActiveTheme as _; @@ -107,6 +107,8 @@ pub struct Toasts { current: Option, exiting: Option, next_id: usize, + /// Pending dismiss and fade timers; held so they die with the store. + tasks: Vec>, } impl Toasts { @@ -125,11 +127,11 @@ impl Toasts { cx.notify(); if let Some(duration) = duration.filter(|duration| !duration.is_zero()) { - cx.spawn(async move |this, cx| { + let task = cx.spawn(async move |this, cx| { cx.background_executor().timer(duration).await; this.update(cx, |toasts, cx| toasts.dismiss(id, cx)).ok(); - }) - .detach(); + }); + self.tasks.push(task); } id @@ -178,12 +180,11 @@ impl Toasts { cx.notify(); let fade = cx.theme().motion.overlay; - - cx.spawn(async move |this, cx| { + let task = cx.spawn(async move |this, cx| { cx.background_executor().timer(fade).await; this.update(cx, |toasts, cx| toasts.finish_exit(cx)).ok(); - }) - .detach(); + }); + self.tasks.push(task); } fn finish_exit(&mut self, cx: &mut Context) { diff --git a/crates/core/tests/combobox.rs b/crates/core/tests/combobox.rs index 1f2ec27..bea30c9 100644 --- a/crates/core/tests/combobox.rs +++ b/crates/core/tests/combobox.rs @@ -34,6 +34,25 @@ struct ComboHost { } fn combo_host(keep_unmatched: bool, create: bool) -> ComboHost { + combo_host_with(|builder, created| { + let builder = if keep_unmatched { + builder.keep_unmatched_text() + } else { + builder + }; + if create { + builder.on_create(move |text, _, _| created.borrow_mut().push(text)) + } else { + builder + } + }) +} + +/// Builds a host whose combobox the caller configures; the created-recorder +/// is handed over for `on_create` wiring. +fn combo_host_with( + configure: impl FnOnce(Combobox, CreatedRecorder) -> Combobox, +) -> ComboHost { let cx = TestAppContext::single(); // `init` registers the input bindings and then the combobox's, in the // load-bearing order. @@ -47,19 +66,15 @@ fn combo_host(keep_unmatched: bool, create: bool) -> ComboHost { let created_for_builder = created.clone(); let cell = state_cell.clone(); cx.open_window(gpui::WindowOptions::default(), |window, cx| { - let mut builder = Combobox::new( - "test-combo", - "Fruit", - vec![option("main"), option("maintain"), option("release")], - ) - .on_change(move |value, _, _| changes_for_builder.borrow_mut().push(value)); - if keep_unmatched { - builder = builder.keep_unmatched_text(); - } - if create { - builder = builder - .on_create(move |text, _, _| created_for_builder.borrow_mut().push(text)); - } + let builder = configure( + Combobox::new( + "test-combo", + "Fruit", + vec![option("main"), option("maintain"), option("release")], + ) + .on_change(move |value, _, _| changes_for_builder.borrow_mut().push(value)), + created_for_builder, + ); let combobox = cx.new(|cx| ComboboxState::new(builder, window, cx)); cell.borrow_mut().replace(combobox.clone()); cx.new(|_| Host { combobox }) @@ -118,6 +133,7 @@ fn enter_commits_the_highlighted_option() { type_text(&mut host, "mai"); host.visual.simulate_keystrokes("enter"); + host.visual.run_until_parked(); draw(&mut host.visual); assert_eq!( @@ -143,6 +159,7 @@ fn typing_clears_the_value() { // Typing again first clears the value it no longer names. host.visual.simulate_keystrokes("r"); + host.visual.run_until_parked(); draw(&mut host.visual); assert_eq!( *host.changes.borrow(), @@ -197,6 +214,7 @@ fn create_hands_unmatched_text_to_the_handler() { type_text(&mut host, "feature"); host.visual.update(|window, cx| window.blur(cx)); + host.visual.run_until_parked(); draw(&mut host.visual); assert_eq!( @@ -217,6 +235,7 @@ fn clicking_a_row_commits_it() { .expect("release row missing"); host.visual .simulate_click(row.center(), gpui::Modifiers::default()); + host.visual.run_until_parked(); draw(&mut host.visual); assert_eq!( @@ -225,3 +244,99 @@ fn clicking_a_row_commits_it() { ); assert!(host.visual.debug_bounds("test-combo-panel").is_none()); } + +/// Regression: handlers used to run inside an update of the combobox +/// entity, so a handler that touched its own state re-entered and panicked. +/// Both handlers now run on the executor and may update the state freely. +#[test] +fn handlers_may_update_their_own_combobox() { + let cx = TestAppContext::single(); + cx.update(init); + let state_cell: Rc>>>> = + Rc::new(RefCell::new(None)); + let cell = state_cell.clone(); + let window = cx.update(|cx| { + cx.open_window(gpui::WindowOptions::default(), |window, cx| { + let builder = Combobox::new( + "self-updating-combo", + "Label", + vec![option("bug"), option("docs")], + ) + .on_create(move |text: SharedString, window, cx| { + let state = cell.borrow().clone().expect("state captured"); + state.update(cx, |state, cx| { + let mut options = state.options_snapshot(); + if !options.iter().any(|(value, _)| value == &text) { + options.push((text.clone(), text.clone())); + state.set_options(options, cx); + } + state.set_selected(Some(text), window, cx); + }); + }); + let combobox = cx.new(|cx| ComboboxState::new(builder, window, cx)); + state_cell.borrow_mut().replace(combobox.clone()); + cx.new(|_| Host { combobox }) + }) + .unwrap() + }); + let state = state_cell.borrow().clone().expect("state captured"); + let mut visual = VisualTestContext::from_window(window.into(), &cx); + visual.update(|window, cx| window.draw(cx).clear(cx)); + + // Typing an unmatched name and blurring hands it to `on_create`, which + // updates this very combobox — the crash the deferral fixes. + visual.update(|window, cx| { + state.update(cx, |state, cx| state.focus(window, cx)); + }); + for letter in "feature".chars() { + visual.simulate_keystrokes(&letter.to_string()); + } + visual.update(|window, cx| window.blur(cx)); + visual.run_until_parked(); + visual.update(|window, cx| window.draw(cx).clear(cx)); + + let selected = cx.update(|cx| state.read(cx).selected().cloned()); + assert_eq!(selected, Some(SharedString::from("feature"))); + let options = cx.update(|cx| state.read(cx).options_snapshot()); + assert!(options.iter().any(|(value, _)| value == "feature")); +} + +/// Matches when the query's characters appear in the label in order, so +/// "mn" finds "main" though the substring matcher would find nothing. +fn subsequence(query: &str, _value: &SharedString, label: &SharedString) -> bool { + let label = label.to_lowercase(); + let mut letters = label.chars(); + query + .to_lowercase() + .chars() + .all(|letter| letters.any(|found| found == letter)) +} + +#[test] +fn a_custom_filter_decides_which_options_survive() { + let mut host = combo_host_with(|builder, _| builder.filter(subsequence)); + // "mn" is no substring of any option, so only the custom matcher finds + // "main" and "maintain"; "release" has no "n" and drops out. + type_text(&mut host, "mn"); + + assert!(host.visual.debug_bounds("test-combo-option-0").is_some()); + assert!(host.visual.debug_bounds("test-combo-option-1").is_some()); + assert!(host.visual.debug_bounds("test-combo-option-2").is_none()); +} + +#[test] +fn replacing_the_options_reruns_the_custom_filter() { + let mut host = combo_host_with(|builder, _| builder.filter(subsequence)); + type_text(&mut host, "mn"); + + host.cx.update(|cx| { + host.state.update(cx, |state, cx| { + state.set_options(vec![option("metro"), option("mainline")], cx) + }) + }); + draw(&mut host.visual); + + // "metro" has no "n" after its "m"; "mainline" survives as the only row. + assert!(host.visual.debug_bounds("test-combo-option-0").is_some()); + assert!(host.visual.debug_bounds("test-combo-option-1").is_none()); +} diff --git a/docs/CHECKLIST.md b/docs/CHECKLIST.md index 62dfe07..a05caa4 100644 --- a/docs/CHECKLIST.md +++ b/docs/CHECKLIST.md @@ -19,9 +19,6 @@ Gaps surfaced by the signed comparison; ordered by signed's usage. - [ ] **Editable code field** — syntax-highlighted editable multiline field, the code view's highlighter over the input's editing model; signed's repo file browser. -- [ ] **Searchable list filter** — searchable collection behind combobox and - list option sources; the combobox's pluggable filter covers part of it - (signed's `SearchableVec`). - [ ] **Dropdown menu** — trigger button + popover + menu composite, three signed call sites; must stay a real component, not a wrapper. - [ ] **Motion easing helpers** — cubic ease-out and lerp beside @@ -71,7 +68,8 @@ Gaps surfaced by the signed comparison; ordered by signed's usage. settings-row variants - [x] Status dot — colored state bead, Badge's variant vocabulary, optional screen-reader label -- [x] Combobox — type-to-filter picker, create / keep / revert blur modes +- [x] Combobox — type-to-filter picker with a caller-owned matcher, create / + keep / revert blur modes - [x] Command palette — filtered actions; the matcher stays caller-owned - [x] Scrollbar — overlay thumbs for gpui's `list` and `uniform_list`, platform autohide with a `visibility` override diff --git a/examples/gallery/src/main.rs b/examples/gallery/src/main.rs index 4ced94e..0c592cb 100644 --- a/examples/gallery/src/main.rs +++ b/examples/gallery/src/main.rs @@ -1146,6 +1146,7 @@ fn combo_option(label: &'static str) -> ComboboxOption { struct ComboboxDemo { branch: Entity>, label: Entity>, + fuzzy: Entity>, label_created: Option, _subscriptions: Vec, } @@ -1199,15 +1200,43 @@ impl ComboboxDemo { }); }); let label = cx.new(|cx| ComboboxState::new(label_builder, window, cx)); + // A caller-owned matcher: characters in order, so "md" finds "markdown". + let fuzzy = cx.new(|cx| { + ComboboxState::new( + Combobox::new( + "combo-demo-fuzzy", + "Module", + vec![ + combo_option("combobox"), + combo_option("dock"), + combo_option("editor"), + combo_option("markdown"), + ], + ) + .placeholder("Filter modules…") + .filter(|query, _value, label| { + let label = label.to_lowercase(); + let mut letters = label.chars(); + query + .to_lowercase() + .chars() + .all(|letter| letters.any(|found| found == letter)) + }), + window, + cx, + ) + }); // The panel is a cached view in the dock: the value readouts re-render // only when the picker states it reads are notified. let subscriptions = vec![ cx.observe(&branch, |_, _, cx| cx.notify()), cx.observe(&label, |_, _, cx| cx.notify()), + cx.observe(&fuzzy, |_, _, cx| cx.notify()), ]; Self { branch, label, + fuzzy, label_created: None, _subscriptions: subscriptions, } @@ -1266,6 +1295,17 @@ impl Render for ComboboxDemo { .child(format!("label: {label_value}")), ), )) + .child(demo_section( + "custom matcher", + colors, + v_flex().gap_2().child( + Field::new("combo-demo-fuzzy-field", "Module") + .description( + "The matcher is caller-owned: this one matches characters in order.", + ) + .child(self.fuzzy.clone()), + ), + )) } }