diff --git a/crates/core/src/actions/app_impl/revert.rs b/crates/core/src/actions/app_impl/revert.rs index 6ffa2bec..cce2c501 100644 --- a/crates/core/src/actions/app_impl/revert.rs +++ b/crates/core/src/actions/app_impl/revert.rs @@ -461,8 +461,8 @@ impl PlotiumApp { self.doc.canvases.remove(*canvas_index); } if *dataset_index < self.doc.datasets.len() { - self.doc.datasets.remove(*dataset_index); - self.session.dataset_epoch += 1; + let dataset = self.doc.datasets.remove(*dataset_index); + self.forget_removed_dataset(*dataset_index, dataset.resource_id()); } self.session.active_canvas = *active_canvas_before; self.set_active_dataset(*active_dataset_before); diff --git a/crates/core/src/properties/service.rs b/crates/core/src/properties/service.rs index 55fb6e17..1049814d 100644 --- a/crates/core/src/properties/service.rs +++ b/crates/core/src/properties/service.rs @@ -189,7 +189,11 @@ impl PlotiumApp { .ok_or_else(|| PropertyError::UnknownProperty(property.as_str().to_owned()))?; let address = PropertyAddress::new(target.clone(), *property); transaction.begin_target(); - match provider.edit(self, &mut transaction, &address, &EditOp::Set(value)) { + let result = provider.edit(self, &mut transaction, &address, &EditOp::Set(value)); + if result.is_ok() { + transaction.record_shared_x_edit(&address); + } + match result { Ok(()) if transaction.target_changed() => applied.push(address), Ok(()) => skipped.push(PropertySkip::new( target.clone(), @@ -248,7 +252,11 @@ impl PlotiumApp { for target in targets { let address = PropertyAddress::new(target.clone(), property); transaction.begin_target(); - match provider.edit(self, &mut transaction, &address, &EditOp::Reset) { + let result = provider.edit(self, &mut transaction, &address, &EditOp::Reset); + if result.is_ok() { + transaction.record_shared_x_edit(&address); + } + match result { Ok(()) if transaction.target_changed() => applied.push(address), Ok(()) => { released.push(address); @@ -580,7 +588,11 @@ impl PlotiumApp { for target in targets { let address = PropertyAddress::new(target.clone(), property); transaction.begin_target(); - match provider.edit(self, &mut transaction, &address, operation) { + let result = provider.edit(self, &mut transaction, &address, operation); + if result.is_ok() { + transaction.record_shared_x_edit(&address); + } + match result { Ok(()) if transaction.target_changed() => applied.push(address), Ok(()) => { if matches!(operation, EditOp::Reset) { diff --git a/crates/core/src/properties/transaction.rs b/crates/core/src/properties/transaction.rs index 91f0c365..65ec33a1 100644 --- a/crates/core/src/properties/transaction.rs +++ b/crates/core/src/properties/transaction.rs @@ -41,6 +41,7 @@ pub(crate) struct PropertyTransaction { bindings: BindingPlan, canvases: Vec, axis_overrides: Vec, + shared_x_edits: Vec, objects: ObjectPlans, typography: Option<(FigureTypography, FigureTypography)>, processing: Vec<(DatasetId, DatasetProcessingState, DatasetProcessingState)>, diff --git a/crates/core/src/properties/transaction_shared_x.rs b/crates/core/src/properties/transaction_shared_x.rs index 33c2dd79..86aa072b 100644 --- a/crates/core/src/properties/transaction_shared_x.rs +++ b/crates/core/src/properties/transaction_shared_x.rs @@ -7,6 +7,21 @@ use crate::state::{AxisOverrides, AxisRange, ObjectId, PlotObject, PlotiumApp}; use plotium_figure::AxisScale; impl PropertyTransaction { + pub(crate) fn record_shared_x_edit(&mut self, address: &PropertyAddress) { + if matches!( + address.definition, + plot_display::X_RANGE + | plot_display::X_BREAK + | plot_display::X_BREAK_FRACTION + | plot_display::X_UNIT + | axis::X_SCALE + | axis::X_THOUSANDS_SEPARATOR + ) && !self.shared_x_edits.contains(address) + { + self.shared_x_edits.push(address.clone()); + } + } + /// Extend the staged axis edits to every stack peer, rejecting the whole /// commit when a peer cannot take a value, and report each peer edit. pub(crate) fn fan_out_shared_x( @@ -14,10 +29,9 @@ impl PropertyTransaction { app: &PlotiumApp, applied: &mut Vec, ) -> Result<(), PropertyError> { - let sources: Vec<_> = self + let requests: Vec<_> = self .axis_overrides .iter() - .filter(|plan| plan.before.shared_x_differs(&plan.after)) .map(|plan| { ( plan.canvas, @@ -27,7 +41,11 @@ impl PropertyTransaction { ) }) .collect(); - for (canvas, object, before, after) in sources { + for (canvas, object, before, after) in &requests { + if !before.shared_x_differs(after) { + continue; + } + let (canvas, object) = (*canvas, *object); let Some(page) = app.doc.canvases.get(canvas) else { continue; }; @@ -47,12 +65,38 @@ impl PropertyTransaction { app, plot, &source_unit, - &before, - &after, + before, + after, &mut self.axis_overrides[index].after, ) .map_err(|error| peer_error(page.object(peer).map(|o| o.name.as_str()), error))?; if let Some(target) = app.object_target(canvas, peer) { + if let Some((_, _, _, requested)) = requests + .iter() + .find(|(c, o, _, _)| *c == canvas && *o == peer) + { + let proposed = &mut self.axis_overrides[index].after; + for address in self.shared_x_edits.iter().filter(|a| a.target == target) { + if !compatible( + address.definition, + requested, + proposed, + source_unit != x_unit(plot), + ) { + return Err(peer_error( + page.object(peer).map(|o| o.name.as_str()), + PropertyError::InvalidValue { + property: address.definition, + message: + "Conflicting explicit settings for one shared X axis." + .into(), + }, + )); + } + // Keep the exact authored value after allowing conversion roundoff. + preserve(address.definition, requested, proposed); + } + } for property in changed { let address = PropertyAddress::new(target.clone(), property); if !applied.contains(&address) { @@ -83,6 +127,51 @@ impl PropertyTransaction { } } +fn compatible(property: PropertyId, a: &AxisOverrides, b: &AxisOverrides, converted: bool) -> bool { + // Only unit conversion may introduce roundoff; distinct native values conflict. + let close = + |a: f64, b: f64| a == b || (converted && (a - b).abs() <= 1e-12 * a.abs().max(b.abs())); + match property { + plot_display::X_RANGE => match (a.x_range, b.x_range) { + (Some(a), Some(b)) => close(a.min, b.min) && close(a.max, b.max), + (a, b) => a == b, + }, + plot_display::X_BREAK => match (a.x_break, b.x_break) { + (Some(a), Some(b)) => close(a.from, b.from) && close(a.to, b.to), + (a, b) => a == b, + }, + plot_display::X_BREAK_FRACTION => { + a.x_break.map(|b| b.lower_fraction) == b.x_break.map(|b| b.lower_fraction) + } + plot_display::X_UNIT => a.x_display_unit == b.x_display_unit, + axis::X_SCALE => a.x_scale == b.x_scale, + axis::X_THOUSANDS_SEPARATOR => a.x_thousands_separator == b.x_thousands_separator, + _ => true, + } +} + +fn preserve(property: PropertyId, from: &AxisOverrides, to: &mut AxisOverrides) { + match property { + plot_display::X_RANGE => to.x_range = from.x_range, + plot_display::X_BREAK => { + let fraction = to.x_break.and_then(|b| b.lower_fraction); + to.x_break = from.x_break.map(|b| plotium_figure::AxisBreak { + lower_fraction: fraction, + ..b + }); + } + plot_display::X_BREAK_FRACTION => { + if let Some(brk) = &mut to.x_break { + brk.lower_fraction = from.x_break.and_then(|b| b.lower_fraction); + } + } + plot_display::X_UNIT => to.x_display_unit.clone_from(&from.x_display_unit), + axis::X_SCALE => to.x_scale = from.x_scale, + axis::X_THOUSANDS_SEPARATOR => to.x_thousands_separator = from.x_thousands_separator, + _ => {} + } +} + fn x_unit(plot: &PlotObject) -> Option { plot.binding .coordinates @@ -136,7 +225,11 @@ fn copy_into( changed.push(plot_display::X_RANGE); } if before.x_break != after.x_break { - target.x_break = after + let endpoints_changed = + before.x_break.map(|b| (b.from, b.to)) != after.x_break.map(|b| (b.from, b.to)); + let fraction_changed = + before.x_break.map(|b| b.lower_fraction) != after.x_break.map(|b| b.lower_fraction); + let converted = after .x_break .map(|brk| { let convert = |value| { @@ -154,7 +247,23 @@ fn copy_into( }) }) .transpose()?; - changed.push(plot_display::X_BREAK); + if endpoints_changed { + let fraction = target.x_break.and_then(|b| b.lower_fraction); + target.x_break = converted.map(|brk| plotium_figure::AxisBreak { + lower_fraction: if fraction_changed { + brk.lower_fraction + } else { + fraction + }, + ..brk + }); + changed.push(plot_display::X_BREAK); + } else if let Some(brk) = &mut target.x_break { + brk.lower_fraction = converted.and_then(|b| b.lower_fraction); + } + if fraction_changed { + changed.push(plot_display::X_BREAK_FRACTION); + } } if before.x_scale != after.x_scale { if after.x_scale == Some(AxisScale::Log10) diff --git a/crates/core/src/state/analysis_collection_tests.rs b/crates/core/src/state/analysis_collection_tests.rs new file mode 100644 index 00000000..ce7d90af --- /dev/null +++ b/crates/core/src/state/analysis_collection_tests.rs @@ -0,0 +1,187 @@ +use super::*; +use std::sync::Arc; + +fn source(kind: ComputeKind) -> Dataset { + if kind == ComputeKind::Craft { + Dataset::Nmr(Box::new(NmrDataset::load(craft_data()).unwrap())) + } else { + Dataset::Nmr2D(Box::new( + Nmr2DDataset::load(data_2d("analysis source")).unwrap(), + )) + } +} + +fn fixture(kind: ComputeKind) -> PlotiumApp { + let mut app = PlotiumApp::new_with_settings(crate::settings::Settings::default()); + app.doc.datasets.push(source(kind)); + app.doc.datasets.push(source(kind)); + app +} + +fn queue(app: &mut PlotiumApp, index: usize, kind: ComputeKind) -> impl FnOnce() + use<> { + let (service, complete) = ComputeService::controlled_for_test(); + app.session.compute = service; + let dataset = app.doc.datasets[index].resource_id(); + if kind == ComputeKind::Craft { + app.session.ui.craft_task_dataset = Some(dataset); + let mut params = plotium_processing::craft::CraftParams::conventional(); + params.fir_filter_taps = 31; + params.maximum_modeled_sample_count = 512; + params.maximum_model_order = 2; + assert!(app.request_craft_analysis( + index, + plotium_processing::craft::CraftParamOverrides::from_params(params), + None, + )); + } else { + // Exercise the direct completion path. Retained scientific API tasks + // separately require their submitted document revision to stay current. + let stack = Arc::new(plotium_processing::StackSpectrum { + direct_domain: Domain::Frequency, + ppm: vec![0.0, 1.0], + traces: [1.0, 0.5, 0.25] + .into_iter() + .map(|value| vec![Complex64::new(value, 0.0); 2]) + .collect(), + direct: plotium_processing::AxisMeta { + nucleus: "1H".into(), + observe_freq_mhz: Some(400.0), + unit: Some(nmr::axis::AxisUnit::Ppm), + }, + source: "analysis source".into(), + }); + let input = crate::DiffusionInput::direct(&[0.0, 1e9, 2e9], "s/m²").unwrap(); + if kind == ComputeKind::Dosy { + app.session + .compute + .enqueue_dosy( + dataset, + app.session.dataset_epoch, + stack, + input, + "1H".into(), + "source".into(), + ) + .unwrap(); + } else { + let params = crate::IltParams { + n_grid: 8, + ..crate::IltParams::default() + }; + app.session + .compute + .enqueue_ilt( + dataset, + app.session.dataset_epoch, + stack, + log_grid(params.d_min, params.d_max, params.n_grid), + params.lambda, + params, + input, + "1H".into(), + "source".into(), + ) + .unwrap(); + } + } + complete +} + +fn installed(dataset: &Dataset, kind: ComputeKind) -> bool { + match kind { + ComputeKind::Craft => !dataset.as_nmr().unwrap().craft_runs.is_empty(), + ComputeKind::Dosy => dataset.as_nmr2d().unwrap().dosy_map.is_some(), + ComputeKind::Ilt => dataset.as_nmr2d().unwrap().ilt_map.is_some(), + _ => unreachable!(), + } +} + +#[test] +fn analysis_completion_survives_unrelated_delete_and_undo_insertion() { + for kind in [ComputeKind::Craft, ComputeKind::Dosy, ComputeKind::Ilt] { + for undo_insert in [false, true] { + let mut app = fixture(kind); + let target = app.doc.datasets[1].resource_id(); + if undo_insert { + app.delete_dataset(0); + } + let target_index = app.doc.dataset_index(target).unwrap(); + let complete = queue(&mut app, target_index, kind); + // Finish before the edit, but deliver after it: cancellation must + // also handle successes already waiting in the completion channel. + complete(); + if undo_insert { + app.undo(); + } else { + app.delete_dataset(0); + } + app.poll_compute(); + assert!(!app.compute_busy()); + assert!( + installed(app.doc.dataset_by_id(target).unwrap(), kind), + "{kind:?}" + ); + if kind == ComputeKind::Craft { + assert_eq!( + app.session.ui.craft_feedback.get(&target), + Some(&CraftRunFeedback::Completed(CraftRunId(0))) + ); + } + } + } +} + +#[test] +fn removed_source_completion_cannot_land_on_replacement_or_undo_restoration() { + for kind in [ComputeKind::Craft, ComputeKind::Dosy, ComputeKind::Ilt] { + for restore in [false, true] { + let mut app = fixture(kind); + let removed = app.doc.datasets[1].resource_id(); + let complete = queue(&mut app, 1, kind); + complete(); + app.delete_dataset(1); + if restore { + app.undo(); + assert_eq!(app.doc.datasets[1].resource_id(), removed); + } else { + app.doc.datasets.push(source(kind)); + assert_ne!(app.doc.datasets[1].resource_id(), removed); + } + app.poll_compute(); + assert!(!app.compute_busy()); + if kind == ComputeKind::Craft { + assert_eq!( + app.session.ui.craft_feedback.get(&removed), + Some(&CraftRunFeedback::Cancelled) + ); + } + assert!( + app.doc + .datasets + .iter() + .all(|dataset| !installed(dataset, kind)), + "{kind:?}" + ); + } + } +} + +#[test] +fn project_replacement_drops_queued_analysis_even_when_dataset_ids_are_reused() { + for kind in [ComputeKind::Craft, ComputeKind::Dosy, ComputeKind::Ilt] { + let mut app = fixture(kind); + let complete = queue(&mut app, 1, kind); + complete(); + let mut loaded = PlotiumApp::new_with_settings(crate::settings::Settings::default()); + loaded.doc.datasets = app.doc.datasets.clone(); + app.install_loaded_project(loaded); + app.poll_compute(); + assert!(!app.compute_busy()); + assert!( + app.doc + .datasets + .iter() + .all(|dataset| !installed(dataset, kind)) + ); + } +} diff --git a/crates/core/src/state/app_impl_compute.rs b/crates/core/src/state/app_impl_compute.rs index eb60d0b4..276565d7 100644 --- a/crates/core/src/state/app_impl_compute.rs +++ b/crates/core/src/state/app_impl_compute.rs @@ -309,6 +309,9 @@ impl PlotiumApp { /// until it lands). pub fn poll_compute(&mut self) -> bool { self.finish_phase_previews(); + // Analysis jobs resolve stable dataset IDs and generations. The global + // epoch protects positional consumers; unrelated collection edits do + // not invalidate these sources. Removal explicitly retires their jobs. for done in self.session.compute.try_drain() { if self.finish_scientific_compute(&done) { continue; @@ -320,17 +323,16 @@ impl PlotiumApp { Done::Ilt { generation, dataset, - epoch, + epoch: _, result, params, provenance, figure, } => { - if epoch != self.session.dataset_epoch - || !self - .session - .compute - .is_current(dataset, ComputeKind::Ilt, generation) + if !self + .session + .compute + .is_current(dataset, ComputeKind::Ilt, generation) { continue; } @@ -374,16 +376,15 @@ impl PlotiumApp { Done::Dosy { generation, dataset, - epoch, + epoch: _, result, provenance, figure, } => { - if epoch != self.session.dataset_epoch - || !self - .session - .compute - .is_current(dataset, ComputeKind::Dosy, generation) + if !self + .session + .compute + .is_current(dataset, ComputeKind::Dosy, generation) { continue; } @@ -421,16 +422,15 @@ impl PlotiumApp { Done::Craft { generation, dataset, - epoch, + epoch: _, result, invocation, parent_run, } => { - if epoch != self.session.dataset_epoch - || !self - .session - .compute - .is_current(dataset, ComputeKind::Craft, generation) + if !self + .session + .compute + .is_current(dataset, ComputeKind::Craft, generation) { continue; } @@ -482,14 +482,13 @@ impl PlotiumApp { Done::CraftFailed { generation, dataset, - epoch, + epoch: _, message, } => { - if epoch == self.session.dataset_epoch - && self - .session - .compute - .is_current(dataset, ComputeKind::Craft, generation) + if self + .session + .compute + .is_current(dataset, ComputeKind::Craft, generation) { self.session.status = format!("CRAFT analysis failed: {message}"); self.session.ui.craft_feedback.insert( diff --git a/crates/core/src/state/app_impl_compute_tests.rs b/crates/core/src/state/app_impl_compute_tests.rs index 29273829..cd309049 100644 --- a/crates/core/src/state/app_impl_compute_tests.rs +++ b/crates/core/src/state/app_impl_compute_tests.rs @@ -362,3 +362,6 @@ fn successful_processing_promotes_fresh_runtime_versions_for_each_scalar_field() ); } } + +#[path = "analysis_collection_tests.rs"] +mod analysis_collection_tests; diff --git a/crates/core/src/state/compute.rs b/crates/core/src/state/compute.rs index 2c52ddc1..62881956 100644 --- a/crates/core/src/state/compute.rs +++ b/crates/core/src/state/compute.rs @@ -718,6 +718,18 @@ impl ComputeService { cancelled } + /// Retire work for a removed source even if undo later restores the same ID. + /// Other datasets keep their generations when collection positions change. + pub(crate) fn invalidate_dataset(&mut self, dataset: DatasetId) { + for ((id, _), active) in &self.active { + if *id == dataset { + active.token.cancel(); + } + } + self.latest.retain(|(id, _), _| *id != dataset); + self.deferred_processing.remove(&dataset); + } + pub(crate) fn latest_generation(&self, dataset: DatasetId, kind: ComputeKind) -> Option { self.latest.get(&(dataset, kind)).copied() } diff --git a/crates/core/src/state/compute_results.rs b/crates/core/src/state/compute_results.rs index c5dd7f85..6468f72d 100644 --- a/crates/core/src/state/compute_results.rs +++ b/crates/core/src/state/compute_results.rs @@ -2,6 +2,30 @@ use super::*; impl ComputeService { + /// Run an enqueued job synchronously and hold its success in the normal + /// completion channel, so tests can edit the document before delivery. + #[cfg(test)] + pub(in crate::state) fn controlled_for_test() -> (Self, impl FnOnce()) { + let (job_tx, job_rx) = std::sync::mpsc::channel(); + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let service = Self { + job_tx, + done_rx, + ..Self::inactive() + }; + let complete = move || { + let done = run_job(job_rx.try_recv().expect("one queued computation")); + assert!(matches!( + done, + Done::Craft { .. } | Done::Dosy { .. } | Done::Ilt { .. } + )); + done_tx + .send(done) + .expect("completion receiver remains live"); + }; + (service, complete) + } + pub fn try_drain(&mut self) -> Vec { self.dispatch_ready_processing(); let mut out = std::mem::take(&mut self.failures); diff --git a/crates/core/src/state/dataset_delete.rs b/crates/core/src/state/dataset_delete.rs index 7f24a77f..9ac6f75c 100644 --- a/crates/core/src/state/dataset_delete.rs +++ b/crates/core/src/state/dataset_delete.rs @@ -90,6 +90,16 @@ impl PlotiumApp { /// Repair index-addressed session state after `datasets.remove(index)`. pub(crate) fn forget_removed_dataset(&mut self, index: usize, id: DatasetId) { + self.session.compute.invalidate_dataset(id); + if matches!( + self.session.ui.craft_feedback.get(&id), + Some(CraftRunFeedback::Running(_)) + ) { + self.session + .ui + .craft_feedback + .insert(id, CraftRunFeedback::Cancelled); + } self.remap_dataset_indices(|stored| match stored.cmp(&index) { std::cmp::Ordering::Less => Some(stored), std::cmp::Ordering::Equal => None, diff --git a/crates/core/src/state/shared_x_stack_review_tests.rs b/crates/core/src/state/shared_x_stack_review_tests.rs index d8f7e8ff..f21d0d3d 100644 --- a/crates/core/src/state/shared_x_stack_review_tests.rs +++ b/crates/core/src/state/shared_x_stack_review_tests.rs @@ -4,9 +4,13 @@ use crate::properties::{PropertyValue, axis}; use crate::state::{Dataset, FloatSeries, PlotiumApp, materialized_float_series_table}; fn table(first: i32, last: i32, tag: &str) -> Dataset { + table_in_unit(first, last, tag, "") +} + +fn table_in_unit(first: i32, last: i32, tag: &str, unit: &str) -> Dataset { let xs: Vec<_> = (first..=last).map(|x| Some(f64::from(x))).collect(); let table = materialized_float_series_table( - ("x".to_owned(), String::new(), xs.clone()), + ("x".to_owned(), unit.to_owned(), xs.clone()), vec![FloatSeries { name: "signal".to_owned(), unit: String::new(), @@ -248,6 +252,264 @@ fn outside_legends_line_up_beside_the_widest_right_axis() { } } +fn range_value(min: f64, max: f64) -> PropertyValue { + PropertyValue::AxisRange(crate::properties::AxisRangeValue::Manual { + min, + max, + unit: None, + }) +} + +fn range_overrides(app: &PlotiumApp, ids: &[ObjectId]) -> Vec { + ids.iter() + .map(|id| { + app.doc.canvases[0] + .object(*id) + .unwrap() + .plot() + .unwrap() + .axis_overrides + .clone() + }) + .collect() +} + +#[test] +fn conflicting_shared_x_batch_is_rejected_without_changes() { + use crate::properties::plot_display::X_RANGE; + let (app, _, ids) = stacked(vec![ + table(1, 10, "plotium.test.stack-conflict-a.v1"), + table(1, 10, "plotium.test.stack-conflict-b.v1"), + ]); + let before = range_overrides(&app, &ids); + let revision = app.doc.automation_revision; + let history = app.session.undo_stack.len(); + let writes = vec![ + ( + app.object_target(0, ids[0]).unwrap(), + X_RANGE, + range_value(0.0, 10.0), + ), + ( + app.object_target(0, ids[1]).unwrap(), + X_RANGE, + range_value(0.0, 20.0), + ), + ]; + for writes in [writes.clone(), writes.into_iter().rev().collect()] { + let error = app + .plan_property_writes(&writes) + .expect_err("conflicting shared ranges"); + assert!( + error.to_string().contains("Conflicting explicit settings"), + "{error}" + ); + assert_eq!(range_overrides(&app, &ids), before); + assert_eq!(app.doc.automation_revision, revision); + assert_eq!(app.session.undo_stack.len(), history); + } +} + +#[test] +fn same_value_shared_x_request_cannot_be_overridden_by_another_member() { + use crate::properties::plot_display::X_RANGE; + let (app, _, ids) = stacked(vec![ + table(1, 10, "plotium.test.stack-noop-a.v1"), + table(1, 10, "plotium.test.stack-noop-b.v1"), + ]); + let range = range_overrides(&app, &ids)[0].x_range.unwrap(); + let error = app + .plan_property_writes(&[ + ( + app.object_target(0, ids[0]).unwrap(), + X_RANGE, + range_value(range.min, range.max), + ), + ( + app.object_target(0, ids[1]).unwrap(), + X_RANGE, + range_value(0.0, 20.0), + ), + ]) + .expect_err("the first request is explicit even though it changes nothing"); + assert!( + error.to_string().contains("Conflicting explicit settings"), + "{error}" + ); +} + +#[test] +fn compatible_shared_x_batch_commits_and_undoes_together() { + use crate::properties::plot_display::X_RANGE; + let (mut app, _, ids) = stacked(vec![ + table(1, 10, "plotium.test.stack-compatible-a.v1"), + table(1, 10, "plotium.test.stack-compatible-b.v1"), + ]); + let before = range_overrides(&app, &ids); + let writes: Vec<_> = ids + .iter() + .map(|id| { + ( + app.object_target(0, *id).unwrap(), + X_RANGE, + range_value(0.0, 20.0), + ) + }) + .collect(); + let commit = app + .plan_property_writes(&writes) + .expect("equivalent shared ranges"); + app.commit_property(commit); + assert_eq!(app.session.undo_stack.len(), 1); + for overrides in range_overrides(&app, &ids) { + assert_eq!(overrides.x_range, Some(AxisRange::new(0.0, 20.0))); + } + app.undo(); + assert_eq!(range_overrides(&app, &ids), before); + app.redo(); + assert_eq!(x_axis(&app, ids[0]), x_axis(&app, ids[1])); + assert_eq!(x_axis(&app, ids[0]).1, 20.0); +} + +#[test] +fn shared_x_batch_and_bulk_writes_respect_coordinate_units() { + use crate::properties::plot_display::X_RANGE; + let (mut app, panel, ids) = stacked(vec![ + table_in_unit(1, 10, "plotium.test.stack-units-a.v1", "s"), + table_in_unit(1, 10, "plotium.test.stack-units-b.v1", "ms"), + ]); + let registry = plotium_data::UnitRegistry::plotium_v1(); + for (id, code) in ids.iter().zip(["s", "ms"]) { + let plot = app.doc.canvases[0] + .object_mut(*id) + .unwrap() + .plot_mut() + .unwrap(); + plot.binding + .coordinates + .get_or_insert_with(crate::state::CurveCoordinates::default) + .x = Some(registry.resolve(code).unwrap()); + } + app.doc.canvases[0].unify_shared_x_window(panel).unwrap(); + let targets: Vec<_> = ids + .iter() + .map(|id| app.object_target(0, *id).unwrap()) + .collect(); + let before = range_overrides(&app, &ids); + let revision = app.doc.automation_revision; + let history = app.session.undo_stack.len(); + app.plan_property_write(X_RANGE, &targets, &range_value(0.0, 20.0)) + .expect_err("known coordinate units require an explicit input unit"); + assert_eq!(range_overrides(&app, &ids), before); + assert_eq!(app.doc.automation_revision, revision); + assert_eq!(app.session.undo_stack.len(), history); + let with_unit = |max, unit: &str| { + PropertyValue::AxisRange(crate::properties::AxisRangeValue::Manual { + min: 0.0, + max, + unit: Some(unit.into()), + }) + }; + let error = app + .plan_property_writes(&[ + (targets[0].clone(), X_RANGE, with_unit(20.0, "s")), + (targets[1].clone(), X_RANGE, with_unit(30_000.0, "ms")), + ]) + .expect_err("different physical windows still conflict across units"); + assert!( + error.to_string().contains("Conflicting explicit settings"), + "{error}" + ); + assert_eq!(range_overrides(&app, &ids), before); + assert_eq!(app.doc.automation_revision, revision); + assert_eq!(app.session.undo_stack.len(), history); + let commit = app + .plan_property_writes(&[ + (targets[0].clone(), X_RANGE, with_unit(20.0, "s")), + (targets[1].clone(), X_RANGE, with_unit(20_000.0, "ms")), + ]) + .expect("physically equivalent ranges in native units"); + app.commit_property(commit); + let overrides = range_overrides(&app, &ids); + assert_eq!(overrides[0].x_range, Some(AxisRange::new(0.0, 20.0))); + assert_eq!(overrides[1].x_range, Some(AxisRange::new(0.0, 20_000.0))); + let commit = app + .plan_property_write(X_RANGE, &targets, &with_unit(30.0, "s")) + .expect("bulk writes convert one explicit unit for each member"); + app.commit_property(commit); + let overrides = range_overrides(&app, &ids); + assert_eq!(overrides[0].x_range, Some(AxisRange::new(0.0, 30.0))); + assert_eq!(overrides[1].x_range, Some(AxisRange::new(0.0, 30_000.0))); +} + +#[test] +fn shared_x_break_endpoints_and_fraction_compose_across_members() { + use crate::properties::plot_display::{X_BREAK, X_BREAK_FRACTION}; + let (mut app, _, ids) = stacked(vec![ + table(1, 10, "plotium.test.stack-break-a.v1"), + table(1, 10, "plotium.test.stack-break-b.v1"), + ]); + let targets: Vec<_> = ids + .iter() + .map(|id| app.object_target(0, *id).unwrap()) + .collect(); + let commit = app + .plan_property_write(X_BREAK, &targets[..1], &range_value(3.0, 5.0)) + .expect("initial shared break"); + app.commit_property(commit); + let writes = vec![ + (targets[0].clone(), X_BREAK, range_value(4.0, 6.0)), + ( + targets[1].clone(), + X_BREAK_FRACTION, + PropertyValue::Float(0.3), + ), + ]; + for writes in [writes.clone(), writes.into_iter().rev().collect()] { + let commit = app + .plan_property_writes(&writes) + .expect("disjoint break edits compose"); + app.commit_property(commit); + for overrides in range_overrides(&app, &ids) { + let brk = overrides.x_break.unwrap(); + assert_eq!((brk.from, brk.to), (4.0, 6.0)); + assert_eq!(brk.lower_fraction, Some(0.3)); + } + app.undo(); + } +} + +#[test] +fn shared_x_scale_and_separator_conflicts_are_rejected() { + let (app, _, ids) = stacked(vec![ + table(1, 10, "plotium.test.stack-settings-a.v1"), + table(1, 10, "plotium.test.stack-settings-b.v1"), + ]); + for (property, first, second) in [ + ( + axis::X_SCALE, + PropertyValue::Enum(axis::LOG), + PropertyValue::Enum(axis::LINEAR), + ), + ( + axis::X_THOUSANDS_SEPARATOR, + PropertyValue::Bool(true), + PropertyValue::Bool(false), + ), + ] { + let error = app + .plan_property_writes(&[ + (app.object_target(0, ids[0]).unwrap(), property, first), + (app.object_target(0, ids[1]).unwrap(), property, second), + ]) + .expect_err("one shared axis cannot keep contradictory settings"); + assert!( + error.to_string().contains("Conflicting explicit settings"), + "{error}" + ); + } +} + #[test] fn a_major_step_reaches_every_member_in_its_own_x_unit() { let (mut app, _, ids) = stacked(vec![ diff --git a/crates/core/src/state/table_update.rs b/crates/core/src/state/table_update.rs index 7f097173..73e3f096 100644 --- a/crates/core/src/state/table_update.rs +++ b/crates/core/src/state/table_update.rs @@ -54,8 +54,61 @@ impl PlotiumApp { if current.table_id != after.envelope.revision.table_id { return Err("The prepared revision belongs to another table.".into()); } + // Plot-owned columns can impose stricter value constraints than the table schema. + // Validate before touching document state or history, including refresh results. + let mut candidate = table.clone(); + candidate.typed_state = after.clone(); + self.validate_table_curve_bindings(&candidate)?; let delta = TableEditDelta::from_revision(table, after); self.try_execute_action(crate::actions::Action::edit_table(dataset, delta)) .map_err(|e| e.to_string()) } + + fn validate_table_curve_bindings(&self, table: &super::TableDataset) -> Result<(), String> { + for plot in self + .doc + .canvases + .iter() + .flat_map(|canvas| canvas.objects.iter().filter_map(|object| object.plot())) + { + let Some(coordinates) = &plot.binding.coordinates else { + continue; + }; + for series in &plot.binding.series { + if series.source.resource != table.resource_id { + continue; + } + let columns = if let Some(columns) = series.source.columns() { + vec![columns] + } else { + let x = table.x_binding.ok_or("Curve X is unbound.")?; + table + .series_bindings + .iter() + .map(|binding| super::TableCurveColumns { + table_id: table.typed_state.envelope.revision.table_id, + x_column_id: x, + y_column_id: binding.value_column, + y_error_column_id: binding.uncertainty_column, + }) + .collect() + }; + for columns in columns { + super::extract_table_curve( + table, + columns, + &coordinates.for_axis(series.y_axis()), + ) + .map_err(|error| { + format!("Table update would invalidate a plotted curve: {error}") + })?; + } + } + } + Ok(()) + } } + +#[cfg(test)] +#[path = "table_update_tests.rs"] +mod tests; diff --git a/crates/core/src/state/table_update_tests.rs b/crates/core/src/state/table_update_tests.rs new file mode 100644 index 00000000..fb6b363c --- /dev/null +++ b/crates/core/src/state/table_update_tests.rs @@ -0,0 +1,224 @@ +use super::*; +use crate::automation::*; +use crate::state::*; +use plotium_data::*; +use serde_json::json; +use std::sync::atomic::AtomicBool; + +fn fixture(derived: bool) -> (PlotiumApp, usize) { + let mut app = PlotiumApp::new(); + let table = TableDataset::from_typed( + TypedTableState::materialized( + vec![ + materialized_float_column("x", "", [Some(0.), Some(1.)]), + materialized_float_column("y", "", [Some(2.), Some(3.)]), + materialized_float_column("sigma", "", [Some(0.1), Some(0.2)]), + ], + vec![], + "plotium.test.table-update.v1", + ) + .unwrap(), + ); + app.doc.datasets.push(Dataset::Table(Box::new(table))); + let index = if derived { + let revision = &app.doc.datasets[0] + .as_table() + .unwrap() + .typed_state + .envelope + .revision; + let plan = RelPlanV1::new(Relation::Project { + input: Box::new(Relation::SnapshotRead(SnapshotRead { + table: revision.table_id, + revision: revision.id, + fingerprint: revision.snapshot.fingerprint, + })), + columns: revision + .snapshot + .schema + .columns + .iter() + .map(|c| c.id) + .collect(), + }); + app.derive_table_from_plan(plan, &[0], "Projected".into(), 16777216) + .unwrap() + } else { + 0 + }; + let dataset = &app.doc.datasets[index]; + let revision = &dataset.as_table().unwrap().typed_state.envelope.revision; + let columns = &revision.snapshot.schema.columns; + let parameters = PlotCreateParams { + parent: document_root_target(), + name: "Curve with errors".into(), + canvas: None, + panel_name: None, + panel_frame_pt: None, + series: vec![NewCurve { + y_axis: Default::default(), + kind: CurveKind::Line, + source: TableCurveSource { + dataset_id: dataset.resource_id(), + field_id: dataset.default_field_id().unwrap(), + table_id: revision.table_id, + table_revision: revision.id, + x_column_id: columns[0].id, + y_column_id: columns[1].id, + y_error_column_id: Some(columns[2].id), + }, + }], + intent: None, + }; + let plan = app + .plan_plot_creation(DocumentRevision(app.doc.automation_revision), ¶meters) + .unwrap(); + app.commit_curve_bindings(plan).unwrap(); + app.clear_history(); + (app, index) +} + +fn patch( + app: &mut PlotiumApp, + index: usize, + value: f64, + automation: bool, +) -> std::result::Result<(), String> { + let dataset = &app.doc.datasets[index]; + let table = dataset.as_table().unwrap(); + let revision = &table.typed_state.envelope.revision; + let column = revision.snapshot.schema.columns[2].id; + let row = table.typed_rows(1, &[column]).unwrap().row_ids[0]; + if automation { + let request = ToolRequest { + tool_id: TABLE_PATCH.into(), + tool_version: 1, + parameters: json!({"table_revision": revision.id, "edits": [{ + "row_id": row, "column_id": column, "value": {"kind": "float64", "value": value} + }]}), + targets: TargetSelector::Explicit { + ids: vec![dataset.resource_id().to_string()], + }, + expected_revision: DocumentRevision(app.doc.automation_revision), + caller: CallerType::Human, + }; + let plan = plan_tool(app, request).map_err(|e| e.to_string())?; + let prepared = TableOperationSnapshot::capture(app, &plan.request) + .and_then(|snapshot| snapshot.prepare(&AtomicBool::new(false))) + .map_err(|e| e.to_string())?; + execute_prepared_table_operation(app, plan, prepared, ExecutionAuthority::ReversibleModify) + .map_err(|e| e.to_string())?; + Ok(()) + } else { + let after = prepare_table_cells( + &table.typed_state, + &[CellPatch { + row, + column, + value: LiteralValue::Float64(FiniteOrSpecial::new(value)), + }], + )?; + app.commit_table_revision(dataset.resource_id(), revision.id, after) + } +} + +fn assert_saveable(app: &PlotiumApp) { + for snapshots in [false, true] { + let path = std::env::temp_dir().join(format!( + "plotium-table-update-{}.plotium", + uuid::Uuid::new_v4() + )); + crate::project::save_project(app, &path, snapshots).unwrap(); + let reopened = crate::project::load_project(&path).unwrap(); + std::fs::remove_file(path).unwrap(); + assert_eq!( + reopened.doc.canvases[0].objects[0].plot().unwrap().binding, + app.doc.canvases[0].objects[0].plot().unwrap().binding + ); + } +} + +#[test] +fn sheet_and_automation_reject_invalid_bound_error_atomically() { + for automation in [false, true] { + let (mut app, _) = fixture(false); + patch(&mut app, 0, 0.4, automation).unwrap(); + app.undo(); + let before = app.doc.datasets[0] + .as_table() + .unwrap() + .typed_state + .envelope + .clone(); + let revision = app.doc.automation_revision; + let figure = app.doc.canvases[0].objects[0] + .plot() + .unwrap() + .figure() + .clone(); + let error = patch(&mut app, 0, -1., automation).unwrap_err(); + assert!( + error.contains("Y error must be a finite non-negative"), + "{error}" + ); + assert_eq!( + app.doc.datasets[0].as_table().unwrap().typed_state.envelope, + before + ); + assert_eq!(app.doc.automation_revision, revision); + assert!(!app.can_undo()); + assert!(app.can_redo()); + assert_eq!( + app.doc.canvases[0].objects[0] + .plot() + .unwrap() + .figure() + .series[0] + .points, + figure.series[0].points + ); + assert_saveable(&app); + app.redo(); + assert!(app.can_undo()); + assert!(!app.can_redo()); + assert_saveable(&app); + patch(&mut app, 0, 0., automation).unwrap(); + assert_saveable(&app); + } +} + +#[test] +fn refresh_rejects_invalid_bound_error_without_adopting_candidate() { + let (mut app, derived) = fixture(true); + // The unbound source accepts negative numeric values; the plotted result cannot. + patch(&mut app, 0, -1., false).unwrap(); + let before = app.doc.datasets[derived] + .as_table() + .unwrap() + .typed_state + .envelope + .clone(); + let revision = app.doc.automation_revision; + let undo = app.session.undo_stack.len(); + let error = app + .refresh_derived_table(derived, &[0], 16777216) + .unwrap_err(); + assert!( + error.contains("Y error must be a finite non-negative"), + "{error}" + ); + assert_eq!( + app.doc.datasets[derived] + .as_table() + .unwrap() + .typed_state + .envelope, + before + ); + assert_eq!(app.doc.automation_revision, revision); + assert_eq!(app.session.undo_stack.len(), undo); + assert_saveable(&app); + patch(&mut app, 0, 0.5, false).unwrap(); + app.refresh_derived_table(derived, &[0], 16777216).unwrap(); + assert_saveable(&app); +} diff --git a/crates/core/src/state/ui_state.rs b/crates/core/src/state/ui_state.rs index e144ac6c..a82cdb54 100644 --- a/crates/core/src/state/ui_state.rs +++ b/crates/core/src/state/ui_state.rs @@ -681,11 +681,9 @@ pub struct Session { /// Unified numerical export worker. Serialization and file I/O stay off the UI thread. pub data_export_job: Option, pub data_export_operation: Option, - /// Bumped whenever a dataset index may now address a different dataset than - /// it did when a background job captured it, so index-addressed results can - /// be rejected. Only removal does that: datasets are always appended, so an - /// insertion leaves every existing index pointing at the same dataset and must - /// not invalidate work in flight. + /// Invalidates background jobs that still capture dataset positions when + /// collection edits can change what an index addresses. ID-based compute + /// completions use their source generation instead. pub dataset_epoch: u64, /// Latched once the user has confirmed (Save or Discard) a close on a dirty /// project, so the re-issued close request passes through instead of looping diff --git a/docs/src/content/docs/guides/craft.md b/docs/src/content/docs/guides/craft.md index b6b12137..113b48e8 100644 --- a/docs/src/content/docs/guides/craft.md +++ b/docs/src/content/docs/guides/craft.md @@ -33,6 +33,9 @@ when you want to inspect the FID without relying on a fully processed spectrum. 7. Choose **Run CRAFT**. The calculation runs in the background; choose **Cancel CRAFT** to stop it. +Deleting an unrelated dataset does not interrupt the run. Deleting its source +dataset cancels the calculation; Undo restores the dataset, but does not restart it. + The setup page shows the acquisition carrier, the applied reference shift, the acquisition duration, and the number of usable points. Invalid FID samples or acquisition metadata, an invalid reference, overlapping or out-of-band groups, diff --git a/docs/src/content/docs/reference/mcp.md b/docs/src/content/docs/reference/mcp.md index 0b4bf935..add4d359 100644 --- a/docs/src/content/docs/reference/mcp.md +++ b/docs/src/content/docs/reference/mcp.md @@ -758,6 +758,9 @@ patch. It cannot insert/delete rows, change schema, reimport files or write to t original CSV/TSV. Preparation produces `update: {status:"prepared", output, before_table_revision, changed}`; nothing is applied until execution. A no-op returns `changed:false` without advancing the document revision or adding Undo. +Before a patch or refresh is committed, its values must remain valid for existing +curve bindings. For example, a column used as Y error cannot contain negative +amplitudes. A rejected update preserves the table, plots and Undo history. Table metadata, row pages and Series sources include `freshness: {status, refreshable, reason, sources, mcp_refreshable, mcp_refresh_reason}`. @@ -1164,7 +1167,10 @@ member's x direction without storing it. The plots share one x viewport, and later x range, x scale, x display unit, x thousands-separator and x major step edits on any member apply to all of them in one Undo step; the write is rejected when any -member cannot take the value, and `applied` lists the member edits. `shared_x: false` returns the Panel to free layout and keeps +member cannot take the value, and `applied` lists the member edits. A batch that +requests conflicting shared x settings is rejected without changes; equivalent +ranges expressed in compatible coordinate units are accepted. +`shared_x: false` returns the Panel to free layout and keeps the frames. The result lists each operation in `panel_edits` with `layout`, `gap_pt`, `members_top_to_bottom` and `merged_panels`. `plotium_document_inspect` reports every Panel's `layout` (`free`, `vertical_stack`, `horizontal_stack`, `grid` or diff --git a/docs/src/content/docs/zh-cn/guides/craft.md b/docs/src/content/docs/zh-cn/guides/craft.md index 7c8ea5a0..2c21e50d 100644 --- a/docs/src/content/docs/zh-cn/guides/craft.md +++ b/docs/src/content/docs/zh-cn/guides/craft.md @@ -26,6 +26,9 @@ CRAFT 直接拟合一维 NMR 采集中的原始复数 FID,并报告共振分 可以运行但需要复核。若显示 **Cannot run**,请按其下方的操作建议处理。 7. 选择 **Run CRAFT**。计算会在后台运行;需要停止时选择 **Cancel CRAFT**。 +删除无关数据集不会中断运行。删除其源数据集会取消计算;Undo 会恢复数据集, +但不会重新启动计算。 + 设置页会显示采集载频、应用的参考位移、采集时长和可用点数。FID 样本或采集元数据 无效、参考信息无效、信号组重叠或超出带宽,或可用点数过少时,运行会被阻止。记录 过短、没有清晰信号或信号组过于拥挤时只会产生警告;请先检查结果再使用。 diff --git a/docs/src/content/docs/zh-cn/reference/mcp.md b/docs/src/content/docs/zh-cn/reference/mcp.md index 108c6a73..75435842 100644 --- a/docs/src/content/docs/zh-cn/reference/mcp.md +++ b/docs/src/content/docs/zh-cn/reference/mcp.md @@ -623,6 +623,8 @@ Int64 列;分类值只能指定已有 level 的索引。此操作不修改扩 旧 revision 均拒绝整批,不留下部分修改。不增删行、改 schema、重导入或回写源 CSV/TSV。准备返回 `update:{status:"prepared",output,before_table_revision,changed}`, 执行前不修改项目。相同值返回 `changed:false`,不推进文档 revision 或增加 Undo。 +补丁或刷新提交前,其数值必须仍满足已有曲线绑定的要求。例如,用作 Y 误差的列 +不能包含负幅度。拒绝更新时,表格、图和 Undo 历史均保持不变。 表格元数据、行页和 Series 来源返回 `freshness:{status,refreshable,reason,sources,mcp_refreshable,mcp_refresh_reason}`。 @@ -954,7 +956,9 @@ Series 身份和来源绑定。已知目标可复用同步提交结果的 contex x 视窗;之后在任一成员上修改 x 范围、x 刻度类型、x 显示单位、x 千位分隔符或 x 主刻度间隔,都会 写到所有成员,并合为一次 Undo;若任一成员不能接受该值则拒绝写入,`applied` 会 -列出各成员的修改。`shared_x: false` 恢复为自由布局并保留各 frame。结果在 `panel_edits` 中列出 +列出各成员的修改。同一批次若请求相互冲突的共享 x 设置,会整批拒绝且不留下修改; +按兼容坐标单位表达的等价范围可以接受。 +`shared_x: false` 恢复为自由布局并保留各 frame。结果在 `panel_edits` 中列出 每个操作的 `layout`、`gap_pt`、`members_top_to_bottom` 和 `merged_panels`。 `plotium_document_inspect` 为每个面板返回 `layout`(`free`、`vertical_stack`、 `horizontal_stack`、`grid` 或 `shared_x_stack`);堆叠面板另含