From ed737afda061a5d3170f4ae5c5295ce950301466 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Sun, 11 Oct 2026 21:17:12 +0200 Subject: [PATCH] fix: a folder listed twice in Local can't be collapsed (#1093) Local lists a folder once per place it is in: Pictures as a place of its own and again inside Home. Each Local row keyed its open state and its triangle's egui id by the folder's path alone, so once Home was open the two Pictures triangles shared one id: the copy drawn last took the clicks of the copy drawn first, and the first Pictures could not be collapsed. The open state was shared too, so both copies opened and closed together. The tree now keys both by the top-level place it hangs from as well, so every row opens and closes by itself. Regression test: two Local places, one inside the other, and a click on each copy's triangle. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/left.rs | 12 ++++++--- crates/ui-egui/src/tests_masking.rs | 42 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/panels/left.rs b/crates/ui-egui/src/panels/left.rs index a14332c82..258a804b7 100644 --- a/crates/ui-egui/src/panels/left.rs +++ b/crates/ui-egui/src/panels/left.rs @@ -798,7 +798,7 @@ fn local_rows(app: &mut LightcraftApp, ui: &mut egui::Ui, now: &LocalNow) { for (i, (name, path)) in local.places.iter().enumerate() { let transient = local.browse_root.as_deref() == Some(path.as_str()); let reveal = local.owner == Some(i); - folder_tree(app, ui, name, path, 0.0, current, reveal, transient); + folder_tree(app, ui, path, name, path, 0.0, current, reveal, transient); } if app.services.pick_folder.is_some() && row(app, ui, "local:browse", Icon::Plus, crate::i18n::tr("Browse Folder…"), None, false, 0.0).clicked() { @@ -990,10 +990,14 @@ pub(crate) fn fs_cached(ui: &egui::Ui, kind: &'static /// subfolders. In the tree that holds the folder being browsed (`reveal`), the folders on the /// way down to it open whenever the browsed folder changes, so it shows highlighted in place. /// `transient`: a top-level row listed for this session only (it offers Keep in Local). +/// `place`: the top-level folder this tree hangs from. A folder can be listed in more than one +/// tree (Pictures on its own and inside Home): each row opens and closes by itself, and their +/// triangles have ids of their own, else the copy drawn last takes the other's clicks (#1093). #[allow(clippy::too_many_arguments)] fn folder_tree( app: &mut LightcraftApp, ui: &mut egui::Ui, + place: &str, name: &str, path: &str, indent: f32, @@ -1001,13 +1005,13 @@ fn folder_tree( reveal: bool, transient: bool, ) { - let open_id = egui::Id::new(("folder-open", path.to_string())); + let open_id = egui::Id::new(("folder-open", place.to_string(), path.to_string())); let sel = current.is_some_and(|c| same_folder(c, path)); let on_the_way = !sel && current.is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)); let mut open = tree_open(ui, open_id, false); if reveal && on_the_way { // opened once per browsed folder: collapsing it again afterwards sticks - let revealed_id = egui::Id::new(("folder-revealed", path.to_string())); + let revealed_id = egui::Id::new(("folder-revealed", place.to_string(), path.to_string())); let target = current.map(str::to_string); if ui.data(|d| d.get_temp::>(revealed_id)) != Some(target.clone()) { open = true; @@ -1070,7 +1074,7 @@ fn folder_tree( }); if open && indent < 12.0 * 8.0 { for (n, p) in subfolders(ui, path) { - folder_tree(app, ui, &n, &p, indent + 12.0, current, reveal, false); + folder_tree(app, ui, place, &n, &p, indent + 12.0, current, reveal, false); } } } diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index 8024c62c5..7c7acac4c 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -896,6 +896,48 @@ fn local_folder_tree_expands_and_browses() { h.settle(SETTLE); } +/// A folder listed twice in Local (Pictures as a place of its own and again inside Home) opens +/// and closes on each row by itself: both copies of its triangle used to share one egui id, so +/// the copy drawn later took the clicks of the one drawn first (issue #1093). +#[test] +fn local_folder_listed_twice_opens_and_closes_each_row() { + let base = std::env::temp_dir().join(format!("lc-ui-twice-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let inner = base.join("Inner"); + std::fs::create_dir_all(inner.join("Sub")).unwrap(); + let mut h = detail("panel.edit"); + exec(&mut h, "view.leftPanel", json!({"show": true})); + h.hide_home_above(&base); + let (base_s, inner_s) = (base.to_string_lossy().to_string(), inner.to_string_lossy().to_string()); + let sub = inner.join("Sub").to_string_lossy().to_string(); + exec(&mut h, "local.addRoot", json!({"path": base_s})); + exec(&mut h, "local.addRoot", json!({"path": inner_s})); + h.settle(SETTLE); + let r = h.request("ui.clickWidget", json!({"id": format!("folderToggle:{base_s}")}), T); + assert_eq!(r["ok"], true, "{r}"); + let toggle = format!("folderToggle:{inner_s}"); + h.step_until(SETTLE, |h| h.app.widgets.iter().filter(|(w, _)| *w == toggle).count() == 2); + let rows = |h: &Headless, id: &str| h.app.widgets.iter().filter(|(w, _)| w == id).count(); + assert_eq!(rows(&h, &toggle), 2, "Inner is listed inside the base folder and as a place of its own"); + let sub_row = format!("source:local:{sub}"); + assert_eq!(rows(&h, &sub_row), 0); + // each copy's triangle opens that copy only, and closes it again + for copy in 0..2 { + let at = h.app.widgets.iter().filter(|(w, _)| *w == toggle).nth(copy).map(|(_, r)| r.center()).unwrap(); + let r = h.request("ui.click", json!({"x": at.x, "y": at.y}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step_until(SETTLE, |h| h.app.widgets.iter().any(|(w, _)| *w == sub_row)); + assert_eq!(rows(&h, &sub_row), 1, "copy {copy} opens, the other stays closed"); + let at = h.app.widgets.iter().filter(|(w, _)| *w == toggle).nth(copy).map(|(_, r)| r.center()).unwrap(); + let r = h.request("ui.click", json!({"x": at.x, "y": at.y}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert_eq!(rows(&h, &sub_row), 0, "copy {copy} closes again"); + } + let _ = std::fs::remove_dir_all(&base); + h.settle(SETTLE); +} + #[test] fn slideshow_advances_pauses_and_ends() { let mut h = detail("panel.edit");