mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
theme_selector: Preserve selected theme after empty filter (#52461)
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.
This commit is contained in:
parent
aa8150527e
commit
972731cb1a
5 changed files with 335 additions and 2 deletions
3
Cargo.lock
generated
3
Cargo.lock
generated
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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<Item = ThemeFamily>) {
|
||||
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<Item = IconTheme>) {
|
||||
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<Item = Theme>) {
|
||||
let mut state = self.state.write();
|
||||
|
|
|
|||
|
|
@ -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"] }
|
||||
|
|
|
|||
|
|
@ -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<workspace::AppState> {
|
||||
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<workspace::AppState> {
|
||||
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<workspace::Workspace>,
|
||||
cx: &mut VisualTestContext,
|
||||
) -> Entity<Picker<IconThemeSelectorDelegate>> {
|
||||
cx.dispatch_action(zed_actions::icon_theme_selector::Toggle {
|
||||
themes_filter: None,
|
||||
});
|
||||
cx.run_until_parked();
|
||||
workspace.update(cx, |workspace, cx| {
|
||||
workspace
|
||||
.active_modal::<IconThemeSelector>(cx)
|
||||
.expect("icon theme selector should be open")
|
||||
.read(cx)
|
||||
.picker
|
||||
.clone()
|
||||
})
|
||||
}
|
||||
|
||||
fn selected_theme_name(
|
||||
picker: &Entity<Picker<IconThemeSelectorDelegate>>,
|
||||
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<Picker<IconThemeSelectorDelegate>>,
|
||||
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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<workspace::AppState> {
|
||||
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<workspace::AppState> {
|
||||
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<workspace::Workspace>,
|
||||
cx: &mut VisualTestContext,
|
||||
) -> Entity<Picker<ThemeSelectorDelegate>> {
|
||||
cx.dispatch_action(zed_actions::theme_selector::Toggle {
|
||||
themes_filter: None,
|
||||
});
|
||||
cx.run_until_parked();
|
||||
workspace.update(cx, |workspace, cx| {
|
||||
workspace
|
||||
.active_modal::<ThemeSelector>(cx)
|
||||
.expect("theme selector should be open")
|
||||
.read(cx)
|
||||
.picker
|
||||
.clone()
|
||||
})
|
||||
}
|
||||
|
||||
fn selected_theme_name(
|
||||
picker: &Entity<Picker<ThemeSelectorDelegate>>,
|
||||
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<Picker<ThemeSelectorDelegate>>,
|
||||
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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue