From ef1956e8e3b5897af824faaf555697870ff88973 Mon Sep 17 00:00:00 2001 From: Skyler Lehmkuhl Date: Wed, 3 Dec 2025 02:28:23 -0500 Subject: [PATCH] Prevent video and audio clips from overlapping --- .../lightningbeam-core/src/action.rs | 114 ++++--- .../src/actions/add_clip_instance.rs | 103 ++++--- .../src/actions/add_layer.rs | 16 +- .../src/actions/add_shape.rs | 24 +- .../src/actions/move_clip_instances.rs | 84 +++++- .../src/actions/move_objects.rs | 14 +- .../src/actions/paint_bucket.rs | 16 +- .../src/actions/set_document_properties.rs | 22 +- .../src/actions/set_instance_properties.rs | 22 +- .../src/actions/set_layer_properties.rs | 42 +-- .../src/actions/set_shape_properties.rs | 14 +- .../src/actions/transform_clip_instances.rs | 26 +- .../src/actions/transform_objects.rs | 28 +- .../src/actions/trim_clip_instances.rs | 112 ++++++- .../lightningbeam-core/src/document.rs | 284 ++++++++++++++++++ .../lightningbeam-editor/src/main.rs | 16 +- .../lightningbeam-editor/src/panes/stage.rs | 52 ++++ .../src/panes/timeline.rs | 120 ++++++-- 18 files changed, 880 insertions(+), 229 deletions(-) diff --git a/lightningbeam-ui/lightningbeam-core/src/action.rs b/lightningbeam-ui/lightningbeam-core/src/action.rs index 1c2acbc..15e4207 100644 --- a/lightningbeam-ui/lightningbeam-core/src/action.rs +++ b/lightningbeam-ui/lightningbeam-core/src/action.rs @@ -58,10 +58,14 @@ pub struct BackendContext<'a> { /// only affect the document (vector graphics) don't need to implement these. pub trait Action: Send { /// Apply this action to the document - fn execute(&mut self, document: &mut Document); + /// + /// Returns Ok(()) if successful, or Err(message) if the action cannot be performed + fn execute(&mut self, document: &mut Document) -> Result<(), String>; /// Undo this action (rollback changes) - fn rollback(&mut self, document: &mut Document); + /// + /// Returns Ok(()) if successful, or Err(message) if rollback fails + fn rollback(&mut self, document: &mut Document) -> Result<(), String>; /// Get a human-readable description of this action (for UI display) fn description(&self) -> String; @@ -158,9 +162,11 @@ impl ActionExecutor { /// Execute an action and add it to the undo stack /// /// This clears the redo stack since we're creating a new timeline branch. - pub fn execute(&mut self, mut action: Box) { + /// + /// Returns Ok(()) if successful, or Err(message) if the action failed + pub fn execute(&mut self, mut action: Box) -> Result<(), String> { // Apply the action (uses copy-on-write if other Arc holders exist) - action.execute(Arc::make_mut(&mut self.document)); + action.execute(Arc::make_mut(&mut self.document))?; // Clear redo stack (new action invalidates redo history) self.redo_stack.clear(); @@ -172,39 +178,55 @@ impl ActionExecutor { if self.undo_stack.len() > self.max_undo_depth { self.undo_stack.remove(0); } + + Ok(()) } /// Undo the last action /// - /// Returns true if an action was undone, false if undo stack is empty. - pub fn undo(&mut self) -> bool { + /// Returns Ok(true) if an action was undone, Ok(false) if undo stack is empty, + /// or Err(message) if rollback failed + pub fn undo(&mut self) -> Result { if let Some(mut action) = self.undo_stack.pop() { // Rollback the action (uses copy-on-write if other Arc holders exist) - action.rollback(Arc::make_mut(&mut self.document)); - - // Move to redo stack - self.redo_stack.push(action); - - true + match action.rollback(Arc::make_mut(&mut self.document)) { + Ok(()) => { + // Move to redo stack + self.redo_stack.push(action); + Ok(true) + } + Err(e) => { + // Put action back on undo stack if rollback failed + self.undo_stack.push(action); + Err(e) + } + } } else { - false + Ok(false) } } /// Redo the last undone action /// - /// Returns true if an action was redone, false if redo stack is empty. - pub fn redo(&mut self) -> bool { + /// Returns Ok(true) if an action was redone, Ok(false) if redo stack is empty, + /// or Err(message) if re-execution failed + pub fn redo(&mut self) -> Result { if let Some(mut action) = self.redo_stack.pop() { // Re-execute the action (uses copy-on-write if other Arc holders exist) - action.execute(Arc::make_mut(&mut self.document)); - - // Move back to undo stack - self.undo_stack.push(action); - - true + match action.execute(Arc::make_mut(&mut self.document)) { + Ok(()) => { + // Move back to undo stack + self.undo_stack.push(action); + Ok(true) + } + Err(e) => { + // Put action back on redo stack if re-execution failed + self.redo_stack.push(action); + Err(e) + } + } } else { - false + Ok(false) } } @@ -273,12 +295,12 @@ impl ActionExecutor { backend: &mut BackendContext, ) -> Result<(), String> { // 1. Execute document changes - action.execute(Arc::make_mut(&mut self.document)); + action.execute(Arc::make_mut(&mut self.document))?; // 2. Execute backend changes (pass document for reading clip data) if let Err(e) = action.execute_backend(backend, &self.document) { // ATOMIC ROLLBACK: Backend failed → undo document - action.rollback(Arc::make_mut(&mut self.document)); + action.rollback(Arc::make_mut(&mut self.document))?; return Err(e); } @@ -309,7 +331,7 @@ impl ActionExecutor { if let Some(mut action) = self.undo_stack.pop() { // Rollback in REVERSE order: backend first, then document action.rollback_backend(backend, &self.document)?; - action.rollback(Arc::make_mut(&mut self.document)); + action.rollback(Arc::make_mut(&mut self.document))?; // Move to redo stack self.redo_stack.push(action); @@ -334,11 +356,15 @@ impl ActionExecutor { pub fn redo_with_backend(&mut self, backend: &mut BackendContext) -> Result { if let Some(mut action) = self.redo_stack.pop() { // Re-execute in same order: document first, then backend - action.execute(Arc::make_mut(&mut self.document)); + if let Err(e) = action.execute(Arc::make_mut(&mut self.document)) { + // Put action back on redo stack if document execute fails + self.redo_stack.push(action); + return Err(e); + } if let Err(e) = action.execute_backend(backend, &self.document) { // Rollback document if backend fails - action.rollback(Arc::make_mut(&mut self.document)); + action.rollback(Arc::make_mut(&mut self.document))?; // Put action back on redo stack self.redo_stack.push(action); return Err(e); @@ -374,12 +400,14 @@ mod tests { } impl Action for TestAction { - fn execute(&mut self, _document: &mut Document) { + fn execute(&mut self, _document: &mut Document) -> Result<(), String> { self.executed = true; + Ok(()) } - fn rollback(&mut self, _document: &mut Document) { + fn rollback(&mut self, _document: &mut Document) -> Result<(), String> { self.executed = false; + Ok(()) } fn description(&self) -> String { @@ -397,20 +425,20 @@ mod tests { // Execute an action let action = Box::new(TestAction::new("Test Action")); - executor.execute(action); + executor.execute(action).unwrap(); assert!(executor.can_undo()); assert!(!executor.can_redo()); assert_eq!(executor.undo_depth(), 1); // Undo - assert!(executor.undo()); + assert!(executor.undo().unwrap()); assert!(!executor.can_undo()); assert!(executor.can_redo()); assert_eq!(executor.redo_depth(), 1); // Redo - assert!(executor.redo()); + assert!(executor.redo().unwrap()); assert!(executor.can_undo()); assert!(!executor.can_redo()); } @@ -420,12 +448,12 @@ mod tests { let document = Document::new("Test"); let mut executor = ActionExecutor::new(document); - executor.execute(Box::new(TestAction::new("Action 1"))); - executor.execute(Box::new(TestAction::new("Action 2"))); + executor.execute(Box::new(TestAction::new("Action 1"))).unwrap(); + executor.execute(Box::new(TestAction::new("Action 2"))).unwrap(); assert_eq!(executor.undo_description(), Some("Action 2".to_string())); - executor.undo(); + executor.undo().unwrap(); assert_eq!(executor.redo_description(), Some("Action 2".to_string())); assert_eq!(executor.undo_description(), Some("Action 1".to_string())); } @@ -435,14 +463,14 @@ mod tests { let document = Document::new("Test"); let mut executor = ActionExecutor::new(document); - executor.execute(Box::new(TestAction::new("Action 1"))); - executor.execute(Box::new(TestAction::new("Action 2"))); - executor.undo(); + executor.execute(Box::new(TestAction::new("Action 1"))).unwrap(); + executor.execute(Box::new(TestAction::new("Action 2"))).unwrap(); + executor.undo().unwrap(); assert!(executor.can_redo()); // Execute new action should clear redo stack - executor.execute(Box::new(TestAction::new("Action 3"))); + executor.execute(Box::new(TestAction::new("Action 3"))).unwrap(); assert!(!executor.can_redo()); assert_eq!(executor.undo_depth(), 2); @@ -454,10 +482,10 @@ mod tests { let mut executor = ActionExecutor::new(document); executor.set_max_undo_depth(3); - executor.execute(Box::new(TestAction::new("Action 1"))); - executor.execute(Box::new(TestAction::new("Action 2"))); - executor.execute(Box::new(TestAction::new("Action 3"))); - executor.execute(Box::new(TestAction::new("Action 4"))); + executor.execute(Box::new(TestAction::new("Action 1"))).unwrap(); + executor.execute(Box::new(TestAction::new("Action 2"))).unwrap(); + executor.execute(Box::new(TestAction::new("Action 3"))).unwrap(); + executor.execute(Box::new(TestAction::new("Action 4"))).unwrap(); // Should only keep last 3 assert_eq!(executor.undo_depth(), 3); diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/add_clip_instance.rs b/lightningbeam-ui/lightningbeam-core/src/actions/add_clip_instance.rs index e36aea1..5bcaa41 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/add_clip_instance.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/add_clip_instance.rs @@ -59,50 +59,81 @@ impl AddClipInstanceAction { } impl Action for AddClipInstanceAction { - fn execute(&mut self, document: &mut Document) { - if let Some(layer) = document.get_layer_mut(&self.layer_id) { - match layer { - AnyLayer::Vector(vector_layer) => { - vector_layer.clip_instances.push(self.clip_instance.clone()); - } - AnyLayer::Audio(audio_layer) => { - audio_layer.clip_instances.push(self.clip_instance.clone()); - } - AnyLayer::Video(video_layer) => { - video_layer.clip_instances.push(self.clip_instance.clone()); - } - } - self.executed = true; + fn execute(&mut self, document: &mut Document) -> Result<(), String> { + // Calculate the clip's effective duration + let clip_duration = document.get_clip_duration(&self.clip_instance.clip_id) + .ok_or_else(|| format!("Clip {} not found", self.clip_instance.clip_id))?; + + let trim_start = self.clip_instance.trim_start; + let trim_end = self.clip_instance.trim_end.unwrap_or(clip_duration); + let effective_duration = trim_end - trim_start; + + // Auto-adjust position for audio/video layers to avoid overlaps + let adjusted_start = document.find_nearest_valid_position( + &self.layer_id, + self.clip_instance.timeline_start, + effective_duration, + None, // Not excluding any instance + ); + + if let Some(valid_start) = adjusted_start { + // Update instance to use the valid position + self.clip_instance.timeline_start = valid_start; + } else { + // No valid position found - reject the operation + return Err("Cannot add clip: no valid position found on layer (layer is full)".to_string()); } + + // Add the clip instance with adjusted position + let layer = document.get_layer_mut(&self.layer_id) + .ok_or_else(|| format!("Layer {} not found", self.layer_id))?; + + match layer { + AnyLayer::Vector(vector_layer) => { + vector_layer.clip_instances.push(self.clip_instance.clone()); + } + AnyLayer::Audio(audio_layer) => { + audio_layer.clip_instances.push(self.clip_instance.clone()); + } + AnyLayer::Video(video_layer) => { + video_layer.clip_instances.push(self.clip_instance.clone()); + } + } + self.executed = true; + + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { if !self.executed { - return; + return Ok(()); } let instance_id = self.clip_instance.id; - if let Some(layer) = document.get_layer_mut(&self.layer_id) { - match layer { - AnyLayer::Vector(vector_layer) => { - vector_layer - .clip_instances - .retain(|ci| ci.id != instance_id); - } - AnyLayer::Audio(audio_layer) => { - audio_layer - .clip_instances - .retain(|ci| ci.id != instance_id); - } - AnyLayer::Video(video_layer) => { - video_layer - .clip_instances - .retain(|ci| ci.id != instance_id); - } + let layer = document.get_layer_mut(&self.layer_id) + .ok_or_else(|| format!("Layer {} not found", self.layer_id))?; + + match layer { + AnyLayer::Vector(vector_layer) => { + vector_layer + .clip_instances + .retain(|ci| ci.id != instance_id); + } + AnyLayer::Audio(audio_layer) => { + audio_layer + .clip_instances + .retain(|ci| ci.id != instance_id); + } + AnyLayer::Video(video_layer) => { + video_layer + .clip_instances + .retain(|ci| ci.id != instance_id); } - self.executed = false; } + self.executed = false; + + Ok(()) } fn description(&self) -> String { @@ -261,7 +292,7 @@ mod tests { // Execute action let mut action = AddClipInstanceAction::new(layer_id, clip_instance); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify clip instance was added if let Some(AnyLayer::Vector(vector_layer)) = document.get_layer(&layer_id) { @@ -272,7 +303,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify clip instance was removed if let Some(AnyLayer::Vector(vector_layer)) = document.get_layer(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/add_layer.rs b/lightningbeam-ui/lightningbeam-core/src/actions/add_layer.rs index 510b088..296713c 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/add_layer.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/add_layer.rs @@ -49,15 +49,17 @@ impl AddLayerAction { } impl Action for AddLayerAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { // Add layer to the document's root let layer_id = document.root_mut().add_child(self.layer.clone()); // Store the ID for rollback self.created_layer_id = Some(layer_id); + + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { // Remove the created layer if it exists if let Some(layer_id) = self.created_layer_id { document.root_mut().remove_child(&layer_id); @@ -65,6 +67,8 @@ impl Action for AddLayerAction { // Clear the stored ID self.created_layer_id = None; } + + Ok(()) } fn description(&self) -> String { @@ -88,7 +92,7 @@ mod tests { // Create and execute action let mut action = AddLayerAction::new_vector("New Layer"); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify layer was added assert_eq!(document.root.children.len(), 1); @@ -97,7 +101,7 @@ mod tests { assert!(matches!(layer, AnyLayer::Vector(_))); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify layer was removed assert_eq!(document.root.children.len(), 0); @@ -116,8 +120,8 @@ mod tests { let mut action1 = AddLayerAction::new_vector("Layer 1"); let mut action2 = AddLayerAction::new_vector("Layer 2"); - action1.execute(&mut document); - action2.execute(&mut document); + action1.execute(&mut document).unwrap(); + action2.execute(&mut document).unwrap(); assert_eq!(document.root.children.len(), 2); assert_eq!(document.root.children[0].layer().name, "Layer 1"); diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/add_shape.rs b/lightningbeam-ui/lightningbeam-core/src/actions/add_shape.rs index b146695..c3cbb61 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/add_shape.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/add_shape.rs @@ -50,10 +50,10 @@ impl AddShapeAction { } impl Action for AddShapeAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; if let AnyLayer::Vector(vector_layer) = layer { @@ -65,14 +65,15 @@ impl Action for AddShapeAction { self.created_shape_id = Some(shape_id); self.created_object_id = Some(object_id); } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { // Remove the created shape and object if they exist if let (Some(shape_id), Some(object_id)) = (self.created_shape_id, self.created_object_id) { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; if let AnyLayer::Vector(vector_layer) = layer { @@ -85,6 +86,7 @@ impl Action for AddShapeAction { self.created_shape_id = None; self.created_object_id = None; } + Ok(()) } fn description(&self) -> String { @@ -114,7 +116,7 @@ mod tests { // Create and execute action let mut action = AddShapeAction::new(layer_id, shape, object); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify shape and object were added if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -129,7 +131,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify shape and object were removed if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -157,7 +159,7 @@ mod tests { assert_eq!(action.description(), "Add shape"); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { assert_eq!(layer.shapes.len(), 1); @@ -180,8 +182,8 @@ mod tests { // Execute twice - shapes are stored in HashMap (keyed by ID, so same shape overwrites) // while shape_instances are stored in Vec (so duplicates accumulate) - action.execute(&mut document); - action.execute(&mut document); + action.execute(&mut document).unwrap(); + action.execute(&mut document).unwrap(); if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { // Shapes use HashMap keyed by shape.id, so same shape overwrites = 1 @@ -209,8 +211,8 @@ mod tests { let mut action1 = AddShapeAction::new(layer_id, shape1, object1); let mut action2 = AddShapeAction::new(layer_id, shape2, object2); - action1.execute(&mut document); - action2.execute(&mut document); + action1.execute(&mut document).unwrap(); + action2.execute(&mut document).unwrap(); if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { // Two different shapes = 2 entries in HashMap diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/move_clip_instances.rs b/lightningbeam-ui/lightningbeam-core/src/actions/move_clip_instances.rs index 1660caa..57cead7 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/move_clip_instances.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/move_clip_instances.rs @@ -26,7 +26,7 @@ impl MoveClipInstancesAction { } impl Action for MoveClipInstancesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { // Expand moves to include grouped instances let mut expanded_moves = self.layer_moves.clone(); let mut already_processed = std::collections::HashSet::new(); @@ -71,15 +71,67 @@ impl Action for MoveClipInstancesAction { } } - // Store expanded moves for rollback - self.layer_moves = expanded_moves.clone(); + // Auto-adjust moves to avoid overlaps + let mut adjusted_moves: HashMap> = HashMap::new(); - // Apply all moves (including expanded) for (layer_id, moves) in &expanded_moves { - let layer = match document.get_layer_mut(layer_id) { - Some(l) => l, - None => continue, - }; + let layer = document.get_layer(layer_id) + .ok_or_else(|| format!("Layer {} not found", layer_id))?; + + // Vector layers don't need adjustment + if matches!(layer, AnyLayer::Vector(_)) { + adjusted_moves.insert(*layer_id, moves.clone()); + continue; + } + + let mut adjusted_layer_moves = Vec::new(); + + for (instance_id, old_start, new_start) in moves { + // Get the instance to calculate its duration + let clip_instances = match layer { + AnyLayer::Audio(al) => &al.clip_instances, + AnyLayer::Video(vl) => &vl.clip_instances, + AnyLayer::Vector(vl) => &vl.clip_instances, + }; + + let instance = clip_instances.iter() + .find(|ci| &ci.id == instance_id) + .ok_or_else(|| format!("Instance {} not found", instance_id))?; + + let clip_duration = document.get_clip_duration(&instance.clip_id) + .ok_or_else(|| format!("Clip {} not found", instance.clip_id))?; + + let trim_start = instance.trim_start; + let trim_end = instance.trim_end.unwrap_or(clip_duration); + let effective_duration = trim_end - trim_start; + + // Find nearest valid position, excluding this instance from overlap checks + let adjusted_start = document.find_nearest_valid_position( + layer_id, + *new_start, + effective_duration, + Some(instance_id), + ); + + if let Some(valid_start) = adjusted_start { + adjusted_layer_moves.push((*instance_id, *old_start, valid_start)); + } else { + return Err(format!( + "Cannot move clip: no valid position found on layer" + )); + } + } + + adjusted_moves.insert(*layer_id, adjusted_layer_moves); + } + + // Store adjusted moves for rollback + self.layer_moves = adjusted_moves.clone(); + + // Apply all adjusted moves + for (layer_id, moves) in &adjusted_moves { + let layer = document.get_layer_mut(layer_id) + .ok_or_else(|| format!("Layer {} not found", layer_id))?; // Get mutable reference to clip_instances for this layer type let clip_instances = match layer { @@ -96,14 +148,14 @@ impl Action for MoveClipInstancesAction { } } } + + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { for (layer_id, moves) in &self.layer_moves { - let layer = match document.get_layer_mut(layer_id) { - Some(l) => l, - None => continue, - }; + let layer = document.get_layer_mut(layer_id) + .ok_or_else(|| format!("Layer {} not found", layer_id))?; // Get mutable reference to clip_instances for this layer type let clip_instances = match layer { @@ -120,6 +172,8 @@ impl Action for MoveClipInstancesAction { } } } + + Ok(()) } fn description(&self) -> String { @@ -296,7 +350,7 @@ mod tests { let mut action = MoveClipInstancesAction::new(layer_moves); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify position changed if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -309,7 +363,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify position restored if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/move_objects.rs b/lightningbeam-ui/lightningbeam-core/src/actions/move_objects.rs index b87af29..d31e892 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/move_objects.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/move_objects.rs @@ -34,10 +34,10 @@ impl MoveShapeInstancesAction { } impl Action for MoveShapeInstancesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; if let AnyLayer::Vector(vector_layer) = layer { @@ -48,12 +48,13 @@ impl Action for MoveShapeInstancesAction { }); } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; if let AnyLayer::Vector(vector_layer) = layer { @@ -64,6 +65,7 @@ impl Action for MoveShapeInstancesAction { }); } } + Ok(()) } fn description(&self) -> String { @@ -109,7 +111,7 @@ mod tests { let mut action = MoveShapeInstancesAction::new(layer_id, positions); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify position changed if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -119,7 +121,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify position restored if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/paint_bucket.rs b/lightningbeam-ui/lightningbeam-core/src/actions/paint_bucket.rs index 560b3b2..f580d61 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/paint_bucket.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/paint_bucket.rs @@ -68,7 +68,7 @@ impl PaintBucketAction { } impl Action for PaintBucketAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { println!("=== PaintBucketAction::execute ==="); // Optimization: Check if we're clicking on an existing shape first @@ -116,7 +116,7 @@ impl Action for PaintBucketAction { println!("Updated shape fill color"); } - return; // Done! No need to create a new shape + return Ok(()); // Done! No need to create a new shape } } } @@ -131,7 +131,7 @@ impl Action for PaintBucketAction { if all_curves.is_empty() { println!("No curves found, returning"); - return; + return Ok(()); } // Step 2: Build planar graph @@ -176,14 +176,15 @@ impl Action for PaintBucketAction { } println!("=== Paint Bucket Complete: Face filled with curves ==="); + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { // Remove the created shape and object if they exist if let (Some(shape_id), Some(object_id)) = (self.created_shape_id, self.created_shape_instance_id) { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; if let AnyLayer::Vector(vector_layer) = layer { @@ -194,6 +195,7 @@ impl Action for PaintBucketAction { self.created_shape_id = None; self.created_shape_instance_id = None; } + Ok(()) } fn description(&self) -> String { @@ -331,7 +333,7 @@ mod tests { GapHandlingMode::BridgeSegment, ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify a filled shape was created if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -343,7 +345,7 @@ mod tests { } // Test rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { // Should only have original shape diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/set_document_properties.rs b/lightningbeam-ui/lightningbeam-core/src/actions/set_document_properties.rs index c073df3..f564ef7 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/set_document_properties.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/set_document_properties.rs @@ -88,7 +88,7 @@ impl SetDocumentPropertiesAction { } impl Action for SetDocumentPropertiesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { // Store old value if not already stored if self.old_value.is_none() { self.old_value = Some(self.get_current_value(document)); @@ -97,12 +97,14 @@ impl Action for SetDocumentPropertiesAction { // Apply new value let new_value = self.property.value(); self.apply_value(document, new_value); + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { if let Some(old_value) = self.old_value { self.apply_value(document, old_value); } + Ok(()) } fn description(&self) -> String { @@ -126,10 +128,10 @@ mod tests { document.width = 1920.0; let mut action = SetDocumentPropertiesAction::set_width(1280.0); - action.execute(&mut document); + action.execute(&mut document).unwrap(); assert_eq!(document.width, 1280.0); - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); assert_eq!(document.width, 1920.0); } @@ -139,10 +141,10 @@ mod tests { document.height = 1080.0; let mut action = SetDocumentPropertiesAction::set_height(720.0); - action.execute(&mut document); + action.execute(&mut document).unwrap(); assert_eq!(document.height, 720.0); - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); assert_eq!(document.height, 1080.0); } @@ -152,10 +154,10 @@ mod tests { document.duration = 10.0; let mut action = SetDocumentPropertiesAction::set_duration(30.0); - action.execute(&mut document); + action.execute(&mut document).unwrap(); assert_eq!(document.duration, 30.0); - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); assert_eq!(document.duration, 10.0); } @@ -165,10 +167,10 @@ mod tests { document.framerate = 30.0; let mut action = SetDocumentPropertiesAction::set_framerate(60.0); - action.execute(&mut document); + action.execute(&mut document).unwrap(); assert_eq!(document.framerate, 60.0); - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); assert_eq!(document.framerate, 30.0); } diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/set_instance_properties.rs b/lightningbeam-ui/lightningbeam-core/src/actions/set_instance_properties.rs index 49cf7ed..25e7645 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/set_instance_properties.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/set_instance_properties.rs @@ -109,7 +109,7 @@ impl SetInstancePropertiesAction { } impl Action for SetInstancePropertiesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { let new_value = self.property.value(); let layer_id = self.layer_id; @@ -140,14 +140,16 @@ impl Action for SetInstancePropertiesAction { for (instance_id, _) in &self.instance_changes { self.apply_to_instance(document, instance_id, new_value); } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { for (instance_id, old_value) in &self.instance_changes { if let Some(value) = old_value { self.apply_to_instance(document, instance_id, *value); } } + Ok(()) } fn description(&self) -> String { @@ -195,7 +197,7 @@ mod tests { instance_id, InstancePropertyChange::X(50.0), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify position changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -205,7 +207,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -233,7 +235,7 @@ mod tests { instance_id, InstancePropertyChange::Rotation(45.0), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify rotation changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -242,7 +244,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -270,7 +272,7 @@ mod tests { instance_id, InstancePropertyChange::Opacity(0.5), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify opacity changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -279,7 +281,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -314,7 +316,7 @@ mod tests { vec![instance1_id, instance2_id], InstancePropertyChange::ScaleX(2.0), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -323,7 +325,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/set_layer_properties.rs b/lightningbeam-ui/lightningbeam-core/src/actions/set_layer_properties.rs index 988fad2..9a8753d 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/set_layer_properties.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/set_layer_properties.rs @@ -74,7 +74,7 @@ impl SetLayerPropertiesAction { } impl Action for SetLayerPropertiesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { for (i, &layer_id) in self.layer_ids.iter().enumerate() { // Find the layer in the document if let Some(layer) = document.root_mut().get_child_mut(&layer_id) { @@ -101,9 +101,10 @@ impl Action for SetLayerPropertiesAction { } } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { for (i, &layer_id) in self.layer_ids.iter().enumerate() { // Find the layer in the document if let Some(layer) = document.root_mut().get_child_mut(&layer_id) { @@ -120,6 +121,7 @@ impl Action for SetLayerPropertiesAction { } } } + Ok(()) } fn description(&self) -> String { @@ -157,14 +159,14 @@ mod tests { // Create and execute action let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Volume(0.5)); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify volume changed let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.volume(), 0.5); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify volume restored let layer_ref = document.root.get_child(&layer_id).unwrap(); @@ -183,13 +185,13 @@ mod tests { // Mute let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Muted(true)); - action.execute(&mut document); + action.execute(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.muted(), true); // Unmute via rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.muted(), false); @@ -208,14 +210,14 @@ mod tests { vec![id1, id2], LayerProperty::Soloed(true), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both soloed assert_eq!(document.root.get_child(&id1).unwrap().soloed(), true); assert_eq!(document.root.get_child(&id2).unwrap().soloed(), true); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both unsoloed assert_eq!(document.root.get_child(&id1).unwrap().soloed(), false); @@ -234,13 +236,13 @@ mod tests { // Lock let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Locked(true)); - action.execute(&mut document); + action.execute(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.locked(), true); // Unlock via rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.locked(), false); @@ -258,13 +260,13 @@ mod tests { // Set opacity to 0.5 let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Opacity(0.5)); - action.execute(&mut document); + action.execute(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.opacity(), 0.5); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.opacity(), 1.0); @@ -282,13 +284,13 @@ mod tests { // Hide let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Visible(false)); - action.execute(&mut document); + action.execute(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.visible(), false); // Show via rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); let layer_ref = document.root.get_child(&layer_id).unwrap(); assert_eq!(layer_ref.visible(), true); @@ -307,14 +309,14 @@ mod tests { vec![id1, id2], LayerProperty::Locked(true), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both locked assert_eq!(document.root.get_child(&id1).unwrap().locked(), true); assert_eq!(document.root.get_child(&id2).unwrap().locked(), true); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both unlocked assert_eq!(document.root.get_child(&id1).unwrap().locked(), false); @@ -334,14 +336,14 @@ mod tests { vec![id1, id2], LayerProperty::Opacity(0.25), ); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both have reduced opacity assert_eq!(document.root.get_child(&id1).unwrap().opacity(), 0.25); assert_eq!(document.root.get_child(&id2).unwrap().opacity(), 0.25); // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both restored to 1.0 assert_eq!(document.root.get_child(&id1).unwrap().opacity(), 1.0); @@ -386,7 +388,7 @@ mod tests { let mut action = SetLayerPropertiesAction::new(fake_id, LayerProperty::Locked(true)); // Should not panic - action.execute(&mut document); - action.rollback(&mut document); + action.execute(&mut document).unwrap(); + action.rollback(&mut document).unwrap(); } } diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/set_shape_properties.rs b/lightningbeam-ui/lightningbeam-core/src/actions/set_shape_properties.rs index 3024557..4629776 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/set_shape_properties.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/set_shape_properties.rs @@ -60,7 +60,7 @@ impl SetShapePropertiesAction { } impl Action for SetShapePropertiesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { if let Some(layer) = document.get_layer_mut(&self.layer_id) { if let AnyLayer::Vector(vector_layer) = layer { if let Some(shape) = vector_layer.shapes.get_mut(&self.shape_id) { @@ -107,9 +107,10 @@ impl Action for SetShapePropertiesAction { } } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { if let Some(old_value) = &self.old_value { if let Some(layer) = document.get_layer_mut(&self.layer_id) { if let AnyLayer::Vector(vector_layer) = layer { @@ -131,6 +132,7 @@ impl Action for SetShapePropertiesAction { } } } + Ok(()) } fn description(&self) -> String { @@ -187,7 +189,7 @@ mod tests { // Create and execute action let new_color = Some(ShapeColor::rgb(0, 255, 0)); let mut action = SetShapePropertiesAction::set_fill_color(layer_id, shape_id, new_color); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify color changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -196,7 +198,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -224,7 +226,7 @@ mod tests { // Create and execute action let mut action = SetShapePropertiesAction::set_stroke_width(layer_id, shape_id, 5.0); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify width changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -233,7 +235,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/transform_clip_instances.rs b/lightningbeam-ui/lightningbeam-core/src/actions/transform_clip_instances.rs index 7a390c7..5974589 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/transform_clip_instances.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/transform_clip_instances.rs @@ -29,10 +29,10 @@ impl TransformClipInstancesAction { } impl Action for TransformClipInstancesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; // Get mutable reference to clip_instances for this layer type @@ -48,12 +48,13 @@ impl Action for TransformClipInstancesAction { clip_instance.transform = new.clone(); } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { let layer = match document.get_layer_mut(&self.layer_id) { Some(l) => l, - None => return, + None => return Ok(()), }; // Get mutable reference to clip_instances for this layer type @@ -69,6 +70,7 @@ impl Action for TransformClipInstancesAction { clip_instance.transform = old.clone(); } } + Ok(()) } fn description(&self) -> String { @@ -108,7 +110,7 @@ mod tests { let mut action = TransformClipInstancesAction::new(layer_id, transforms); // Execute action - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify transform changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -120,7 +122,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify transform restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -153,7 +155,7 @@ mod tests { transforms.insert(instance_id, (old_transform, new_transform)); let mut action = TransformClipInstancesAction::new(layer_id, transforms); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify if let Some(AnyLayer::Audio(al)) = document.get_layer_mut(&layer_id) { @@ -194,7 +196,7 @@ mod tests { transforms.insert(instance_id, (old_transform, new_transform)); let mut action = TransformClipInstancesAction::new(layer_id, transforms); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify rotation and scale if let Some(AnyLayer::Video(vl)) = document.get_layer_mut(&layer_id) { @@ -240,7 +242,7 @@ mod tests { ); let mut action = TransformClipInstancesAction::new(layer_id, transforms); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both transformed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -256,7 +258,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -287,8 +289,8 @@ mod tests { let mut action = TransformClipInstancesAction::new(fake_layer_id, transforms); // Should not panic, just return early - action.execute(&mut document); - action.rollback(&mut document); + action.execute(&mut document).unwrap(); + action.rollback(&mut document).unwrap(); } #[test] diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/transform_objects.rs b/lightningbeam-ui/lightningbeam-core/src/actions/transform_objects.rs index 37faf31..5ea4276 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/transform_objects.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/transform_objects.rs @@ -30,7 +30,7 @@ impl TransformShapeInstancesAction { } impl Action for TransformShapeInstancesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { if let Some(layer) = document.get_layer_mut(&self.layer_id) { if let AnyLayer::Vector(vector_layer) = layer { for (shape_instance_id, (_old, new)) in &self.shape_instance_transforms { @@ -40,9 +40,10 @@ impl Action for TransformShapeInstancesAction { } } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { if let Some(layer) = document.get_layer_mut(&self.layer_id) { if let AnyLayer::Vector(vector_layer) = layer { for (shape_instance_id, (old, _new)) in &self.shape_instance_transforms { @@ -52,6 +53,7 @@ impl Action for TransformShapeInstancesAction { } } } + Ok(()) } fn description(&self) -> String { @@ -89,7 +91,7 @@ mod tests { let mut action = TransformShapeInstancesAction::new(layer_id, transforms); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify transform changed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -101,7 +103,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -140,7 +142,7 @@ mod tests { transforms.insert(instance_id, (old_transform, new_transform)); let mut action = TransformShapeInstancesAction::new(layer_id, transforms); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -187,7 +189,7 @@ mod tests { ); let mut action = TransformShapeInstancesAction::new(layer_id, transforms); - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify both transformed if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -203,7 +205,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify both restored if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { @@ -234,8 +236,8 @@ mod tests { let mut action = TransformShapeInstancesAction::new(fake_layer_id, transforms); // Should not panic - action.execute(&mut document); - action.rollback(&mut document); + action.execute(&mut document).unwrap(); + action.rollback(&mut document).unwrap(); } #[test] @@ -254,8 +256,8 @@ mod tests { let mut action = TransformShapeInstancesAction::new(layer_id, transforms); // Should not panic - just silently skip nonexistent instance - action.execute(&mut document); - action.rollback(&mut document); + action.execute(&mut document).unwrap(); + action.rollback(&mut document).unwrap(); } #[test] @@ -276,8 +278,8 @@ mod tests { let mut action = TransformShapeInstancesAction::new(layer_id, transforms); // Should not panic - action only operates on vector layers - action.execute(&mut document); - action.rollback(&mut document); + action.execute(&mut document).unwrap(); + action.rollback(&mut document).unwrap(); } #[test] diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/trim_clip_instances.rs b/lightningbeam-ui/lightningbeam-core/src/actions/trim_clip_instances.rs index 31af726..1d28314 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/trim_clip_instances.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/trim_clip_instances.rs @@ -62,7 +62,7 @@ impl TrimClipInstancesAction { } impl Action for TrimClipInstancesAction { - fn execute(&mut self, document: &mut Document) { + fn execute(&mut self, document: &mut Document) -> Result<(), String> { // Expand trims to include grouped instances let mut expanded_trims = self.layer_trims.clone(); let mut already_processed = std::collections::HashSet::new(); @@ -155,11 +155,103 @@ impl Action for TrimClipInstancesAction { } } - // Store expanded trims for rollback - self.layer_trims = expanded_trims.clone(); + // Auto-clamp trims to avoid overlaps when extending clips + let mut clamped_trims: HashMap> = HashMap::new(); - // Apply all trims (including expanded) for (layer_id, trims) in &expanded_trims { + let layer = document.get_layer(layer_id) + .ok_or_else(|| format!("Layer {} not found", layer_id))?; + + // Only validate for audio/video layers + let should_validate = matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)); + + let mut clamped_layer_trims = Vec::new(); + + for (instance_id, trim_type, old, new) in trims { + let clip_instances = match layer { + AnyLayer::Audio(al) => &al.clip_instances, + AnyLayer::Video(vl) => &vl.clip_instances, + AnyLayer::Vector(vl) => &vl.clip_instances, + }; + + let instance = clip_instances.iter() + .find(|ci| &ci.id == instance_id) + .ok_or_else(|| format!("Instance {} not found", instance_id))?; + + let clip_duration = document.get_clip_duration(&instance.clip_id) + .ok_or_else(|| format!("Clip {} not found", instance.clip_id))?; + + let mut clamped_new = new.clone(); + + match trim_type { + TrimType::TrimLeft => { + if let (Some(old_trim), Some(new_trim), Some(old_timeline), Some(new_timeline)) = + (old.trim_value, new.trim_value, old.timeline_start, new.timeline_start) + { + // If extending to the left (new_trim < old_trim) + if should_validate && new_trim < old_trim { + // Find the maximum we can extend left + let max_extend = document.find_max_trim_extend_left( + layer_id, + instance_id, + instance.timeline_start, + ); + + // Calculate how much we want to extend + let desired_extend = old_trim - new_trim; + + // Clamp to max allowed + let actual_extend = desired_extend.min(max_extend); + let clamped_trim_start = old_trim - actual_extend; + let clamped_timeline_start = old_timeline - actual_extend; + + clamped_new = TrimData::left(clamped_trim_start, clamped_timeline_start); + } + } + } + TrimType::TrimRight => { + let old_trim_end = old.trim_value.unwrap_or(clip_duration); + let new_trim_end = new.trim_value.unwrap_or(clip_duration); + + // If extending to the right (new_trim_end > old_trim_end) + if should_validate && new_trim_end > old_trim_end { + // Calculate current effective duration + let current_effective_duration = old_trim_end - instance.trim_start; + + // Find the maximum we can extend right + let max_extend = document.find_max_trim_extend_right( + layer_id, + instance_id, + instance.timeline_start, + current_effective_duration, + ); + + // Calculate how much we want to extend + let desired_extend = new_trim_end - old_trim_end; + + // Clamp to max allowed + let actual_extend = desired_extend.min(max_extend); + let clamped_trim_end = old_trim_end + actual_extend; + + // Don't exceed clip duration + let final_trim_end = clamped_trim_end.min(clip_duration); + + clamped_new = TrimData::right(Some(final_trim_end)); + } + } + } + + clamped_layer_trims.push((*instance_id, *trim_type, old.clone(), clamped_new)); + } + + clamped_trims.insert(*layer_id, clamped_layer_trims); + } + + // Store clamped trims for rollback + self.layer_trims = clamped_trims.clone(); + + // Apply all clamped trims + for (layer_id, trims) in &clamped_trims { let layer = match document.get_layer_mut(layer_id) { Some(l) => l, None => continue, @@ -192,9 +284,10 @@ impl Action for TrimClipInstancesAction { } } } + Ok(()) } - fn rollback(&mut self, document: &mut Document) { + fn rollback(&mut self, document: &mut Document) -> Result<(), String> { for (layer_id, trims) in &self.layer_trims { let layer = match document.get_layer_mut(layer_id) { Some(l) => l, @@ -228,6 +321,7 @@ impl Action for TrimClipInstancesAction { } } } + Ok(()) } fn description(&self) -> String { @@ -427,7 +521,7 @@ mod tests { let mut action = TrimClipInstancesAction::new(layer_trims); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify trim applied if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -441,7 +535,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -486,7 +580,7 @@ mod tests { let mut action = TrimClipInstancesAction::new(layer_trims); // Execute - action.execute(&mut document); + action.execute(&mut document).unwrap(); // Verify trim applied if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { @@ -499,7 +593,7 @@ mod tests { } // Rollback - action.rollback(&mut document); + action.rollback(&mut document).unwrap(); // Verify restored if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) { diff --git a/lightningbeam-ui/lightningbeam-core/src/document.rs b/lightningbeam-ui/lightningbeam-core/src/document.rs index 1a389ba..7203013 100644 --- a/lightningbeam-ui/lightningbeam-core/src/document.rs +++ b/lightningbeam-ui/lightningbeam-core/src/document.rs @@ -333,6 +333,290 @@ impl Document { pub fn remove_image_asset(&mut self, id: &Uuid) -> Option { self.image_assets.remove(id) } + + // === CLIP OVERLAP DETECTION METHODS === + + /// Get the duration of any clip type by ID + /// + /// Searches through all clip libraries to find the clip and return its duration + pub fn get_clip_duration(&self, clip_id: &Uuid) -> Option { + if let Some(clip) = self.vector_clips.get(clip_id) { + Some(clip.duration) + } else if let Some(clip) = self.video_clips.get(clip_id) { + Some(clip.duration) + } else if let Some(clip) = self.audio_clips.get(clip_id) { + Some(clip.duration) + } else { + None + } + } + + /// Calculate the end time of a clip instance on the timeline + pub fn get_clip_instance_end_time(&self, layer_id: &Uuid, instance_id: &Uuid) -> Option { + let layer = self.get_layer(layer_id)?; + + // Find the clip instance + let instances = match layer { + AnyLayer::Audio(audio) => &audio.clip_instances, + AnyLayer::Video(video) => &video.clip_instances, + AnyLayer::Vector(vector) => &vector.clip_instances, + }; + + let instance = instances.iter().find(|inst| &inst.id == instance_id)?; + let clip_duration = self.get_clip_duration(&instance.clip_id)?; + + let trim_start = instance.trim_start; + let trim_end = instance.trim_end.unwrap_or(clip_duration); + let effective_duration = trim_end - trim_start; + + Some(instance.timeline_start + effective_duration) + } + + /// Check if a time range overlaps with any existing clip on the layer + /// + /// Returns (overlaps, conflicting_instance_id) + /// + /// Only checks audio and video layers - vector/MIDI layers return false + pub fn check_overlap_on_layer( + &self, + layer_id: &Uuid, + start_time: f64, + end_time: f64, + exclude_instance_id: Option<&Uuid>, + ) -> (bool, Option) { + let Some(layer) = self.get_layer(layer_id) else { + return (false, None); + }; + + // Only check audio and video layers + if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) { + return (false, None); + } + + let instances = match layer { + AnyLayer::Audio(audio) => &audio.clip_instances, + AnyLayer::Video(video) => &video.clip_instances, + AnyLayer::Vector(vector) => &vector.clip_instances, + }; + + for instance in instances { + // Skip the instance we're checking against itself + if let Some(exclude_id) = exclude_instance_id { + if &instance.id == exclude_id { + continue; + } + } + + // Calculate instance end time + let Some(clip_duration) = self.get_clip_duration(&instance.clip_id) else { + continue; + }; + + let instance_start = instance.timeline_start; + let trim_start = instance.trim_start; + let trim_end = instance.trim_end.unwrap_or(clip_duration); + let instance_end = instance_start + (trim_end - trim_start); + + // Check overlap: start_a < end_b AND start_b < end_a + if start_time < instance_end && instance_start < end_time { + return (true, Some(instance.id)); + } + } + + (false, None) + } + + /// Find the nearest valid position for a clip on a layer to avoid overlaps + /// + /// Returns adjusted timeline_start, or None if no valid position exists + /// + /// Strategy: Prefers snapping to the right (later in timeline) over left + pub fn find_nearest_valid_position( + &self, + layer_id: &Uuid, + desired_start: f64, + clip_duration: f64, + exclude_instance_id: Option<&Uuid>, + ) -> Option { + let layer = self.get_layer(layer_id)?; + + // Vector/MIDI layers don't need adjustment + if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) { + return Some(desired_start); + } + + // Check if desired position is already valid + let desired_end = desired_start + clip_duration; + let (overlaps, _) = self.check_overlap_on_layer(layer_id, desired_start, desired_end, exclude_instance_id); + if !overlaps { + return Some(desired_start); + } + + // Collect all existing clip time ranges on this layer + let instances = match layer { + AnyLayer::Audio(audio) => &audio.clip_instances, + AnyLayer::Video(video) => &video.clip_instances, + _ => return Some(desired_start), // Shouldn't reach here + }; + + let mut occupied_ranges: Vec<(f64, f64, Uuid)> = Vec::new(); + for instance in instances { + if let Some(exclude_id) = exclude_instance_id { + if &instance.id == exclude_id { + continue; + } + } + + if let Some(clip_dur) = self.get_clip_duration(&instance.clip_id) { + let inst_start = instance.timeline_start; + let trim_start = instance.trim_start; + let trim_end = instance.trim_end.unwrap_or(clip_dur); + let inst_end = inst_start + (trim_end - trim_start); + occupied_ranges.push((inst_start, inst_end, instance.id)); + } + } + + // Sort by start time + occupied_ranges.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(std::cmp::Ordering::Equal)); + + // Find the clip we're overlapping with + for (occupied_start, occupied_end, _) in &occupied_ranges { + if desired_start < *occupied_end && *occupied_start < desired_end { + // Try snapping to the right (after this clip) + let snap_right = *occupied_end; + let snap_right_end = snap_right + clip_duration; + + let (overlaps_right, _) = self.check_overlap_on_layer( + layer_id, + snap_right, + snap_right_end, + exclude_instance_id, + ); + + if !overlaps_right { + return Some(snap_right); + } + + // Try snapping to the left (before this clip) + let snap_left = occupied_start - clip_duration; + if snap_left >= 0.0 { + let (overlaps_left, _) = self.check_overlap_on_layer( + layer_id, + snap_left, + *occupied_start, + exclude_instance_id, + ); + + if !overlaps_left { + return Some(snap_left); + } + } + } + } + + // If no gap found, try placing at timeline start + if occupied_ranges.is_empty() || occupied_ranges[0].0 >= clip_duration { + return Some(0.0); + } + + // No valid position found + None + } + + /// Find the maximum amount we can extend a clip to the left without overlapping + /// + /// Returns the distance to the nearest clip to the left, or the distance to + /// timeline start (0.0) if no clips exist to the left. + pub fn find_max_trim_extend_left( + &self, + layer_id: &Uuid, + instance_id: &Uuid, + current_timeline_start: f64, + ) -> f64 { + let Some(layer) = self.get_layer(layer_id) else { + return current_timeline_start; // No limit if layer not found + }; + + // Only check audio and video layers + if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) { + return current_timeline_start; // No limit for vector layers + }; + + // Find the nearest clip to the left + let mut nearest_end = 0.0; // Can extend to timeline start by default + + let instances = match layer { + AnyLayer::Audio(audio) => &audio.clip_instances, + AnyLayer::Video(video) => &video.clip_instances, + AnyLayer::Vector(vector) => &vector.clip_instances, + }; + + for other in instances { + if &other.id == instance_id { + continue; + } + + // Calculate other clip's end time + if let Some(clip_duration) = self.get_clip_duration(&other.clip_id) { + let trim_end = other.trim_end.unwrap_or(clip_duration); + let other_end = other.timeline_start + (trim_end - other.trim_start); + + // If this clip is to the left and closer than current nearest + if other_end <= current_timeline_start && other_end > nearest_end { + nearest_end = other_end; + } + } + } + + current_timeline_start - nearest_end + } + + /// Find the maximum amount we can extend a clip to the right without overlapping + /// + /// Returns the distance to the nearest clip to the right, or f64::MAX if no + /// clips exist to the right. + pub fn find_max_trim_extend_right( + &self, + layer_id: &Uuid, + instance_id: &Uuid, + current_timeline_start: f64, + current_effective_duration: f64, + ) -> f64 { + let Some(layer) = self.get_layer(layer_id) else { + return f64::MAX; // No limit if layer not found + }; + + // Only check audio and video layers + if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) { + return f64::MAX; // No limit for vector layers + } + + let instances = match layer { + AnyLayer::Audio(audio) => &audio.clip_instances, + AnyLayer::Video(video) => &video.clip_instances, + AnyLayer::Vector(vector) => &vector.clip_instances, + }; + + let mut nearest_start = f64::MAX; + let current_end = current_timeline_start + current_effective_duration; + + for other in instances { + if &other.id == instance_id { + continue; + } + + // If this clip is to the right and closer than current nearest + if other.timeline_start >= current_end && other.timeline_start < nearest_start { + nearest_start = other.timeline_start; + } + } + + if nearest_start == f64::MAX { + f64::MAX // No clip to the right, can extend freely + } else { + nearest_start - current_timeline_start // Distance to next clip + } + } } #[cfg(test)] diff --git a/lightningbeam-ui/lightningbeam-editor/src/main.rs b/lightningbeam-ui/lightningbeam-editor/src/main.rs index 3890a7b..a67d5c7 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/main.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/main.rs @@ -1024,10 +1024,10 @@ impl EditorApp { Err(e) => eprintln!("Undo failed: {}", e), } } else { - if self.action_executor.undo() { - println!("Undid: {}", self.action_executor.redo_description().unwrap_or_default()); - } else { - println!("Nothing to undo"); + match self.action_executor.undo() { + Ok(true) => println!("Undid: {}", self.action_executor.redo_description().unwrap_or_default()), + Ok(false) => println!("Nothing to undo"), + Err(e) => eprintln!("Undo failed: {}", e), } } } @@ -1046,10 +1046,10 @@ impl EditorApp { Err(e) => eprintln!("Redo failed: {}", e), } } else { - if self.action_executor.redo() { - println!("Redid: {}", self.action_executor.undo_description().unwrap_or_default()); - } else { - println!("Nothing to redo"); + match self.action_executor.redo() { + Ok(true) => println!("Redid: {}", self.action_executor.undo_description().unwrap_or_default()), + Ok(false) => println!("Nothing to redo"), + Err(e) => eprintln!("Redo failed: {}", e), } } } diff --git a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs index 1dc927f..6db5c7f 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs @@ -1283,6 +1283,8 @@ pub struct StagePane { instance_id: u64, // Eyedropper state pending_eyedropper_sample: Option<(egui::Pos2, super::ColorMode)>, + // Last known viewport rect (for zoom-to-fit calculation) + last_viewport_rect: Option, } // Global counter for generating unique instance IDs @@ -1301,6 +1303,7 @@ impl StagePane { last_pan_pos: None, instance_id, pending_eyedropper_sample: None, + last_viewport_rect: None, } } @@ -1338,6 +1341,35 @@ impl StagePane { self.zoom = 1.0; } + /// Zoom to fit the canvas (document dimensions) in the available viewport + pub fn zoom_to_fit(&mut self, shared: &SharedPaneState) { + let document = shared.action_executor.document(); + + // Get document dimensions + let doc_width = document.width as f32; + let doc_height = document.height as f32; + + // Get viewport size from last known rect + let viewport_size = if let Some(rect) = self.last_viewport_rect { + rect.size() + } else { + // Fallback if we don't have a rect yet + egui::vec2(800.0, 600.0) + }; + + // Calculate zoom to fit both width and height (no padding - use entire space) + let zoom_x = viewport_size.x / doc_width; + let zoom_y = viewport_size.y / doc_height; + + // Use the smaller zoom to ensure both dimensions fit + self.zoom = zoom_x.min(zoom_y).clamp(0.1, 10.0); + + // Center the document in the viewport + let canvas_center = egui::vec2(doc_width / 2.0, doc_height / 2.0) * self.zoom; + let viewport_center = viewport_size / 2.0; + self.pan_offset = viewport_center - canvas_center; + } + /// Apply zoom while keeping the point under the mouse cursor stationary fn apply_zoom_at_point(&mut self, zoom_delta: f32, mouse_canvas_pos: egui::Vec2) { let old_zoom = self.zoom; @@ -4124,6 +4156,23 @@ impl StagePane { } impl PaneRenderer for StagePane { + fn render_header(&mut self, ui: &mut egui::Ui, shared: &mut SharedPaneState) -> bool { + ui.horizontal(|ui| { + // Zoom to fit button + if ui.button("⊡ Fit").on_hover_text("Zoom to fit canvas in view").clicked() { + self.zoom_to_fit(shared); + } + + ui.separator(); + + // Zoom level display + let text_style = shared.theme.style(".text-primary", ui.ctx()); + let text_color = text_style.text_color.unwrap_or(egui::Color32::from_gray(200)); + ui.colored_label(text_color, format!("Zoom: {:.0}%", self.zoom * 100.0)); + }); + true + } + fn render_content( &mut self, ui: &mut egui::Ui, @@ -4131,6 +4180,9 @@ impl PaneRenderer for StagePane { _path: &NodePath, shared: &mut SharedPaneState, ) { + // Store viewport rect for zoom-to-fit calculation + self.last_viewport_rect = Some(rect); + // Check for completed eyedropper samples from GPU readback and apply them if let Ok(mut results) = EYEDROPPER_RESULTS .get_or_init(|| Arc::new(Mutex::new(std::collections::HashMap::new()))) diff --git a/lightningbeam-ui/lightningbeam-editor/src/panes/timeline.rs b/lightningbeam-ui/lightningbeam-editor/src/panes/timeline.rs index e0a594c..30afc48 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/panes/timeline.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/panes/timeline.rs @@ -746,7 +746,7 @@ impl TimelinePane { lightningbeam_core::layer::AudioLayerType::Sampled => ("Audio", egui::Color32::from_rgb(100, 180, 255)), // Blue } } - lightningbeam_core::layer::AnyLayer::Video(_) => ("Video", egui::Color32::from_rgb(255, 150, 100)), // Orange/Red + lightningbeam_core::layer::AnyLayer::Video(_) => ("Video", egui::Color32::from_rgb(180, 100, 255)), // Purple }; // Color indicator bar on the left edge @@ -1061,21 +1061,51 @@ impl TimelinePane { let layer_data = layer.layer(); let mut instance_start = clip_instance.timeline_start; - // Apply drag offset preview for selected clips + // Apply drag offset preview for selected clips with snapping let is_selected = selection.contains_clip_instance(&clip_instance.id); if let Some(drag_type) = self.clip_drag_state { if is_selected { match drag_type { ClipDragType::Move => { - // Move: shift the entire clip along the timeline - instance_start += self.drag_offset; + // Move: shift the entire clip along the timeline with auto-snap preview + let desired_start = clip_instance.timeline_start + self.drag_offset; + let current_duration = instance_duration; + + // Find snapped position for preview + let snapped_start = document + .find_nearest_valid_position( + &layer.id(), + desired_start, + current_duration, + Some(&clip_instance.id), + ) + .unwrap_or(desired_start); + + instance_start = snapped_start; } ClipDragType::TrimLeft => { - // Trim left: calculate new trim_start and clamp to valid range - let new_trim_start = (clip_instance.trim_start + self.drag_offset) + // Trim left: calculate new trim_start with snap to adjacent clips + let desired_trim_start = (clip_instance.trim_start + self.drag_offset) .max(0.0) .min(clip_duration); + + let new_trim_start = if desired_trim_start < clip_instance.trim_start { + // Extending left - check for adjacent clips + let max_extend = document.find_max_trim_extend_left( + &layer.id(), + &clip_instance.id, + clip_instance.timeline_start, + ); + + let desired_extend = clip_instance.trim_start - desired_trim_start; + let actual_extend = desired_extend.min(max_extend); + clip_instance.trim_start - actual_extend + } else { + // Shrinking - no snap needed + desired_trim_start + }; + let actual_offset = new_trim_start - clip_instance.trim_start; // Move start and reduce duration by actual clamped offset @@ -1089,11 +1119,32 @@ impl TimelinePane { } } ClipDragType::TrimRight => { - // Trim right: extend or reduce duration, clamped to available content - let max_duration = clip_duration - clip_instance.trim_start; - instance_duration = (instance_duration + self.drag_offset) - .max(0.0) - .min(max_duration); + // Trim right: extend or reduce duration with snap to adjacent clips + let old_trim_end = clip_instance.trim_end.unwrap_or(clip_duration); + let desired_change = self.drag_offset; + let desired_trim_end = (old_trim_end + desired_change) + .max(clip_instance.trim_start) + .min(clip_duration); + + let new_trim_end = if desired_trim_end > old_trim_end { + // Extending right - check for adjacent clips + let current_duration = old_trim_end - clip_instance.trim_start; + let max_extend = document.find_max_trim_extend_right( + &layer.id(), + &clip_instance.id, + clip_instance.timeline_start, + current_duration, + ); + + let desired_extend = desired_trim_end - old_trim_end; + let actual_extend = desired_extend.min(max_extend); + old_trim_end + actual_extend + } else { + // Shrinking - no snap needed + desired_trim_end + }; + + instance_duration = (new_trim_end - clip_instance.trim_start).max(0.0); } } } @@ -1128,8 +1179,8 @@ impl TimelinePane { } } lightningbeam_core::layer::AnyLayer::Video(_) => ( - egui::Color32::from_rgb(255, 150, 100), // Orange/Red - egui::Color32::from_rgb(255, 200, 150), // Bright orange/red + egui::Color32::from_rgb(150, 80, 220), // Purple + egui::Color32::from_rgb(200, 150, 255), // Bright purple ), }; @@ -2081,8 +2132,31 @@ impl PaneRenderer for TimelinePane { ui.painter().rect_filled(highlight_rect, 0.0, highlight_color); - // Show drop time indicator - let drop_time = self.x_to_time(pointer_pos.x - content_rect.min.x); + // Show drop time indicator with snap preview + let raw_drop_time = self.x_to_time(pointer_pos.x - content_rect.min.x).max(0.0); + + // Calculate snapped drop time for preview + let drop_time = if is_compatible { + // Get clip duration to calculate snapped position + let clip_duration = { + let doc = shared.action_executor.document(); + doc.get_clip_duration(&dragging.clip_id).unwrap_or(1.0) + }; + + // Find nearest valid position (auto-snap for preview) + let snapped = shared.action_executor.document() + .find_nearest_valid_position( + &layer.id(), + raw_drop_time, + clip_duration, + None, + ); + + snapped.unwrap_or(raw_drop_time) + } else { + raw_drop_time + }; + let drop_x = self.time_to_x(drop_time); if drop_x >= 0.0 && drop_x <= content_rect.width() { ui.painter().line_segment( @@ -2106,8 +2180,7 @@ impl PaneRenderer for TimelinePane { let center_y = doc.height / 2.0; let mut clip_instance = ClipInstance::new(dragging.clip_id) - .with_timeline_start(drop_time) - .with_position(center_x, center_y); + .with_timeline_start(drop_time); // For video clips, scale to fill document dimensions if dragging.clip_type == DragClipType::Video { @@ -2118,7 +2191,20 @@ impl PaneRenderer for TimelinePane { clip_instance.transform.scale_x = scale_x; clip_instance.transform.scale_y = scale_y; + + // Position at (0, 0) to center the scaled video + // (scaled dimensions = document dimensions, so top-left at origin centers it) + clip_instance.transform.x = 0.0; + clip_instance.transform.y = 0.0; + } else { + // No dimensions available, use document center + clip_instance.transform.x = center_x; + clip_instance.transform.y = center_y; } + } else { + // Non-video clips: center at document center + clip_instance.transform.x = center_x; + clip_instance.transform.y = center_y; } (center_x, center_y, clip_instance)