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)); } }