Correct tape delete

Command diffs shoulid be applied sequentially, should not mix changesets
since later command may depend on the results of the previous.
New issues:
* Gracefully abort or avoid commands that are not currently applicable.

src/changeset.rs

@@ -74,7 +74,19 @@ impl Changeset {
     }
 
     pub fn add(&mut self, action: EventAction) {
-        self.changes.insert(action.event_id(), action);
+        let id = action.event_id();
+        if let Some(prev) = self.changes.insert(id, action) {
+            // Check for consistency.
+            use EventAction::*;
+            match (&prev, &self.changes.get(&id).unwrap()) {
+                // In theory these are OK (the latter just takes precedence) but not expected.
+                (Insert(_), Insert(_)) => panic!("double insert patch, ev.id={}", id),
+                (Delete(_), Delete(_)) => panic!("double delete patch, ev.id={}", id),
+                // Likely CommandDiffs were not applied in the expected order.
+                (Delete(_), Update(_, _)) => panic!("update of a deleted event, ev.id={}", id),
+                (_, _) => (),
+            }
+        }
     }
 
     pub fn add_all(&mut self, actions: &EventActionsList) {

src/edit_commands.rs

@@ -30,20 +30,19 @@ pub enum EditCommandId {
 // Commands that do not usually generate large patches can use generic Changeset, this is the default.
 // Commands that cannot be stored efficiently should use custom diffs.
 // Note to support undo, the event updates must be reversible.
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Serialize, Deserialize)]
 pub enum CommandDiff {
     Changeset { patch: EventActionsList },
     TailShift { at: Time, delta: Time },
 }
 
-// TODO (cleanup) Use a constant, ctx field, or a setting for delta value (see KEYBOARD_TIME_STEP).
-const TIME_DELTA: Time = 10_000;
-
 pub type AppliedCommand = (EditCommandId, Vec<CommandDiff>);
 
-pub fn into_changeset(track: &Track, diffs: &Vec<CommandDiff>, changeset: &mut Changeset) {
+pub fn apply_diffs(track: &mut Track, diffs: &Vec<CommandDiff>) {
     for d in diffs {
-        apply_diff(track, d, changeset);
+        let mut changeset = Changeset::empty();
+        apply_diff(track, d, &mut changeset);
+        track.patch(&changeset);
     }
 }
 
@@ -54,9 +53,11 @@ pub fn apply_diff(track: &Track, diff: &CommandDiff, changeset: &mut Changeset)
     }
 }
 
-pub fn revert_diffs(track: &Track, diffs: &Vec<CommandDiff>, changeset: &mut Changeset) {
+pub fn revert_diffs(track: &mut Track, diffs: &Vec<CommandDiff>) {
     for d in diffs.iter().rev() {
-        revert_diff(track, d, changeset);
+        let mut changeset = Changeset::empty();
+        revert_diff(track, d, &mut changeset);
+        track.patch(&changeset);
     }
 }
 
@@ -87,13 +88,21 @@ fn do_shift_tail(track: &Track, at: &Time, delta: &Time, changeset: &mut Changes
     let mut actions = vec![];
     for ev in &track.events {
         if *at < ev.at {
-            if (ev.at + delta) < *at {
-                /* The action should be replayable and undoable. If we allow events to move earlier
-                then 'at' time, then on undo we should somehow find them still while not confusing them
-                with unchanged events in (at - delta, at] range (when if delta > 0).
-                Track events are expected to be in sorted order, so this will likely trigger early. */
-                break;
-            }
+            /* The action should be replayable and undoable. If we allow events to move earlier
+            then 'at' time, then on undo we should somehow find them still while not confusing them
+            with unchanged events in (at - delta, at] range (when if delta > 0).
+            Track events are expected to be in sorted order, so this will likely trigger early.
+            TODO (implementation) This is a normal situation, should just not apply the command,
+              and maybe hint user that we hit the wall already. Need a way to interrupt this
+              gracefully, or do the check before applying the command diff.
+             */
+            assert!(
+                *at <= (ev.at + delta),
+                "the shift_tail is not undoable at={}, delta={}",
+                at,
+                delta
+            );
+
             actions.push(shift_event(&ev, delta));
         }
     }
@@ -117,14 +126,14 @@ pub fn tape_insert(range: &Range<Time>) -> AppliedCommand {
 }
 
 pub fn tape_delete(track: &Track, range: &Range<Time>) -> AppliedCommand {
+    let delta = range.1 - range.0;
+    assert!(delta >= 0);
     let mut patch = vec![];
     for ev in &track.events {
         if range_contains(range, ev.at) {
             patch.push(EventAction::Delete(ev.clone()));
         }
     }
-    let delta = range.1 - range.0;
-    assert!(delta >= 0);
     (
         EditCommandId::TapeInsert,
         vec![
@@ -397,10 +406,8 @@ mod tests {
     fn check_set_damper_to() {
         let mut track = make_test_track();
         let id_seq = IdSeq::new(0);
-        let applied_command = set_damper(&id_seq, &mut track, &(13, 17), true);
-        let mut changeset = Changeset::empty();
-        into_changeset(&track, &applied_command.1, &mut changeset);
-        track.patch(&changeset);
+        let applied_command = set_damper(&id_seq, &track, &(13, 17), true);
+        apply_diffs(&mut track, &applied_command.1);
 
         let expected_ids: Vec<EventId> = vec![10, 0, 20, 30, 1, 40];
         assert_eq!(

src/track_history.rs

@@ -1,4 +1,3 @@
-use std::cell::RefCell;
 use std::fs;
 use std::path::PathBuf;
 use std::sync::{Arc, RwLock};
@@ -10,7 +9,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::changeset::{Changeset, EventAction, HistoryLogEntry, Snapshot};
 use crate::common::VersionId;
-use crate::edit_commands::{into_changeset, revert_diffs, CommandDiff, EditCommandId};
+use crate::edit_commands::{apply_diffs, revert_diffs, CommandDiff, EditCommandId};
 use crate::track::{import_smf, Track};
 use crate::util;
 use crate::util::IdSeq;
@@ -68,9 +67,7 @@ impl TrackHistory {
         let applied_command = {
             let mut track = self.track.write().expect("Write to track.");
             let applied_command = action(&track);
-            let mut changeset = Changeset::empty();
-            into_changeset(&track, &applied_command.1, &mut changeset);
-            track.patch(&changeset);
+            apply_diffs(&mut track, &applied_command.1);
             track.commit();
             applied_command
         };
@@ -167,19 +164,15 @@ impl TrackHistory {
                 let entry: HistoryLogEntry = util::load(&self.diff_path(self.version + 1));
                 assert_eq!(entry.base_version, self.version);
                 assert!(entry.version > self.version);
-                let mut changeset = Changeset::empty();
-                into_changeset(&track, &entry.diff, &mut changeset);
-                track.patch(&changeset);
+                apply_diffs(&mut track, &entry.diff);
                 self.set_version(entry.version);
             }
             // Rollbacks
             while self.version > version_id {
                 let entry: HistoryLogEntry = util::load(&self.diff_path(self.version));
                 assert_eq!(entry.version, self.version);
                 assert!(entry.base_version < self.version);
-                let mut changeset = Changeset::empty();
-                revert_diffs(&track, &entry.diff, &mut changeset);
-                track.patch(&changeset);
+                revert_diffs(&mut track, &entry.diff);
                 self.set_version(entry.base_version);
             }
         }