From d8655f06566c5318260ac7a4ee6dc17b20faea3d Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 4 Nov 2025 09:46:16 -0800 Subject: [PATCH] settings_ui: Fix dropdowns after #41036 (#41920) Closes #41533 Both of the issues in the release notes that are fixed in this PR, were caused by incorrect usage of the `window.use_state` API. The first issue was caused by calling `window.use_state` in a render helper, resulting in the element ID used to share state being the same across different pages, resulting in the state being re-used when it should have been re-created. The fix for this was to move the `window.state` (and rendering logic) into a `impl RenderOnce` component, so that the IDs are resolved during the render, avoiding the state conflicts. The second issue is caused by using a `move` closure in the `window.use_state` call, resulting in stale closure values when the window state is re-used. Release Notes: - settings_ui: Fixed an issue where some dropdown menus would show options from a different dropdown when clicked - settings_ui: Fixed an issue where attempting to change a setting in a dropdown back to it's original value after changing it would do nothing --- crates/settings_ui/src/components.rs | 2 + crates/settings_ui/src/components/dropdown.rs | 108 ++++++++++++++++++ crates/settings_ui/src/settings_ui.rs | 66 +++-------- 3 files changed, 125 insertions(+), 51 deletions(-) create mode 100644 crates/settings_ui/src/components/dropdown.rs diff --git a/crates/settings_ui/src/components.rs b/crates/settings_ui/src/components.rs index 5026ca806128baf27e0c95e0c45b47eba24c8e41..b073372ac9b625036252e0a1722a960c8f6b3c45 100644 --- a/crates/settings_ui/src/components.rs +++ b/crates/settings_ui/src/components.rs @@ -1,8 +1,10 @@ +mod dropdown; mod font_picker; mod icon_theme_picker; mod input_field; mod theme_picker; +pub use dropdown::*; pub use font_picker::font_picker; pub use icon_theme_picker::icon_theme_picker; pub use input_field::*; diff --git a/crates/settings_ui/src/components/dropdown.rs b/crates/settings_ui/src/components/dropdown.rs new file mode 100644 index 0000000000000000000000000000000000000000..ec9ecb4eafa64e1e061a0e44c2b6a1f4b23c8216 --- /dev/null +++ b/crates/settings_ui/src/components/dropdown.rs @@ -0,0 +1,108 @@ +use std::rc::Rc; + +use gpui::{App, ElementId, IntoElement, RenderOnce}; +use heck::ToTitleCase as _; +use ui::{ + ButtonSize, ContextMenu, DropdownMenu, DropdownStyle, FluentBuilder as _, IconPosition, px, +}; + +#[derive(IntoElement)] +pub struct EnumVariantDropdown +where + T: strum::VariantArray + strum::VariantNames + Copy + PartialEq + Send + Sync + 'static, +{ + id: ElementId, + current_value: T, + variants: &'static [T], + labels: &'static [&'static str], + should_do_title_case: bool, + tab_index: Option, + on_change: Rc, +} + +impl EnumVariantDropdown +where + T: strum::VariantArray + strum::VariantNames + Copy + PartialEq + Send + Sync + 'static, +{ + pub fn new( + id: impl Into, + current_value: T, + variants: &'static [T], + labels: &'static [&'static str], + on_change: impl Fn(T, &mut App) + 'static, + ) -> Self { + Self { + id: id.into(), + current_value, + variants, + labels, + should_do_title_case: true, + tab_index: None, + on_change: Rc::new(on_change), + } + } + + pub fn title_case(mut self, title_case: bool) -> Self { + self.should_do_title_case = title_case; + self + } + + pub fn tab_index(mut self, tab_index: isize) -> Self { + self.tab_index = Some(tab_index); + self + } +} + +impl RenderOnce for EnumVariantDropdown +where + T: strum::VariantArray + strum::VariantNames + Copy + PartialEq + Send + Sync + 'static, +{ + fn render(self, window: &mut ui::Window, cx: &mut ui::App) -> impl gpui::IntoElement { + let current_value_label = self.labels[self + .variants + .iter() + .position(|v| *v == self.current_value) + .unwrap()]; + + let context_menu = window.use_keyed_state(current_value_label, cx, |window, cx| { + ContextMenu::new(window, cx, move |mut menu, _, _| { + for (&value, &label) in std::iter::zip(self.variants, self.labels) { + let on_change = self.on_change.clone(); + let current_value = self.current_value; + menu = menu.toggleable_entry( + if self.should_do_title_case { + label.to_title_case() + } else { + label.to_string() + }, + value == current_value, + IconPosition::End, + None, + move |_, cx| { + on_change(value, cx); + }, + ); + } + menu + }) + }); + + DropdownMenu::new( + self.id, + if self.should_do_title_case { + current_value_label.to_title_case() + } else { + current_value_label.to_string() + }, + context_menu, + ) + .when_some(self.tab_index, |elem, tab_index| elem.tab_index(tab_index)) + .trigger_size(ButtonSize::Medium) + .style(DropdownStyle::Outlined) + .offset(gpui::Point { + x: px(0.0), + y: px(2.0), + }) + .into_any_element() + } +} diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index fbe9d86c5d7c8989653d657ed1df1ab0684882a4..e528b358e45d41be87207cbb5976a8ac97da95c3 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -11,7 +11,6 @@ use gpui::{ Subscription, Task, TitlebarOptions, UniformListScrollHandle, Window, WindowBounds, WindowHandle, WindowOptions, actions, div, list, point, prelude::*, px, uniform_list, }; -use heck::ToTitleCase as _; use project::{Project, WorktreeId}; use release_channel::ReleaseChannel; use schemars::JsonSchema; @@ -38,7 +37,9 @@ use util::{ResultExt as _, paths::PathStyle, rel_path::RelPath}; use workspace::{AppState, OpenOptions, OpenVisible, Workspace, client_side_decorations}; use zed_actions::{OpenSettings, OpenSettingsAt}; -use crate::components::{SettingsInputField, font_picker, icon_theme_picker, theme_picker}; +use crate::components::{ + EnumVariantDropdown, SettingsInputField, font_picker, icon_theme_picker, theme_picker, +}; const NAVBAR_CONTAINER_TAB_INDEX: isize = 0; const NAVBAR_GROUP_TAB_INDEX: isize = 1; @@ -3428,7 +3429,7 @@ fn render_dropdown( field: SettingField, file: SettingsUiFile, metadata: Option<&SettingsFieldMetadata>, - window: &mut Window, + _window: &mut Window, cx: &mut App, ) -> AnyElement where @@ -3444,56 +3445,19 @@ where SettingsStore::global(cx).get_value_from_file(file.to_settings(), field.pick); let current_value = current_value.copied().unwrap_or(variants()[0]); - let current_value_label = - labels()[variants().iter().position(|v| *v == current_value).unwrap()]; - - DropdownMenu::new( - "dropdown", - if should_do_titlecase { - current_value_label.to_title_case() - } else { - current_value_label.to_string() - }, - window.use_state(cx, |window, cx| { - ContextMenu::new(window, cx, move |mut menu, _, _| { - for (&value, &label) in std::iter::zip(variants(), labels()) { - let file = file.clone(); - menu = menu.toggleable_entry( - if should_do_titlecase { - label.to_title_case() - } else { - label.to_string() - }, - value == current_value, - IconPosition::End, - None, - move |_, cx| { - if value == current_value { - return; - } - update_settings_file( - file.clone(), - field.json_path, - cx, - move |settings, _cx| { - (field.write)(settings, Some(value)); - }, - ) - .log_err(); // todo(settings_ui) don't log err - }, - ); - } - menu + EnumVariantDropdown::new("dropdown", current_value, variants(), labels(), { + move |value, cx| { + if value == current_value { + return; + } + update_settings_file(file.clone(), field.json_path, cx, move |settings, _cx| { + (field.write)(settings, Some(value)); }) - }), - ) - .tab_index(0) - .trigger_size(ButtonSize::Medium) - .style(DropdownStyle::Outlined) - .offset(gpui::Point { - x: px(0.0), - y: px(2.0), + .log_err(); // todo(settings_ui) don't log err + } }) + .tab_index(0) + .title_case(should_do_titlecase) .into_any_element() }