From ba559c782de78127d1b73c9e0934d2b657816194 Mon Sep 17 00:00:00 2001 From: lucascouts Date: Wed, 26 Aug 2026 14:12:46 -0300 Subject: [PATCH] feat(agent_ui): require Shift to make a thread menu choice the default Picking a permission mode, a model, a thinking effort or one of the config options an ACP agent exposes used to do two things at once: move the thread in front of you, and rewrite the default every later thread starts on. The two intents are different -- "just for this one" is the common case and "from now on" is the rare, deliberate one -- and only the second one was reachable, so a one-off pick silently followed the user into every new thread with nothing on screen saying it had. Split them on the modifier. A plain confirm, by mouse or keyboard, is now scoped to the session; holding Shift also persists the choice. The keyboard cycling actions never persist: cycling is a thread-local gesture too, and it has no menu in which to hold the modifier. Because the current value and the persisted default can now differ, the menus say which is which. The check mark keeps meaning "this thread is on it"; a pin marks the entry new threads start on. Each menu carries the hint that teaches the gesture -- a footer in the two Picker-based menus, a label in the two ContextMenu-based ones, and the tooltip for the boolean switches, which have no menu at all. The predicate reads window.modifiers() rather than a ClickEvent so one answer serves clicks and keyboard confirms alike, and so neither `ui` nor `picker` has to grow modifier plumbing nothing else needs. Two menus move from ContextMenuEntry to a custom entry: the built-in entry renders a single icon at the end, and these menus now have two things to say about a row. ModeSelector keeps set_mode as it was and gains set_mode_as_default beside it, both delegating to a private apply_mode, rather than growing a bool parameter. A parameter would have been the smaller diff but it rewrites every existing call site, including ones this patch cannot see -- the tests that other downstream patches add, and whatever upstream writes next. select_model does take the flag as a parameter: it is a trait method with no Window to read the modifier from, its implementors are all upstream, and every caller is updated here. It also gains default_model_id so the picker can mark the default without the UI knowing how an agent stores it. Co-Authored-By: Claude Opus 5 (1M context) --- diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 5f0cf69..8ea6077 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -458,11 +458,27 @@ pub trait AgentModelSelector: 'static { /// /// # Parameters /// - `model_id`: The model to select (should be one from [list_models]). + /// - `persist_default`: Whether the choice also becomes the default for new + /// threads. Selecting a model is otherwise scoped to this session, so a + /// one-off model for one thread does not follow the user into the next. /// - `cx`: The GPUI app context. /// /// # Returns /// A task resolving to `Ok(())` on success or an error. - fn select_model(&self, model_id: AgentModelId, cx: &mut App) -> Task>; + fn select_model( + &self, + model_id: AgentModelId, + persist_default: bool, + cx: &mut App, + ) -> Task>; + + /// The model new threads start on, when the agent has one. + /// + /// Only used to mark it in the picker, so an agent that keeps no such + /// setting can leave it unanswered. + fn default_model_id(&self, _cx: &App) -> Option { + None + } /// Retrieves the currently selected model for a specific session (thread). /// @@ -1116,7 +1132,12 @@ mod test_support { Task::ready(Ok(AgentModelList::Flat(vec![model]))) } - fn select_model(&self, model_id: AgentModelId, _cx: &mut App) -> Task> { + fn select_model( + &self, + model_id: AgentModelId, + _persist_default: bool, + _cx: &mut App, + ) -> Task> { self.selected_model.lock().id = model_id; Task::ready(Ok(())) } diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index 6139ce7..58311da 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -2523,7 +2523,12 @@ impl acp_thread::AgentModelSelector for NativeAgentModelSelector { }) } - fn select_model(&self, model_id: AgentModelId, cx: &mut App) -> Task> { + fn select_model( + &self, + model_id: AgentModelId, + persist_default: bool, + cx: &mut App, + ) -> Task> { log::debug!( "Setting model for session {}: {}", self.session_id, @@ -2569,30 +2574,45 @@ impl acp_thread::AgentModelSelector for NativeAgentModelSelector { } }); - update_settings_file( - self.connection.0.read(cx).fs.clone(), - cx, - move |settings, cx| { - let provider = model.provider_id().0.to_string(); - let model = model.id().0.to_string(); - let enable_thinking = thread.read(cx).thinking_enabled(); - let speed = thread.read(cx).speed(); - settings - .agent - .get_or_insert_default() - .set_model(LanguageModelSelection { - provider: provider.into(), - model, - enable_thinking, - effort, - speed, - }); - }, - ); + // The thread is already on the new model; writing it to settings is the + // separate, deliberate act of making it the model new threads start on. + if persist_default { + update_settings_file( + self.connection.0.read(cx).fs.clone(), + cx, + move |settings, cx| { + let provider = model.provider_id().0.to_string(); + let model = model.id().0.to_string(); + let enable_thinking = thread.read(cx).thinking_enabled(); + let speed = thread.read(cx).speed(); + settings + .agent + .get_or_insert_default() + .set_model(LanguageModelSelection { + provider: provider.into(), + model, + enable_thinking, + effort, + speed, + }); + }, + ); + } Task::ready(Ok(())) } + fn default_model_id(&self, cx: &App) -> Option { + let default = agent_settings::AgentSettings::get_global(cx) + .default_model + .as_ref()?; + + Some(AgentModelId::new(format!( + "{}/{}", + default.provider.0, default.model + ))) + } + fn selected_model(&self, cx: &mut App) -> Task> { let Some(thread) = self .connection @@ -5832,6 +5852,86 @@ mod internal_tests { ); } + /// The mirror of test_model_selection_persists_to_settings: without the + /// flag, the thread moves to the new model and the default that new + /// threads start on is left exactly as the user left it. + #[gpui::test] + async fn test_model_selection_without_persisting_leaves_settings_alone( + cx: &mut TestAppContext, + ) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + fs.create_dir(paths::settings_file().parent().unwrap()) + .await + .unwrap(); + fs.insert_file( + paths::settings_file(), + json!({ + "agent": { + "default_model": { + "provider": "foo", + "model": "bar" + } + } + }) + .to_string() + .into_bytes(), + ) + .await; + let project = Project::test(fs.clone(), [], cx).await; + + let thread_store = cx.new(|cx| ThreadStore::new(cx)); + + let agent = + cx.update(|cx| NativeAgent::new(thread_store, Templates::new(), fs.clone(), cx)); + let connection = NativeAgentConnection(agent.clone()); + + let acp_thread = cx + .update(|cx| { + Rc::new(connection.clone()).new_session( + project.clone(), + PathList::new(&[Path::new("/a")]), + cx, + ) + }) + .await + .unwrap(); + + let session_id = cx.update(|cx| acp_thread.read(cx).session_id().clone()); + + let selector = connection.model_selector(&session_id).unwrap(); + cx.update(|cx| selector.select_model(AgentModelId::new("fake/fake"), false, cx)) + .await + .unwrap(); + + agent.read_with(cx, |agent, _| { + let session = agent.sessions.get(&session_id).unwrap(); + session.thread.read_with(cx, |thread, _| { + assert_eq!( + thread.model().unwrap().id().0, + "fake", + "the thread must still move to the picked model" + ); + }); + }); + + cx.run_until_parked(); + + let settings_content = fs.load(paths::settings_file()).await.unwrap(); + let settings_json: serde_json::Value = serde_json::from_str(&settings_content).unwrap(); + + assert_eq!( + settings_json["agent"]["default_model"]["model"], + json!("bar"), + "a plain model pick must not rewrite the default" + ); + assert_eq!( + settings_json["agent"]["default_model"]["provider"], + json!("foo"), + "a plain model pick must not rewrite the default" + ); + } + #[gpui::test] async fn test_model_selection_persists_to_settings(cx: &mut TestAppContext) { init_test(cx); @@ -5879,7 +5979,7 @@ mod internal_tests { // Select a model let selector = connection.model_selector(&session_id).unwrap(); let model_id = AgentModelId::new("fake/fake"); - cx.update(|cx| selector.select_model(model_id.clone(), cx)) + cx.update(|cx| selector.select_model(model_id.clone(), true, cx)) .await .unwrap(); @@ -5929,9 +6029,11 @@ mod internal_tests { agent.update(cx, |agent, cx| agent.models.refresh_list(cx)); let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), true, cx) + }) + .await + .unwrap(); cx.run_until_parked(); // Verify enable_thinking was written to settings as true. @@ -6003,9 +6105,11 @@ mod internal_tests { // Select the thinking model via select_model. let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), true, cx) + }) + .await + .unwrap(); // select_model should have enabled thinking based on the model's supports_thinking(). agent.read_with(cx, |agent, _| { @@ -6020,7 +6124,7 @@ mod internal_tests { // Switch back to the non-thinking model. let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake/fake"), cx)) + cx.update(|cx| selector.select_model(AgentModelId::new("fake/fake"), true, cx)) .await .unwrap(); @@ -6137,9 +6241,11 @@ mod internal_tests { let session_id = acp_thread.read_with(cx, |thread, _| thread.session_id().clone()); let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("fake-corp/fake-thinking"), true, cx) + }) + .await + .unwrap(); // Verify thinking is enabled after selecting the thinking model. let thread = agent.read_with(cx, |agent, _| { @@ -6240,9 +6346,11 @@ mod internal_tests { let session_id = acp_thread.read_with(cx, |thread, _| thread.session_id().clone()); let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake-corp/custom-model-id"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("fake-corp/custom-model-id"), true, cx) + }) + .await + .unwrap(); let thread = agent.read_with(cx, |agent, _| { agent.sessions.get(&session_id).unwrap().thread.clone() @@ -6350,9 +6458,11 @@ mod internal_tests { let session_id = acp_thread.read_with(cx, |thread, _| thread.session_id().clone()); let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("fake-corp/custom-model-id"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("fake-corp/custom-model-id"), true, cx) + }) + .await + .unwrap(); let send = acp_thread.update(cx, |thread, cx| thread.send(vec!["Hello".into()], cx)); let send = cx.foreground_executor().spawn(send); @@ -6479,9 +6589,11 @@ mod internal_tests { cx.run_until_parked(); let selector = connection.model_selector(&session_id).unwrap(); - cx.update(|cx| selector.select_model(AgentModelId::new("other-corp/other-model-id"), cx)) - .await - .unwrap(); + cx.update(|cx| { + selector.select_model(AgentModelId::new("other-corp/other-model-id"), true, cx) + }) + .await + .unwrap(); thread.read_with(cx, |thread, _| { assert_eq!(thread.model().unwrap().id().0.as_ref(), "other-model-id"); diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 1570315..7fecb19 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -24,6 +24,7 @@ mod mode_selector; mod model_selector; mod model_selector_popover; mod profile_selector; +mod set_as_default; mod terminal_codegen; mod terminal_inline_assistant; pub mod terminal_thread_metadata_store; diff --git a/crates/agent_ui/src/config_options.rs b/crates/agent_ui/src/config_options.rs index 196dc74..a93fea9 100644 --- a/crates/agent_ui/src/config_options.rs +++ b/crates/agent_ui/src/config_options.rs @@ -22,6 +22,9 @@ use unicode_segmentation::UnicodeSegmentation; use util::ResultExt as _; use zed_actions::agent::ToggleModelSelector; +use crate::set_as_default::{ + SET_AS_DEFAULT_HINT, default_hint_footer, default_marker, persists_default, +}; use crate::ui::documentation_aside_side; use crate::{ CycleFavoriteModels, CycleModeSelector, CycleThinkingEffort, ToggleProfileSelector, @@ -107,15 +110,10 @@ impl ConfigOptionsView { let Some(next_value) = self.next_value_for_config(&config_id, favorites_only, cx) else { return false; }; - let default_value = setting_value_for_config_option_value(&next_value); - - self.agent_server.set_default_config_option( - config_id.0.as_ref(), - default_value, - self.fs.clone(), - cx, - ); + // Cycling is a thread-local gesture like every other one: it moves the + // thread in front of you and leaves the default alone. Persisting is + // reached through the menu, with Shift held. let task = self .config_options .set_config_option(config_id, next_value, cx); @@ -581,6 +579,14 @@ impl Render for ConfigOptionSelector { .color(Color::Muted), ); } + // A switch has no menu to carry the footer the other + // surfaces use, so the tooltip is where the gesture is + // taught. + content = content.child( + Label::new(SET_AS_DEFAULT_HINT) + .size(LabelSize::Small) + .color(Color::Muted), + ); content.into_any() }); @@ -611,14 +617,16 @@ impl Render for ConfigOptionSelector { .label_size(LabelSize::Small) .label_color(Color::Muted) .disabled(self.setting_value) - .on_click(move |state, _window, cx| { + .on_click(move |state, window, cx| { let next_value = matches!(state, ToggleState::Selected); - agent_server.set_default_config_option( - config_id.0.as_ref(), - Some(AgentConfigOptionValue::Boolean(next_value)), - fs.clone(), - cx, - ); + if persists_default(window) { + agent_server.set_default_config_option( + config_id.0.as_ref(), + Some(AgentConfigOptionValue::Boolean(next_value)), + fs.clone(), + cx, + ); + } let task = config_options.set_config_option( config_id.clone(), @@ -799,16 +807,18 @@ impl PickerDelegate for ConfigOptionPickerDelegate { }) } - fn confirm(&mut self, _secondary: bool, _window: &mut Window, cx: &mut Context>) { + fn confirm(&mut self, _secondary: bool, window: &mut Window, cx: &mut Context>) { if let Some(ConfigOptionPickerEntry::Option(option)) = self.filtered_entries.get(self.selected_index) { - self.agent_server.set_default_config_option( - self.config_id.0.as_ref(), - Some(AgentConfigOptionValue::ValueId(option.value.0.to_string())), - self.fs.clone(), - cx, - ); + if persists_default(window) { + self.agent_server.set_default_config_option( + self.config_id.0.as_ref(), + Some(AgentConfigOptionValue::ValueId(option.value.0.to_string())), + self.fs.clone(), + cx, + ); + } let task = self.config_options.set_config_option( self.config_id.clone(), acp::SessionConfigOptionValue::value_id(option.value.clone()), @@ -857,6 +867,13 @@ impl PickerDelegate for ConfigOptionPickerDelegate { let current_value = self.current_value(); let is_selected = current_value.as_ref() == Some(&option.value); + let is_default = matches!( + self.agent_server + .default_config_option(self.config_id.0.as_ref(), cx), + Some(AgentConfigOptionValue::ValueId(ref id)) + if id.as_str() == option.value.0.as_ref() + ); + let is_favorite = self.favorites.contains(&option.value); let option_name = option.name.clone(); @@ -883,9 +900,22 @@ impl PickerDelegate for ConfigOptionPickerDelegate { .spacing(ListItemSpacing::Sparse) .toggle_state(selected) .child(h_flex().w_full().child(Label::new(option_name).truncate())) - .end_slot(div().pr_2().when(is_selected, |this| { - this.child(Icon::new(IconName::Check).color(Color::Accent)) - })) + .end_slot( + h_flex() + .gap_1() + .pr_2() + .when(is_default, |this| { + this.child(default_marker(( + "config-option-default", + ix, + ))) + }) + .when(is_selected, |this| { + this.child( + Icon::new(IconName::Check).color(Color::Accent), + ) + }), + ) .end_slot_on_hover(div().pr_1p5().child({ let (icon, color, tooltip) = if is_favorite { (IconName::StarFilled, Color::Accent, "Unfavorite") @@ -940,6 +970,14 @@ impl PickerDelegate for ConfigOptionPickerDelegate { fn documentation_aside_index(&self) -> Option { self.selected_description.as_ref().map(|(ix, _)| *ix) } + + fn render_footer( + &self, + _window: &mut Window, + cx: &mut Context>, + ) -> Option { + Some(default_hint_footer(cx).into_any_element()) + } } fn extract_options( @@ -1012,20 +1050,6 @@ fn shows_manual_mode_badge_for_option(option: &acp::SessionConfigOption) -> bool } } -fn setting_value_for_config_option_value( - value: &acp::SessionConfigOptionValue, -) -> Option { - match value { - acp::SessionConfigOptionValue::ValueId { value } => { - Some(AgentConfigOptionValue::ValueId(value.0.to_string())) - } - acp::SessionConfigOptionValue::Boolean { value } => { - Some(AgentConfigOptionValue::Boolean(*value)) - } - _ => None, - } -} - fn options_to_picker_entries( options: &[ConfigOptionValue], favorites: &HashSet, @@ -1148,7 +1172,7 @@ mod tests { use std::{any::Any, cell::RefCell}; #[gpui::test] - fn cycling_config_option_saves_selected_value_as_default(cx: &mut TestAppContext) { + fn cycling_config_option_leaves_the_default_alone(cx: &mut TestAppContext) { let agent_server = Rc::new(TestAgentServer::default()); let config_options = Rc::new(TestSessionConfigOptions::new(vec![ acp::SessionConfigOption::select( @@ -1182,12 +1206,9 @@ mod tests { })); }); - assert_eq!( - agent_server.saved_defaults.lock().as_slice(), - &[( - "mode".to_string(), - Some(AgentConfigOptionValue::ValueId("manual".to_string())) - )] + assert!( + agent_server.saved_defaults.lock().is_empty(), + "cycling moves the thread, not the default" ); assert_eq!( config_options.set_values.borrow().as_slice(), @@ -1199,7 +1220,7 @@ mod tests { } #[gpui::test] - fn cycling_boolean_config_option_saves_selected_value_as_default(cx: &mut TestAppContext) { + fn cycling_boolean_config_option_leaves_the_default_alone(cx: &mut TestAppContext) { let agent_server = Rc::new(TestAgentServer::default()); let config_options = Rc::new(TestSessionConfigOptions::new(vec![ acp::SessionConfigOption::boolean("web_search", "Web Search", false) @@ -1225,12 +1246,9 @@ mod tests { })); }); - assert_eq!( - agent_server.saved_defaults.lock().as_slice(), - &[( - "web_search".to_string(), - Some(AgentConfigOptionValue::Boolean(true)) - )] + assert!( + agent_server.saved_defaults.lock().is_empty(), + "cycling moves the thread, not the default" ); assert_eq!( config_options.set_values.borrow().as_slice(), @@ -1278,12 +1296,9 @@ mod tests { })); }); - assert_eq!( - agent_server.saved_defaults.lock().as_slice(), - &[( - "web_search".to_string(), - Some(AgentConfigOptionValue::Boolean(true)) - )] + assert!( + agent_server.saved_defaults.lock().is_empty(), + "cycling moves the thread, not the default" ); assert_eq!( config_options.set_values.borrow().as_slice(), diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index 7c4367e..ce9a342 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -24,6 +24,7 @@ use sandbox::{SandboxFsPolicy, SandboxNetPolicy, SandboxPolicy}; use crate::completion_provider::{AvailableSkill, PromptLocalCommand, pluralize}; use crate::message_editor::SharedSessionCapabilities; +use crate::set_as_default::{SET_AS_DEFAULT_HINT, default_marker, persists_default}; use crate::ui::{ SandboxGroup, SandboxRow, SandboxSection, SandboxStatusTooltip, TerminalSandboxWarning, TerminalToolHeader, @@ -5175,12 +5176,17 @@ impl ThreadView { .tooltip(move |_, cx| { Tooltip::for_action_in(tooltip_label, &ToggleThinkingMode, &focus_handle, cx) }) - .on_click(cx.listener(move |this, _, _window, cx| { + .on_click(cx.listener(move |this, _, window, cx| { + let persist_default = persists_default(window); if let Some(thread) = this.as_native_thread(cx) { thread.update(cx, |thread, cx| { let enable_thinking = !thread.thinking_enabled(); thread.set_thinking_enabled(enable_thinking, cx); + if !persist_default { + return; + } + let favorite_key = thread.model().map(|model| { (model.provider_id().0.to_string(), model.id().0.to_string()) }); @@ -5320,21 +5326,56 @@ impl ThreadView { tooltip, ) .menu(move |window, cx| { - Some(ContextMenu::build(window, cx, |mut menu, _window, _cx| { + Some(ContextMenu::build(window, cx, |mut menu, _window, cx| { menu = menu.header("Change Thinking Effort"); - for effort_level in supported_effort_levels.clone() { + let persisted_effort = AgentSettings::get_global(cx) + .default_model + .as_ref() + .and_then(|model| model.effort.clone()); + + for (ix, effort_level) in + supported_effort_levels.clone().into_iter().enumerate() + { let is_selected = selected .as_ref() .is_some_and(|selected| selected.value == effort_level.value); - let entry = ContextMenuEntry::new(effort_level.name) - .toggleable(IconPosition::End, is_selected); + let is_default = + persisted_effort.as_deref() == Some(effort_level.value.as_ref()); + + // A custom entry rather than a ContextMenuEntry: the + // built-in entry renders one icon at the end, and this + // menu now has two things to say about a level. + let name = effort_level.name.clone(); + let render_entry = move |_window: &mut Window, _cx: &mut App| { + h_flex() + .w_full() + .gap_2() + .justify_between() + .child(Label::new(name.clone())) + .child( + h_flex() + .gap_1() + .when(is_default, |this| { + this.child(default_marker(("effort-default", ix))) + }) + .when(is_selected, |this| { + this.child( + Icon::new(IconName::Check) + .size(IconSize::Small) + .color(Color::Accent), + ) + }), + ) + .into_any_element() + }; - menu.push_item(entry.handler({ + menu = menu.custom_entry(render_entry, { let effort = effort_level.value.clone(); let weak_self = weak_self.clone(); - move |_window, cx| { + move |window, cx| { let effort = effort.clone(); + let persist_default = persists_default(window); weak_self .update(cx, |this, cx| { if let Some(thread) = this.as_native_thread(cx) { @@ -5344,6 +5385,10 @@ impl ThreadView { cx, ); + if !persist_default { + return; + } + let favorite_key = thread.model().map(|model| { ( model.provider_id().0.to_string(), @@ -5378,10 +5423,10 @@ impl ThreadView { }) .ok(); } - })); + }); } - menu + menu.separator().label(SET_AS_DEFAULT_HINT) })) }) .with_handle(self.thinking_effort_menu_handle.clone()) @@ -11998,7 +12043,9 @@ impl ThreadView { else { return; }; - let select = selector.select_model(model_id, cx); + // A fallback picked for the user after a failure, not a choice they + // made: it must not become the default. + let select = selector.select_model(model_id, false, cx); cx.spawn(async move |this, cx| { select.await?; this.update(cx, |this, cx| this.retry_generation(cx))?; diff --git a/crates/agent_ui/src/mode_selector.rs b/crates/agent_ui/src/mode_selector.rs index 253aee8..9d8aa55 100644 --- a/crates/agent_ui/src/mode_selector.rs +++ b/crates/agent_ui/src/mode_selector.rs @@ -7,10 +7,11 @@ use gpui::{Context, Entity, WeakEntity, Window, prelude::*}; use std::{rc::Rc, sync::Arc}; use ui::{ - Button, ContextMenu, ContextMenuEntry, KeyBinding, PopoverMenu, PopoverMenuHandle, Tooltip, + Button, ContextMenu, DocumentationAside, KeyBinding, PopoverMenu, PopoverMenuHandle, Tooltip, prelude::*, }; +use crate::set_as_default::{SET_AS_DEFAULT_HINT, default_marker, persists_default}; use crate::{CycleModeSelector, ToggleProfileSelector, ui::documentation_aside_side}; pub struct ModeSelector { @@ -54,6 +55,8 @@ impl ModeSelector { .unwrap_or(0); if let Some(next_mode) = all_modes.get((current_index + 1) % all_modes.len()) { + // Cycling moves the thread only; the default is changed from the + // menu, with Shift held. self.set_mode(next_mode.id.clone(), cx); } } @@ -62,9 +65,31 @@ impl ModeSelector { self.connection.current_mode() } + /// Moves this thread to `mode`, leaving the mode new threads start on + /// alone. See [`Self::set_mode_as_default`] for the other half. + /// + /// The two intents get two methods rather than one flag argument so that + /// callers which only ever mean "just this thread" -- and the tests that + /// cover them -- keep reading, and compiling, unchanged. pub fn set_mode(&mut self, mode: acp::SessionModeId, cx: &mut Context) { - self.agent_server - .set_default_mode(Some(mode.clone()), self.fs.clone(), cx); + self.apply_mode(mode, false, cx); + } + + /// Moves this thread to `mode` and makes it the mode new threads start on. + pub fn set_mode_as_default(&mut self, mode: acp::SessionModeId, cx: &mut Context) { + self.apply_mode(mode, true, cx); + } + + fn apply_mode( + &mut self, + mode: acp::SessionModeId, + persist_default: bool, + cx: &mut Context, + ) { + if persist_default { + self.agent_server + .set_default_mode(Some(mode.clone()), self.fs.clone(), cx); + } let task = self.connection.set_mode(mode, cx); self.setting_mode = true; @@ -93,38 +118,75 @@ impl ModeSelector { ContextMenu::build(window, cx, move |mut menu, _window, cx| { let all_modes = self.connection.all_modes(); let current_mode = self.connection.current_mode(); + let default_mode = self.agent_server.default_mode(cx); let side = documentation_aside_side(cx); - for mode in all_modes { - let is_selected = &mode.id == ¤t_mode; - let entry = ContextMenuEntry::new(mode.name.clone()) - .toggleable(IconPosition::End, is_selected); + // A custom entry rather than a ContextMenuEntry: the built-in entry + // renders one icon at the end, and this menu now has two things to + // say about a mode -- that the thread is on it, and that it is the + // one new threads start on. + for (ix, mode) in all_modes.into_iter().enumerate() { + let is_selected = mode.id == current_mode; + let is_default = default_mode.as_ref() == Some(&mode.id); - let entry = if let Some(description) = &mode.description { - entry.documentation_aside(side, { - let description = description.clone(); + let documentation = mode.description.as_ref().map(|description| { + let description = description.clone(); - move |_| Label::new(description.clone()).into_any_element() - }) - } else { - entry - }; + DocumentationAside::new( + side, + Rc::new(move |_| Label::new(description.clone()).into_any_element()), + ) + }); - menu.push_item(entry.handler({ - let mode_id = mode.id.clone(); - let weak_self = weak_self.clone(); - move |_window, cx| { - weak_self - .update(cx, |this, cx| { - this.set_mode(mode_id.clone(), cx); - }) - .ok(); - } - })); + let name = mode.name.clone(); + + menu = menu.custom_entry_with_docs( + move |_window, _cx| { + h_flex() + .w_full() + .gap_2() + .justify_between() + .child(Label::new(name.clone())) + .child( + h_flex() + .gap_1() + .when(is_default, |this| { + this.child(default_marker(("mode-default", ix))) + }) + .when(is_selected, |this| { + this.child( + Icon::new(IconName::Check) + .size(IconSize::Small) + .color(Color::Accent), + ) + }), + ) + .into_any_element() + }, + { + let mode_id = mode.id.clone(); + let weak_self = weak_self.clone(); + move |window, cx| { + let persist_default = persists_default(window); + weak_self + .update(cx, |this, cx| { + if persist_default { + this.set_mode_as_default(mode_id.clone(), cx); + } else { + this.set_mode(mode_id.clone(), cx); + } + }) + .ok(); + } + }, + documentation, + ); } - menu.key_context("ModeSelector") + menu.separator() + .label(SET_AS_DEFAULT_HINT) + .key_context("ModeSelector") }) } } @@ -246,21 +308,27 @@ mod tests { use project::{AgentId, Project}; use std::{any::Any, cell::RefCell}; + /// set_mode with the flag off is the plain gesture: the thread moves and + /// settings are left alone. #[gpui::test] - fn setting_mode_saves_selected_mode_as_default(cx: &mut TestAppContext) { - let agent_server = Rc::new(TestAgentServer::default()); - let session_modes = Rc::new(TestSessionModes::new()); - let fs: Arc = FakeFs::new(cx.executor()); + fn setting_mode_without_persisting_leaves_the_default_alone(cx: &mut TestAppContext) { + let (agent_server, session_modes) = set_mode_in_test(false, cx); - cx.update(|cx| { - let session_modes: Rc = session_modes.clone(); - let agent_server: Rc = agent_server.clone(); - let selector = cx.new(|_| ModeSelector::new(session_modes, agent_server, fs)); + assert!( + agent_server.saved_defaults.lock().is_empty(), + "a plain mode pick must not rewrite the default" + ); + assert_eq!( + session_modes.set_modes.borrow().as_slice(), + &[acp::SessionModeId::new("manual")] + ); + } - selector.update(cx, |selector, cx| { - selector.set_mode(acp::SessionModeId::new("manual"), cx); - }); - }); + /// set_mode with the flag on is the Shift gesture: the thread moves and the + /// choice sticks. + #[gpui::test] + fn setting_mode_persisting_saves_selected_mode_as_default(cx: &mut TestAppContext) { + let (agent_server, session_modes) = set_mode_in_test(true, cx); assert_eq!( agent_server.saved_defaults.lock().as_slice(), @@ -272,6 +340,31 @@ mod tests { ); } + fn set_mode_in_test( + persist_default: bool, + cx: &mut TestAppContext, + ) -> (Rc, Rc) { + let agent_server = Rc::new(TestAgentServer::default()); + let session_modes = Rc::new(TestSessionModes::new()); + let fs: Arc = FakeFs::new(cx.executor()); + + cx.update(|cx| { + let modes: Rc = session_modes.clone(); + let server: Rc = agent_server.clone(); + let selector = cx.new(|_| ModeSelector::new(modes, server, fs)); + + selector.update(cx, |selector, cx| { + if persist_default { + selector.set_mode_as_default(acp::SessionModeId::new("manual"), cx); + } else { + selector.set_mode(acp::SessionModeId::new("manual"), cx); + } + }); + }); + + (agent_server, session_modes) + } + #[derive(Default)] struct TestAgentServer { saved_defaults: Arc>>>, diff --git a/crates/agent_ui/src/model_selector.rs b/crates/agent_ui/src/model_selector.rs index d177f0a..560fd70 100644 --- a/crates/agent_ui/src/model_selector.rs +++ b/crates/agent_ui/src/model_selector.rs @@ -20,6 +20,7 @@ use ui::{DocumentationAside, IntoElement, prelude::*}; use util::ResultExt; use zed_actions::agent::OpenSettings; +use crate::set_as_default::{default_hint_footer, persists_default}; use crate::ui::{ ModelSelectorFooter, ModelSelectorHeader, ModelSelectorListItem, documentation_aside_side, }; @@ -171,8 +172,10 @@ impl ModelPickerDelegate { let next_model = favorite_models[next_index].clone(); + // Cycling moves the thread only; the default is changed from the menu, + // with Shift held. self.selector - .select_model(next_model.id.clone(), cx) + .select_model(next_model.id.clone(), false, cx) .detach_and_log_err(cx); self.selected_model = Some(next_model); @@ -272,7 +275,7 @@ impl PickerDelegate for ModelPickerDelegate { && model_info.disabled.is_none() { self.selector - .select_model(model_info.id.clone(), cx) + .select_model(model_info.id.clone(), persists_default(window), cx) .detach_and_log_err(cx); self.selected_model = Some(model_info.clone()); let current_index = self.selected_index; @@ -301,6 +304,8 @@ impl PickerDelegate for ModelPickerDelegate { } ModelPickerEntry::Model(model_info, is_favorite) => { let is_selected = Some(model_info) == self.selected_model.as_ref(); + let is_default = + self.selector.default_model_id(cx).as_ref() == Some(&model_info.id); let is_favorite = *is_favorite; let handle_action_click = { @@ -337,6 +342,7 @@ impl PickerDelegate for ModelPickerDelegate { }) .disabled(model_info.disabled.clone()) .is_selected(is_selected) + .is_default(is_default) .is_focused(selected) .is_latest(model_info.is_latest) .is_favorite(is_favorite) @@ -373,15 +379,21 @@ impl PickerDelegate for ModelPickerDelegate { fn render_footer( &self, _window: &mut Window, - _cx: &mut Context>, + cx: &mut Context>, ) -> Option { let focus_handle = self.focus_handle.clone(); - if !self.selector.should_render_footer() { - return None; - } - - Some(ModelSelectorFooter::new(OpenSettings.boxed_clone(), focus_handle).into_any_element()) + Some( + v_flex() + .child(default_hint_footer(cx)) + .when(self.selector.should_render_footer(), |this| { + this.child(ModelSelectorFooter::new( + OpenSettings.boxed_clone(), + focus_handle, + )) + }) + .into_any_element(), + ) } } @@ -646,7 +658,12 @@ mod tests { Task::ready(Ok(AgentModelList::Flat(self.models.clone()))) } - fn select_model(&self, model_id: AgentModelId, _cx: &mut App) -> Task> { + fn select_model( + &self, + model_id: AgentModelId, + _persist_default: bool, + _cx: &mut App, + ) -> Task> { self.selected_models.borrow_mut().push(model_id.clone()); if let Some(model) = self.models.iter().find(|model| model.id == model_id) { *self.selected_model.borrow_mut() = model.clone(); diff --git a/crates/agent_ui/src/set_as_default.rs b/crates/agent_ui/src/set_as_default.rs new file mode 100644 index 0000000..a633d24 --- /dev/null +++ b/crates/agent_ui/src/set_as_default.rs @@ -0,0 +1,68 @@ +//! Picking a value in a thread menu is thread-local; holding Shift makes it stick. +//! +//! Every thread menu -- permission mode, model, thinking effort, and the config +//! options an ACP agent exposes -- used to write the picked value straight into +//! settings, so a one-off choice for the thread in front of you silently became +//! the default for every thread after it. The modifier splits the two intents +//! apart: the plain gesture changes only this thread, and Shift is the +//! deliberate "make this the default" gesture. +//! +//! The pieces live together because the gesture is only usable if it is the +//! same everywhere: one predicate decides it, one marker shows which entry +//! currently holds the default, and one line of copy teaches it. + +use ui::{Tooltip, prelude::*}; + +/// Whether the interaction in flight should also persist the choice as the +/// default for new threads. +/// +/// Read from the window rather than from a `ClickEvent` so the same predicate +/// answers for a mouse click and for a keyboard confirm, and so no shared +/// component in `ui` or `picker` has to grow modifier plumbing it has no other +/// use for. +pub(crate) fn persists_default(window: &Window) -> bool { + window.modifiers().shift +} + +const DEFAULT_MARKER_TOOLTIP: &str = "Default for new threads"; + +/// The hint that teaches the gesture, shown at the bottom of every menu that +/// honours it. +pub(crate) const SET_AS_DEFAULT_HINT: &str = "Shift-click to set as default"; + +/// The marker on the entry that is the persisted default. +/// +/// Deliberately not the check mark: the check says "this is what the thread is +/// using right now", which is usually -- but, now that the two can be set +/// apart, no longer always -- the same entry. +pub(crate) fn default_marker(id: impl Into) -> impl IntoElement { + div() + .id(id) + .child( + Icon::new(IconName::Pin) + .size(IconSize::XSmall) + .color(Color::Muted), + ) + .tooltip(Tooltip::text(DEFAULT_MARKER_TOOLTIP)) +} + +/// The hint as a menu footer, for the two menus built out of a `Picker`. +pub(crate) fn default_hint_footer(cx: &App) -> impl IntoElement { + h_flex() + .w_full() + .px_2() + .py_1() + .gap_1() + .border_t_1() + .border_color(cx.theme().colors().border_variant) + .child( + Icon::new(IconName::Pin) + .size(IconSize::XSmall) + .color(Color::Muted), + ) + .child( + Label::new(SET_AS_DEFAULT_HINT) + .size(LabelSize::Small) + .color(Color::Muted), + ) +} diff --git a/crates/agent_ui/src/ui/model_selector_components.rs b/crates/agent_ui/src/ui/model_selector_components.rs index 34e01d7..29c5f7d 100644 --- a/crates/agent_ui/src/ui/model_selector_components.rs +++ b/crates/agent_ui/src/ui/model_selector_components.rs @@ -50,6 +50,7 @@ pub struct ModelSelectorListItem { title: SharedString, icon: Option, is_selected: bool, + is_default: bool, is_focused: bool, is_latest: bool, is_favorite: bool, @@ -65,6 +66,7 @@ impl ModelSelectorListItem { title: title.into(), icon: None, is_selected: false, + is_default: false, is_focused: false, is_latest: false, is_favorite: false, @@ -89,6 +91,13 @@ impl ModelSelectorListItem { self } + /// Marks the model new threads start on, which is not necessarily the one + /// this thread is on. + pub fn is_default(mut self, is_default: bool) -> Self { + self.is_default = is_default; + self + } + pub fn disabled(mut self, disabled: Option) -> Self { self.disabled = disabled; self @@ -181,6 +190,12 @@ impl RenderOnce for ModelSelectorListItem { h_flex() .pr_2() .gap_1p5() + .when(self.is_default, |this| { + this.child(crate::set_as_default::default_marker(( + "model-default", + self.index, + ))) + }) .when(self.is_selected, |this| { this.child(Icon::new(IconName::Check).color(Color::Accent)) })