From 385463e3bb1c5a527ee3ca9166bf898f9bd04189 Mon Sep 17 00:00:00 2001 From: Jiekang Tian Date: Thu, 23 Jul 2026 20:02:41 +0800 Subject: [PATCH] fix: unify tool toggle semantics --- crates/app/AGENTS.md | 3 + crates/app/src/ui/command_exec.rs | 12 ++-- crates/app/src/ui/commands.rs | 19 ++++++- crates/app/src/ui/commands_tests.rs | 57 +++++++++++++++++++ crates/app/src/ui/primary_sidebar.rs | 2 + crates/app/src/ui/shortcuts.rs | 24 +++++++- crates/app/src/ui/tools/line_fit.rs | 6 +- crates/app/src/ui/tools/mod.rs | 20 ++----- crates/app/src/ui/tools/slice.rs | 6 +- crates/core/src/actions/tests/interaction.rs | 38 +++++++++++++ crates/core/src/state/app_impl.rs | 12 ++++ crates/core/src/state/units.rs | 21 +++++++ docs/src/content/docs/reference/shortcuts.md | 6 +- .../content/docs/zh-cn/reference/shortcuts.md | 6 +- 14 files changed, 196 insertions(+), 36 deletions(-) diff --git a/crates/app/AGENTS.md b/crates/app/AGENTS.md index 8a13d93..2068595 100644 --- a/crates/app/AGENTS.md +++ b/crates/app/AGENTS.md @@ -11,6 +11,9 @@ show/hide/disable behavior regardless of who adds them. command palette, and shortcuts share one enabled/placement decision. Do not add ad-hoc business-action buttons that bypass the catalog. Panel-local widget interactions (rename fields, tree-row toggles) are exempt. +- A command whose checked state derives from `session.tool` is a toggle and + must execute through `toggle_tool`; reserve `set_tool` for navigation and + other ensure-on flows. ## Hide vs disable diff --git a/crates/app/src/ui/command_exec.rs b/crates/app/src/ui/command_exec.rs index d5cfb05..c97ca0d 100644 --- a/crates/app/src/ui/command_exec.rs +++ b/crates/app/src/ui/command_exec.rs @@ -103,7 +103,7 @@ pub fn execute( CommandId::SpectrumArithmetic => super::arithmetic::open_spectrum_arithmetic_dialog(app), CommandId::AlignSpectra => super::align::open_align_spectra_dialog(app), CommandId::StackData => app.stack_selected_data(), - CommandId::SelectRange => app.set_tool(Tool::SelectRegion), + CommandId::SelectRange => app.toggle_tool(Tool::SelectRegion), CommandId::ClearRange => app.clear_analysis_selection(), CommandId::Regions => toggle_regions(app), CommandId::SeriesTable => open_active_region_table(app), @@ -116,7 +116,7 @@ pub fn execute( CommandId::Statistics => open_statistics(app), CommandId::ChartType => open_chart_type(app), CommandId::FigureTypography => app.session.ui.figure_typography_open = true, - CommandId::Integrate => app.set_tool(Tool::Integrate), + CommandId::Integrate => app.toggle_tool(Tool::Integrate), CommandId::Multiplets => analyze_multiplets(app), CommandId::TidyBoard => app.tidy_board(), CommandId::CanvasSettings => { @@ -144,7 +144,7 @@ pub fn execute( app.apply_theme(&theme); } } - CommandId::Tool(tool) => app.set_tool(tool), + CommandId::Tool(tool) => app.toggle_tool(tool), } } @@ -222,13 +222,13 @@ fn analyze_multiplets(app: &mut PlotxApp) { } fn reveal_tool_group(app: &mut PlotxApp, tool: Tool, group: ToolGroup) { - app.set_tool(tool); + app.toggle_tool(tool); reveal_group(app, group); } fn toggle_regions(app: &mut PlotxApp) { if app.session.tool == Tool::Regions { - app.set_tool(Tool::BrowseZoom); + app.toggle_tool(Tool::Regions); return; } let Some(dataset) = app @@ -237,7 +237,7 @@ fn toggle_regions(app: &mut PlotxApp) { else { return; }; - app.set_tool(Tool::Regions); + app.toggle_tool(Tool::Regions); super::tools::open_region_task(app, dataset); } diff --git a/crates/app/src/ui/commands.rs b/crates/app/src/ui/commands.rs index ce77f4b..fd64c6c 100644 --- a/crates/app/src/ui/commands.rs +++ b/crates/app/src/ui/commands.rs @@ -122,6 +122,18 @@ pub enum CommandExecutionClass { } impl CommandId { + fn tool_target(self) -> Option { + match self { + Self::SelectRange => Some(Tool::SelectRegion), + Self::Regions => Some(Tool::Regions), + Self::PeakList => Some(Tool::Peaks), + Self::LineFit => Some(Tool::LineFit), + Self::Integrate => Some(Tool::Integrate), + Self::Tool(tool) => Some(tool), + _ => None, + } + } + pub fn execution_class(self) -> CommandExecutionClass { match self { Self::RunBatchWorkflow => CommandExecutionClass::ToolEditor, @@ -520,7 +532,12 @@ pub fn describe(app: &PlotxApp, id: CommandId) -> CommandDescriptor { && app.session.ui.spectrum_arithmetic_dialog.is_none() && app.session.ui.align_spectra_dialog.is_none() && !app.session.ui.interaction.is_active()); - let enabled = gate.is_ok() && palette_available; + // Activation requirements must not trap an already-active tool after the + // dataset context changes: its command remains available for deactivation. + let active_tool = id + .tool_target() + .is_some_and(|tool| app.session.tool == tool); + let enabled = (gate.is_ok() || active_tool) && palette_available; let disabled_reason = if enabled { None } else { gate.err() }; // A contextual group is dropped from the Ribbon entirely rather than shown // permanently dead: a dead group still consumes the width budget in diff --git a/crates/app/src/ui/commands_tests.rs b/crates/app/src/ui/commands_tests.rs index 807ba87..5e7f3b1 100644 --- a/crates/app/src/ui/commands_tests.rs +++ b/crates/app/src/ui/commands_tests.rs @@ -373,6 +373,63 @@ fn live_checked_and_enabled_state_comes_from_one_descriptor() { assert_eq!(describe(&app, CommandId::CurveFit).label, "Fit Curves"); } +#[test] +fn commands_that_activate_tools_toggle_back_to_their_rest_tool() { + let catalog_app = app_with_nmr(); + let command_ids: Vec<_> = catalog(&catalog_app) + .into_iter() + // Plain actions may open native dialogs or external URLs; the checked + // contract is what identifies catalog toggles without enumerating tools. + .filter(|command| command.checked.is_some()) + .map(|command| command.id) + .collect(); + + for id in command_ids { + let mut app = app_with_nmr(); + let before = app.session.tool; + let ctx = egui::Context::default(); + let mut clipboard = crate::ui::clipboard_table::ClipboardTablePaste::default(); + + execute(id, &mut app, &mut clipboard, &ctx); + let activated = app.session.tool; + if activated == before { + continue; + } + + execute(id, &mut app, &mut clipboard, &ctx); + assert_eq!( + app.session.tool, + activated.rest(), + "command {id:?} activated {activated:?} but did not toggle to its rest tool" + ); + } +} + +#[test] +fn active_tool_can_be_deactivated_after_its_requirements_become_unmet() { + let mut app = app_with_nmr(); + let ctx = egui::Context::default(); + let mut clipboard = crate::ui::clipboard_table::ClipboardTablePaste::default(); + execute(CommandId::Integrate, &mut app, &mut clipboard, &ctx); + assert_eq!(app.session.tool, Tool::Integrate); + + let mut table_app = app_with_table(0); + let table = table_app.doc.datasets.remove(0); + let action = Action::insert_dataset_with_default_canvas( + &app, + table, + "Canvas — table".to_owned(), + DEFAULT_CANVAS_SIZE_MM, + ); + app.execute_action(action); + let integrate = describe(&app, CommandId::Integrate); + assert!(integrate.enabled); + assert_eq!(integrate.checked, Some(true)); + + execute(CommandId::Integrate, &mut app, &mut clipboard, &ctx); + assert_eq!(app.session.tool, Tool::BrowseZoom); +} + /// The disabled tooltip must name the requirement that actually blocked the /// command, not one fixed sentence per command. #[test] diff --git a/crates/app/src/ui/primary_sidebar.rs b/crates/app/src/ui/primary_sidebar.rs index ccff2bc..a76b667 100644 --- a/crates/app/src/ui/primary_sidebar.rs +++ b/crates/app/src/ui/primary_sidebar.rs @@ -663,6 +663,8 @@ fn select_dataset(app: &mut PlotxApp, ui: &Ui, di: usize, extend: bool) { fn select_analysis(app: &mut PlotxApp, ui: &Ui, di: usize, item: &AnalysisItem, open: bool) { app.focus_single(di); app.session.ui.data_browser_selected_node = Some(item.kind.key(di)); + // Opening a browser result must reveal its editor even when that tool is + // already active, so this navigation path intentionally uses ensure-on. match item.kind { AnalysisKind::Peak(id) => { app.session.ui.selected_peak = Some(id); diff --git a/crates/app/src/ui/shortcuts.rs b/crates/app/src/ui/shortcuts.rs index b256da3..fc2fab9 100644 --- a/crates/app/src/ui/shortcuts.rs +++ b/crates/app/src/ui/shortcuts.rs @@ -267,7 +267,10 @@ pub(super) fn handle_escape_shortcut(app: &mut PlotxApp, ctx: &egui::Context) { if !escape { return; } + handle_escape(app, now); +} +fn handle_escape(app: &mut PlotxApp, now: f64) { if app.interaction().is_active() { let phase = matches!(app.interaction(), Interaction::Phase(_)); app.cancel_interaction(); @@ -284,7 +287,8 @@ pub(super) fn handle_escape_shortcut(app: &mut PlotxApp, ctx: &egui::Context) { app.session.status = "Cancelled interaction.".to_owned(); return; } - // Esc steps back one level: analysis region -> panel-letter sub-selection -> selection. + // Esc steps back one level: analysis region -> panel-letter sub-selection -> + // selection -> active tool. if app.session.ui.analysis_selection.is_some() { if let Some(ci) = app.session.active_canvas { canvas::clear_canvas_interaction_state( @@ -307,6 +311,13 @@ pub(super) fn handle_escape_shortcut(app: &mut PlotxApp, ctx: &egui::Context) { if !matches!(app.session.ui.selection, Selection::None) { exit_to_page(app, "Selection cleared."); + return; + } + + let rest = app.session.tool.rest(); + if app.session.tool != rest { + app.set_tool(rest); + app.session.status = "Exited tool mode.".to_owned(); } } @@ -533,4 +544,15 @@ mod tests { ); assert!(shortcut_label(commands::CommandId::About).is_none()); } + + #[test] + fn escape_exits_an_active_tool_after_other_fallbacks() { + let mut app = PlotxApp::new_with_settings(plotx_core::settings::Settings::default()); + app.set_tool(Tool::Integrate); + + handle_escape(&mut app, 0.0); + + assert_eq!(app.session.tool, Tool::BrowseZoom); + assert_eq!(app.session.status, "Exited tool mode."); + } } diff --git a/crates/app/src/ui/tools/line_fit.rs b/crates/app/src/ui/tools/line_fit.rs index fec3cf9..0431225 100644 --- a/crates/app/src/ui/tools/line_fit.rs +++ b/crates/app/src/ui/tools/line_fit.rs @@ -31,11 +31,7 @@ pub(super) fn line_fit_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) -> bool .on_hover_text("Deconvolve a region into overlapping lineshape components.") .clicked() { - app.set_tool(if active { - Tool::BrowseZoom - } else { - Tool::LineFit - }); + app.toggle_tool(Tool::LineFit); } if active { ui.small( diff --git a/crates/app/src/ui/tools/mod.rs b/crates/app/src/ui/tools/mod.rs index 41a7cea..ab0b898 100644 --- a/crates/app/src/ui/tools/mod.rs +++ b/crates/app/src/ui/tools/mod.rs @@ -121,7 +121,7 @@ fn analysis_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) -> bool { ui.horizontal(|ui| { let selected = app.session.tool == Tool::SelectRegion; if ui.selectable_label(selected, "Analysis range").clicked() { - app.set_tool(Tool::SelectRegion); + app.toggle_tool(Tool::SelectRegion); } if ui .add_enabled(has_selection, Button::new("Clear")) @@ -186,11 +186,7 @@ fn peaks_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) -> bool { .on_hover_text("Pick peaks by region, click, or one-shot detection — one set.") .clicked() { - app.set_tool(if active { - Tool::BrowseZoom - } else { - Tool::Peaks - }); + app.toggle_tool(Tool::Peaks); } if active { ui.small( @@ -303,11 +299,7 @@ pub(super) fn integrate_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) { .on_hover_text("Drag across a multiplet to add an integral; drag its edges to adjust it.") .clicked() { - app.set_tool(if drawing { - Tool::BrowseZoom - } else { - Tool::Integrate - }); + app.toggle_tool(Tool::Integrate); } if drawing { ui.small( @@ -389,11 +381,7 @@ fn integrate_2d_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) { ) .clicked() { - app.set_tool(if drawing { - Tool::BrowseZoom - } else { - Tool::Integrate - }); + app.toggle_tool(Tool::Integrate); } if drawing { ui.small("Drag to add · corners/edges resize · interior moves · right-click for reference or delete."); diff --git a/crates/app/src/ui/tools/slice.rs b/crates/app/src/ui/tools/slice.rs index 10986d2..9ca89c3 100644 --- a/crates/app/src/ui/tools/slice.rs +++ b/crates/app/src/ui/tools/slice.rs @@ -31,11 +31,7 @@ pub(super) fn slice_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) { .on_hover_text("Turn on, then hover the plot to position the cut (S).") .clicked() { - app.set_tool(if active { - Tool::BrowseZoom - } else { - Tool::Slice - }); + app.toggle_tool(Tool::Slice); } if is_true_2d { diff --git a/crates/core/src/actions/tests/interaction.rs b/crates/core/src/actions/tests/interaction.rs index f0d81e8..3e325f7 100644 --- a/crates/core/src/actions/tests/interaction.rs +++ b/crates/core/src/actions/tests/interaction.rs @@ -163,6 +163,44 @@ fn switching_to_data_tool_resets_in_flight_interaction() { assert!(!app.interaction().is_active()); } +#[test] +fn toggling_manual_phase_cancels_the_in_flight_drag() { + use crate::actions::DatasetProcessingState; + use crate::state::{Interaction, PhaseAxis, PhaseDrag, PhaseDragKind, Tool}; + + let mut app = sample_app(); + app.set_tool(Tool::ManualPhase); + let before = DatasetProcessingState::from_dataset(&app.doc.datasets[0]); + let original_phase0 = app.doc.datasets[0] + .phase_params_mut(PhaseAxis::Direct) + .unwrap() + .phase0; + app.doc.datasets[0] + .phase_params_mut(PhaseAxis::Direct) + .unwrap() + .phase0 = original_phase0 + 0.5; + app.apply_dataset_edit(0); + app.set_interaction(Interaction::Phase(PhaseDrag { + kind: PhaseDragKind::Ph0, + dataset: 0, + axis: PhaseAxis::Direct, + preview_pivot_ppm: None, + gesture_before: before, + })); + + app.toggle_tool(Tool::ManualPhase); + + assert_eq!(app.session.tool, Tool::BrowseZoom); + assert!(!app.interaction().is_active()); + assert_eq!( + app.doc.datasets[0] + .phase_params_mut(PhaseAxis::Direct) + .unwrap() + .phase0, + original_phase0 + ); +} + #[test] fn switching_tools_preserves_selection() { use crate::state::{Selection, Tool}; diff --git a/crates/core/src/state/app_impl.rs b/crates/core/src/state/app_impl.rs index 06856bf..38cd1be 100644 --- a/crates/core/src/state/app_impl.rs +++ b/crates/core/src/state/app_impl.rs @@ -425,6 +425,18 @@ impl PlotxApp { } } + pub fn toggle_tool(&mut self, tool: Tool) { + let next = if self.session.tool == tool { + tool.rest() + } else { + tool + }; + if self.session.tool != next && self.interaction().is_active() { + self.cancel_interaction(); + } + self.set_tool(next); + } + /// Select a whole object in page space. Clicking a grouped member selects the /// whole group, with the clicked object primary. pub fn select_object(&mut self, ci: usize, id: ObjectId) { diff --git a/crates/core/src/state/units.rs b/crates/core/src/state/units.rs index d37e8df..2719d8c 100644 --- a/crates/core/src/state/units.rs +++ b/crates/core/src/state/units.rs @@ -185,6 +185,14 @@ impl Tool { ) } + pub fn rest(self) -> Self { + if self.is_data_tool() { + Self::BrowseZoom + } else { + Self::Select + } + } + pub fn is_layout_tool(self) -> bool { matches!(self, Tool::Select) } @@ -200,6 +208,19 @@ impl Tool { } } +#[cfg(test)] +mod tool_tests { + use super::Tool; + + #[test] + fn rest_uses_the_tool_family_neutral_tool() { + assert_eq!(Tool::Integrate.rest(), Tool::BrowseZoom); + assert_eq!(Tool::Text.rest(), Tool::Select); + assert_eq!(Tool::BrowseZoom.rest(), Tool::BrowseZoom); + assert_eq!(Tool::Select.rest(), Tool::Select); + } +} + /// A phaseable/processable frequency dimension. `Direct` is the 1D axis (and the /// direct axis of a pseudo-2D stack); `F2`/`F1` are the direct/indirect axes of a /// true-2D spectrum. Lets the phase panel and canvas drag address any dimension diff --git a/docs/src/content/docs/reference/shortcuts.md b/docs/src/content/docs/reference/shortcuts.md index 5f542e0..017144a 100644 --- a/docs/src/content/docs/reference/shortcuts.md +++ b/docs/src/content/docs/reference/shortcuts.md @@ -10,6 +10,10 @@ keyboard focus. Single keys, no modifiers: +Press the same key again to leave a tool. Analysis tools return to **Zoom**; +drawing tools such as **Text** and **Rectangle** return to **Select**. Pressing +`V` or `Z` again leaves that neutral tool active. + | Key | Tool | | --- | --- | | `V` | Select | @@ -54,7 +58,7 @@ the cursor, or on the board when the cursor is over empty space. | `Ctrl` + `Shift` + `G` | Ungroup | | `Delete` or `Backspace` | Delete the selected annotation objects; in the Peaks and Integrate tools, delete the selected peak or region | | `F2` | Rename the selected dataset or canvas | -| `Esc` | Cancel the active drag; otherwise step the selection back one level until nothing is selected | +| `Esc` | Cancel the active drag; further presses clear the Analysis Range and selections one at a time, then leave the active tool | | `Ctrl` + `C` | Copy the selected frame (or the active canvas) to the clipboard as bitmap + vector | | `Ctrl` + `Shift` + `V` | Paste a delimited table (comma, tab, or semicolon) from the clipboard as a new data table | | `Ctrl` + `,` | Open Preferences | diff --git a/docs/src/content/docs/zh-cn/reference/shortcuts.md b/docs/src/content/docs/zh-cn/reference/shortcuts.md index 0dc93c3..8688e9f 100644 --- a/docs/src/content/docs/zh-cn/reference/shortcuts.md +++ b/docs/src/content/docs/zh-cn/reference/shortcuts.md @@ -9,6 +9,10 @@ description: 键盘与鼠标快捷操作。 单键、无修饰键： +再次按下同一快捷键即可退出当前工具。分析工具会回到**缩放**；**文本**、 +**矩形**等绘图工具会回到**选择**。再次按 `V` 或 `Z` 时，对应的中性工具 +会保持激活。 + | 按键 | 工具 | | --- | --- | | `V` | 选择 | @@ -53,7 +57,7 @@ description: 键盘与鼠标快捷操作。 | `Ctrl` + `Shift` + `G` | 取消编组 | | `Delete` 或 `Backspace` | 删除所选标注对象；在峰 / 积分工具中删除所选的峰或区域 | | `F2` | 重命名所选数据集或画布 | -| `Esc` | 取消进行中的拖动；否则逐级回退选择，直至清空 | +| `Esc` | 取消进行中的拖动；继续按则依次清除分析范围和各级选择，最后退出当前工具 | | `Ctrl` + `C` | 把选中的图框（未选中时为活动画布）以位图 + 矢量格式复制到剪贴板 | | `Ctrl` + `Shift` + `V` | 把剪贴板中的分隔文本（逗号、制表符或分号）粘贴为新数据表 | | `Ctrl` + `,` | 打开首选项 |