From 972731cb1a1c971a947dc28a90c0d3c20c09996a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Soares?= <37777652+Dnreikronos@users.noreply.github.com> Date: Tue, 7 Apr 2026 20:41:34 -0300 Subject: [PATCH] theme_selector: Preserve selected theme after empty filter (#52461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When filtering themes with a query that matches nothing (e.g., "zzz"), `show_selected_theme` returned `None` and overwrote `selected_theme`. Clearing the filter then lost track of the previous selection and defaulted to index 0. The fix only updates `selected_theme` when `show_selected_theme` returns `Some`. Same change in both the theme selector and the icon theme selector. ## Context When `update_matches` runs a filter that yields zero results, `show_selected_theme` returns `None`. The old code unconditionally assigned that back to `selected_theme`, wiping out the previous selection. When the user clears the filter, the selector falls into the `query.is_empty() && selected_theme.is_none()` branch and resets to index 0 instead of restoring the original pick. ## Demo ### Before: https://github.com/user-attachments/assets/62b1531b-d059-4f30-b1f4-a830f2d13a09 ### After: https://github.com/user-attachments/assets/72348666-8dbb-4f35-9446-fa2618340b6c ## How to review The fix is the same one-line change in two files: 1. `crates/theme_selector/src/theme_selector.rs` — line 458 2. `crates/theme_selector/src/icon_theme_selector.rs` — line 272 The rest is test infrastructure: 3. `crates/theme/src/registry.rs` — `register_test_themes` / `register_test_icon_themes` helpers 4. Tests in both selector files covering the empty filter → clear filter flow ## Self-review checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Release notes - Fixed theme selector losing the selected theme after filtering with a query that matches nothing and then clearing the filter. --- Cargo.lock | 3 + crates/theme/src/registry.rs | 17 ++ crates/theme_selector/Cargo.toml | 4 + .../theme_selector/src/icon_theme_selector.rs | 160 +++++++++++++++++- crates/theme_selector/src/theme_selector.rs | 153 ++++++++++++++++- 5 files changed, 335 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f426d0da3392240300d15ca174013a6bdbdbb31d..afbf73bf6cc99d7fbb6917c3f1a29d0657d9402c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17731,12 +17731,15 @@ dependencies = [ name = "theme_selector" version = "0.1.0" dependencies = [ + "editor", "fs", "fuzzy", "gpui", "log", "picker", + "project", "serde", + "serde_json", "settings", "telemetry", "theme", diff --git a/crates/theme/src/registry.rs b/crates/theme/src/registry.rs index fbe535309773fa5c90c2031d44b420cf5fad2dc7..8630af9deda0d403e257bbc173f0f260ef32e184 100644 --- a/crates/theme/src/registry.rs +++ b/crates/theme/src/registry.rs @@ -126,6 +126,23 @@ impl ThemeRegistry { } } + /// Registers theme families for use in tests. + #[cfg(any(test, feature = "test-support"))] + pub fn register_test_themes(&self, families: impl IntoIterator) { + self.insert_theme_families(families); + } + + /// Registers icon themes for use in tests. + #[cfg(any(test, feature = "test-support"))] + pub fn register_test_icon_themes(&self, icon_themes: impl IntoIterator) { + let mut state = self.state.write(); + for icon_theme in icon_themes { + state + .icon_themes + .insert(icon_theme.name.clone(), Arc::new(icon_theme)); + } + } + /// Inserts the given themes into the registry. pub fn insert_themes(&self, themes: impl IntoIterator) { let mut state = self.state.write(); diff --git a/crates/theme_selector/Cargo.toml b/crates/theme_selector/Cargo.toml index 41e0e7681436f1fd8d6bfe743528af7d4f3d3ad6..68f9c7d0c31ce02d79a51bfbb51c23447009a648 100644 --- a/crates/theme_selector/Cargo.toml +++ b/crates/theme_selector/Cargo.toml @@ -29,3 +29,7 @@ workspace.workspace = true zed_actions.workspace = true [dev-dependencies] +editor = { workspace = true, features = ["test-support"] } +project.workspace = true +serde_json.workspace = true +theme = { workspace = true, features = ["test-support"] } diff --git a/crates/theme_selector/src/icon_theme_selector.rs b/crates/theme_selector/src/icon_theme_selector.rs index 13d6a87c4ac9911bef7a86c9df84171644ca6cf9..be8e5e01d0a5e2240dcf4e50d1ddd557b641d145 100644 --- a/crates/theme_selector/src/icon_theme_selector.rs +++ b/crates/theme_selector/src/icon_theme_selector.rs @@ -267,7 +267,10 @@ impl PickerDelegate for IconThemeSelectorDelegate { } else { this.delegate.selected_index = 0; } - this.delegate.selected_theme = this.delegate.show_selected_theme(cx); + // Preserve the previously selected theme when the filter yields no results. + if let Some(theme) = this.delegate.show_selected_theme(cx) { + this.delegate.selected_theme = Some(theme); + } }) .log_err(); }) @@ -335,3 +338,158 @@ impl PickerDelegate for IconThemeSelectorDelegate { ) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + use gpui::{TestAppContext, VisualTestContext}; + use project::Project; + use serde_json::json; + use theme::{ChevronIcons, DirectoryIcons, IconTheme, ThemeRegistry}; + use util::path; + use workspace::MultiWorkspace; + + fn init_test(cx: &mut TestAppContext) -> Arc { + cx.update(|cx| { + let app_state = workspace::AppState::test(cx); + settings::init(cx); + theme::init(theme::LoadThemes::JustBase, cx); + editor::init(cx); + crate::init(cx); + app_state + }) + } + + fn register_test_icon_themes(cx: &mut TestAppContext) { + cx.update(|cx| { + let registry = ThemeRegistry::global(cx); + let make_icon_theme = |name: &str, appearance: Appearance| IconTheme { + id: name.to_lowercase().replace(' ', "-"), + name: SharedString::from(name.to_string()), + appearance, + directory_icons: DirectoryIcons { + collapsed: None, + expanded: None, + }, + named_directory_icons: HashMap::default(), + chevron_icons: ChevronIcons { + collapsed: None, + expanded: None, + }, + file_icons: HashMap::default(), + file_stems: HashMap::default(), + file_suffixes: HashMap::default(), + }; + registry.register_test_icon_themes([ + make_icon_theme("Test Icons A", Appearance::Dark), + make_icon_theme("Test Icons B", Appearance::Dark), + ]); + }); + } + + async fn setup_test(cx: &mut TestAppContext) -> Arc { + let app_state = init_test(cx); + register_test_icon_themes(cx); + app_state + .fs + .as_fake() + .insert_tree(path!("/test"), json!({})) + .await; + app_state + } + + fn open_icon_theme_selector( + workspace: &Entity, + cx: &mut VisualTestContext, + ) -> Entity> { + cx.dispatch_action(zed_actions::icon_theme_selector::Toggle { + themes_filter: None, + }); + cx.run_until_parked(); + workspace.update(cx, |workspace, cx| { + workspace + .active_modal::(cx) + .expect("icon theme selector should be open") + .read(cx) + .picker + .clone() + }) + } + + fn selected_theme_name( + picker: &Entity>, + cx: &mut VisualTestContext, + ) -> String { + picker.read_with(cx, |picker, _| { + picker + .delegate + .matches + .get(picker.delegate.selected_index) + .expect("selected index should point to a match") + .string + .clone() + }) + } + + fn previewed_theme_name( + _picker: &Entity>, + cx: &mut VisualTestContext, + ) -> String { + cx.read(|cx| { + ThemeSettings::get_global(cx) + .icon_theme + .name(SystemAppearance::global(cx).0) + .0 + .to_string() + }) + } + + #[gpui::test] + async fn test_icon_theme_selector_preserves_selection_on_empty_filter(cx: &mut TestAppContext) { + let app_state = setup_test(cx).await; + let project = Project::test(app_state.fs.clone(), [path!("/test").as_ref()], cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = + multi_workspace.read_with(cx, |multi_workspace, _| multi_workspace.workspace().clone()); + let picker = open_icon_theme_selector(&workspace, cx); + + let target_index = picker.read_with(cx, |picker, _| { + picker + .delegate + .matches + .iter() + .position(|m| m.string == "Test Icons A") + .unwrap() + }); + picker.update_in(cx, |picker, window, cx| { + picker.set_selected_index(target_index, None, true, window, cx); + }); + cx.run_until_parked(); + + assert_eq!(previewed_theme_name(&picker, cx), "Test Icons A"); + + picker.update_in(cx, |picker, window, cx| { + picker.update_matches("zzz".to_string(), window, cx); + }); + cx.run_until_parked(); + + picker.update_in(cx, |picker, window, cx| { + picker.update_matches("".to_string(), window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + selected_theme_name(&picker, cx), + "Test Icons A", + "selected icon theme should be preserved after clearing an empty filter" + ); + assert_eq!( + previewed_theme_name(&picker, cx), + "Test Icons A", + "previewed icon theme should be preserved after clearing an empty filter" + ); + } +} diff --git a/crates/theme_selector/src/theme_selector.rs b/crates/theme_selector/src/theme_selector.rs index fb4d68a9da6f4a96e52fef288e58bdec90cae6fa..8a0e835fa865f6e4e275248b1da44c332b728459 100644 --- a/crates/theme_selector/src/theme_selector.rs +++ b/crates/theme_selector/src/theme_selector.rs @@ -455,7 +455,10 @@ impl PickerDelegate for ThemeSelectorDelegate { } else { this.delegate.selected_index = 0; } - this.delegate.selected_theme = this.delegate.show_selected_theme(cx); + // Preserve the previously selected theme when the filter yields no results. + if let Some(theme) = this.delegate.show_selected_theme(cx) { + this.delegate.selected_theme = Some(theme); + } }) .log_err(); }) @@ -523,3 +526,151 @@ impl PickerDelegate for ThemeSelectorDelegate { ) } } + +#[cfg(test)] +mod tests { + use super::*; + use gpui::{TestAppContext, VisualTestContext}; + use project::Project; + use serde_json::json; + use theme::{Appearance, ThemeFamily, ThemeRegistry, default_color_scales}; + use util::path; + use workspace::MultiWorkspace; + + fn init_test(cx: &mut TestAppContext) -> Arc { + cx.update(|cx| { + let app_state = workspace::AppState::test(cx); + settings::init(cx); + theme::init(theme::LoadThemes::JustBase, cx); + editor::init(cx); + super::init(cx); + app_state + }) + } + + fn register_test_themes(cx: &mut TestAppContext) { + cx.update(|cx| { + let registry = ThemeRegistry::global(cx); + let base_theme = registry.get("One Dark").unwrap(); + + let mut test_light = (*base_theme).clone(); + test_light.id = "test-light".to_string(); + test_light.name = "Test Light".into(); + test_light.appearance = Appearance::Light; + + let mut test_dark_a = (*base_theme).clone(); + test_dark_a.id = "test-dark-a".to_string(); + test_dark_a.name = "Test Dark A".into(); + + let mut test_dark_b = (*base_theme).clone(); + test_dark_b.id = "test-dark-b".to_string(); + test_dark_b.name = "Test Dark B".into(); + + registry.register_test_themes([ThemeFamily { + id: "test-family".to_string(), + name: "Test Family".into(), + author: "test".into(), + themes: vec![test_light, test_dark_a, test_dark_b], + scales: default_color_scales(), + }]); + }); + } + + async fn setup_test(cx: &mut TestAppContext) -> Arc { + let app_state = init_test(cx); + register_test_themes(cx); + app_state + .fs + .as_fake() + .insert_tree(path!("/test"), json!({})) + .await; + app_state + } + + fn open_theme_selector( + workspace: &Entity, + cx: &mut VisualTestContext, + ) -> Entity> { + cx.dispatch_action(zed_actions::theme_selector::Toggle { + themes_filter: None, + }); + cx.run_until_parked(); + workspace.update(cx, |workspace, cx| { + workspace + .active_modal::(cx) + .expect("theme selector should be open") + .read(cx) + .picker + .clone() + }) + } + + fn selected_theme_name( + picker: &Entity>, + cx: &mut VisualTestContext, + ) -> String { + picker.read_with(cx, |picker, _| { + picker + .delegate + .matches + .get(picker.delegate.selected_index) + .expect("selected index should point to a match") + .string + .clone() + }) + } + + fn previewed_theme_name( + picker: &Entity>, + cx: &mut VisualTestContext, + ) -> String { + picker.read_with(cx, |picker, _| picker.delegate.new_theme.name.to_string()) + } + + #[gpui::test] + async fn test_theme_selector_preserves_selection_on_empty_filter(cx: &mut TestAppContext) { + let app_state = setup_test(cx).await; + let project = Project::test(app_state.fs.clone(), [path!("/test").as_ref()], cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = + multi_workspace.read_with(cx, |multi_workspace, _| multi_workspace.workspace().clone()); + let picker = open_theme_selector(&workspace, cx); + + let target_index = picker.read_with(cx, |picker, _| { + picker + .delegate + .matches + .iter() + .position(|m| m.string == "Test Light") + .unwrap() + }); + picker.update_in(cx, |picker, window, cx| { + picker.set_selected_index(target_index, None, true, window, cx); + }); + cx.run_until_parked(); + + assert_eq!(previewed_theme_name(&picker, cx), "Test Light"); + + picker.update_in(cx, |picker, window, cx| { + picker.update_matches("zzz".to_string(), window, cx); + }); + cx.run_until_parked(); + + picker.update_in(cx, |picker, window, cx| { + picker.update_matches("".to_string(), window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + selected_theme_name(&picker, cx), + "Test Light", + "selected theme should be preserved after clearing an empty filter" + ); + assert_eq!( + previewed_theme_name(&picker, cx), + "Test Light", + "previewed theme should be preserved after clearing an empty filter" + ); + } +}