From 66c441ac7cb86d656fab8c646e31f99aaa81bb8e Mon Sep 17 00:00:00 2001 From: Jiekang Tian Date: Fri, 24 Jul 2026 20:13:24 +0800 Subject: [PATCH] feat(core): address series, steps, and background jobs by stable identity Extends the stable identity family from datasets and canvases to the remaining mutable resources, so collection positions no longer cross an action, job, or persistence boundary. - SeriesBinding carries a SeriesId from a per-plot allocator; reordering a plot's series changes their order, not their identity. - StepId becomes owner-local, minted from a dataset-owned allocator that sits outside the processing undo snapshot and therefore never rewinds into reissuing a live id. ProcessingStep::new takes the identity as its first parameter, so every construction site must state where its id came from and appending a placeholder-id step to a live pipeline is a compile error rather than a duplicated step. - Dataset action payloads and the Ilt, Dosy and Process2D job/completion pairs carry DatasetId. Completions resolve their target by id and drop the result when the dataset is gone, so a long-running job can no longer install onto whichever dataset occupies the old position. - Allocator repair on load rejects an exhausted id space instead of defaulting to zero and aliasing an existing id on the next allocation. - Projects persist allocators and identities; scheme recipes omit step ids, which an adopting dataset remints on apply. Identities are read through checked lookups, so a stale index is inert rather than fatal. --- crates/app/src/ui/canvas/board.rs | 13 +- crates/app/src/ui/canvas/integrals.rs | 6 +- crates/app/src/ui/canvas/integrals2d.rs | 6 +- crates/app/src/ui/canvas/mod.rs | 4 +- crates/app/src/ui/canvas/regions.rs | 6 +- crates/app/src/ui/data_sheet.rs | 5 +- crates/app/src/ui/file_dialogs/tests.rs | 5 +- crates/app/src/ui/object_inspector.rs | 16 +- crates/app/src/ui/primary_sidebar.rs | 6 +- crates/app/src/ui/tools/mod.rs | 4 +- crates/app/src/ui/tools/processing/mod.rs | 62 +++++- crates/app/src/ui/tools/pseudo.rs | 12 +- crates/app/src/ui/tools/region_analysis.rs | 6 +- crates/core/src/actions/app_impl/mod.rs | 38 ++-- .../core/src/actions/app_impl/processing.rs | 46 +++- crates/core/src/actions/app_impl/revert.rs | 36 ++-- crates/core/src/actions/app_impl/validate.rs | 15 +- crates/core/src/actions/build.rs | 26 ++- crates/core/src/actions/mod.rs | 34 +-- crates/core/src/actions/processing_state.rs | 2 + crates/core/src/actions/tests/align.rs | 56 ++++- crates/core/src/actions/tests/board.rs | 14 +- .../core/src/actions/tests/integral_curve.rs | 14 +- crates/core/src/actions/tests/linefit.rs | 40 +++- crates/core/src/actions/tests/mod.rs | 14 +- crates/core/src/actions/tests/multiplet.rs | 10 +- crates/core/src/actions/tests/scheme_apply.rs | 61 ++++++ .../core/src/actions/tests/stable_identity.rs | 202 +++++++++++++++++- crates/core/src/automation/tests.rs | 6 +- crates/core/src/automation/tools.rs | 2 +- crates/core/src/automation/types.rs | 5 + crates/core/src/automation/types_tests.rs | 2 +- crates/core/src/data_export/tests.rs | 9 +- crates/core/src/export/precheck.rs | 1 + crates/core/src/project/cleanup_tests.rs | 3 +- crates/core/src/project/convert.rs | 69 ++---- crates/core/src/project/convert_dimensions.rs | 51 +++++ crates/core/src/project/convert_recipes.rs | 18 ++ crates/core/src/project/dto.rs | 41 ++-- crates/core/src/project/dto_tests.rs | 16 ++ crates/core/src/project/mod.rs | 4 + crates/core/src/project/pipeline_conv.rs | 26 ++- crates/core/src/project/reference_tests.rs | 60 ++++++ crates/core/src/project/scheme.rs | 65 +++++- .../core/src/project/step_identity_tests.rs | 88 ++++++++ crates/core/src/project/tests.rs | 57 +---- crates/core/src/state/app_impl_align.rs | 26 ++- crates/core/src/state/app_impl_analysis.rs | 8 +- crates/core/src/state/app_impl_compute.rs | 29 ++- .../core/src/state/app_impl_compute_tests.rs | 80 +++++++ crates/core/src/state/app_impl_linefit.rs | 12 +- crates/core/src/state/app_impl_multiplet.rs | 8 +- crates/core/src/state/app_impl_peaks.rs | 27 ++- crates/core/src/state/app_impl_slice.rs | 9 +- crates/core/src/state/app_impl_statistics.rs | 15 +- crates/core/src/state/compute.rs | 57 ++--- crates/core/src/state/compute/tests.rs | 94 +++++--- crates/core/src/state/datasets.rs | 78 ++++++- crates/core/src/state/document.rs | 5 + crates/core/src/state/mod.rs | 2 + crates/core/src/state/plot_object.rs | 36 +++- crates/core/src/state/stack.rs | 25 ++- crates/core/src/state/table_edit.rs | 2 +- crates/core/src/state/table_execution_job.rs | 32 ++- crates/core/src/state/ui_state.rs | 9 +- crates/core/src/workflow.rs | 1 + crates/processing/src/align.rs | 32 ++- crates/processing/src/fft.rs | 26 ++- crates/processing/src/lib.rs | 94 +++++--- crates/processing/src/tests.rs | 8 +- 70 files changed, 1580 insertions(+), 417 deletions(-) create mode 100644 crates/core/src/project/convert_dimensions.rs create mode 100644 crates/core/src/project/dto_tests.rs create mode 100644 crates/core/src/project/step_identity_tests.rs create mode 100644 crates/core/src/state/app_impl_compute_tests.rs diff --git a/crates/app/src/ui/canvas/board.rs b/crates/app/src/ui/canvas/board.rs index 2260a70..0c898be 100644 --- a/crates/app/src/ui/canvas/board.rs +++ b/crates/app/src/ui/canvas/board.rs @@ -618,10 +618,17 @@ fn handle_frame_drag(app: &mut PlotxApp, rect: egui::Rect, ui: &Ui) -> bool { app.reset_interaction(); if let Some(after) = frame_board_pos(app, drag.frame) { let action = match drag.frame { - FrameRef::Page(ci) => Action::move_canvas_on_board(ci, drag.before, after), - FrameRef::Sheet(di) => Action::move_sheet_on_board(di, drag.before, after), + FrameRef::Page(ci) => Some(Action::move_canvas_on_board(ci, drag.before, after)), + FrameRef::Sheet(di) => app + .doc + .datasets + .get(di) + .map(plotx_core::state::Dataset::resource_id) + .map(|id| Action::move_sheet_on_board(id, drag.before, after)), }; - app.execute_action(action); + if let Some(action) = action { + app.execute_action(action); + } } } diff --git a/crates/app/src/ui/canvas/integrals.rs b/crates/app/src/ui/canvas/integrals.rs index dfa5bc2..11098fd 100644 --- a/crates/app/src/ui/canvas/integrals.rs +++ b/crates/app/src/ui/canvas/integrals.rs @@ -274,7 +274,11 @@ fn finish_integral_drag(app: &mut PlotxApp, dataset: usize, xspan: f64) { .unwrap() .integrals .clone(); - app.execute_action(Action::set_integrals(dataset, drag.before, after)); + app.execute_action(Action::set_integrals( + app.doc.datasets[dataset].resource_id(), + drag.before, + after, + )); } fn integral_context_menu( diff --git a/crates/app/src/ui/canvas/integrals2d.rs b/crates/app/src/ui/canvas/integrals2d.rs index b61fc54..32424fe 100644 --- a/crates/app/src/ui/canvas/integrals2d.rs +++ b/crates/app/src/ui/canvas/integrals2d.rs @@ -413,7 +413,11 @@ fn finish_integral_2d_drag( .unwrap() .integrals .clone(); - app.execute_action(Action::set_integrals_2d(dataset, drag.before, after)); + app.execute_action(Action::set_integrals_2d( + app.doc.datasets[dataset].resource_id(), + drag.before, + after, + )); } fn integral_2d_context_menu( diff --git a/crates/app/src/ui/canvas/mod.rs b/crates/app/src/ui/canvas/mod.rs index 4830bb6..305dce5 100644 --- a/crates/app/src/ui/canvas/mod.rs +++ b/crates/app/src/ui/canvas/mod.rs @@ -591,6 +591,7 @@ mod tests { visible: true, group: None, kind: CanvasObjectKind::Plot(Box::new(PlotObject { + next_series_id: plotx_core::state::SeriesId::new(1), binding: plotx_core::state::DataBinding::single(DatasetId::new()), chart: plotx_core::state::ChartSpec::default(), stack: plotx_core::state::StackSpec::default(), @@ -623,6 +624,7 @@ mod tests { visible: true, group: None, kind: CanvasObjectKind::Plot(Box::new(PlotObject { + next_series_id: plotx_core::state::SeriesId::new(1), binding: plotx_core::state::DataBinding::single(DatasetId::new()), chart: plotx_core::state::ChartSpec::default(), stack: plotx_core::state::StackSpec::default(), @@ -734,7 +736,7 @@ mod tests { app.sync_phase_interaction(); assert_eq!(count(&mut app), 0, "no pivot before the Phase editor opens"); - app.session.ui.proc_expanded_step = Some(phase_id); + app.session.ui.proc_expanded_step = Some((app.doc.datasets[0].resource_id(), phase_id)); app.sync_phase_interaction(); assert_eq!(app.session.tool, Tool::ManualPhase); assert!( diff --git a/crates/app/src/ui/canvas/regions.rs b/crates/app/src/ui/canvas/regions.rs index d47f7c5..790b2a1 100644 --- a/crates/app/src/ui/canvas/regions.rs +++ b/crates/app/src/ui/canvas/regions.rs @@ -250,5 +250,9 @@ fn finish_region_drag(app: &mut PlotxApp, dataset: usize, xspan: f64) { .unwrap() .regions .clone(); - app.execute_action(Action::set_regions(dataset, drag.before, after)); + app.execute_action(Action::set_regions( + app.doc.datasets[dataset].resource_id(), + drag.before, + after, + )); } diff --git a/crates/app/src/ui/data_sheet.rs b/crates/app/src/ui/data_sheet.rs index f8187ad..25faa3a 100644 --- a/crates/app/src/ui/data_sheet.rs +++ b/crates/app/src/ui/data_sheet.rs @@ -82,7 +82,10 @@ pub(super) fn data_sheet_window(app: &mut PlotxApp, ctx: &egui::Context) { }); if let Some(delta) = commit { let typed_diagnostic = delta.typed_diagnostic.clone(); - app.execute_action(Action::edit_table(di, delta)); + app.execute_action(Action::edit_table( + app.doc.datasets[di].resource_id(), + delta, + )); app.session.status = typed_diagnostic.unwrap_or_else(|| "Edited data table.".to_owned()); } if let Some(request) = transform diff --git a/crates/app/src/ui/file_dialogs/tests.rs b/crates/app/src/ui/file_dialogs/tests.rs index 3196835..3f648be 100644 --- a/crates/app/src/ui/file_dialogs/tests.rs +++ b/crates/app/src/ui/file_dialogs/tests.rs @@ -48,7 +48,10 @@ fn mixed_columns_import_as_typed_text_without_being_discarded() { assert!(delta.typed_diagnostic.is_none()); delta }; - app.execute_action(plotx_core::actions::Action::edit_table(0, delta)); + app.execute_action(plotx_core::actions::Action::edit_table( + app.doc.datasets[0].resource_id(), + delta, + )); let edited_fingerprint = app.doc.datasets[0] .as_table() .unwrap() diff --git a/crates/app/src/ui/object_inspector.rs b/crates/app/src/ui/object_inspector.rs index 87f0b18..481b8bd 100644 --- a/crates/app/src/ui/object_inspector.rs +++ b/crates/app/src/ui/object_inspector.rs @@ -258,8 +258,20 @@ fn data_section(app: &mut PlotxApp, ci: usize, object: ObjectId, ui: &mut Ui) { let label = app.doc.datasets[*di].display_name(); if ui.selectable_label(false, label).clicked() { let mut b = binding.clone(); - b.series - .push(SeriesBinding::new(app.doc.datasets[*di].resource_id())); + let Some(series_id) = app + .doc + .canvases + .get_mut(ci) + .and_then(|canvas| canvas.object_mut(object)) + .and_then(|object| object.plot_mut()) + .map(|plot| plot.allocate_series_id()) + else { + continue; + }; + let mut series = + SeriesBinding::new(app.doc.datasets[*di].resource_id()); + series.id = series_id; + b.series.push(series); next_binding = Some(b); } } diff --git a/crates/app/src/ui/primary_sidebar.rs b/crates/app/src/ui/primary_sidebar.rs index a7e4282..b521731 100644 --- a/crates/app/src/ui/primary_sidebar.rs +++ b/crates/app/src/ui/primary_sidebar.rs @@ -618,7 +618,11 @@ fn apply_browser_event(app: &mut PlotxApp, ui: &Ui, event: Option) let trimmed = name.trim(); let before = app.doc.datasets[di].name(); let after = (!trimmed.is_empty()).then(|| trimmed.to_owned()); - app.execute_action(Action::rename_dataset(di, before, after)); + app.execute_action(Action::rename_dataset( + app.doc.datasets[di].resource_id(), + before, + after, + )); app.session.ui.rename = None; } Some(BrowserEvent::RenameCancel) => app.session.ui.rename = None, diff --git a/crates/app/src/ui/tools/mod.rs b/crates/app/src/ui/tools/mod.rs index c0233cb..a8912b0 100644 --- a/crates/app/src/ui/tools/mod.rs +++ b/crates/app/src/ui/tools/mod.rs @@ -511,7 +511,7 @@ pub(super) fn begin_processing_widget( ) { if resp.drag_started() { app.session.ui.processing_edit = Some(PendingProcessingEdit { - dataset: di, + dataset: app.doc.datasets[di].resource_id(), before, }); } @@ -532,7 +532,7 @@ pub(super) fn commit_processing_widget( .ui .processing_edit .take() - .filter(|edit| edit.dataset == di) + .filter(|edit| edit.dataset == app.doc.datasets[di].resource_id()) .map(|edit| edit.before) .unwrap_or(fallback_before); let after = DatasetProcessingState::from_dataset(&app.doc.datasets[di]); diff --git a/crates/app/src/ui/tools/processing/mod.rs b/crates/app/src/ui/tools/processing/mod.rs index 147e3f8..cce7f2e 100644 --- a/crates/app/src/ui/tools/processing/mod.rs +++ b/crates/app/src/ui/tools/processing/mod.rs @@ -7,7 +7,7 @@ mod editors; use egui::{Button, Ui}; use egui_phosphor::regular as icon; use plotx_core::actions::DatasetProcessingState; -use plotx_core::state::{Dataset, PhaseAxis, PlotxApp}; +use plotx_core::state::{Dataset, DatasetId, PhaseAxis, PlotxApp}; use plotx_processing::{ Apodization, AutoPhaseMethod, AxisPipeline, BaselineMethod, BinParams, NormalizeMethod, PhaseParams, ProcessingStep, ReferenceParams, SmoothMethod, StepDomain, StepId, StepKind, @@ -101,6 +101,9 @@ fn step_list(app: &mut PlotxApp, di: usize, axis: PhaseAxis, ui: &mut Ui) { return; }; + let Some(owner) = app.doc.datasets.get(di).map(Dataset::resource_id) else { + return; + }; let last = steps.len().saturating_sub(1); let mut op: Option<(StepId, RowOp)> = None; for (i, step) in steps.iter().enumerate() { @@ -108,7 +111,7 @@ fn step_list(app: &mut PlotxApp, di: usize, axis: PhaseAxis, ui: &mut Ui) { fft_anchor(ui); continue; } - row(app, di, axis, step, i == 0, i == last, ui, &mut op); + row(app, di, owner, axis, step, i == 0, i == last, ui, &mut op); } if let Some((id, o)) = op { apply_row_op(app, di, axis, id, o); @@ -132,6 +135,7 @@ fn fft_anchor(ui: &mut Ui) { fn row( app: &mut PlotxApp, di: usize, + owner: DatasetId, axis: PhaseAxis, step: &ProcessingStep, first: bool, @@ -140,7 +144,7 @@ fn row( op: &mut Option<(StepId, RowOp)>, ) { let id = step.id; - let expanded = app.session.ui.proc_expanded_step == Some(id); + let expanded = app.session.ui.proc_expanded_step == Some((owner, id)); ui.horizontal(|ui| { ui.weak(icon::DOTS_SIX_VERTICAL); let mut enabled = step.enabled; @@ -152,14 +156,14 @@ fn row( .selectable_label(expanded, editors::kind_label(&step.kind)) .clicked() { - app.session.ui.proc_expanded_step = if expanded { None } else { Some(id) }; + app.session.ui.proc_expanded_step = if expanded { None } else { Some((owner, id)) }; } if step.source == StepSource::User { ui.weak("•"); } ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { ui.menu_button(icon::DOTS_THREE, |ui| { - row_menu(app, id, first, last, op, ui) + row_menu(app, owner, id, first, last, op, ui) }); ui.weak(editors::kind_summary(&step.kind)); }); @@ -172,8 +176,10 @@ fn row( } } +#[allow(clippy::too_many_arguments)] fn row_menu( app: &mut PlotxApp, + owner: DatasetId, id: StepId, first: bool, last: bool, @@ -181,7 +187,7 @@ fn row_menu( ui: &mut Ui, ) { if ui.button("Edit").clicked() { - app.session.ui.proc_expanded_step = Some(id); + app.session.ui.proc_expanded_step = Some((owner, id)); ui.close(); } if ui.button(format!("{} Duplicate", icon::COPY)).clicked() { @@ -409,15 +415,31 @@ fn set_phase_method( } fn apply_row_op(app: &mut PlotxApp, di: usize, axis: PhaseAxis, id: StepId, op: RowOp) { - let before = DatasetProcessingState::from_dataset(&app.doc.datasets[di]); + let Some(dataset) = app.doc.datasets.get(di) else { + return; + }; + let before = DatasetProcessingState::from_dataset(dataset); let mut after = before.clone(); + let duplicate_id = if matches!(op, RowOp::Duplicate) { + match allocate_step_id(app, di) { + Some(id) => Some(id), + // Only spectral datasets expose processing rows; a stale index or a + // non-spectral dataset is a no-op, not a crash. + None => return, + } + } else { + None + }; if let Some(pipe) = state_pipe(&mut after, axis) && let Some(idx) = pipe.steps.iter().position(|s| s.id == id) { match op { RowOp::Duplicate => { + let Some(duplicate_id) = duplicate_id else { + return; + }; let mut clone = pipe.steps[idx].clone(); - clone.id = StepId::fresh(); + clone.id = duplicate_id; clone.source = StepSource::User; pipe.steps.insert(idx + 1, clone); } @@ -445,8 +467,24 @@ fn apply_row_op(app: &mut PlotxApp, di: usize, axis: PhaseAxis, id: StepId, op: app.commit_processing_edit(di, before, after); } +/// Reserve a step identity from the dataset that will own it. `None` for a +/// stale index or a dataset kind with no processing pipeline. +fn allocate_step_id(app: &mut PlotxApp, di: usize) -> Option { + match app.doc.datasets.get_mut(di)? { + Dataset::Nmr(dataset) => Some(dataset.allocate_step_id()), + Dataset::Nmr2D(dataset) => Some(dataset.allocate_step_id()), + _ => None, + } +} + fn add_step(app: &mut PlotxApp, di: usize, axis: PhaseAxis, kind: StepKind) { - let before = DatasetProcessingState::from_dataset(&app.doc.datasets[di]); + let Some(dataset) = app.doc.datasets.get(di) else { + return; + }; + let before = DatasetProcessingState::from_dataset(dataset); + let Some(id) = allocate_step_id(app, di) else { + return; + }; let mut after = before.clone(); if let Some(pipe) = state_pipe(&mut after, axis) { let fft = pipe @@ -458,7 +496,7 @@ fn add_step(app: &mut PlotxApp, di: usize, axis: PhaseAxis, kind: StepKind) { _ => pipe.steps.len(), }; pipe.steps - .insert(at, ProcessingStep::new(kind, StepSource::User)); + .insert(at, ProcessingStep::new(id, kind, StepSource::User)); } app.commit_processing_edit(di, before, after); } @@ -470,7 +508,9 @@ fn reset_to_default(app: &mut PlotxApp, di: usize) { app.session.ui.proc_pending = None; let before = DatasetProcessingState::from_dataset(&app.doc.datasets[di]); app.execute_action(plotx_core::actions::Action::update_dataset_processing( - di, before, after, + app.doc.datasets[di].resource_id(), + before, + after, )); } diff --git a/crates/app/src/ui/tools/pseudo.rs b/crates/app/src/ui/tools/pseudo.rs index 5e16923..d4a3e6b 100644 --- a/crates/app/src/ui/tools/pseudo.rs +++ b/crates/app/src/ui/tools/pseudo.rs @@ -20,7 +20,11 @@ pub(super) fn experiment_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) -> bo *preset = chosen; params.layout = chosen.layout(); } - app.execute_action(Action::update_dataset_processing(di, before, after)); + app.execute_action(Action::update_dataset_processing( + app.doc.datasets[di].resource_id(), + before, + after, + )); } let is_stack = { @@ -279,7 +283,11 @@ fn pseudo_group(app: &mut PlotxApp, di: usize, ui: &mut Ui) { } } - let progress = app.session.compute.dosy_progress(di); + let progress = app + .doc + .datasets + .get(di) + .and_then(|dataset| app.session.compute.dosy_progress(dataset.resource_id())); ui.horizontal(|ui| { if is_dosy && !is_ilt diff --git a/crates/app/src/ui/tools/region_analysis.rs b/crates/app/src/ui/tools/region_analysis.rs index 785ff34..f7850ba 100644 --- a/crates/app/src/ui/tools/region_analysis.rs +++ b/crates/app/src/ui/tools/region_analysis.rs @@ -278,7 +278,11 @@ fn region_task_body(app: &mut PlotxApp, di: usize, ui: &mut Ui) { } if name_lost && let Some(before) = app.session.ui.region_edit_before.take() { let after = app.doc.datasets[di].as_nmr2d().unwrap().regions.clone(); - app.execute_action(Action::set_regions(di, before, after)); + app.execute_action(Action::set_regions( + app.doc.datasets[di].resource_id(), + before, + after, + )); } if let Some((id, m)) = metric_change { app.edit_regions(di, |regions, _| { diff --git a/crates/core/src/actions/app_impl/mod.rs b/crates/core/src/actions/app_impl/mod.rs index e426504..99d78c8 100644 --- a/crates/core/src/actions/app_impl/mod.rs +++ b/crates/core/src/actions/app_impl/mod.rs @@ -107,7 +107,6 @@ impl PlotxApp { self.recompute_integrals_2d_after_processing(dataset); self.rebuild_canvases_for(dataset); } - pub fn finish_pending_wheel_zoom(&mut self, now: f64, force: bool) { let Some(pending) = self.session.ui.wheel_zoom.clone() else { return; @@ -131,8 +130,15 @@ impl PlotxApp { ); } } - fn apply_action(&mut self, action: &Action) { + macro_rules! dataset_index { + ($id:expr) => { + match self.doc.dataset_index($id) { + Some(index) => index, + None => return, + } + }; + } match action { Action::Composite(actions) => { for action in actions { @@ -140,7 +146,7 @@ impl PlotxApp { } } Action::UpdateDatasetProcessing { dataset, after, .. } => { - self.set_dataset_processing_state(*dataset, after); + self.set_dataset_processing_state(dataset_index!(*dataset), after); } Action::SetObjectViewport { canvas, @@ -186,10 +192,11 @@ impl PlotxApp { } } Action::MoveSheetOnBoard { dataset, after, .. } => { + let dataset = dataset_index!(*dataset); if let Some(t) = self .doc .datasets - .get_mut(*dataset) + .get_mut(dataset) .and_then(Dataset::as_table_mut) { t.board_pos = *after; @@ -294,39 +301,40 @@ impl PlotxApp { } } Action::RenameDataset { dataset, after, .. } => { - if let Some(d) = self.doc.datasets.get_mut(*dataset) { + let dataset = dataset_index!(*dataset); + if let Some(d) = self.doc.datasets.get_mut(dataset) { d.set_name(after.clone()); } } Action::SetCurveFitAnalyses { dataset, after, .. } => { - self.set_curve_fit_analyses(*dataset, after); + self.set_curve_fit_analyses(dataset_index!(*dataset), after); } Action::EditTable { dataset, delta } => { - self.apply_table_edit(*dataset, delta, true); + self.apply_table_edit(dataset_index!(*dataset), delta, true); } Action::SetTypedTableState { dataset, after, .. } => { - self.set_typed_table_state(*dataset, after); + self.set_typed_table_state(dataset_index!(*dataset), after); } Action::SetRegions { dataset, after, .. } => { - self.set_regions(*dataset, after); + self.set_regions(dataset_index!(*dataset), after); } Action::SetIntegrals { dataset, after, .. } => { - self.set_integrals(*dataset, after); + self.set_integrals(dataset_index!(*dataset), after); } Action::SetIntegrals2D { dataset, after, .. } => { - self.set_integrals_2d(*dataset, after); + self.set_integrals_2d(dataset_index!(*dataset), after); } Action::SetPeaks { dataset, after, .. } => { - self.set_peaks(*dataset, after); + self.set_peaks(dataset_index!(*dataset), after); } Action::SetLineFits { dataset, after, .. } => { - self.set_line_fits(*dataset, after); + self.set_line_fits(dataset_index!(*dataset), after); } Action::SetMultiplets { dataset, after, .. } => { - self.set_multiplets(*dataset, after); + self.set_multiplets(dataset_index!(*dataset), after); } Action::SetTableStatistics { dataset, after, .. } => { - self.set_table_statistics(*dataset, after); + self.set_table_statistics(dataset_index!(*dataset), after); } Action::InsertObject { canvas, object, .. } => { self.insert_object_value(*canvas, object.as_ref().clone()); diff --git a/crates/core/src/actions/app_impl/processing.rs b/crates/core/src/actions/app_impl/processing.rs index c5974b9..ddfcf7d 100644 --- a/crates/core/src/actions/app_impl/processing.rs +++ b/crates/core/src/actions/app_impl/processing.rs @@ -53,12 +53,15 @@ impl PlotxApp { before: DatasetProcessingState, after: DatasetProcessingState, ) { + let Some(dataset_id) = self.doc.datasets.get(dataset).map(Dataset::resource_id) else { + return; + }; if self .session .ui .processing_session .as_ref() - .is_some_and(|edit| edit.dataset == dataset) + .is_some_and(|edit| edit.dataset == dataset_id) && !self.session.ui.proc_paused { if DatasetProcessingState::from_dataset(&self.doc.datasets[dataset]) != after { @@ -69,10 +72,10 @@ impl PlotxApp { if self.session.ui.proc_paused { self.set_recipe_no_recompute(dataset, &after); if self.session.ui.proc_pending.is_none() { - self.session.ui.proc_pending = Some((dataset, before)); + self.session.ui.proc_pending = Some((dataset_id, before)); } } else { - self.execute_action(Action::update_dataset_processing(dataset, before, after)); + self.execute_action(Action::update_dataset_processing(dataset_id, before, after)); } } @@ -83,19 +86,22 @@ impl PlotxApp { /// Start a processing transaction whose live edits may come from multiple UI /// surfaces. Re-entering the same dataset preserves the original snapshot. pub fn begin_processing_session(&mut self, dataset: usize) { + let Some(dataset_id) = self.doc.datasets.get(dataset).map(Dataset::resource_id) else { + return; + }; if self .session .ui .processing_session .as_ref() - .is_some_and(|edit| edit.dataset == dataset) + .is_some_and(|edit| edit.dataset == dataset_id) { return; } self.finish_processing_session(); if let Some(current) = self.doc.datasets.get(dataset) { self.session.ui.processing_session = Some(PendingProcessingEdit { - dataset, + dataset: dataset_id, before: DatasetProcessingState::from_dataset(current), }); } @@ -107,9 +113,10 @@ impl PlotxApp { let Some(edit) = self.session.ui.processing_session.take() else { return; }; - let Some(dataset) = self.doc.datasets.get(edit.dataset) else { + let Some(dataset_index) = self.doc.dataset_index(edit.dataset) else { return; }; + let dataset = &self.doc.datasets[dataset_index]; let after = DatasetProcessingState::from_dataset(dataset); let action = Action::update_dataset_processing(edit.dataset, edit.before, after); if let Err(error) = self.record_applied_processing_action(action) { @@ -121,10 +128,13 @@ impl PlotxApp { let Some((dataset, before)) = self.session.ui.proc_pending.take() else { return; }; - let after = DatasetProcessingState::from_dataset(&self.doc.datasets[dataset]); + let Some(dataset_index) = self.doc.dataset_index(dataset) else { + return; + }; + let after = DatasetProcessingState::from_dataset(&self.doc.datasets[dataset_index]); // Restore the pre-edit recipe so the commit sees a real diff and picks // retransform vs rebuild correctly, then apply the accumulated recipe. - self.set_recipe_no_recompute(dataset, &before); + self.set_recipe_no_recompute(dataset_index, &before); self.execute_action(Action::update_dataset_processing(dataset, before, after)); } @@ -135,6 +145,9 @@ impl PlotxApp { let Some((dataset, before)) = self.session.ui.proc_pending.take() else { return; }; + let Some(dataset) = self.doc.dataset_index(dataset) else { + return; + }; self.set_recipe_no_recompute(dataset, &before); self.session.status = "Discarded pending processing changes.".to_owned(); } @@ -147,9 +160,15 @@ impl PlotxApp { } fn phase_editor_dataset(&self) -> Option { - let id = self.session.ui.proc_expanded_step?; + let (owner, id) = self.session.ui.proc_expanded_step?; let di = self.active_dataset()?; - let dataset = &self.doc.datasets[di]; + let dataset = self.doc.datasets.get(di)?; + // The expanded row belongs to one dataset. Without this check a step + // that merely shares its owner-local number would read as "the user is + // phasing" the moment the active dataset changes. + if dataset.resource_id() != owner { + return None; + } dataset .phase_axes() .iter() @@ -175,7 +194,12 @@ impl PlotxApp { .ui .processing_session .as_ref() - .is_some_and(|edit| edit.dataset == dataset) + .is_some_and(|edit| { + self.doc + .datasets + .get(dataset) + .is_some_and(|value| value.resource_id() == edit.dataset) + }) }); if phasing == self.session.ui.phase_edit_active && (!phasing || session_matches) { return; diff --git a/crates/core/src/actions/app_impl/revert.rs b/crates/core/src/actions/app_impl/revert.rs index 6ddb343..509aa50 100644 --- a/crates/core/src/actions/app_impl/revert.rs +++ b/crates/core/src/actions/app_impl/revert.rs @@ -3,6 +3,14 @@ use crate::state::{Dataset, Interaction, PlotxApp, Selection}; impl PlotxApp { pub(super) fn revert_action(&mut self, action: &Action) { + macro_rules! dataset_index { + ($id:expr) => { + match self.doc.dataset_index($id) { + Some(index) => index, + None => return, + } + }; + } match action { Action::Composite(actions) => { for action in actions.iter().rev() { @@ -12,7 +20,7 @@ impl PlotxApp { Action::UpdateDatasetProcessing { dataset, before, .. } => { - self.set_dataset_processing_state(*dataset, before); + self.set_dataset_processing_state(dataset_index!(*dataset), before); } Action::SetObjectViewport { canvas, @@ -60,10 +68,11 @@ impl PlotxApp { Action::MoveSheetOnBoard { dataset, before, .. } => { + let dataset = dataset_index!(*dataset); if let Some(t) = self .doc .datasets - .get_mut(*dataset) + .get_mut(dataset) .and_then(Dataset::as_table_mut) { t.board_pos = *before; @@ -170,57 +179,58 @@ impl PlotxApp { Action::RenameDataset { dataset, before, .. } => { - if let Some(d) = self.doc.datasets.get_mut(*dataset) { + let dataset = dataset_index!(*dataset); + if let Some(d) = self.doc.datasets.get_mut(dataset) { d.set_name(before.clone()); } } Action::SetCurveFitAnalyses { dataset, before, .. } => { - self.set_curve_fit_analyses(*dataset, before); + self.set_curve_fit_analyses(dataset_index!(*dataset), before); } Action::EditTable { dataset, delta } => { - self.apply_table_edit(*dataset, delta, false); + self.apply_table_edit(dataset_index!(*dataset), delta, false); } Action::SetTypedTableState { dataset, before, .. } => { - self.set_typed_table_state(*dataset, before); + self.set_typed_table_state(dataset_index!(*dataset), before); } Action::SetRegions { dataset, before, .. } => { - self.set_regions(*dataset, before); + self.set_regions(dataset_index!(*dataset), before); } Action::SetIntegrals { dataset, before, .. } => { - self.set_integrals(*dataset, before); + self.set_integrals(dataset_index!(*dataset), before); } Action::SetIntegrals2D { dataset, before, .. } => { - self.set_integrals_2d(*dataset, before); + self.set_integrals_2d(dataset_index!(*dataset), before); } Action::SetPeaks { dataset, before, .. } => { - self.set_peaks(*dataset, before); + self.set_peaks(dataset_index!(*dataset), before); } Action::SetLineFits { dataset, before, .. } => { - self.set_line_fits(*dataset, before); + self.set_line_fits(dataset_index!(*dataset), before); } Action::SetMultiplets { dataset, before, .. } => { - self.set_multiplets(*dataset, before); + self.set_multiplets(dataset_index!(*dataset), before); } Action::SetTableStatistics { dataset, before, .. } => { - self.set_table_statistics(*dataset, before); + self.set_table_statistics(dataset_index!(*dataset), before); } Action::InsertObject { canvas, diff --git a/crates/core/src/actions/app_impl/validate.rs b/crates/core/src/actions/app_impl/validate.rs index 5536f80..1ce5979 100644 --- a/crates/core/src/actions/app_impl/validate.rs +++ b/crates/core/src/actions/app_impl/validate.rs @@ -6,9 +6,15 @@ pub enum ActionApplyError { StaleTarget(String), } +/// The document shape a composite has projected so far. Validation runs before +/// anything is applied, so a child action that targets a dataset an earlier +/// child inserts must be judged against this projection, not against the live +/// document. pub(super) struct ValidationShape { datasets: usize, canvases: usize, + /// Datasets inserted by earlier children of the composite under validation. + inserted: Vec, } impl ValidationShape { @@ -16,8 +22,13 @@ impl ValidationShape { Self { datasets: app.doc.datasets.len(), canvases: app.doc.canvases.len(), + inserted: Vec::new(), } } + + fn has_dataset(&self, app: &PlotxApp, id: crate::state::DatasetId) -> bool { + app.doc.dataset_index(id).is_some() || self.inserted.contains(&id) + } } pub(super) fn validate_action( @@ -32,7 +43,7 @@ pub(super) fn validate_action( } } Action::RenameDataset { dataset, .. } | Action::UpdateDatasetProcessing { dataset, .. } => { - if *dataset >= shape.datasets { + if !shape.has_dataset(app, *dataset) { return Err(ActionApplyError::StaleTarget(format!("dataset {dataset}"))); } } @@ -49,6 +60,7 @@ pub(super) fn validate_action( dataset_index, canvas_index, inserted_into_existing_canvas, + dataset, .. } => { if *dataset_index != shape.datasets { @@ -69,6 +81,7 @@ pub(super) fn validate_action( shape.canvases += 1; } shape.datasets += 1; + shape.inserted.push(dataset.resource_id()); } Action::DeleteCanvas { index, .. } => { if *index >= shape.canvases { diff --git a/crates/core/src/actions/build.rs b/crates/core/src/actions/build.rs index 11fb50d..e9ebd59 100644 --- a/crates/core/src/actions/build.rs +++ b/crates/core/src/actions/build.rs @@ -13,7 +13,7 @@ use crate::{Integral2D, IntegralResult}; impl Action { pub fn update_dataset_processing( - dataset: usize, + dataset: DatasetId, before: DatasetProcessingState, after: DatasetProcessingState, ) -> Self { @@ -135,7 +135,7 @@ impl Action { } } - pub fn move_sheet_on_board(dataset: usize, before: [f32; 2], after: [f32; 2]) -> Self { + pub fn move_sheet_on_board(dataset: DatasetId, before: [f32; 2], after: [f32; 2]) -> Self { Self::MoveSheetOnBoard { dataset, before, @@ -258,7 +258,11 @@ impl Action { } } - pub fn rename_dataset(dataset: usize, before: Option, after: Option) -> Self { + pub fn rename_dataset( + dataset: DatasetId, + before: Option, + after: Option, + ) -> Self { Self::RenameDataset { dataset, before, @@ -267,7 +271,7 @@ impl Action { } pub fn set_curve_fit_analyses( - dataset: usize, + dataset: DatasetId, before: (Vec>, Vec), after: (Vec>, Vec), ) -> Self { @@ -278,14 +282,14 @@ impl Action { } } - pub fn edit_table(dataset: usize, delta: TableEditDelta) -> Self { + pub fn edit_table(dataset: DatasetId, delta: TableEditDelta) -> Self { Self::EditTable { dataset, delta: Box::new(delta), } } - pub fn set_regions(dataset: usize, before: Vec, after: Vec) -> Self { + pub fn set_regions(dataset: DatasetId, before: Vec, after: Vec) -> Self { Self::SetRegions { dataset, before, @@ -294,7 +298,7 @@ impl Action { } pub fn set_integrals( - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, ) -> Self { @@ -306,7 +310,7 @@ impl Action { } pub fn set_integrals_2d( - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, ) -> Self { @@ -318,7 +322,7 @@ impl Action { } pub fn set_peaks( - dataset: usize, + dataset: DatasetId, before: crate::state::PeakSet, after: crate::state::PeakSet, ) -> Self { @@ -330,7 +334,7 @@ impl Action { } pub fn set_line_fits( - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, ) -> Self { @@ -342,7 +346,7 @@ impl Action { } pub fn set_multiplets( - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, ) -> Self { diff --git a/crates/core/src/actions/mod.rs b/crates/core/src/actions/mod.rs index 378f3e9..d3833e6 100644 --- a/crates/core/src/actions/mod.rs +++ b/crates/core/src/actions/mod.rs @@ -1,9 +1,9 @@ use crate::layout::PageLayout; use crate::state::{ AxisOverrides, AxisProjections, CanvasDocument, CanvasObject, CanvasViewport, ChartSpec, - CurveFitReference, DataBinding, Dataset, NamedView, ObjectFrame, ObjectId, ObjectStyle, - PanelLabelStyle, PanelMeta, PlotxApp, PrimaryView, Region, Selection, StackSpec, StatAnalysis, - StoredCurveFitAnalysis, StoredLineFit, StoredMultiplet, TableEditDelta, TextBox, + CurveFitReference, DataBinding, Dataset, DatasetId, NamedView, ObjectFrame, ObjectId, + ObjectStyle, PanelLabelStyle, PanelMeta, PlotxApp, PrimaryView, Region, Selection, StackSpec, + StatAnalysis, StoredCurveFitAnalysis, StoredLineFit, StoredMultiplet, TableEditDelta, TextBox, TypedTableState, }; use crate::theme::ThemeSnapshot; @@ -73,7 +73,7 @@ pub struct PendingCanvasSizeEdit { #[derive(Clone)] pub struct PendingProcessingEdit { - pub dataset: usize, + pub dataset: DatasetId, pub before: DatasetProcessingState, } @@ -97,7 +97,7 @@ pub struct PendingInspectorEdit { pub enum Action { Composite(Vec), UpdateDatasetProcessing { - dataset: usize, + dataset: DatasetId, before: DatasetProcessingState, after: DatasetProcessingState, }, @@ -147,7 +147,7 @@ pub enum Action { after: [f32; 2], }, MoveSheetOnBoard { - dataset: usize, + dataset: DatasetId, before: [f32; 2], after: [f32; 2], }, @@ -249,23 +249,23 @@ pub enum Action { after: PanelLabelStyle, }, RenameDataset { - dataset: usize, + dataset: DatasetId, before: Option, after: Option, }, /// Replace a table's analysis snapshots and per-column references atomically. SetCurveFitAnalyses { - dataset: usize, + dataset: DatasetId, before: (Vec>, Vec), after: (Vec>, Vec), }, /// Apply a stable-identity incremental table transaction. EditTable { - dataset: usize, + dataset: DatasetId, delta: Box, }, SetTypedTableState { - dataset: usize, + dataset: DatasetId, before: Box, after: Box, }, @@ -273,44 +273,44 @@ pub enum Action { /// resize / rename / delete). The linked series table is re-derived on apply /// and undo, so it stays consistent without a second action. SetRegions { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, SetIntegrals { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, /// Replace a true-2D dataset's rectangular volume measurements as one /// undoable edit (create, geometry, metadata, reference, or deletion). SetIntegrals2D { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, /// Replace a dataset's peak set (detector recipe, hand-placed marks, and /// suppressed detections) as one undoable step. SetPeaks { - dataset: usize, + dataset: DatasetId, before: crate::state::PeakSet, after: crate::state::PeakSet, }, /// Replace a 1D dataset's stored lineshape deconvolutions as one undoable step. SetLineFits { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, /// Replace a 1D NMR dataset's stored multiplet analyses as one undoable step. SetMultiplets { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, /// Replace a table dataset's stored statistics analyses as one undoable step. SetTableStatistics { - dataset: usize, + dataset: DatasetId, before: Vec, after: Vec, }, diff --git a/crates/core/src/actions/processing_state.rs b/crates/core/src/actions/processing_state.rs index a7783b9..bfaf2b6 100644 --- a/crates/core/src/actions/processing_state.rs +++ b/crates/core/src/actions/processing_state.rs @@ -39,6 +39,7 @@ impl DatasetProcessingState { n.group_delay_correct, ); n.pipeline = pipeline.clone(); + n.repair_step_allocator(); n.group_delay_correct = *group_delay_correct; let rebuild = if full { n.retransform(); @@ -53,6 +54,7 @@ impl DatasetProcessingState { (Dataset::Nmr2D(n), Self::Nmr2D { params, preset }) => { let full = plotx_processing::needs_retransform_2d(params, &n.params); n.params = params.clone(); + n.repair_step_allocator(); n.preset = *preset; if full { n.retransform(); diff --git a/crates/core/src/actions/tests/align.rs b/crates/core/src/actions/tests/align.rs index 964c8de..0ef5d44 100644 --- a/crates/core/src/actions/tests/align.rs +++ b/crates/core/src/actions/tests/align.rs @@ -175,7 +175,7 @@ fn pending_paused_processing_blocks_alignment() { let mut app = app_with(&[2.0, 2.5]); app.session.ui.proc_paused = true; app.session.ui.proc_pending = Some(( - 0, + dataset_id(&app, 0), crate::actions::DatasetProcessingState::from_dataset(&app.doc.datasets[0]), )); @@ -195,3 +195,57 @@ fn empty_selection_scopes_to_all_datasets() { assert_eq!(plan.shift_count(), 2); assert!(app.can_align_spectra()); } + +/// P0-1 regression. `apply_reference_shift` appends a step to a *live* recipe, +/// so its identity has to come from the owning dataset's allocator. When the +/// step was minted with template-local numbering it landed on `StepId(0)`, +/// aliasing the pipeline's first row: the Processing panel resolves rows with +/// `position(|s| s.id == id)`, so deleting the new Reference row deleted +/// Apodize instead. Reverting the fix makes the uniqueness assertion fail. +#[test] +fn reference_alignment_gives_the_appended_step_a_unique_identity() { + let mut app = app_with(&[2.0]); + let before_ids: Vec<_> = app.doc.datasets[0] + .axis_pipeline(crate::state::PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .map(|step| step.id) + .collect(); + assert!( + !before_ids.is_empty(), + "the fixture starts with a populated pipeline" + ); + + let plan = app.plan_spectrum_alignment(0.0, 5.0, AlignTargetMode::Custom(3.0)); + app.apply_spectrum_alignment(&plan); + + let steps = app.doc.datasets[0] + .axis_pipeline(crate::state::PhaseAxis::Direct) + .unwrap() + .steps + .clone(); + let appended = steps + .iter() + .find(|step| matches!(step.kind, plotx_processing::StepKind::Reference(_))) + .expect("alignment appended a referencing step"); + assert!( + !before_ids.contains(&appended.id), + "the appended step reused an identity already in the live pipeline" + ); + + let mut ids: Vec<_> = steps.iter().map(|step| step.id).collect(); + ids.sort(); + let unique = ids.len(); + ids.dedup(); + assert_eq!( + ids.len(), + unique, + "step ids must be unique within a dataset" + ); + + // The allocator moved past the id it handed out, so the next step cannot + // collide either. + let next = app.doc.datasets[0].as_nmr().unwrap().next_step_id; + assert!(ids.iter().all(|id| id.get() < next)); +} diff --git a/crates/core/src/actions/tests/board.rs b/crates/core/src/actions/tests/board.rs index 427b487..9857335 100644 --- a/crates/core/src/actions/tests/board.rs +++ b/crates/core/src/actions/tests/board.rs @@ -1,4 +1,4 @@ -use super::{sample_app, table_app}; +use super::{dataset_id, sample_app, table_app}; use crate::actions::Action; use crate::state::FrameRef; use crate::state::page_frame_showing_dataset; @@ -71,7 +71,11 @@ fn move_sheet_on_board_applies_reverts_and_noops() { let before = app.doc.datasets[0].as_table().unwrap().board_pos; let after = [3240.0, 720.0]; - app.execute_action(Action::move_sheet_on_board(0, before, after)); + app.execute_action(Action::move_sheet_on_board( + dataset_id(&app, 0), + before, + after, + )); assert_eq!(app.doc.datasets[0].as_table().unwrap().board_pos, after); app.undo(); assert_eq!(app.doc.datasets[0].as_table().unwrap().board_pos, before); @@ -79,7 +83,11 @@ fn move_sheet_on_board_applies_reverts_and_noops() { assert_eq!(app.doc.datasets[0].as_table().unwrap().board_pos, after); // An unchanged position is a no-op: no new undo entry. let len = app.session.undo_stack.len(); - app.execute_action(Action::move_sheet_on_board(0, after, after)); + app.execute_action(Action::move_sheet_on_board( + dataset_id(&app, 0), + after, + after, + )); assert_eq!(app.session.undo_stack.len(), len); } diff --git a/crates/core/src/actions/tests/integral_curve.rs b/crates/core/src/actions/tests/integral_curve.rs index 4a58365..bb07e21 100644 --- a/crates/core/src/actions/tests/integral_curve.rs +++ b/crates/core/src/actions/tests/integral_curve.rs @@ -1,4 +1,4 @@ -use super::{push_canvas, sample_app}; +use super::{dataset_id, push_canvas, sample_app}; use crate::actions::Action; use crate::state::{IntegralDrag, Interaction, RegionDragKind, Tool}; use crate::{DisplayModeLabel, IntegralResult}; @@ -23,7 +23,11 @@ fn set_integrals_apply_undo_redo_keeps_all_primary_figures_synced() { let mut app = sample_app(); push_canvas(&mut app, 0, "second", [120.0, 80.0]); let integral = sample_integral(7, 3.0, Some(3.0)); - app.execute_action(Action::set_integrals(0, Vec::new(), vec![integral])); + app.execute_action(Action::set_integrals( + dataset_id(&app, 0), + Vec::new(), + vec![integral], + )); assert!(app.doc.canvases.iter().all(|canvas| { let curve = &canvas.objects[0].plot().unwrap().figure.integral_curves; curve.len() == 1 && curve[0].label == "3.000" @@ -227,7 +231,11 @@ fn processing_action_apply_undo_and_redo_recompute_integrals() { } } - app.execute_action(Action::update_dataset_processing(0, before, after)); + app.execute_action(Action::update_dataset_processing( + dataset_id(&app, 0), + before, + after, + )); assert_ne!( app.doc.datasets[0].as_nmr().unwrap().integrals[0].area, 999.0 diff --git a/crates/core/src/actions/tests/linefit.rs b/crates/core/src/actions/tests/linefit.rs index 970ce29..c29054f 100644 --- a/crates/core/src/actions/tests/linefit.rs +++ b/crates/core/src/actions/tests/linefit.rs @@ -206,7 +206,11 @@ fn set_line_fits_applies_reverts_and_skips_noops() { let mut app = two_lorentzian_app(); let fit = stored_sample(0); - app.execute_action(Action::set_line_fits(0, Vec::new(), vec![fit.clone()])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 0), + Vec::new(), + vec![fit.clone()], + )); assert_eq!( app.doc.datasets[0].as_nmr().unwrap().line_fits, vec![fit.clone()] @@ -218,7 +222,11 @@ fn set_line_fits_applies_reverts_and_skips_noops() { assert_eq!(app.doc.datasets[0].line_fits(), std::slice::from_ref(&fit)); let undo_before = app.session.undo_stack.len(); - app.execute_action(Action::set_line_fits(0, vec![fit.clone()], vec![fit])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 0), + vec![fit.clone()], + vec![fit], + )); assert_eq!(app.session.undo_stack.len(), undo_before); } @@ -226,7 +234,7 @@ fn set_line_fits_applies_reverts_and_skips_noops() { fn remove_line_fit_deletes_by_id_and_is_undoable() { let mut app = two_lorentzian_app(); app.execute_action(Action::set_line_fits( - 0, + dataset_id(&app, 0), Vec::new(), vec![stored_sample(0), stored_sample(1)], )); @@ -250,7 +258,7 @@ fn remove_line_fit_deletes_by_id_and_is_undoable() { fn background_fit_rebuilds_undo_snapshot_at_completion() { let mut app = two_lorentzian_app(); app.execute_action(Action::set_line_fits( - 0, + dataset_id(&app, 0), Vec::new(), vec![stored_sample(5), stored_sample(6)], )); @@ -360,7 +368,11 @@ fn single_table_figure_gains_line_fit_overlays() { ) .unwrap(); app.doc.datasets.push(Dataset::Table(Box::new(table))); - app.execute_action(Action::set_line_fits(1, Vec::new(), vec![stored_sample(0)])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 1), + Vec::new(), + vec![stored_sample(0)], + )); assert_eq!(app.doc.datasets[1].line_fits().len(), 1); let fig = app.build_binding_figure( @@ -393,7 +405,11 @@ fn non_line_table_charts_skip_line_fit_overlays() { ) .unwrap(); app.doc.datasets.push(Dataset::Table(Box::new(table))); - app.execute_action(Action::set_line_fits(1, Vec::new(), vec![stored_sample(0)])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 1), + Vec::new(), + vec![stored_sample(0)], + )); // The stored fit lives in the table's x/y space; distribution and part-of- // whole charts draw in other coordinates where the curve is unrelated ink. @@ -426,7 +442,11 @@ fn non_line_table_charts_skip_line_fit_overlays() { fn stacked_figures_exclude_line_fit_overlays() { let mut app = two_lorentzian_app(); app.doc.datasets.push(two_lorentzian_dataset("second")); - app.execute_action(Action::set_line_fits(0, Vec::new(), vec![stored_sample(0)])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 0), + Vec::new(), + vec![stored_sample(0)], + )); let binding = DataBinding { series: vec![ @@ -446,7 +466,11 @@ fn stacked_figures_exclude_line_fit_overlays() { fn single_plot_color_override_leaves_overlay_colors_alone() { use plotx_figure::Color; let mut app = two_lorentzian_app(); - app.execute_action(Action::set_line_fits(0, Vec::new(), vec![stored_sample(0)])); + app.execute_action(Action::set_line_fits( + dataset_id(&app, 0), + Vec::new(), + vec![stored_sample(0)], + )); let override_color = Color::rgb(0x11, 0x22, 0x33); let mut binding = DataBinding::single(app.doc.datasets[0].resource_id()); diff --git a/crates/core/src/actions/tests/mod.rs b/crates/core/src/actions/tests/mod.rs index 6d0c8ef..9b29299 100644 --- a/crates/core/src/actions/tests/mod.rs +++ b/crates/core/src/actions/tests/mod.rs @@ -75,6 +75,10 @@ pub(super) fn push_canvas(app: &mut PlotxApp, dataset: usize, name: &str, size_m app.doc.canvases.push(canvas); } +pub(super) fn dataset_id(app: &PlotxApp, index: usize) -> crate::state::DatasetId { + app.doc.datasets[index].resource_id() +} + fn first_plot(app: &PlotxApp) -> &crate::state::PlotObject { app.doc.canvases[0].objects[0].plot().unwrap() } @@ -209,7 +213,11 @@ fn processing_undo_redo_rebuilds_spectrum_and_canvas() { } } - app.execute_action(Action::update_dataset_processing(0, before, after)); + app.execute_action(Action::update_dataset_processing( + dataset_id(&app, 0), + before, + after, + )); let edited_y = first_plot(&app).figure.series[0].points[3][1]; assert_ne!(edited_y, original_y); assert_eq!(phase0_of(&app), (0.5, false)); @@ -238,7 +246,7 @@ fn phase_editor_session_is_one_undo_step() { .id; let before = DatasetProcessingState::from_dataset(&app.doc.datasets[0]); - app.session.ui.proc_expanded_step = Some(phase_id); + app.session.ui.proc_expanded_step = Some((dataset_id(&app, 0), phase_id)); app.sync_phase_interaction(); assert!(app.session.ui.processing_session.is_some()); @@ -579,7 +587,7 @@ fn rename_and_redo_stack_behave() { assert!(app.can_redo()); app.execute_action(Action::rename_dataset( - 0, + dataset_id(&app, 0), None, Some("data name".to_owned()), )); diff --git a/crates/core/src/actions/tests/multiplet.rs b/crates/core/src/actions/tests/multiplet.rs index 07256fa..02f26b7 100644 --- a/crates/core/src/actions/tests/multiplet.rs +++ b/crates/core/src/actions/tests/multiplet.rs @@ -125,7 +125,11 @@ fn set_multiplets_applies_reverts_and_skips_noops() { area: 1.0, peak_ppm: vec![2.02, 2.0], }; - app.execute_action(Action::set_multiplets(0, Vec::new(), vec![m.clone()])); + app.execute_action(Action::set_multiplets( + dataset_id(&app, 0), + Vec::new(), + vec![m.clone()], + )); assert_eq!(app.doc.datasets[0].multiplets(), std::slice::from_ref(&m)); app.undo(); @@ -133,7 +137,9 @@ fn set_multiplets_applies_reverts_and_skips_noops() { app.redo(); assert_eq!(app.doc.datasets[0].multiplets(), std::slice::from_ref(&m)); - assert!(Action::set_multiplets(0, vec![m.clone()], vec![m.clone()]).is_noop()); + assert!( + Action::set_multiplets(dataset_id(&app, 0), vec![m.clone()], vec![m.clone()]).is_noop() + ); app.remove_multiplet(0, 0); assert!(app.doc.datasets[0].multiplets().is_empty()); diff --git a/crates/core/src/actions/tests/scheme_apply.rs b/crates/core/src/actions/tests/scheme_apply.rs index 0c72939..33a31ec 100644 --- a/crates/core/src/actions/tests/scheme_apply.rs +++ b/crates/core/src/actions/tests/scheme_apply.rs @@ -71,3 +71,64 @@ fn batch_template_apply_filters_incompatible_targets_and_undoes_as_one_step() { assert!(group_delay(&app, 1)); assert!(!group_delay(&app, 0)); } + +/// P1-2 regression. A `.plotxproc` is a documented, hand-writable recipe and +/// carries no identities: `apply_scheme` remints every step from the target +/// dataset's allocator, so a required `id` field would make authors spell out a +/// value that is thrown away. Dropping `#[serde(default)]` from +/// `ProcessingStepDto::id` makes the parse below fail with `missing field id`. +#[test] +fn a_hand_written_scheme_without_step_ids_loads_and_applies() { + let json = r#"{ + "schema_version": 1, + "dimension_count": 1, + "pipelines": [{"steps": [ + {"kind": "Fft", "enabled": true, "source": "User"}, + {"kind": {"Phase": {"phase0": 0.25, "phase1": 0.0, "pivot_frac": 0.5, "auto": null}}, + "enabled": true, "source": "User"} + ]}], + "group_delay_correct": false + }"#; + let scheme: crate::project::ProcessingScheme = + serde_json::from_str(json).expect("a recipe may omit step identities"); + + let mut app = PlotxApp::new(); + app.doc + .datasets + .push(Dataset::Nmr(Box::new(NmrDataset::load(synthetic_1d())))); + + let plan = plan_scheme_application(&scheme, &app.doc.datasets, &[0]); + assert_eq!(plan.compatible_count(), 1); + let prepared = plan.prepare(SchemeApplicationPolicy::StrictAll).unwrap(); + app.execute_action(prepared.action); + + // Reminting is what makes the missing ids harmless: the adopted steps must + // be unique and sit below the owner's allocator. + let n = app.doc.datasets[0].as_nmr().unwrap(); + let mut ids: Vec<_> = n.pipeline.steps.iter().map(|step| step.id).collect(); + assert_eq!(ids.len(), 2); + assert!(ids.iter().all(|id| id.get() < n.next_step_id)); + ids.sort(); + ids.dedup(); + assert_eq!(ids.len(), 2, "adopted steps must not share an identity"); + assert!(!group_delay(&app, 0)); +} + +/// The other half of P1-2: a saved recipe must not contain the discarded field +/// at all, so what the app writes matches what the docs ask users to write. +#[test] +fn a_saved_scheme_omits_step_identities() { + let mut app = PlotxApp::new(); + app.doc + .datasets + .push(Dataset::Nmr(Box::new(NmrDataset::load(synthetic_1d())))); + let path = temp_scheme("no-step-ids"); + save_scheme(&path, &app.doc.datasets[0]).unwrap(); + let written = std::fs::read_to_string(&path).unwrap(); + let _ = std::fs::remove_file(&path); + + assert!( + !written.contains("\"id\""), + "a detached recipe carries no identities:\n{written}" + ); +} diff --git a/crates/core/src/actions/tests/stable_identity.rs b/crates/core/src/actions/tests/stable_identity.rs index f833a00..1953f8c 100644 --- a/crates/core/src/actions/tests/stable_identity.rs +++ b/crates/core/src/actions/tests/stable_identity.rs @@ -1,9 +1,10 @@ -use super::{sample_app, synthetic_1d}; +use super::{dataset_id, sample_app, synthetic_1d}; use crate::actions::Action; use crate::state::{ AxisProjection, DEFAULT_CANVAS_SIZE_MM, Dataset, DatasetId, DatasetLineage, DerivationKind, NmrDataset, ObjectFrame, ProjectionSource, SeriesBinding, }; +use plotx_processing::{ProcessingStep, StepKind, StepSource}; #[test] fn dataset_delete_undo_restores_identity_and_persistent_references() { @@ -98,3 +99,202 @@ fn syncing_integral_curves_ignores_a_stale_dataset_index() { app.sync_integral_curves_for(stale_index); } + +#[test] +fn series_reorder_preserves_ids_and_only_changes_order() { + let mut app = sample_app(); + let second = Dataset::Nmr(Box::new(NmrDataset::load(synthetic_1d()))); + let second_id = second.resource_id(); + app.doc.datasets.push(second); + let plot = app.doc.canvases[0].objects[0].plot_mut().unwrap(); + let id = plot.allocate_series_id(); + let mut series = SeriesBinding::new(second_id); + series.id = id; + plot.binding.series.push(series); + let before: Vec<_> = plot.binding.series.iter().map(|series| series.id).collect(); + + plot.binding.series.swap(0, 1); + + let after: Vec<_> = plot.binding.series.iter().map(|series| series.id).collect(); + assert_eq!(after, before.into_iter().rev().collect::>()); +} + +#[test] +fn step_and_series_allocators_do_not_rollback_with_undo() { + let mut app = sample_app(); + let second = Dataset::Nmr(Box::new(NmrDataset::load(synthetic_1d()))); + let second_id = second.resource_id(); + app.doc.datasets.push(second); + + let before_processing = + crate::actions::DatasetProcessingState::from_dataset(&app.doc.datasets[0]); + let step_id = app.doc.datasets[0].as_nmr_mut().unwrap().allocate_step_id(); + let step_high_water = app.doc.datasets[0].as_nmr().unwrap().next_step_id; + let mut after_processing = before_processing.clone(); + let crate::actions::DatasetProcessingState::Nmr { pipeline, .. } = &mut after_processing else { + unreachable!() + }; + pipeline.steps.push(ProcessingStep { + id: step_id, + kind: StepKind::Invert, + enabled: true, + source: StepSource::User, + }); + app.execute_action(Action::update_dataset_processing( + app.doc.datasets[0].resource_id(), + before_processing, + after_processing, + )); + + let (series_id, series_high_water, before_binding, after_binding, object_id) = { + let object = &mut app.doc.canvases[0].objects[0]; + let object_id = object.id; + let plot = object.plot_mut().unwrap(); + let before = plot.binding.clone(); + let series_id = plot.allocate_series_id(); + let high_water = plot.next_series_id; + let mut after = before.clone(); + let mut series = SeriesBinding::new(second_id); + series.id = series_id; + after.series.push(series); + (series_id, high_water, before, after, object_id) + }; + app.execute_action(Action::set_data_binding( + 0, + object_id, + before_binding, + after_binding, + )); + + app.undo(); + assert_eq!( + app.doc.canvases[0].objects[0] + .plot() + .unwrap() + .next_series_id, + series_high_water + ); + assert!( + app.doc.canvases[0].objects[0] + .plot() + .unwrap() + .binding + .series + .iter() + .all(|series| series.id != series_id) + ); + app.undo(); + assert_eq!( + app.doc.datasets[0].as_nmr().unwrap().next_step_id, + step_high_water + ); + assert!( + app.doc.datasets[0] + .axis_pipeline(crate::state::PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .all(|step| step.id != step_id) + ); + + app.redo(); + assert!( + app.doc.datasets[0] + .axis_pipeline(crate::state::PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .any(|step| step.id == step_id) + ); + app.redo(); + assert!( + app.doc.canvases[0].objects[0] + .plot() + .unwrap() + .binding + .series + .iter() + .any(|series| series.id == series_id) + ); +} + +/// P0-2 regression. `StepId` is owner-local: every dataset numbers its steps +/// from zero, so two datasets genuinely hold equal `StepId` values. With the +/// expanded-row state stored as a bare `StepId`, switching the active dataset +/// made the same-numbered row on the new dataset read as expanded, and +/// `phase_editor_dataset` then reported phasing on a dataset the user never +/// touched — flipping the tool to ManualPhase and opening a processing session. +/// Reverting to `Option` makes both assertions below fail. +#[test] +fn an_expanded_step_does_not_leak_onto_another_dataset_with_the_same_id() { + let mut app = sample_app(); + app.doc + .datasets + .push(Dataset::Nmr(Box::new(NmrDataset::load(synthetic_1d())))); + + let phase_id = |app: &crate::state::PlotxApp, index: usize| { + app.doc.datasets[index] + .axis_pipeline(crate::state::PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .find(|step| matches!(step.kind, StepKind::Phase(_))) + .unwrap() + .id + }; + // The premise: the two datasets really do share the id value. + assert_eq!(phase_id(&app, 0), phase_id(&app, 1)); + + // Expand the Phase row on dataset 0 while dataset 0 is active. + app.focus_single(0); + app.session.ui.proc_expanded_step = Some((dataset_id(&app, 0), phase_id(&app, 0))); + app.sync_phase_interaction(); + assert!(app.phase_editor_open()); + assert_eq!(app.session.tool, crate::state::Tool::ManualPhase); + + // Switching to the other dataset must not inherit that expansion. + app.focus_single(1); + app.sync_phase_interaction(); + assert!( + !app.phase_editor_open(), + "an equal StepId on another dataset must not read as an open Phase editor" + ); + assert_ne!( + app.session.tool, + crate::state::Tool::ManualPhase, + "no tool switch on a dataset the user never expanded a row on" + ); +} + +/// P0-3 regression. These entry points take a positional index that a +/// concurrent delete can invalidate. Reading `self.doc.datasets[index]` up +/// front to fetch the stable id turned every stale call into a panic; each one +/// must guard and no-op instead. Reverting any guard aborts this test. +#[test] +fn stale_dataset_indices_are_inert_rather_than_fatal() { + let mut app = sample_app(); + let stale = app.doc.datasets.len() + 5; + let state = crate::actions::DatasetProcessingState::from_dataset(&app.doc.datasets[0]); + + app.commit_processing_edit(stale, state.clone(), state); + app.begin_processing_session(stale); + app.edit_peaks(stale, |_| unreachable!("no peak set behind a stale index")); + app.edit_integrals(stale, |_, _| { + unreachable!("no integrals behind a stale index") + }); + app.edit_integrals_2d(stale, |_, _| { + unreachable!("no 2D integrals behind a stale index") + }); + app.edit_regions(stale, |_, _| { + unreachable!("no regions behind a stale index") + }); + app.remove_statistics(stale, 0); + app.remove_line_fit(stale, 0); + app.remove_multiplet(stale, 0); + app.cancel_compute(stale, crate::state::ComputeKind::Dosy); + + assert!( + !app.can_undo(), + "a stale target records no history instead of crashing" + ); +} diff --git a/crates/core/src/automation/tests.rs b/crates/core/src/automation/tests.rs index eba5399..4bc99f3 100644 --- a/crates/core/src/automation/tests.rs +++ b/crates/core/src/automation/tests.rs @@ -233,7 +233,11 @@ fn composite_validation_prevents_partial_application() { let mut app = app_with_table_and_canvas(); let before = app.doc.datasets[0].display_name(); let action = Action::Composite(vec![ - Action::rename_dataset(0, app.doc.datasets[0].name(), Some("partial".to_owned())), + Action::rename_dataset( + app.doc.datasets[0].resource_id(), + app.doc.datasets[0].name(), + Some("partial".to_owned()), + ), Action::rename_canvas(99, String::new(), "invalid".to_owned()), ]); assert!(app.try_execute_action(action).is_err()); diff --git a/crates/core/src/automation/tools.rs b/crates/core/src/automation/tools.rs index b357dfe..6c8fcff 100644 --- a/crates/core/src/automation/tools.rs +++ b/crates/core/src/automation/tools.rs @@ -204,7 +204,7 @@ fn execute_rename(app: &mut PlotxApp, plan: &ToolPlan) -> Result, } +/// An owner-local component identity. Both variants are only meaningful +/// relative to the [`TargetRef::resource`] they travel with — `SeriesId` is +/// local to a plot object and `StepId` is local to a dataset pipeline, and both +/// number from zero in every owner. A `ComponentRef` must therefore never be +/// stored or compared on its own; keep it inside its `TargetRef`. #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde( tag = "kind", diff --git a/crates/core/src/automation/types_tests.rs b/crates/core/src/automation/types_tests.rs index 8ed7773..dde9d7b 100644 --- a/crates/core/src/automation/types_tests.rs +++ b/crates/core/src/automation/types_tests.rs @@ -60,7 +60,7 @@ fn target_ref_serializes_resource_and_typed_components() { let step_target = TargetRef { resource, - component: Some(ComponentRef::ProcessingStep(StepId(9))), + component: Some(ComponentRef::ProcessingStep(StepId::new(9))), }; let encoded = serde_json::to_string(&step_target).unwrap(); assert_eq!( diff --git a/crates/core/src/data_export/tests.rs b/crates/core/src/data_export/tests.rs index 5ab8523..3c43a3b 100644 --- a/crates/core/src/data_export/tests.rs +++ b/crates/core/src/data_export/tests.rs @@ -323,9 +323,12 @@ fn default_channel_tracks_the_enabled_magnitude_display_step() { DataExportAvailability::for_dataset(&dataset).default_channel, IntensityChannel::Real ); - nmr.pipeline - .steps - .push(ProcessingStep::new(StepKind::Magnitude, StepSource::User)); + let id = nmr.allocate_step_id(); + nmr.pipeline.steps.push(ProcessingStep::new( + id, + StepKind::Magnitude, + StepSource::User, + )); let dataset = Dataset::Nmr(Box::new(nmr)); assert_eq!( DataExportAvailability::for_dataset(&dataset).default_channel, diff --git a/crates/core/src/export/precheck.rs b/crates/core/src/export/precheck.rs index 092bb91..25d65fd 100644 --- a/crates/core/src/export/precheck.rs +++ b/crates/core/src/export/precheck.rs @@ -274,6 +274,7 @@ mod tests { visible: true, group: None, kind: CanvasObjectKind::Plot(Box::new(PlotObject { + next_series_id: crate::state::SeriesId::new(1), binding: DataBinding::single(crate::state::DatasetId::new()), chart: ChartSpec::default(), stack: StackSpec::default(), diff --git a/crates/core/src/project/cleanup_tests.rs b/crates/core/src/project/cleanup_tests.rs index 5753fe8..4c1a9a4 100644 --- a/crates/core/src/project/cleanup_tests.rs +++ b/crates/core/src/project/cleanup_tests.rs @@ -20,10 +20,11 @@ fn project_and_scheme_roundtrips_preserve_cleanup_steps() { StepKind::Invert, ]; for kind in &cleanup { + let id = dataset.allocate_step_id(); dataset .pipeline .steps - .push(ProcessingStep::new(kind.clone(), StepSource::User)); + .push(ProcessingStep::new(id, kind.clone(), StepSource::User)); } dataset.retransform(); let expected: Vec = cleanup.to_vec(); diff --git a/crates/core/src/project/convert.rs b/crates/core/src/project/convert.rs index 0060f9b..22e1105 100644 --- a/crates/core/src/project/convert.rs +++ b/crates/core/src/project/convert.rs @@ -4,6 +4,7 @@ use super::electrophysiology_convert::{ electrophysiology_from_object, electrophysiology_to_objects, }; use super::*; +use crate::state::SeriesId; pub fn dataset_to_objects( dataset: &Dataset, data_id: &str, @@ -42,6 +43,9 @@ pub fn dataset_to_objects( ..RecipeParameters::default() }, extensions: serde_json::json!({ + "plotx.step_allocator": { + "next_id": n.next_step_id + }, "plotx.analysis": { "peaks": &n.peaks, "integrals": &n.integrals, @@ -394,6 +398,7 @@ pub fn canvas_to_view( name: object.name.clone(), kind: kind.to_owned(), input: String::new(), + next_series_id: 0, series: Vec::new(), chart_type: None, chart_column: None, @@ -457,6 +462,7 @@ pub fn canvas_to_view( )) })?; Ok(SeriesBindingDto { + id: Some(sb.id.get()), input: format!("recipe_{}", dataset.resource_id()), color: sb.color.map(|c| [c.r, c.g, c.b]), label: sb.label.clone(), @@ -469,6 +475,7 @@ pub fn canvas_to_view( .then(|| StackDto::from_spec(&plot.stack)); Ok(ViewCanvasObject { input: format!("recipe_{}", primary_dataset.resource_id()), + next_series_id: plot.next_series_id.get(), series, chart_type: Some(plot.chart.type_id.clone()), chart_column: plot.chart.column.map(|column| column.to_string()), @@ -575,7 +582,7 @@ pub fn view_to_canvas( .parse::() .map_err(|_| ProjectError::Invalid(format!("invalid object id {}", view_object.id)))?; let frame = view_object.frame.into_frame(); - let kind = match view_object.kind.as_str() { + let mut kind = match view_object.kind.as_str() { "text" => CanvasObjectKind::Text(text_box_from(view_object, false)), "panel_label" => CanvasObjectKind::PanelLabel(text_box_from(view_object, true)), "shape" => CanvasObjectKind::Shape( @@ -598,8 +605,9 @@ pub fn view_to_canvas( DataBinding::single(app.doc.datasets[index].resource_id()) } else { let mut series = Vec::with_capacity(view_object.series.len()); - for sb in &view_object.series { + for (position, sb) in view_object.series.iter().enumerate() { series.push(SeriesBinding { + id: SeriesId::new(sb.id.unwrap_or(position as u64)), dataset: { let index = resolve(&sb.input)?; app.doc.datasets[index].resource_id() @@ -705,6 +713,7 @@ pub fn view_to_canvas( .map(PanelDto::into_panel) .unwrap_or_else(|| PanelMeta::new(app.default_plot_title(di), frame.width)); CanvasObjectKind::Plot(Box::new(PlotObject { + next_series_id: SeriesId::new(view_object.next_series_id), binding, chart, stack, @@ -717,6 +726,14 @@ pub fn view_to_canvas( } _ => continue, }; + if let CanvasObjectKind::Plot(plot) = &mut kind { + plot.repair_series_allocator().ok_or_else(|| { + ProjectError::Invalid(format!( + "view {view_id} object {} exhausts the series id space", + view_object.id + )) + })?; + } canvas.objects.push(CanvasObject { id: object_id, name: view_object.name.clone(), @@ -749,51 +766,3 @@ fn text_box_from(view_object: &ViewCanvasObject, panel: bool) -> TextBox { } }) } -pub fn dimension_from_1d(data: &NmrData) -> Dimension { - Dimension { - id: "f2".to_owned(), - role: "direct".to_owned(), - size: data.points.len(), - storage_axis: 0, - quantity: "time_or_frequency".to_owned(), - display_quantity: Some("chemical_shift".to_owned()), - unit: Some("ppm".to_owned()), - nucleus: Some(data.nucleus.clone()), - spectral_width_hz: Some(data.spectral_width_hz), - observe_freq_mhz: Some(data.observe_freq_mhz), - carrier_ppm: Some(data.carrier_ppm), - group_delay: Some(data.group_delay), - } -} -pub fn dimension_from_dim( - id: &str, - role: &str, - storage_axis: usize, - size: usize, - dim: &Dim, -) -> Dimension { - Dimension { - id: id.to_owned(), - role: role.to_owned(), - size, - storage_axis, - quantity: "time_or_frequency".to_owned(), - display_quantity: Some("chemical_shift".to_owned()), - unit: Some("ppm".to_owned()), - nucleus: Some(dim.nucleus.clone()), - spectral_width_hz: Some(dim.spectral_width_hz), - observe_freq_mhz: Some(dim.observe_freq_mhz), - carrier_ppm: Some(dim.carrier_ppm), - group_delay: Some(dim.group_delay), - } -} - -pub fn dim_from_dimension(dim: &Dimension) -> Result { - Ok(Dim { - spectral_width_hz: required(dim.spectral_width_hz, "spectral_width_hz")?, - observe_freq_mhz: required(dim.observe_freq_mhz, "observe_freq_mhz")?, - carrier_ppm: required(dim.carrier_ppm, "carrier_ppm")?, - nucleus: dim.nucleus.clone().unwrap_or_else(|| "X".to_owned()), - group_delay: dim.group_delay.unwrap_or(0.0), - }) -} diff --git a/crates/core/src/project/convert_dimensions.rs b/crates/core/src/project/convert_dimensions.rs new file mode 100644 index 0000000..25a1bfe --- /dev/null +++ b/crates/core/src/project/convert_dimensions.rs @@ -0,0 +1,51 @@ +use super::*; + +pub fn dimension_from_1d(data: &NmrData) -> Dimension { + Dimension { + id: "f2".to_owned(), + role: "direct".to_owned(), + size: data.points.len(), + storage_axis: 0, + quantity: "time_or_frequency".to_owned(), + display_quantity: Some("chemical_shift".to_owned()), + unit: Some("ppm".to_owned()), + nucleus: Some(data.nucleus.clone()), + spectral_width_hz: Some(data.spectral_width_hz), + observe_freq_mhz: Some(data.observe_freq_mhz), + carrier_ppm: Some(data.carrier_ppm), + group_delay: Some(data.group_delay), + } +} + +pub fn dimension_from_dim( + id: &str, + role: &str, + storage_axis: usize, + size: usize, + dim: &Dim, +) -> Dimension { + Dimension { + id: id.to_owned(), + role: role.to_owned(), + size, + storage_axis, + quantity: "time_or_frequency".to_owned(), + display_quantity: Some("chemical_shift".to_owned()), + unit: Some("ppm".to_owned()), + nucleus: Some(dim.nucleus.clone()), + spectral_width_hz: Some(dim.spectral_width_hz), + observe_freq_mhz: Some(dim.observe_freq_mhz), + carrier_ppm: Some(dim.carrier_ppm), + group_delay: Some(dim.group_delay), + } +} + +pub fn dim_from_dimension(dim: &Dimension) -> Result { + Ok(Dim { + spectral_width_hz: required(dim.spectral_width_hz, "spectral_width_hz")?, + observe_freq_mhz: required(dim.observe_freq_mhz, "observe_freq_mhz")?, + carrier_ppm: required(dim.carrier_ppm, "carrier_ppm")?, + nucleus: dim.nucleus.clone().unwrap_or_else(|| "X".to_owned()), + group_delay: dim.group_delay.unwrap_or(0.0), + }) +} diff --git a/crates/core/src/project/convert_recipes.rs b/crates/core/src/project/convert_recipes.rs index 848aa3d..6dcf4c1 100644 --- a/crates/core/src/project/convert_recipes.rs +++ b/crates/core/src/project/convert_recipes.rs @@ -9,6 +9,13 @@ pub fn apply_1d_recipe(dataset: &mut NmrDataset, recipe: &RecipeObject) -> Resul if let Some(dto) = p.pipelines.first() { dataset.pipeline = pipeline_from_dto(dto); } + dataset.next_step_id = recipe + .extensions + .get("plotx.step_allocator") + .and_then(|value| value.get("next_id")) + .and_then(serde_json::Value::as_u64) + .unwrap_or(0); + dataset.repair_step_allocator(); dataset.group_delay_correct = p.group_delay_correct; dataset.has_imaginary = true; if let Some(analysis) = recipe.extensions.get("plotx.analysis") { @@ -102,6 +109,10 @@ pub(super) fn read_regions(dataset: &mut Nmr2DDataset, recipe: &RecipeObject) { pub(super) fn nmr2d_recipe_extensions(dataset: &Nmr2DDataset) -> serde_json::Value { let mut extensions = serde_json::Map::new(); + extensions.insert( + "plotx.step_allocator".to_owned(), + serde_json::json!({ "next_id": dataset.next_step_id }), + ); if !dataset.regions.is_empty() { extensions.insert( "plotx.regions".to_owned(), @@ -138,6 +149,13 @@ pub fn apply_2d_recipe(dataset: &mut Nmr2DDataset, recipe: &RecipeObject) { if let Some(f1) = p.pipelines.get(1) { dataset.params.f1 = pipeline_from_dto(f1); } + dataset.next_step_id = recipe + .extensions + .get("plotx.step_allocator") + .and_then(|value| value.get("next_id")) + .and_then(serde_json::Value::as_u64) + .unwrap_or(0); + dataset.repair_step_allocator(); dataset.params.layout = p .layout .as_deref() diff --git a/crates/core/src/project/dto.rs b/crates/core/src/project/dto.rs index f7f7c74..21c6ffb 100644 --- a/crates/core/src/project/dto.rs +++ b/crates/core/src/project/dto.rs @@ -142,6 +142,13 @@ pub struct AxisPipelineDto { #[derive(Serialize, Deserialize, Clone)] pub struct ProcessingStepDto { + /// Present in a project archive, where step identities must round-trip. + /// Absent in a `.plotxproc` recipe: a detached recipe carries no identity — + /// the adopting dataset remints every step from its own allocator — so + /// requiring one would make hand-written recipes spell out a field that is + /// immediately discarded. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub id: Option, pub kind: StepKindDto, pub enabled: bool, pub source: StepSourceDto, @@ -331,24 +338,8 @@ impl PageLayoutDto { } #[cfg(test)] -mod page_layout_tests { - use super::*; - - #[test] - fn missing_spacing_mode_defaults_to_visual_and_writes_explicitly() { - let dto: PageLayoutDto = serde_json::from_str( - r#"{"margin_mm":[0.0,0.0,0.0,0.0],"gutter_mm":5.0,"rows":1,"cols":2}"#, - ) - .unwrap(); - assert_eq!( - dto.into_layout().spacing_mode, - crate::layout::SpacingMode::Visual - ); - let encoded = - serde_json::to_string(&PageLayoutDto::from_layout(&PageLayout::default())).unwrap(); - assert!(encoded.contains("\"spacing_mode\":\"visual\"")); - } -} +#[path = "dto_tests.rs"] +mod tests; #[derive(Serialize, Deserialize)] pub struct ViewCanvasObject { @@ -357,6 +348,11 @@ pub struct ViewCanvasObject { pub kind: String, #[serde(default)] pub input: String, + /// Plot-only allocator high-water mark, derivable from `series` via + /// `PlotObject::repair_series_allocator` (which load always runs). Omitting + /// it when zero keeps it out of text/shape/label objects, where it is noise. + #[serde(default, skip_serializing_if = "is_zero_u64")] + pub next_series_id: u64, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub series: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -410,12 +406,21 @@ fn is_zero(n: &usize) -> bool { *n == 0 } +fn is_zero_u64(n: &u64) -> bool { + *n == 0 +} + fn caption_visible_default() -> bool { true } #[derive(Serialize, Deserialize, Clone)] pub struct SeriesBindingDto { + /// Owner-local series identity. Optional on read: a series list written + /// without ids falls back to positional numbering, which keeps the entries + /// distinct — a plain zero default would collapse every series onto id 0. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub id: Option, pub input: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub color: Option<[u8; 3]>, diff --git a/crates/core/src/project/dto_tests.rs b/crates/core/src/project/dto_tests.rs new file mode 100644 index 0000000..cecb613 --- /dev/null +++ b/crates/core/src/project/dto_tests.rs @@ -0,0 +1,16 @@ +use super::*; + +#[test] +fn missing_spacing_mode_defaults_to_visual_and_writes_explicitly() { + let dto: PageLayoutDto = serde_json::from_str( + r#"{"margin_mm":[0.0,0.0,0.0,0.0],"gutter_mm":5.0,"rows":1,"cols":2}"#, + ) + .unwrap(); + assert_eq!( + dto.into_layout().spacing_mode, + crate::layout::SpacingMode::Visual + ); + let encoded = + serde_json::to_string(&PageLayoutDto::from_layout(&PageLayout::default())).unwrap(); + assert!(encoded.contains("\"spacing_mode\":\"visual\"")); +} diff --git a/crates/core/src/project/mod.rs b/crates/core/src/project/mod.rs index b9f4b28..180a8d6 100644 --- a/crates/core/src/project/mod.rs +++ b/crates/core/src/project/mod.rs @@ -26,6 +26,7 @@ mod afm_convert; mod axis_overrides; mod codec; mod convert; +mod convert_dimensions; mod convert_recipes; mod dto; mod electrophysiology_convert; @@ -38,6 +39,7 @@ mod typed_table; pub use codec::*; pub use convert::*; +pub use convert_dimensions::*; pub use convert_recipes::*; pub use dto::*; use integrals2d::read_integrals_2d; @@ -680,6 +682,8 @@ mod reference_tests; #[cfg(test)] mod schema_tests; #[cfg(test)] +mod step_identity_tests; +#[cfg(test)] mod tests; #[cfg(test)] mod tests_charts; diff --git a/crates/core/src/project/pipeline_conv.rs b/crates/core/src/project/pipeline_conv.rs index 320de41..7bf18fa 100644 --- a/crates/core/src/project/pipeline_conv.rs +++ b/crates/core/src/project/pipeline_conv.rs @@ -14,8 +14,17 @@ pub fn pipeline_from_dto(dto: &AxisPipelineDto) -> AxisPipeline { } } +/// Drop step identities from a pipeline destined for a detached recipe +/// (`.plotxproc`), which has no owner to make them meaningful. +pub fn strip_step_identities(dto: &mut AxisPipelineDto) { + for step in &mut dto.steps { + step.id = None; + } +} + fn step_to_dto(step: &ProcessingStep) -> ProcessingStepDto { ProcessingStepDto { + id: Some(step.id.get()), kind: kind_to_dto(&step.kind), enabled: step.enabled, source: source_to_dto(step.source), @@ -23,8 +32,10 @@ fn step_to_dto(step: &ProcessingStep) -> ProcessingStepDto { } fn step_from_dto(dto: &ProcessingStepDto) -> ProcessingStep { + // A recipe without identities decodes to placeholder numbering; every path + // that hands such a pipeline to a dataset remints it (see `apply_scheme`). ProcessingStep { - id: StepId::fresh(), + id: StepId::new(dto.id.unwrap_or(0)), kind: kind_from_dto(&dto.kind), enabled: dto.enabled, source: source_from_dto(dto.source), @@ -289,7 +300,10 @@ mod tests { let pipe = AxisPipeline { steps: kinds .iter() - .map(|k| ProcessingStep::new(k.clone(), StepSource::User)) + .enumerate() + .map(|(index, k)| { + ProcessingStep::new(StepId::new(index as u64), k.clone(), StepSource::User) + }) .collect(), }; let dto = pipeline_to_dto(&pipe); @@ -304,10 +318,10 @@ mod tests { #[test] fn pipeline_json_without_cleanup_variants_still_loads() { let json = r#"{"steps":[ - {"kind":{"Apodize":{"Exponential":{"lb_hz":1.0}}},"enabled":true,"source":"User"}, - {"kind":"Fft","enabled":true,"source":"Default"}, - {"kind":{"Baseline":"Offset"},"enabled":true,"source":"User"}, - {"kind":"Magnitude","enabled":false,"source":"Default"} + {"id":10,"kind":{"Apodize":{"Exponential":{"lb_hz":1.0}}},"enabled":true,"source":"User"}, + {"id":11,"kind":"Fft","enabled":true,"source":"Default"}, + {"id":12,"kind":{"Baseline":"Offset"},"enabled":true,"source":"User"}, + {"id":13,"kind":"Magnitude","enabled":false,"source":"Default"} ]}"#; let dto: AxisPipelineDto = serde_json::from_str(json).unwrap(); let pipe = pipeline_from_dto(&dto); diff --git a/crates/core/src/project/reference_tests.rs b/crates/core/src/project/reference_tests.rs index b4efe0d..b380222 100644 --- a/crates/core/src/project/reference_tests.rs +++ b/crates/core/src/project/reference_tests.rs @@ -104,6 +104,7 @@ fn loading_a_maximum_object_id_reports_exhaustion() { "id": u64::MAX.to_string(), "name": "Label", "kind": "text", + "next_series_id": 0, "frame": { "x": 0.0, "y": 0.0, "width": 10.0, "height": 10.0 }, "locked": false, "visible": true @@ -130,3 +131,62 @@ fn loading_a_maximum_object_id_reports_exhaustion() { "{error}" ); } + +/// P0-4 regression, mirroring `loading_a_maximum_object_id_reports_exhaustion` +/// for the series allocator. `repair_series_allocator` used to map the +/// `u64::MAX` case to `SeriesId(0)` through `unwrap_or_default()`; because +/// `next_series_id < 0` is never true, the repair was skipped entirely and the +/// very next `allocate_series_id` handed back an id the binding already used. +/// Restoring `unwrap_or_default()` makes this load succeed and the test fail. +#[test] +fn loading_a_maximum_series_id_reports_exhaustion() { + let app = sample_app(); + let recipe = format!("recipe_{}", app.doc.datasets[0].resource_id()); + let path = temp_project("maximum-series-id"); + let _ = std::fs::remove_file(&path); + save_project(&app, &path, false).unwrap(); + let file = std::fs::File::open(&path).unwrap(); + let mut zip = zip::ZipArchive::new(file).unwrap(); + let view: ViewObject = serde_json::from_value(serde_json::json!({ + "id": "view-max-series-id", + "role": "canvas", + "classification": { "domain": "visualization", "object": "page" }, + "name": "Maximum series id", + "next_object_id": 2, + "layout": { "size_mm": [120.0, 80.0] }, + "objects": [{ + "id": "1", + "name": "Plot", + "kind": "line_plot", + "input": recipe, + "series": [{ "id": u64::MAX, "input": recipe }], + "frame": { "x": 0.0, "y": 0.0, "width": 100.0, "height": 80.0 }, + "locked": false, + "visible": true + }] + })) + .unwrap(); + + let mut loading_app = PlotxApp::new(); + loading_app.doc.datasets.push(app.doc.datasets[0].clone()); + let mut recipe_to_dataset = HashMap::new(); + recipe_to_dataset.insert(recipe.clone(), 0usize); + + let error = match view_to_canvas( + &mut loading_app, + &mut zip, + "view-max-series-id", + &view, + 0, + &recipe_to_dataset, + ) { + Ok(_) => panic!("an exhausted series id space must be rejected"), + Err(error) => error, + }; + let _ = std::fs::remove_file(&path); + + assert!( + matches!(error, ProjectError::Invalid(ref message) if message.contains("series id space")), + "{error}" + ); +} diff --git a/crates/core/src/project/scheme.rs b/crates/core/src/project/scheme.rs index ba6a26e..2370220 100644 --- a/crates/core/src/project/scheme.rs +++ b/crates/core/src/project/scheme.rs @@ -1,5 +1,6 @@ use super::*; use crate::actions::{Action, DatasetProcessingState}; +use crate::state::DatasetId; use std::collections::HashSet; const SCHEME_VERSION: u32 = 1; @@ -26,7 +27,11 @@ pub enum SchemeApplicationPolicy { #[derive(Clone, Debug, PartialEq)] pub enum SchemeTargetResult { + /// A dataset that accepts the scheme. The identity lives here rather than + /// beside the result because only this variant can have one: an + /// incompatible target may be a stale index with no dataset behind it. Compatible { + dataset_id: DatasetId, before: DatasetProcessingState, after: DatasetProcessingState, }, @@ -86,10 +91,14 @@ impl SchemeApplicationPlan { let mut skipped_targets = Vec::new(); for target in &self.targets { match &target.result { - SchemeTargetResult::Compatible { before, after } => { + SchemeTargetResult::Compatible { + dataset_id, + before, + after, + } => { applied_targets.push(target.dataset); actions.push(Action::update_dataset_processing( - target.dataset, + *dataset_id, before.clone(), after.clone(), )); @@ -131,6 +140,7 @@ pub fn plan_scheme_application( let result = match datasets.get(dataset) { Some(target) => match apply_scheme(scheme, target) { Ok(after) => SchemeTargetResult::Compatible { + dataset_id: target.resource_id(), before: DatasetProcessingState::from_dataset(target), after, }, @@ -191,8 +201,10 @@ pub fn apply_scheme( .first() .ok_or_else(|| incompatible("scheme carries no pipeline"))?; require_fft(dto)?; + let mut pipeline = pipeline_from_dto(dto); + remint_pipeline(&mut pipeline, &mut dataset_next_step_id(dataset)); Ok(DatasetProcessingState::Nmr { - pipeline: pipeline_from_dto(dto), + pipeline, group_delay_correct: scheme.group_delay_correct, }) } @@ -214,12 +226,16 @@ pub fn apply_scheme( .as_deref() .map(layout_from_str) .unwrap_or(n.params.layout); + let mut params = Params2D { + layout, + f2: pipeline_from_dto(f2), + f1: pipeline_from_dto(f1), + }; + let mut next = dataset_next_step_id(dataset); + remint_pipeline(&mut params.f2, &mut next); + remint_pipeline(&mut params.f1, &mut next); Ok(DatasetProcessingState::Nmr2D { - params: Params2D { - layout, - f2: pipeline_from_dto(f2), - f1: pipeline_from_dto(f1), - }, + params, preset: n.preset, }) } @@ -233,8 +249,25 @@ pub fn apply_scheme( } } -pub fn reset_processing(dataset: &Dataset) -> Option { +fn dataset_next_step_id(dataset: &Dataset) -> u64 { match dataset { + Dataset::Nmr(dataset) => dataset.next_step_id, + Dataset::Nmr2D(dataset) => dataset.next_step_id, + _ => 0, + } +} + +/// Renumber a pipeline that is about to be adopted by `dataset`, so its steps +/// take identities the owner's allocator has not handed out. +fn remint_pipeline(pipeline: &mut AxisPipeline, next: &mut u64) { + for step in &mut pipeline.steps { + step.id = StepId::new(*next); + *next = next.checked_add(1).expect("step id overflow"); + } +} + +pub fn reset_processing(dataset: &Dataset) -> Option { + let mut state = match dataset { Dataset::Nmr(_) => Some(DatasetProcessingState::Nmr { pipeline: AxisPipeline::default_1d(), group_delay_correct: true, @@ -246,7 +279,17 @@ pub fn reset_processing(dataset: &Dataset) -> Option { Dataset::Table(_) => None, Dataset::Electrophysiology(_) => None, Dataset::Afm(_) => None, + }?; + let mut next = dataset_next_step_id(dataset); + match &mut state { + DatasetProcessingState::Nmr { pipeline, .. } => remint_pipeline(pipeline, &mut next), + DatasetProcessingState::Nmr2D { params, .. } => { + remint_pipeline(&mut params.f2, &mut next); + remint_pipeline(&mut params.f1, &mut next); + } + _ => {} } + Some(state) } fn scheme_from_dataset(dataset: &Dataset) -> Option { @@ -254,6 +297,7 @@ fn scheme_from_dataset(dataset: &Dataset) -> Option { Dataset::Nmr(n) => { let mut dto = pipeline_to_dto(&n.pipeline); force_user(&mut dto); + strip_step_identities(&mut dto); Some(ProcessingScheme { schema_version: SCHEME_VERSION, dimension_count: 1, @@ -267,6 +311,8 @@ fn scheme_from_dataset(dataset: &Dataset) -> Option { let mut f1 = pipeline_to_dto(&n.params.f1); force_user(&mut f2); force_user(&mut f1); + strip_step_identities(&mut f2); + strip_step_identities(&mut f1); Some(ProcessingScheme { schema_version: SCHEME_VERSION, dimension_count: 2, @@ -317,6 +363,7 @@ mod plan_tests { SchemeApplicationTarget { dataset: 2, result: SchemeTargetResult::Compatible { + dataset_id: DatasetId::new(), before: state(), after: state(), }, diff --git a/crates/core/src/project/step_identity_tests.rs b/crates/core/src/project/step_identity_tests.rs new file mode 100644 index 0000000..0623561 --- /dev/null +++ b/crates/core/src/project/step_identity_tests.rs @@ -0,0 +1,88 @@ +use super::{load_project, save_project}; +use crate::project::tests::{sample_app, synthetic_1d, temp_project}; +use crate::state::{Dataset, NmrDataset, PhaseAxis, PlotxApp}; +use plotx_processing::{Apodization, ProcessingStep, ReferenceParams, StepKind, StepSource}; + +#[test] +fn step_ids_and_allocator_survive_project_roundtrip() { + let app = sample_app(); + let before: Vec<_> = app.doc.datasets[0] + .axis_pipeline(PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .map(|step| step.id) + .collect(); + let next_before = app.doc.datasets[0].as_nmr().unwrap().next_step_id; + let path = temp_project("step-id-roundtrip"); + let _ = std::fs::remove_file(&path); + + save_project(&app, &path, false).unwrap(); + let loaded = load_project(&path).unwrap(); + let _ = std::fs::remove_file(&path); + + let after: Vec<_> = loaded.doc.datasets[0] + .axis_pipeline(PhaseAxis::Direct) + .unwrap() + .steps + .iter() + .map(|step| step.id) + .collect(); + assert_eq!(after, before); + assert_eq!( + loaded.doc.datasets[0].as_nmr().unwrap().next_step_id, + next_before + ); +} + +#[test] +fn project_roundtrip_preserves_custom_pipeline_steps() { + let mut app = PlotxApp::new(); + let mut dataset = NmrDataset::load(synthetic_1d()); + // One time-side step (an exponential window before the FFT) and one + // frequency-side step (a referencing shift) that must both survive. + let fft_pos = dataset + .pipeline + .steps + .iter() + .position(|s| matches!(s.kind, StepKind::Fft)) + .unwrap(); + let apodize_id = dataset.allocate_step_id(); + dataset.pipeline.steps.insert( + fft_pos, + ProcessingStep::new( + apodize_id, + StepKind::Apodize(Apodization::Exponential { lb_hz: 8.0 }), + StepSource::User, + ), + ); + let reference_id = dataset.allocate_step_id(); + dataset.pipeline.steps.push(ProcessingStep::new( + reference_id, + StepKind::Reference(ReferenceParams { + at_ppm: 2.0, + target_ppm: 2.5, + }), + StepSource::User, + )); + dataset.retransform(); + app.doc.datasets.push(Dataset::Nmr(Box::new(dataset))); + + let path = temp_project("pipeline"); + let _ = std::fs::remove_file(&path); + save_project(&app, &path, false).unwrap(); + let loaded = load_project(&path).unwrap(); + let _ = std::fs::remove_file(&path); + + let Dataset::Nmr(n) = &loaded.doc.datasets[0] else { + panic!("expected a 1D NMR dataset"); + }; + assert!(n.pipeline.steps.iter().any(|s| matches!( + &s.kind, + StepKind::Apodize(Apodization::Exponential { lb_hz }) if (*lb_hz - 8.0).abs() < 1e-9 + ))); + assert!(n.pipeline.steps.iter().any(|s| matches!( + &s.kind, + StepKind::Reference(r) if (r.target_ppm - 2.5).abs() < 1e-9 && (r.at_ppm - 2.0).abs() < 1e-9 + ))); +} diff --git a/crates/core/src/project/tests.rs b/crates/core/src/project/tests.rs index 200ef4d..4cafd69 100644 --- a/crates/core/src/project/tests.rs +++ b/crates/core/src/project/tests.rs @@ -584,6 +584,7 @@ fn project_roundtrip_preserves_overlay_binding() { series: vec![ crate::state::SeriesBinding::new(app.doc.datasets[0].resource_id()), crate::state::SeriesBinding { + id: crate::state::SeriesId::new(1), dataset: app.doc.datasets[1].resource_id(), color: Some(Color::rgb(10, 20, 30)), label: Some("treated".to_owned()), @@ -637,6 +638,7 @@ fn project_roundtrip_preserves_stack_spec_and_series_fields() { series: vec![ SeriesBinding::new(app.doc.datasets[0].resource_id()), SeriesBinding { + id: crate::state::SeriesId::new(1), dataset: app.doc.datasets[1].resource_id(), color: None, label: None, @@ -675,65 +677,16 @@ fn project_roundtrip_preserves_stack_spec_and_series_fields() { } #[test] -fn legacy_single_input_loads_as_one_series_binding() { +fn single_input_without_explicit_series_is_still_parseable() { let view: ViewCanvasObject = serde_json::from_str( - r#"{"id":"1","name":"Plot","kind":"line_plot","input":"recipe_000000", + r#"{"id":"1","name":"Plot","kind":"line_plot","input":"recipe_000000","next_series_id":0, "frame":{"x":0.0,"y":0.0,"width":100.0,"height":80.0}, "title":null,"snapshot":null,"locked":false,"visible":true}"#, ) .unwrap(); - assert!(view.series.is_empty(), "old files carry no series list"); + assert!(view.series.is_empty()); assert!(view.axis_overrides.is_none()); } - -#[test] -fn project_roundtrip_preserves_custom_pipeline_steps() { - let mut app = PlotxApp::new(); - let mut dataset = NmrDataset::load(synthetic_1d()); - // One time-side step (an exponential window before the FFT) and one - // frequency-side step (a referencing shift) that must both survive. - let fft_pos = dataset - .pipeline - .steps - .iter() - .position(|s| matches!(s.kind, StepKind::Fft)) - .unwrap(); - dataset.pipeline.steps.insert( - fft_pos, - ProcessingStep::new( - StepKind::Apodize(Apodization::Exponential { lb_hz: 8.0 }), - StepSource::User, - ), - ); - dataset.pipeline.steps.push(ProcessingStep::new( - StepKind::Reference(ReferenceParams { - at_ppm: 2.0, - target_ppm: 2.5, - }), - StepSource::User, - )); - dataset.retransform(); - app.doc.datasets.push(Dataset::Nmr(Box::new(dataset))); - - let path = temp_project("pipeline"); - let _ = std::fs::remove_file(&path); - save_project(&app, &path, false).unwrap(); - let loaded = load_project(&path).unwrap(); - let _ = std::fs::remove_file(&path); - - let Dataset::Nmr(n) = &loaded.doc.datasets[0] else { - panic!("expected a 1D NMR dataset"); - }; - assert!(n.pipeline.steps.iter().any(|s| matches!( - &s.kind, - StepKind::Apodize(Apodization::Exponential { lb_hz }) if (*lb_hz - 8.0).abs() < 1e-9 - ))); - assert!(n.pipeline.steps.iter().any(|s| matches!( - &s.kind, - StepKind::Reference(r) if (r.target_ppm - 2.5).abs() < 1e-9 && (r.at_ppm - 2.0).abs() < 1e-9 - ))); -} - #[test] fn scheme_save_load_apply_roundtrips() { use crate::actions::DatasetProcessingState; diff --git a/crates/core/src/state/app_impl_align.rs b/crates/core/src/state/app_impl_align.rs index dbd635b..8486f93 100644 --- a/crates/core/src/state/app_impl_align.rs +++ b/crates/core/src/state/app_impl_align.rs @@ -165,16 +165,28 @@ impl PlotxApp { } let before = DatasetProcessingState::from_dataset(&self.doc.datasets[row.dataset]); let mut pipeline = n.pipeline.clone(); - apply_reference_shift(&mut pipeline, ppm, target); + let group_delay_correct = n.group_delay_correct; + // `pipeline` is a copy of a live recipe, so an appended referencing + // step needs a real identity from this dataset's allocator rather + // than template-local numbering. Reserving it unconditionally is + // safe: the allocator is a monotone high-water mark and gaps are + // expected (it deliberately never rolls back on undo). + let Some(dataset) = self + .doc + .datasets + .get_mut(row.dataset) + .and_then(Dataset::as_nmr_mut) + else { + continue; + }; + let step_id = dataset.allocate_step_id(); + let dataset_id = dataset.resource_id; + apply_reference_shift(&mut pipeline, ppm, target, step_id); let after = DatasetProcessingState::Nmr { pipeline, - group_delay_correct: n.group_delay_correct, + group_delay_correct, }; - actions.push(Action::update_dataset_processing( - row.dataset, - before, - after, - )); + actions.push(Action::update_dataset_processing(dataset_id, before, after)); } if aligned == 0 { self.session.status = "No spectrum had a usable peak in the window.".into(); diff --git a/crates/core/src/state/app_impl_analysis.rs b/crates/core/src/state/app_impl_analysis.rs index 9183fc7..b486f49 100644 --- a/crates/core/src/state/app_impl_analysis.rs +++ b/crates/core/src/state/app_impl_analysis.rs @@ -295,7 +295,11 @@ impl PlotxApp { { d2.next_region_id = next_id; } - self.execute_action(Action::set_regions(dataset, before, after)); + self.execute_action(Action::set_regions( + self.doc.datasets[dataset].resource_id(), + before, + after, + )); } /// Create the live series table for a dataset's regions. @@ -470,7 +474,7 @@ impl PlotxApp { .any(|reference| reference.analysis_id == analysis.id) }); self.execute_action(Action::set_curve_fit_analyses( - dataset, + self.doc.datasets[dataset].resource_id(), (before_refs, before_analyses), (after_refs, after_analyses), )); diff --git a/crates/core/src/state/app_impl_compute.rs b/crates/core/src/state/app_impl_compute.rs index 82cb221..948b556 100644 --- a/crates/core/src/state/app_impl_compute.rs +++ b/crates/core/src/state/app_impl_compute.rs @@ -46,8 +46,9 @@ impl PlotxApp { let nucleus = d2.data.direct.nucleus.clone(); let source = stack.source.clone(); let stack = stack.clone(); + let dataset_id = d2.resource_id; let outcome = self.session.compute.enqueue_dosy( - dataset, + dataset_id, self.session.dataset_epoch, stack, values, @@ -96,8 +97,9 @@ impl PlotxApp { let nucleus = d2.data.direct.nucleus.clone(); let source = stack.source.clone(); let stack = stack.clone(); + let dataset_id = d2.resource_id; let outcome = self.session.compute.enqueue_ilt( - dataset, + dataset_id, self.session.dataset_epoch, stack, b_factors, @@ -114,7 +116,10 @@ impl PlotxApp { } pub fn cancel_compute(&mut self, dataset: usize, kind: ComputeKind) -> bool { - if !self.session.compute.cancel(dataset, kind) { + let Some(dataset_id) = self.doc.datasets.get(dataset).map(Dataset::resource_id) else { + return false; + }; + if !self.session.compute.cancel(dataset_id, kind) { return false; } self.session.status = match kind { @@ -149,6 +154,9 @@ impl PlotxApp { { continue; } + let Some(dataset) = self.doc.dataset_index(dataset) else { + continue; + }; let any = result.amp.iter().flatten().any(|&a| a > 0.0); let Some(d2) = self .doc @@ -186,6 +194,9 @@ impl PlotxApp { { continue; } + let Some(dataset) = self.doc.dataset_index(dataset) else { + continue; + }; let any = result.d.iter().any(|d| d.is_finite()); let Some(d2) = self .doc @@ -227,6 +238,9 @@ impl PlotxApp { { continue; } + let Some(dataset) = self.doc.dataset_index(dataset) else { + continue; + }; let Some(d2) = self .doc .datasets @@ -261,8 +275,8 @@ impl PlotxApp { Done::Failed { dataset, kind, .. } => { let name = self .doc - .datasets - .get(dataset) + .dataset_index(dataset) + .and_then(|index| self.doc.datasets.get(index)) .map_or_else(|| "the dataset".to_owned(), Dataset::display_name); self.session.status = format!( "{} for {name} could not be started; background computation is \ @@ -298,9 +312,10 @@ impl PlotxApp { || plotx_processing::needs_retransform_2d(&d2.params, &d2.base_params); let params = d2.params.clone(); let preset = d2.preset; + let dataset_id = d2.resource_id; let aborted = if full { self.session.compute.request_2d_full( - dataset, + dataset_id, self.session.dataset_epoch, std::sync::Arc::clone(&d2.data), params, @@ -308,7 +323,7 @@ impl PlotxApp { ) } else { self.session.compute.request_2d_reapply( - dataset, + dataset_id, self.session.dataset_epoch, d2.base.clone(), params, diff --git a/crates/core/src/state/app_impl_compute_tests.rs b/crates/core/src/state/app_impl_compute_tests.rs new file mode 100644 index 0000000..2680238 --- /dev/null +++ b/crates/core/src/state/app_impl_compute_tests.rs @@ -0,0 +1,80 @@ +use super::*; +use num_complex::Complex64; +use plotx_io::{Dim, Domain, NmrData2D, QuadMode}; +use plotx_processing::{ProcessingStep, StepKind, StepSource}; +use std::time::{Duration, Instant}; + +fn data_2d(source: &str) -> NmrData2D { + let dim = Dim { + spectral_width_hz: 1000.0, + observe_freq_mhz: 100.0, + carrier_ppm: 5.0, + nucleus: "X".into(), + group_delay: 0.0, + }; + NmrData2D { + data: (0..16) + .map(|value| Complex64::new((value + 1) as f64, 0.0)) + .collect(), + rows: 4, + cols: 4, + domain: Domain::Time, + direct: dim.clone(), + indirect: dim, + quad: QuadMode::Complex, + indirect_conjugate: false, + experiment: None, + pseudo_axis: None, + diffusion: None, + nus: None, + source: source.into(), + } +} + +#[test] +fn process_2d_result_follows_dataset_identity_after_earlier_deletion() { + let mut app = PlotxApp::new(); + app.doc + .datasets + .push(Dataset::Nmr2D(Box::new(Nmr2DDataset::load(data_2d( + "unrelated", + ))))); + app.doc + .datasets + .push(Dataset::Nmr2D(Box::new(Nmr2DDataset::load(data_2d( + "target", + ))))); + let target_id = app.doc.datasets[1].resource_id(); + let before = app.doc.datasets[1].as_nmr2d().unwrap().processed.clone(); + let target = app.doc.datasets[1].as_nmr2d_mut().unwrap(); + let id = target.allocate_step_id(); + target.params.f2.steps.push(ProcessingStep { + id, + kind: StepKind::Invert, + enabled: true, + source: StepSource::User, + }); + assert!(app.schedule_2d_processing(1, false)); + + app.doc.datasets.remove(0); + let deadline = Instant::now() + Duration::from_secs(3); + while app.compute_busy() && Instant::now() < deadline { + app.poll_compute(); + std::thread::sleep(Duration::from_millis(5)); + } + app.poll_compute(); + + assert_eq!(app.doc.datasets[0].resource_id(), target_id); + let after = &app.doc.datasets[0].as_nmr2d().unwrap().processed; + let same_allocation = match (&before, after) { + (Processed2D::Ft(before), Processed2D::Ft(after)) => std::sync::Arc::ptr_eq(before, after), + (Processed2D::Stack(before), Processed2D::Stack(after)) => { + std::sync::Arc::ptr_eq(before, after) + } + _ => false, + }; + assert!( + !same_allocation, + "the completed result must land on the same DatasetId after index shift" + ); +} diff --git a/crates/core/src/state/app_impl_linefit.rs b/crates/core/src/state/app_impl_linefit.rs index a788690..8a285ee 100644 --- a/crates/core/src/state/app_impl_linefit.rs +++ b/crates/core/src/state/app_impl_linefit.rs @@ -238,7 +238,11 @@ impl PlotxApp { let mut after = before.clone(); after.push(stored.clone()); - self.execute_action(Action::set_line_fits(dataset, before, after)); + self.execute_action(Action::set_line_fits( + self.doc.datasets[dataset].resource_id(), + before, + after, + )); self.session.status = format!( "Fitted {} peak(s), R² = {:.4}. Open the result in the Peak Fit panel.", stored.peaks.len(), @@ -285,7 +289,11 @@ impl PlotxApp { return; }; let after: Vec = before.iter().filter(|f| f.id != id).cloned().collect(); - self.execute_action(Action::set_line_fits(dataset, before, after)); + self.execute_action(Action::set_line_fits( + self.doc.datasets[dataset].resource_id(), + before, + after, + )); } } diff --git a/crates/core/src/state/app_impl_multiplet.rs b/crates/core/src/state/app_impl_multiplet.rs index d8d4416..bec8541 100644 --- a/crates/core/src/state/app_impl_multiplet.rs +++ b/crates/core/src/state/app_impl_multiplet.rs @@ -140,7 +140,7 @@ impl PlotxApp { DEFAULT_CANVAS_SIZE_MM, ); self.execute_action(Action::Composite(vec![ - Action::set_multiplets(dataset, before, after), + Action::set_multiplets(self.doc.datasets[dataset].resource_id(), before, after), insert, ])); self.session.status = format!("Classified {} multiplet(s).", multiplets.len()); @@ -157,7 +157,11 @@ impl PlotxApp { return; }; let after: Vec = before.iter().filter(|m| m.id != id).cloned().collect(); - self.execute_action(Action::set_multiplets(dataset, before, after)); + self.execute_action(Action::set_multiplets( + self.doc.datasets[dataset].resource_id(), + before, + after, + )); } } diff --git a/crates/core/src/state/app_impl_peaks.rs b/crates/core/src/state/app_impl_peaks.rs index 78b2508..2f0c6e1 100644 --- a/crates/core/src/state/app_impl_peaks.rs +++ b/crates/core/src/state/app_impl_peaks.rs @@ -41,6 +41,7 @@ impl PlotxApp { let Some(n) = self.doc.datasets.get(dataset).and_then(Dataset::as_nmr2d) else { return; }; + let dataset_id = n.resource_id; let before = n.integrals.clone(); let mut after = before.clone(); let mut next_id = n.next_integral_id; @@ -56,7 +57,12 @@ impl PlotxApp { || original.method != candidate.method }) }); - if let Some(n) = self.doc.datasets[dataset].as_nmr2d_mut() { + if let Some(n) = self + .doc + .datasets + .get_mut(dataset) + .and_then(Dataset::as_nmr2d_mut) + { n.next_integral_id = next_id; n.integrals = after; if needs_recompute { @@ -68,7 +74,7 @@ impl PlotxApp { } after = n.integrals.clone(); } - self.execute_action(Action::set_integrals_2d(dataset, before, after)); + self.execute_action(Action::set_integrals_2d(dataset_id, before, after)); } pub fn set_integral_2d_reference(&mut self, dataset: usize, id: u64, value: f64) { @@ -139,17 +145,23 @@ impl PlotxApp { let Some(n) = self.doc.datasets.get(dataset).and_then(Dataset::as_nmr) else { return; }; + let dataset_id = n.resource_id; let before = n.integrals.clone(); let mut after = before.clone(); let mut next_id = n.next_integral_id; edit(&mut after, &mut next_id); - if let Some(n) = self.doc.datasets[dataset].as_nmr_mut() { + if let Some(n) = self + .doc + .datasets + .get_mut(dataset) + .and_then(Dataset::as_nmr_mut) + { n.next_integral_id = next_id; n.integrals = after; n.recompute_integrals(); after = n.integrals.clone(); } - self.execute_action(Action::set_integrals(dataset, before, after)); + self.execute_action(Action::set_integrals(dataset_id, before, after)); } /// Use one integral as the normalization reference at a user-selected value. @@ -189,18 +201,17 @@ impl PlotxApp { /// Snapshot the peak set, let `edit` mutate a working copy, then commit one /// undoable step. No-ops on domains without a peak set. pub fn edit_peaks(&mut self, dataset: usize, edit: impl FnOnce(&mut PeakSet)) { - let Some(before) = self + let Some((dataset_id, before)) = self .doc .datasets .get(dataset) - .and_then(Dataset::peaks) - .cloned() + .and_then(|value| Some((value.resource_id(), value.peaks().cloned()?))) else { return; }; let mut after = before.clone(); edit(&mut after); - self.execute_action(Action::set_peaks(dataset, before, after)); + self.execute_action(Action::set_peaks(dataset_id, before, after)); } /// Place a hand-picked peak, snapping the clicked `x` to the nearest local diff --git a/crates/core/src/state/app_impl_slice.rs b/crates/core/src/state/app_impl_slice.rs index 06efa0b..6449454 100644 --- a/crates/core/src/state/app_impl_slice.rs +++ b/crates/core/src/state/app_impl_slice.rs @@ -36,14 +36,21 @@ impl NmrDataset { }; // The trace is already a phased spectrum: the pipeline is the bare FFT // anchor, so the transform reproduces the values with no further steps. + // This dataset owns the pipeline it is built with, so the single step + // takes id 0 and the allocator starts one past it. let pipeline = AxisPipeline { - steps: vec![ProcessingStep::new(StepKind::Fft, StepSource::Default)], + steps: vec![ProcessingStep::new( + StepId::new(0), + StepKind::Fft, + StepSource::Default, + )], }; Self { resource_id: DatasetId::new(), data, base: spectrum.clone(), pipeline, + next_step_id: 1, group_delay_correct: true, has_imaginary: true, spectrum, diff --git a/crates/core/src/state/app_impl_statistics.rs b/crates/core/src/state/app_impl_statistics.rs index 26168da..30d154b 100644 --- a/crates/core/src/state/app_impl_statistics.rs +++ b/crates/core/src/state/app_impl_statistics.rs @@ -133,11 +133,19 @@ impl PlotxApp { outcome, }; let status = headline(&analysis); + let Some(dataset_id) = self + .doc + .datasets + .get(draft.dataset) + .map(Dataset::resource_id) + else { + return Err("The selected data table is no longer available.".to_owned()); + }; let before = self.stored_statistics(draft.dataset); let mut after = before.clone(); after.push(analysis); self.execute_action(Action::SetTableStatistics { - dataset: draft.dataset, + dataset: dataset_id, before, after, }); @@ -147,13 +155,16 @@ impl PlotxApp { /// Delete one stored analysis as an undoable step. pub fn remove_statistics(&mut self, dataset: usize, id: u64) { + let Some(dataset_id) = self.doc.datasets.get(dataset).map(Dataset::resource_id) else { + return; + }; let before = self.stored_statistics(dataset); if before.is_empty() { return; } let after: Vec = before.iter().filter(|a| a.id != id).cloned().collect(); self.execute_action(Action::SetTableStatistics { - dataset, + dataset: dataset_id, before, after, }); diff --git a/crates/core/src/state/compute.rs b/crates/core/src/state/compute.rs index d6670fb..513271f 100644 --- a/crates/core/src/state/compute.rs +++ b/crates/core/src/state/compute.rs @@ -13,6 +13,7 @@ use plotx_processing::{ Params2D, Preset2D, Processed2D, StackSpectrum, process_2d_cancellable, reapply_2d_cancellable, }; +use super::DatasetId; use super::build_processed_figure_cancellable; use crate::{IltParams, build_dosy_figure_cancellable, build_ilt_figure_cancellable}; @@ -48,7 +49,7 @@ pub enum EnqueueError { enum Job { Ilt { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, token: Arc, stack: Arc, @@ -61,7 +62,7 @@ enum Job { }, Dosy { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, token: Arc, stack: Arc, @@ -72,7 +73,7 @@ enum Job { }, Process2D { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, token: Arc, input: ProcessingInput, @@ -103,7 +104,7 @@ impl ProcessingInput { struct DeferredProcessing { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, input: ProcessingInput, params: Params2D, @@ -122,7 +123,7 @@ struct ActiveJob { pub enum Done { Ilt { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, result: IltResult, params: IltParams, @@ -130,14 +131,14 @@ pub enum Done { }, Dosy { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, result: DiffusionMap, figure: Arc
, }, Processing2D { generation: u64, - dataset: usize, + dataset: DatasetId, epoch: u64, base: Option, processed: Processed2D, @@ -146,7 +147,7 @@ pub enum Done { }, Cancelled { generation: u64, - dataset: usize, + dataset: DatasetId, kind: ComputeKind, }, /// A request could not be handed to a worker. Reported through the same queue @@ -154,7 +155,7 @@ pub enum Done { /// leaving the caller waiting on work that will never run. Failed { generation: u64, - dataset: usize, + dataset: DatasetId, kind: ComputeKind, }, } @@ -168,9 +169,9 @@ pub struct ComputeService { job_tx: Sender, done_rx: Receiver, next_gen: u64, - latest: HashMap<(usize, ComputeKind), u64>, - active: HashMap<(usize, ComputeKind), ActiveJob>, - deferred_processing: HashMap, + latest: HashMap<(DatasetId, ComputeKind), u64>, + active: HashMap<(DatasetId, ComputeKind), ActiveJob>, + deferred_processing: HashMap, /// Dispatch failures awaiting collection by `try_drain`. failures: Vec, } @@ -204,7 +205,7 @@ impl ComputeService { #[allow(clippy::too_many_arguments)] pub fn enqueue_ilt( &mut self, - dataset: usize, + dataset: DatasetId, epoch: u64, stack: Arc, b_factors: Vec, @@ -254,7 +255,7 @@ impl ComputeService { #[allow(clippy::too_many_arguments)] pub fn enqueue_dosy( &mut self, - dataset: usize, + dataset: DatasetId, epoch: u64, stack: Arc, values: Vec, @@ -301,7 +302,7 @@ impl ComputeService { /// request aborted, so the caller can say so. pub fn request_2d_full( &mut self, - dataset: usize, + dataset: DatasetId, epoch: u64, data: Arc, params: Params2D, @@ -314,7 +315,7 @@ impl ComputeService { /// [`Self::request_2d_full`] does. pub fn request_2d_reapply( &mut self, - dataset: usize, + dataset: DatasetId, epoch: u64, base: Processed2D, params: Params2D, @@ -331,7 +332,7 @@ impl ComputeService { fn request_2d( &mut self, - dataset: usize, + dataset: DatasetId, epoch: u64, input: ProcessingInput, params: Params2D, @@ -356,14 +357,14 @@ impl ComputeService { aborted } - fn next_generation(&mut self, dataset: usize, kind: ComputeKind) -> u64 { + fn next_generation(&mut self, dataset: DatasetId, kind: ComputeKind) -> u64 { let generation = self.next_gen; self.next_gen = self.next_gen.wrapping_add(1); self.latest.insert((dataset, kind), generation); generation } - fn cancel_failed_enqueue(&mut self, dataset: usize, kind: ComputeKind, generation: u64) { + fn cancel_failed_enqueue(&mut self, dataset: DatasetId, kind: ComputeKind, generation: u64) { self.active.remove(&(dataset, kind)); if self.latest.get(&(dataset, kind)) == Some(&generation) { self.latest.remove(&(dataset, kind)); @@ -371,7 +372,7 @@ impl ComputeService { } fn dispatch_ready_processing(&mut self) { - let ready: Vec = self + let ready: Vec = self .deferred_processing .keys() .filter(|dataset| { @@ -450,7 +451,7 @@ impl ComputeService { !self.active.is_empty() || !self.deferred_processing.is_empty() } - pub fn progress(&self, dataset: usize, kind: ComputeKind) -> Option { + pub fn progress(&self, dataset: DatasetId, kind: ComputeKind) -> Option { self.active.get(&(dataset, kind)).and_then(|active| { (!active.token.load(Ordering::Relaxed)).then(|| active.started_at.elapsed()) }) @@ -458,7 +459,7 @@ impl ComputeService { /// Return the active DOSY computation regardless of which method the UI is /// currently displaying. Only one method may run per dataset at a time. - pub fn dosy_progress(&self, dataset: usize) -> Option<(ComputeKind, Duration)> { + pub fn dosy_progress(&self, dataset: DatasetId) -> Option<(ComputeKind, Duration)> { [ComputeKind::Dosy, ComputeKind::Ilt] .into_iter() .find_map(|kind| self.progress(dataset, kind).map(|elapsed| (kind, elapsed))) @@ -468,7 +469,7 @@ impl ComputeService { /// already cancelled does not count even though its entry lives until the /// worker acknowledges: `progress` reports it as gone, so blocking on it would /// reject a re-run against a computation the user cannot see or wait for. - pub fn blocking_work_for(&self, dataset: usize) -> Option { + pub fn blocking_work_for(&self, dataset: DatasetId) -> Option { if self.deferred_processing.contains_key(&dataset) { return Some(ComputeKind::Processing2D); } @@ -487,7 +488,7 @@ impl ComputeService { /// because Full may replace that cached base. fn cancel_incompatible_for_processing( &mut self, - dataset: usize, + dataset: DatasetId, requested_input: ProcessingInputKind, ) -> Vec { let mut aborted = Vec::new(); @@ -516,7 +517,7 @@ impl ComputeService { aborted } - pub fn cancel(&mut self, dataset: usize, kind: ComputeKind) -> bool { + pub fn cancel(&mut self, dataset: DatasetId, kind: ComputeKind) -> bool { let mut cancelled = false; if let Some(active) = self.active.get(&(dataset, kind)) { active.token.store(true, Ordering::Relaxed); @@ -532,7 +533,7 @@ impl ComputeService { cancelled } - pub fn is_current(&self, dataset: usize, kind: ComputeKind, generation: u64) -> bool { + pub fn is_current(&self, dataset: DatasetId, kind: ComputeKind, generation: u64) -> bool { self.latest.get(&(dataset, kind)) == Some(&generation) } } @@ -696,7 +697,7 @@ fn run_job(job: Job) -> Done { } } -fn cancelled_done(generation: u64, dataset: usize) -> Done { +fn cancelled_done(generation: u64, dataset: DatasetId) -> Done { Done::Cancelled { generation, dataset, @@ -704,7 +705,7 @@ fn cancelled_done(generation: u64, dataset: usize) -> Done { } } -fn done_identity(done: &Done) -> (usize, ComputeKind, u64) { +fn done_identity(done: &Done) -> (DatasetId, ComputeKind, u64) { match done { Done::Ilt { dataset, diff --git a/crates/core/src/state/compute/tests.rs b/crates/core/src/state/compute/tests.rs index 417b502..15a45be 100644 --- a/crates/core/src/state/compute/tests.rs +++ b/crates/core/src/state/compute/tests.rs @@ -2,6 +2,10 @@ use super::*; use num_complex::Complex64; use plotx_io::{Dim, Domain, QuadMode}; use plotx_processing::{PhaseParams, ProcessingStep, StepKind, process_2d}; + +fn dataset(value: u128) -> DatasetId { + DatasetId::from_uuid(uuid::Uuid::from_u128(value)) +} fn data_2d() -> Arc { let dim = Dim { spectral_width_hz: 1000.0, @@ -58,12 +62,13 @@ fn repeated_processing_requests_coalesce_to_latest_recipe() { let first = Params2D::default_for(preset); let mut latest = first.clone(); latest.f2.steps.push(ProcessingStep::new( + plotx_processing::StepId::new(99), StepKind::Phase(PhaseParams::MANUAL_ZERO), plotx_processing::StepSource::User, )); - service.request_2d_full(0, 4, data_2d(), first, preset); - service.request_2d_full(0, 4, data_2d(), latest.clone(), preset); + service.request_2d_full(dataset(0), 4, data_2d(), first, preset); + service.request_2d_full(dataset(0), 4, data_2d(), latest.clone(), preset); assert_eq!(service.deferred_processing.len(), 1); let deadline = Instant::now() + Duration::from_secs(2); @@ -88,9 +93,13 @@ fn an_idle_processing_request_dispatches_immediately() { let preset = Preset2D::Cosy; let params = Params2D::default_for(preset); - service.request_2d_full(0, 0, data_2d(), params, preset); + service.request_2d_full(dataset(0), 0, data_2d(), params, preset); assert!(service.deferred_processing.is_empty()); - assert!(service.active.contains_key(&(0, ComputeKind::Processing2D))); + assert!( + service + .active + .contains_key(&(dataset(0), ComputeKind::Processing2D)) + ); } #[test] @@ -102,7 +111,7 @@ fn reapply_to_reapply_keeps_the_active_job_and_replaces_the_deferred_recipe() { let token = Arc::new(AtomicBool::new(false)); service.active.insert( - (0, ComputeKind::Processing2D), + (dataset(0), ComputeKind::Processing2D), ActiveJob { generation: 10, started_at: Instant::now(), @@ -112,16 +121,17 @@ fn reapply_to_reapply_keeps_the_active_job_and_replaces_the_deferred_recipe() { ); first.f2.steps.push(ProcessingStep::new( + plotx_processing::StepId::new(99), StepKind::Phase(PhaseParams::MANUAL_ZERO), plotx_processing::StepSource::User, )); - service.request_2d_reapply(0, 0, base.clone(), first, preset); + service.request_2d_reapply(dataset(0), 0, base.clone(), first, preset); assert!(!token.load(Ordering::Relaxed)); - let first_generation = service.deferred_processing[&0].generation; + let first_generation = service.deferred_processing[&dataset(0)].generation; - service.request_2d_reapply(0, 0, base, Params2D::default_for(preset), preset); + service.request_2d_reapply(dataset(0), 0, base, Params2D::default_for(preset), preset); assert!(!token.load(Ordering::Relaxed)); - assert!(service.deferred_processing[&0].generation > first_generation); + assert!(service.deferred_processing[&dataset(0)].generation > first_generation); } #[test] @@ -129,7 +139,7 @@ fn any_full_retransform_cancels_an_active_reapply() { let mut service = ComputeService::new(); let token = Arc::new(AtomicBool::new(false)); service.active.insert( - (0, ComputeKind::Processing2D), + (dataset(0), ComputeKind::Processing2D), ActiveJob { generation: 10, started_at: Instant::now(), @@ -139,10 +149,16 @@ fn any_full_retransform_cancels_an_active_reapply() { ); let preset = Preset2D::Cosy; - service.request_2d_full(0, 0, data_2d(), Params2D::default_for(preset), preset); + service.request_2d_full( + dataset(0), + 0, + data_2d(), + Params2D::default_for(preset), + preset, + ); assert!(token.load(Ordering::Relaxed)); assert!(matches!( - service.deferred_processing[&0].input, + service.deferred_processing[&dataset(0)].input, ProcessingInput::Full(_) )); } @@ -155,7 +171,7 @@ fn a_processing_request_reports_the_analysis_it_cancels() { let stack = stack_spectrum(); service .enqueue_dosy( - 1, + dataset(1), 0, stack, vec![0.0, 1.0, 2.0], @@ -166,7 +182,13 @@ fn a_processing_request_reports_the_analysis_it_cancels() { .expect("an idle dataset accepts a DOSY job"); let preset = Preset2D::Cosy; - let aborted = service.request_2d_full(1, 0, data_2d(), Params2D::default_for(preset), preset); + let aborted = service.request_2d_full( + dataset(1), + 0, + data_2d(), + Params2D::default_for(preset), + preset, + ); assert_eq!(aborted, vec![ComputeKind::Dosy]); } @@ -178,7 +200,7 @@ fn a_cancelled_analysis_stops_blocking_a_re_run() { let mut service = ComputeService::new(); service .enqueue_dosy( - 2, + dataset(2), 0, stack_spectrum(), vec![0.0, 1.0, 2.0], @@ -187,12 +209,15 @@ fn a_cancelled_analysis_stops_blocking_a_re_run() { "test".into(), ) .expect("an idle dataset accepts a DOSY job"); - assert_eq!(service.blocking_work_for(2), Some(ComputeKind::Dosy)); + assert_eq!( + service.blocking_work_for(dataset(2)), + Some(ComputeKind::Dosy) + ); - assert!(service.cancel(2, ComputeKind::Dosy)); - assert_eq!(service.progress(2, ComputeKind::Dosy), None); + assert!(service.cancel(dataset(2), ComputeKind::Dosy)); + assert_eq!(service.progress(dataset(2), ComputeKind::Dosy), None); assert_eq!( - service.blocking_work_for(2), + service.blocking_work_for(dataset(2)), None, "a cancelled job the user cannot see must not block a re-run" ); @@ -205,9 +230,15 @@ fn a_cancelled_analysis_stops_blocking_a_re_run() { fn pending_processing_blocks_dosy_under_its_own_name() { let mut service = ComputeService::new(); let preset = Preset2D::Cosy; - service.request_2d_full(4, 0, data_2d(), Params2D::default_for(preset), preset); + service.request_2d_full( + dataset(4), + 0, + data_2d(), + Params2D::default_for(preset), + preset, + ); assert_eq!( - service.blocking_work_for(4), + service.blocking_work_for(dataset(4)), Some(ComputeKind::Processing2D) ); } @@ -216,10 +247,19 @@ fn pending_processing_blocks_dosy_under_its_own_name() { fn cancelling_processing_discards_its_result_and_releases_the_service() { let mut service = ComputeService::new(); let preset = Preset2D::Cosy; - service.request_2d_full(3, 2, data_2d(), Params2D::default_for(preset), preset); + service.request_2d_full( + dataset(3), + 2, + data_2d(), + Params2D::default_for(preset), + preset, + ); - assert!(service.cancel(3, ComputeKind::Processing2D)); - assert_eq!(service.progress(3, ComputeKind::Processing2D), None); + assert!(service.cancel(dataset(3), ComputeKind::Processing2D)); + assert_eq!( + service.progress(dataset(3), ComputeKind::Processing2D), + None + ); let deadline = Instant::now() + Duration::from_secs(2); let mut completed = Vec::new(); @@ -238,7 +278,7 @@ fn cancelled_ilt_job_reports_acknowledgement_without_a_result() { let stack = stack_spectrum(); let done = run_job(Job::Ilt { generation: 7, - dataset: 2, + dataset: dataset(2), epoch: 0, token, stack, @@ -254,8 +294,8 @@ fn cancelled_ilt_job_reports_acknowledgement_without_a_result() { done, Done::Cancelled { generation: 7, - dataset: 2, + dataset: id, kind: ComputeKind::Ilt - } + } if id == dataset(2) )); } diff --git a/crates/core/src/state/datasets.rs b/crates/core/src/state/datasets.rs index 77d6bf2..84261f1 100644 --- a/crates/core/src/state/datasets.rs +++ b/crates/core/src/state/datasets.rs @@ -30,6 +30,8 @@ pub struct NmrDataset { pub data: NmrData, pub base: Spectrum, pub pipeline: AxisPipeline, + /// Persistent owner-local allocator; excluded from processing undo snapshots. + pub next_step_id: u64, /// Whether the FFT divides out the digital-filter group delay. An advanced /// escape hatch; on for every computed FID. pub group_delay_correct: bool, @@ -61,11 +63,12 @@ impl NmrDataset { let has_imaginary = data.domain == Domain::Time || data.points.iter().any(|v| v.im != 0.0); let base = fft::transform_base(&data, &pipeline, group_delay_correct); let spectrum = reapply(&base, &pipeline); - Self { + let mut result = Self { resource_id: DatasetId::new(), data, base, pipeline, + next_step_id: 0, group_delay_correct, has_imaginary, spectrum, @@ -78,7 +81,13 @@ impl NmrDataset { next_line_fit_id: 0, multiplets: Vec::new(), next_multiplet_id: 0, - } + }; + // Currently a no-op: the 1D templates already number 0..n and the + // allocator starts at 0. Kept so `load` establishes the "ids are unique + // and below next_step_id" invariant itself, rather than inheriting it + // from whichever template `pipeline` happened to come from. + result.remint_all_steps(); + result } /// Cheap re-apply of the frequency-domain steps from the cached `base` (no FFT). @@ -99,6 +108,30 @@ impl NmrDataset { pub fn pipeline(&self) -> &AxisPipeline { &self.pipeline } + + pub fn allocate_step_id(&mut self) -> StepId { + let id = StepId::new(self.next_step_id); + self.next_step_id = self.next_step_id.checked_add(1).expect("step id overflow"); + id + } + + pub fn repair_step_allocator(&mut self) { + let required = self + .pipeline + .steps + .iter() + .map(|step| step.id.get().saturating_add(1)) + .max() + .unwrap_or(0); + self.next_step_id = self.next_step_id.max(required); + } + + fn remint_all_steps(&mut self) { + for step in &mut self.pipeline.steps { + step.id = StepId::new(self.next_step_id); + self.next_step_id = self.next_step_id.checked_add(1).expect("step id overflow"); + } + } } /// A loaded 2D acquisition and its processing recipe. `base` is the post-FFT, @@ -109,6 +142,8 @@ pub struct Nmr2DDataset { pub resource_id: DatasetId, pub data: Arc, pub params: Params2D, + /// Persistent owner-local allocator shared by both axes. + pub next_step_id: u64, /// Recipe used to produce `base`. While an async retransform is pending, /// `params` may be newer than this snapshot. pub base_params: Params2D, @@ -168,12 +203,13 @@ impl Nmr2DDataset { let base = process_2d(&data, ¶ms); let processed = reapply_2d(&base, ¶ms); let processed_figure = Arc::new(build_processed_figure(&processed, preset)); - Self { + let mut result = Self { resource_id: DatasetId::new(), data: Arc::new(data), base_params: params.clone(), base_stale: false, params, + next_step_id: 0, preset, group_delay_correct, has_imaginary, @@ -194,7 +230,9 @@ impl Nmr2DDataset { integrals: Vec::new(), next_integral_id: 0, integral_error: None, - } + }; + result.remint_all_steps(); + result } /// Cheap re-apply of per-axis phase from the cached `base` (no FFT). pub fn rebuild(&mut self) { @@ -291,6 +329,38 @@ impl Nmr2DDataset { pub fn is_pseudo(&self) -> bool { matches!(self.processed, Processed2D::Stack(_)) && self.data.pseudo_axis.is_some() } + + pub fn allocate_step_id(&mut self) -> StepId { + let id = StepId::new(self.next_step_id); + self.next_step_id = self.next_step_id.checked_add(1).expect("step id overflow"); + id + } + + pub fn repair_step_allocator(&mut self) { + let required = self + .params + .f2 + .steps + .iter() + .chain(&self.params.f1.steps) + .map(|step| step.id.get().saturating_add(1)) + .max() + .unwrap_or(0); + self.next_step_id = self.next_step_id.max(required); + } + + fn remint_all_steps(&mut self) { + for step in self + .params + .f2 + .steps + .iter_mut() + .chain(&mut self.params.f1.steps) + { + step.id = StepId::new(self.next_step_id); + self.next_step_id = self.next_step_id.checked_add(1).expect("step id overflow"); + } + } } #[derive(Clone)] diff --git a/crates/core/src/state/document.rs b/crates/core/src/state/document.rs index faab840..e30a5fc 100644 --- a/crates/core/src/state/document.rs +++ b/crates/core/src/state/document.rs @@ -166,6 +166,7 @@ pub struct NamedView { /// Only `dataset` is required. #[derive(Clone, Debug, PartialEq)] pub struct SeriesBinding { + pub id: SeriesId, pub dataset: DatasetId, pub color: Option, pub label: Option, @@ -176,6 +177,7 @@ pub struct SeriesBinding { impl SeriesBinding { pub fn new(dataset: impl Into) -> Self { Self { + id: SeriesId::default(), dataset: dataset.into(), color: None, label: None, @@ -320,6 +322,9 @@ impl AxisProjections { #[derive(Clone)] pub struct PlotObject { + /// Persistent high-water mark for owner-local series identities. This is + /// deliberately outside `binding`, which actions may replace wholesale. + pub next_series_id: SeriesId, pub binding: DataBinding, /// The selected chart type (registry id) + its context, driving figure /// rebuilds through `state::charts`. Defaults to the dataset domain's default. diff --git a/crates/core/src/state/mod.rs b/crates/core/src/state/mod.rs index 3c48102..8a75629 100644 --- a/crates/core/src/state/mod.rs +++ b/crates/core/src/state/mod.rs @@ -27,6 +27,8 @@ mod app_impl_analysis; mod app_impl_analysis_tests; mod app_impl_arithmetic; mod app_impl_compute; +#[cfg(test)] +mod app_impl_compute_tests; mod app_impl_io; mod app_impl_linefit; mod app_impl_multiplet; diff --git a/crates/core/src/state/plot_object.rs b/crates/core/src/state/plot_object.rs index 4771854..e03bdc8 100644 --- a/crates/core/src/state/plot_object.rs +++ b/crates/core/src/state/plot_object.rs @@ -1,7 +1,41 @@ -use super::{CanvasViewport, DatasetId, PlotObject}; +use super::{CanvasViewport, DatasetId, PlotObject, SeriesId}; use plotx_figure::Figure; impl PlotObject { + pub fn allocate_series_id(&mut self) -> SeriesId { + let id = self.next_series_id; + self.next_series_id = id.checked_advance(1); + id + } + + /// Assign identities to a newly materialized binding in order. Callers that + /// restore persisted bindings must preserve their ids and use + /// `repair_series_allocator` instead. + pub fn mint_series_ids(&mut self) { + let start = self.next_series_id; + for (offset, series) in self.binding.series.iter_mut().enumerate() { + series.id = start.checked_advance(offset as u64); + } + self.next_series_id = start.checked_advance(self.binding.series.len() as u64); + } + + /// Raise the allocator above every id the (possibly persisted) binding + /// already carries, so the next `allocate_series_id` cannot alias one. + /// + /// Returns `None` when the binding's highest id is `u64::MAX` and no + /// successor exists: the file is unusable rather than merely inconsistent, + /// and the caller must reject it. Defaulting to zero here would skip the + /// repair entirely and hand out a duplicate on the very next allocation. + #[must_use] + pub fn repair_series_allocator(&mut self) -> Option<()> { + let Some(highest) = self.binding.series.iter().map(|series| series.id).max() else { + return Some(()); + }; + let required = highest.try_advance(1)?; + self.next_series_id = self.next_series_id.max(required); + Some(()) + } + pub fn primary_dataset(&self) -> Option { self.binding.primary_dataset() } diff --git a/crates/core/src/state/stack.rs b/crates/core/src/state/stack.rs index 9c67902..c32cb76 100644 --- a/crates/core/src/state/stack.rs +++ b/crates/core/src/state/stack.rs @@ -356,6 +356,20 @@ impl PlotxApp { let figure = self.build_binding_figure(&binding, &chart, &stack, canvas.size_mm); let viewport = CanvasViewport::from_figure(&figure); let panel = PanelMeta::new(self.default_plot_title(sel[0]), frame.width); + let mut plot = PlotObject { + next_series_id: SeriesId::new(0), + binding, + chart, + stack, + projections: AxisProjections::default(), + axis_overrides: AxisOverrides::default(), + figure, + viewport, + panel, + }; + // One place decides how a freshly materialized binding is numbered, so + // the ids and the allocator cannot drift apart. + plot.mint_series_ids(); canvas.objects.push(CanvasObject { id, name: "Plot 1".to_owned(), @@ -363,16 +377,7 @@ impl PlotxApp { locked: false, visible: true, group: None, - kind: CanvasObjectKind::Plot(Box::new(PlotObject { - binding, - chart, - stack, - projections: AxisProjections::default(), - axis_overrides: AxisOverrides::default(), - figure, - viewport, - panel, - })), + kind: CanvasObjectKind::Plot(Box::new(plot)), }); let index = self.doc.canvases.len(); self.execute_action(Action::insert_canvas( diff --git a/crates/core/src/state/table_edit.rs b/crates/core/src/state/table_edit.rs index 921df96..56fb5d6 100644 --- a/crates/core/src/state/table_edit.rs +++ b/crates/core/src/state/table_edit.rs @@ -219,7 +219,7 @@ mod tests { let mut app = PlotxApp::new(); app.doc.datasets.push(Dataset::Table(Box::new(dataset))); - app.execute_action(Action::edit_table(0, delta)); + app.execute_action(Action::edit_table(app.doc.datasets[0].resource_id(), delta)); assert!( app.doc.datasets[0].as_table().unwrap().series_bindings[0] .fit diff --git a/crates/core/src/state/table_execution_job.rs b/crates/core/src/state/table_execution_job.rs index 16c9d58..0128b94 100644 --- a/crates/core/src/state/table_execution_job.rs +++ b/crates/core/src/state/table_execution_job.rs @@ -18,7 +18,7 @@ pub struct TableTransformJob { } pub struct TableRefreshJob { - dataset: usize, + dataset: DatasetId, epoch: u64, started_at: Instant, cancel: Arc, @@ -157,12 +157,14 @@ impl crate::state::PlotxApp { if self.session.table_transform_job.is_some() || self.session.table_refresh_job.is_some() { return Err("A table operation is already running.".into()); } - let before = self + let (dataset_id, before) = self .doc .datasets .get(dataset) - .and_then(crate::state::Dataset::as_table) - .map(|table| table.typed_state.clone()) + .and_then(|value| { + let table = value.as_table()?; + Some((value.resource_id(), table.typed_state.clone())) + }) .ok_or_else(|| "Select a derived data table to refresh.".to_owned())?; let inputs = self.typed_inputs(&input_datasets)?; let worker_derived = before.clone(); @@ -182,7 +184,7 @@ impl crate::state::PlotxApp { let _ = tx.send(result); }); self.session.table_refresh_job = Some(TableRefreshJob { - dataset, + dataset: dataset_id, epoch: self.session.dataset_epoch, started_at: Instant::now(), cancel, @@ -302,24 +304,32 @@ impl crate::state::PlotxApp { input_datasets: &[usize], memory_limit_bytes: u64, ) -> Result<(), String> { - let derived = self + let (dataset_id, derived) = self .doc .datasets .get(dataset) - .and_then(crate::state::Dataset::as_table) - .map(|table| table.typed_state.clone()) + .and_then(|value| { + let table = value.as_table()?; + Some((value.resource_id(), table.typed_state.clone())) + }) .ok_or_else(|| "Select a derived data table to refresh.".to_owned())?; let input_ids: Vec = input_datasets .iter() - .map(|&index| self.doc.datasets[index].resource_id()) - .collect(); + .map(|&index| { + self.doc + .datasets + .get(index) + .map(crate::state::Dataset::resource_id) + .ok_or_else(|| "An input table is no longer available.".to_owned()) + }) + .collect::>()?; let inputs = self.typed_inputs(&input_ids)?; let refs = inputs.iter().collect::>(); let refreshed = refresh_typed_plan(&derived, &refs, memory_limit_bytes, &BTreeSet::new()) .map_err(|error| error.to_string())?; let revision = refreshed.envelope.revision.id; self.execute_action(crate::actions::Action::SetTypedTableState { - dataset, + dataset: dataset_id, before: Box::new(derived), after: Box::new(refreshed), }); diff --git a/crates/core/src/state/ui_state.rs b/crates/core/src/state/ui_state.rs index 2e1da96..dc4bd98 100644 --- a/crates/core/src/state/ui_state.rs +++ b/crates/core/src/state/ui_state.rs @@ -454,8 +454,11 @@ pub struct UiState { /// The chosen slice orientation for a true-2D spectrum (ignored for a stack, /// whose slices are always increments). pub slice_kind: plotx_processing::SliceKind, - /// The processing step whose inline editor is expanded, if any. - pub proc_expanded_step: Option, + /// The processing step whose inline editor is expanded, if any. `StepId` is + /// owner-local — every dataset numbers its steps from zero — so the owning + /// dataset is stored alongside it; without it, expanding a row on one + /// dataset would light up the same-numbered row on every other one. + pub proc_expanded_step: Option<(DatasetId, StepId)>, /// Latched result of the last phase-editing sync: `true` while the canvas is /// held in on-plot phase mode because a Phase step's editor is open. Edge- /// detected so a manual tool switch mid-phasing isn't fought each frame. @@ -465,7 +468,7 @@ pub struct UiState { pub proc_paused: bool, /// While paused, the earliest pre-edit snapshot (dataset + state) so all /// staged edits commit as one undoable step on Apply. - pub proc_pending: Option<(usize, DatasetProcessingState)>, + pub proc_pending: Option<(DatasetId, DatasetProcessingState)>, } impl UiState { diff --git a/crates/core/src/workflow.rs b/crates/core/src/workflow.rs index f5c83ec..7400fbd 100644 --- a/crates/core/src/workflow.rs +++ b/crates/core/src/workflow.rs @@ -258,6 +258,7 @@ pub fn build_plot_object( visible: true, group: None, kind: CanvasObjectKind::Plot(Box::new(PlotObject { + next_series_id: crate::state::SeriesId::new(1), binding: DataBinding::single(dataset.resource_id()), chart, stack: StackSpec::default(), diff --git a/crates/processing/src/align.rs b/crates/processing/src/align.rs index 0e14051..2635e83 100644 --- a/crates/processing/src/align.rs +++ b/crates/processing/src/align.rs @@ -1,11 +1,22 @@ //! Pipeline shifting for multi-spectrum reference alignment. -use crate::{AxisPipeline, ProcessingStep, ReferenceParams, StepKind, StepSource}; +use crate::{AxisPipeline, ProcessingStep, ReferenceParams, StepId, StepKind, StepSource}; /// Shift the axis so the point at `at_ppm` reads `target_ppm`, through the /// pipeline's referencing step: the last enabled Reference step absorbs the /// extra translation, or a new user step is appended when none exists. -pub fn apply_reference_shift(pipe: &mut AxisPipeline, at_ppm: f64, target_ppm: f64) { +/// +/// `new_step_id` is only consumed in that append case, and it must come from +/// the owning dataset's allocator: `pipe` is a live recipe, so a step minted +/// with template-local numbering would collide with the steps already there. +/// This crate sits below `plotx-core` and cannot reach the owner itself, so the +/// identity is supplied by the caller. +pub fn apply_reference_shift( + pipe: &mut AxisPipeline, + at_ppm: f64, + target_ppm: f64, + new_step_id: StepId, +) { let existing = pipe .steps .iter_mut() @@ -18,6 +29,7 @@ pub fn apply_reference_shift(pipe: &mut AxisPipeline, at_ppm: f64, target_ppm: f match existing { Some(r) => r.target_ppm += target_ppm - at_ppm, None => pipe.steps.push(ProcessingStep::new( + new_step_id, StepKind::Reference(ReferenceParams { at_ppm, target_ppm }), StepSource::User, )), @@ -40,7 +52,7 @@ mod tests { }) .collect::>() }; - apply_reference_shift(&mut pipe, 2.0, 2.5); + apply_reference_shift(&mut pipe, 2.0, 2.5, StepId::new(7)); assert_eq!(refs(&pipe).len(), 1); assert_eq!( refs(&pipe)[0], @@ -50,9 +62,21 @@ mod tests { } ); - apply_reference_shift(&mut pipe, 4.0, 4.25); + apply_reference_shift(&mut pipe, 4.0, 4.25, StepId::new(8)); let all = refs(&pipe); assert_eq!(all.len(), 1); assert!((all[0].target_ppm - all[0].at_ppm - 0.75).abs() < 1e-12); } + + /// The appended step takes the caller-supplied identity verbatim: the + /// pipeline it lands in already numbers its own steps from zero, so a + /// template-local id would alias an existing row. + #[test] + fn an_appended_reference_step_uses_the_supplied_identity() { + let mut pipe = AxisPipeline::frequency_1d(); + apply_reference_shift(&mut pipe, 2.0, 2.5, StepId::new(9)); + let appended = pipe.steps.last().expect("a step was appended"); + assert_eq!(appended.id, StepId::new(9)); + assert!(pipe.steps.iter().filter(|s| s.id == StepId::new(9)).count() == 1); + } } diff --git a/crates/processing/src/fft.rs b/crates/processing/src/fft.rs index 2453eda..67eb900 100644 --- a/crates/processing/src/fft.rs +++ b/crates/processing/src/fft.rs @@ -142,21 +142,27 @@ fn fftshift(v: &[Complex64]) -> Vec { #[cfg(test)] mod tests { use super::*; - use crate::{Apodization, AxisPipeline, ProcessingStep, StepKind, StepSource, ZeroFill}; + use crate::{ + Apodization, AxisPipeline, ProcessingStep, StepId, StepKind, StepSource, ZeroFill, + }; use plotx_io::Domain; use std::f64::consts::TAU; + // A detached test recipe: no dataset owns it, so numbering its own steps + // 0..n is enough to keep them distinguishable. fn pipe(apo: Option, zf: ZeroFill) -> AxisPipeline { - let mut steps = Vec::new(); - if let Some(a) = apo { - steps.push(ProcessingStep::new(StepKind::Apodize(a), StepSource::User)); + let kinds = apo + .map(StepKind::Apodize) + .into_iter() + .chain([StepKind::ZeroFill(zf), StepKind::Fft]); + AxisPipeline { + steps: kinds + .enumerate() + .map(|(index, kind)| { + ProcessingStep::new(StepId::new(index as u64), kind, StepSource::User) + }) + .collect(), } - steps.push(ProcessingStep::new( - StepKind::ZeroFill(zf), - StepSource::User, - )); - steps.push(ProcessingStep::new(StepKind::Fft, StepSource::User)); - AxisPipeline { steps } } fn decaying_sinusoid( diff --git a/crates/processing/src/lib.rs b/crates/processing/src/lib.rs index d524691..561a858 100644 --- a/crates/processing/src/lib.rs +++ b/crates/processing/src/lib.rs @@ -288,18 +288,22 @@ impl BinParams { }; } -/// A stable identifier for a step, so callers can address it across edits and -/// reorders. Mint fresh ids with [`StepId::fresh`]. +/// A stable identifier for a step, unique within its owning dataset pipeline. +/// Owner-local: two different datasets both number their steps from zero, so a +/// `StepId` is only meaningful next to the dataset that minted it. #[derive( Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize, serde::Deserialize, )] #[serde(transparent)] -pub struct StepId(pub u64); +pub struct StepId(u64); impl StepId { - pub fn fresh() -> Self { - static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); - StepId(NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed)) + pub const fn new(value: u64) -> Self { + Self(value) + } + + pub const fn get(self) -> u64 { + self.0 } } @@ -365,9 +369,15 @@ pub struct ProcessingStep { } impl ProcessingStep { - pub fn new(kind: StepKind, source: StepSource) -> Self { + /// Build a step with an explicit identity. `id` must come from the owning + /// dataset's allocator whenever the step is destined for a *live* pipeline; + /// only detached recipe values (templates, DTO decoding, tests) may number + /// their own steps, and a dataset adopting such a value must remint them. + /// The parameter is deliberately not defaulted so every call site has to + /// answer where its identity comes from. + pub fn new(id: StepId, kind: StepKind, source: StepSource) -> Self { Self { - id: StepId::fresh(), + id, kind, enabled: true, source, @@ -375,6 +385,20 @@ impl ProcessingStep { } } +/// Mints `0..n` for a *detached* pipeline value that no dataset owns yet. +/// A dataset that adopts the pipeline remints from its own allocator, so these +/// ids never reach a live pipeline unchanged. +#[derive(Default)] +struct TemplateIds(u64); + +impl TemplateIds { + fn next(&mut self) -> StepId { + let id = StepId::new(self.0); + self.0 += 1; + id + } +} + /// An ordered processing recipe for one dimension: the source of truth from /// which the base and display spectra are derived. Steps split by cost — those /// at or before the FFT anchor change the transform (a *retransform*), the rest @@ -386,22 +410,32 @@ pub struct AxisPipeline { impl AxisPipeline { pub fn default_1d() -> Self { - let mut apodize = - ProcessingStep::new(StepKind::Apodize(Apodization::None), StepSource::Default); + let mut ids = TemplateIds::default(); + let mut apodize = ProcessingStep::new( + ids.next(), + StepKind::Apodize(Apodization::None), + StepSource::Default, + ); apodize.enabled = false; + let zero_fill = ProcessingStep::new( + ids.next(), + StepKind::ZeroFill(ZeroFill::None), + StepSource::Default, + ); + let fft = ProcessingStep::new(ids.next(), StepKind::Fft, StepSource::Default); + let phase = ProcessingStep::new( + ids.next(), + StepKind::Phase(PhaseParams::AUTO), + StepSource::Default, + ); let mut baseline = ProcessingStep::new( + ids.next(), StepKind::Baseline(BaselineMethod::AUTO), StepSource::Default, ); baseline.enabled = false; Self { - steps: vec![ - apodize, - ProcessingStep::new(StepKind::ZeroFill(ZeroFill::None), StepSource::Default), - ProcessingStep::new(StepKind::Fft, StepSource::Default), - ProcessingStep::new(StepKind::Phase(PhaseParams::AUTO), StepSource::Default), - baseline, - ], + steps: vec![apodize, zero_fill, fft, phase, baseline], } } @@ -409,16 +443,20 @@ impl AxisPipeline { /// transformed by the instrument software. No time-domain or FFT step is /// represented, so editing this recipe cannot imply a fictitious FID. pub fn frequency_1d() -> Self { + let mut ids = TemplateIds::default(); + let phase = ProcessingStep::new( + ids.next(), + StepKind::Phase(PhaseParams::AUTO), + StepSource::Default, + ); let mut baseline = ProcessingStep::new( + ids.next(), StepKind::Baseline(BaselineMethod::AUTO), StepSource::Default, ); baseline.enabled = false; Self { - steps: vec![ - ProcessingStep::new(StepKind::Phase(PhaseParams::AUTO), StepSource::Default), - baseline, - ], + steps: vec![phase, baseline], } } @@ -428,15 +466,21 @@ impl AxisPipeline { } else { PhaseParams::MANUAL_ZERO }; + let mut ids = TemplateIds::default(); Self { steps: vec![ ProcessingStep::new( + ids.next(), StepKind::Apodize(Apodization::CosineBell), StepSource::Default, ), - ProcessingStep::new(StepKind::ZeroFill(ZeroFill::None), StepSource::Default), - ProcessingStep::new(StepKind::Fft, StepSource::Default), - ProcessingStep::new(StepKind::Phase(phase), StepSource::Default), + ProcessingStep::new( + ids.next(), + StepKind::ZeroFill(ZeroFill::None), + StepSource::Default, + ), + ProcessingStep::new(ids.next(), StepKind::Fft, StepSource::Default), + ProcessingStep::new(ids.next(), StepKind::Phase(phase), StepSource::Default), ], } } @@ -447,8 +491,10 @@ impl AxisPipeline { } else { PhaseParams::MANUAL_ZERO }; + let mut ids = TemplateIds::default(); Self { steps: vec![ProcessingStep::new( + ids.next(), StepKind::Phase(phase), StepSource::Default, )], diff --git a/crates/processing/src/tests.rs b/crates/processing/src/tests.rs index d2b3a8c..0dd983a 100644 --- a/crates/processing/src/tests.rs +++ b/crates/processing/src/tests.rs @@ -93,7 +93,9 @@ fn fid(shift_ppm: f64, group_delay: f64) -> NmrData { } fn step(kind: StepKind) -> ProcessingStep { - ProcessingStep::new(kind, StepSource::User) + static NEXT_TEST_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let id = StepId::new(NEXT_TEST_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed)); + ProcessingStep::new(id, kind, StepSource::User) } fn peak(spec: &Spectrum) -> Complex64 { @@ -196,9 +198,7 @@ fn cleanup_steps_are_frequency_domain_and_reapply_cheaply() { for kind in kinds { assert_eq!(kind.domain(), StepDomain::Freq); let mut edited = base.clone(); - edited - .steps - .push(ProcessingStep::new(kind, StepSource::User)); + edited.steps.push(step(kind)); assert!(!needs_retransform(&base, &edited, true, true)); } }