From 4cba355f712afc603b69758675d128afe27c1211 Mon Sep 17 00:00:00 2001 From: nrq6993-a11y Date: Tue, 20 Oct 2026 02:57:40 +0300 Subject: [PATCH] Fix #2995: address PR review on tab reorder and dock drag begin Reuse catalogued window.panel.move errors, keep legacy dock drags from clearing docking on Escape, reorder floating tabs locally without reconcile re-entry, and fix dock_tab_order test setup. Co-authored-by: Cursor --- crates/ui-egui/src/dock.rs | 11 +++++---- crates/ui-egui/src/floating.rs | 20 ++++++++-------- crates/ui-egui/src/panel_docking.rs | 37 +++++++++++++++++++++-------- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/crates/ui-egui/src/dock.rs b/crates/ui-egui/src/dock.rs index ce8874e9d..320baa287 100644 --- a/crates/ui-egui/src/dock.rs +++ b/crates/ui-egui/src/dock.rs @@ -858,12 +858,15 @@ mod tests { #[test] fn partial_dock_tab_order_appends_tabs_missing_from_preferences() { - let mut ui = crate::state::UiState::default(); - ui.dock_tab_order = vec![DockTab::Layers]; + let ui = crate::state::UiState { + dock_tab: DockTab::Layers, + dock_tab_order: vec![DockTab::Layers, DockTab::Properties], + ..Default::default() + }; let order = floating::resolved_dock_tab_order(&ui); assert_eq!(order.len(), DockTab::ALL.len()); - assert_eq!(order[0], DockTab::Layers); - assert!(order.contains(&DockTab::Properties) && order.contains(&DockTab::Libraries)); + assert_eq!(order[..2], [DockTab::Layers, DockTab::Properties]); + assert_eq!(order[2], DockTab::Libraries); } #[test] diff --git a/crates/ui-egui/src/floating.rs b/crates/ui-egui/src/floating.rs index 3ad795e8a..2c7247617 100644 --- a/crates/ui-egui/src/floating.rs +++ b/crates/ui-egui/src/floating.rs @@ -647,6 +647,7 @@ fn apply(app: &mut VectorcraftApp, ctx: &egui::Context, action: Option) return; } let before = app.ui.clone(); + let skip_reconcile = matches!(action, Some(Action::Reorder(_, _, _))); match action { Some(Action::Activate(gi, k)) => { if let Some(g) = app.ui.floating_panels.get_mut(gi) @@ -678,16 +679,15 @@ fn apply(app: &mut VectorcraftApp, ctx: &egui::Context, action: Option) Some(Action::Detach(gi, left)) => detach(app, ctx, gi, left), Some(Action::Tear(id, left)) => tear(app, ctx, &[id], id, strip_grab(ctx, left)), Some(Action::Reorder(gi, from, to)) => { - let ids = app.ui.floating_panels.get(gi).and_then(|g| { - if from == to { None } else { Some((g.panels.get(from).cloned(), g.panels.get(to).cloned())) } - }); - if let Some((Some(from_id), Some(to_id))) = ids - && let Err(error) = app.run( - "window.panel.move", - json!({ "panel": from_id, "anchor": to_id, "before": to_id }), - ) + if let Some(g) = app.ui.floating_panels.get_mut(gi) + && from != to + && from < g.panels.len() + && to < g.panels.len() { - app.ui.status = error; + let active = g.panels.get(g.active).cloned(); + let moved = g.panels.remove(from); + g.panels.insert(to, moved); + g.active = active.and_then(|id| g.panels.iter().position(|p| *p == id)).unwrap_or(0); } } Some(Action::Undock(gi, grab)) => { @@ -705,7 +705,7 @@ fn apply(app: &mut VectorcraftApp, ctx: &egui::Context, action: Option) } None => {} } - if let Err(error) = crate::panel_docking::reconcile_native(app, before.clone()) { + if !skip_reconcile && let Err(error) = crate::panel_docking::reconcile_native(app, before.clone()) { app.ui.status = error; } ctx.data_mut(|data| { diff --git a/crates/ui-egui/src/panel_docking.rs b/crates/ui-egui/src/panel_docking.rs index f714a47e2..58fe6d11f 100644 --- a/crates/ui-egui/src/panel_docking.rs +++ b/crates/ui-egui/src/panel_docking.rs @@ -146,14 +146,15 @@ fn number(params: &Value, key: &str, default: f32) -> Result { /// group without promoting to a shared layout. fn legacy_same_strip_reorder(app: &mut VectorcraftApp, panel: &str, before: &str) -> Result { use crate::state::DockTab; + const BAD_DEST: &str = "move requires an existing destination panel"; if crate::state::DockTab::from_id(panel).is_some() && DockTab::from_id(before).is_some() && crate::floating::group_of(&app.ui, panel).is_none() && crate::floating::group_of(&app.ui, before).is_none() { let mut order: Vec = crate::floating::docked_tabs(&app.ui).collect(); - let from = order.iter().position(|t| t.info().0 == panel).ok_or("panel is not in the dock strip")?; - let to = order.iter().position(|t| t.info().0 == before).ok_or("before is not in the dock strip")?; + let from = order.iter().position(|t| t.info().0 == panel).ok_or(BAD_DEST)?; + let to = order.iter().position(|t| t.info().0 == before).ok_or(BAD_DEST)?; if from != to { let item = order.remove(from); order.insert(to, item); @@ -161,13 +162,13 @@ fn legacy_same_strip_reorder(app: &mut VectorcraftApp, panel: &str, before: &str } return Ok(json!({ "panel": panel, "before": before })); } - let gi = crate::floating::group_of(&app.ui, panel).ok_or("panel is not in a tab group")?; + let gi = crate::floating::group_of(&app.ui, panel).ok_or(BAD_DEST)?; if crate::floating::group_of(&app.ui, before) != Some(gi) { - return Err("before must name a tab in the same group".into()); + return Err(BAD_DEST.into()); } - let g = app.ui.floating_panels.get_mut(gi).ok_or("panel group missing")?; - let from = g.panels.iter().position(|id| id == panel).ok_or("panel is not in the group")?; - let to = g.panels.iter().position(|id| id == before).ok_or("before is not in the group")?; + let g = app.ui.floating_panels.get_mut(gi).ok_or("invalid restored panel group")?; + let from = g.panels.iter().position(|id| id == panel).ok_or(BAD_DEST)?; + let to = g.panels.iter().position(|id| id == before).ok_or(BAD_DEST)?; if from != to { let active = g.panels.get(g.active).cloned(); let moved = g.panels.remove(from); @@ -329,9 +330,25 @@ fn apply_action(app: &mut VectorcraftApp, action: Action) -> Result Result { let panel = params.get("panel").and_then(Value::as_str).and_then(normalize).ok_or("panel must name a registered panel")?; - if app.ui.docking.is_none() && operation == "move" && params.get("before").is_some() { + if operation == "move" && params.get("before").is_some() { let before = params.get("before").and_then(Value::as_str).and_then(normalize).ok_or("before must name a panel")?; - return legacy_same_strip_reorder(app, panel, before); + let in_group = crate::floating::group_of(&app.ui, panel).is_some(); + use crate::state::DockTab; + let dock_strip_reorder = DockTab::from_id(panel).is_some() + && DockTab::from_id(before).is_some() + && crate::floating::group_of(&app.ui, panel).is_none() + && crate::floating::group_of(&app.ui, before).is_none(); + if app.ui.docking.is_none() || in_group || dock_strip_reorder { + let mirror_native_tabs = crate::floating::group_of(&app.ui, panel) + .and_then(|i| app.ui.floating_panels.get(i)) + .is_some_and(decorated); + let snapshot = app.ui.clone(); + let result = legacy_same_strip_reorder(app, panel, before); + if mirror_native_tabs && app.ui.docking.is_some() && result.is_ok() { + reconcile_native(app, snapshot)?; + } + return result; + } } if app.ui.docking.as_ref().is_some_and(|layout| !valid(layout)) { return Err("saved panel layout is invalid; reset the workspace first".into()); @@ -935,7 +952,7 @@ pub(crate) fn legacy_tab_in_strip( let area = egui::Id::new("vectorcraft-panel-docking"); let begun = ui.ctx().data(|d| d.get_temp::(area.with("legacy-origin")).is_some()); let begin = if strip.is_some() { - !inside_strip && !begun && (response.dragged() || response.drag_started()) + !begun && (response.drag_started() || (!inside_strip && response.dragged())) } else { response.drag_started() };