diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index ac39091c8..57b1cfa56 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -28,12 +28,14 @@ use uuid::Uuid; /// during the garbage-collection process. pub struct Replica { taskdb: TaskDb, + added_undo_point: bool, } impl Replica { pub fn new(storage: Box) -> Replica { Replica { taskdb: TaskDb::new(storage), + added_undo_point: false, } } @@ -55,6 +57,7 @@ impl Replica { S1: Into, S2: Into, { + self.add_undo_point(false)?; self.taskdb.apply(SyncOp::Update { uuid, property: property.into(), @@ -98,6 +101,7 @@ impl Replica { /// Create a new task. The task must not already exist. pub fn new_task(&mut self, status: Status, description: String) -> anyhow::Result { + self.add_undo_point(false)?; let uuid = Uuid::new_v4(); let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?; trace!("task {} created", uuid); @@ -112,6 +116,7 @@ impl Replica { /// should only occur through expiration. #[allow(dead_code)] fn delete_task(&mut self, uuid: Uuid) -> anyhow::Result<()> { + self.add_undo_point(false)?; self.taskdb.apply(SyncOp::Delete { uuid })?; trace!("task {} deleted", uuid); Ok(()) @@ -150,11 +155,24 @@ impl Replica { .rebuild_working_set(|t| t.get("status") == Some(&pending), renumber)?; Ok(()) } + + /// Add an UndoPoint, if one has not already been added by this Replica. This occurs + /// automatically when a change is made. The `force` flag allows forcing a new UndoPoint + /// even if one has laready been created by this Replica, and may be useful when a Replica + /// instance is held for a long time and used to apply more than one user-visible change. + pub fn add_undo_point(&mut self, force: bool) -> anyhow::Result<()> { + if force || !self.added_undo_point { + self.taskdb.add_undo_point()?; + self.added_undo_point = true; + } + Ok(()) + } } #[cfg(test)] mod tests { use super::*; + use crate::storage::ReplicaOp; use crate::task::Status; use pretty_assertions::assert_eq; use uuid::Uuid; @@ -187,10 +205,95 @@ mod tests { assert_eq!(t.get_description(), "past tense"); assert_eq!(t.get_status(), Status::Completed); - // check tha values have changed in storage, too + // check that values have changed in storage, too let t = rep.get_task(t.get_uuid()).unwrap().unwrap(); assert_eq!(t.get_description(), "past tense"); assert_eq!(t.get_status(), Status::Completed); + + // and check for the corresponding operations, cleaning out the timestamps + // and modified properties as these are based on the current time + let now = Utc::now(); + let clean_op = |op: ReplicaOp| { + if let ReplicaOp::Update { + uuid, + property, + mut old_value, + mut value, + .. + } = op + { + if property == "modified" { + if value.is_some() { + value = Some("just-now".into()); + } + if old_value.is_some() { + old_value = Some("just-now".into()); + } + } + ReplicaOp::Update { + uuid, + property, + old_value, + value, + timestamp: now, + } + } else { + op + } + }; + assert_eq!( + rep.taskdb + .operations() + .drain(..) + .map(clean_op) + .collect::>(), + vec![ + ReplicaOp::UndoPoint, + ReplicaOp::Create { uuid: t.get_uuid() }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "modified".into(), + old_value: None, + value: Some("just-now".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "description".into(), + old_value: None, + value: Some("a task".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "status".into(), + old_value: None, + value: Some("P".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "modified".into(), + old_value: Some("just-now".into()), + value: Some("just-now".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "description".into(), + old_value: Some("a task".into()), + value: Some("past tense".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "status".into(), + old_value: Some("P".into()), + value: Some("C".into()), + timestamp: now, + }, + ] + ); } #[test] diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index a31366ddc..283c876d9 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -29,26 +29,33 @@ pub enum ReplicaOp { value: Option, timestamp: DateTime, }, + + /// Mark a point in the operations history to which the user might like to undo. Users + /// typically want to undo more than one operation at a time (for example, most changes update + /// both the `modified` property and some other task property -- the user would like to "undo" + /// both updates at the same time). Applying an UndoPoint does nothing. + UndoPoint, } impl ReplicaOp { /// Convert this operation into a [`SyncOp`]. - pub fn into_sync(self) -> SyncOp { + pub fn into_sync(self) -> Option { match self { - Self::Create { uuid } => SyncOp::Create { uuid }, - Self::Delete { uuid, .. } => SyncOp::Delete { uuid }, + Self::Create { uuid } => Some(SyncOp::Create { uuid }), + Self::Delete { uuid, .. } => Some(SyncOp::Delete { uuid }), Self::Update { uuid, property, value, timestamp, .. - } => SyncOp::Update { + } => Some(SyncOp::Update { uuid, property, value, timestamp, - }, + }), + Self::UndoPoint => None, } } } @@ -145,7 +152,7 @@ mod test { #[test] fn test_into_sync_create() { let uuid = Uuid::new_v4(); - assert_eq!(Create { uuid }.into_sync(), SyncOp::Create { uuid }); + assert_eq!(Create { uuid }.into_sync(), Some(SyncOp::Create { uuid })); } #[test] @@ -157,7 +164,7 @@ mod test { old_task: TaskMap::new() } .into_sync(), - SyncOp::Delete { uuid } + Some(SyncOp::Delete { uuid }) ); } @@ -174,12 +181,17 @@ mod test { timestamp, } .into_sync(), - SyncOp::Update { + Some(SyncOp::Update { uuid, property: "prop".into(), value: Some("v".into()), timestamp, - } + }) ); } + + #[test] + fn test_into_sync_undo_point() { + assert_eq!(UndoPoint.into_sync(), None); + } } diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index 69c0a91e3..4861746e5 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -1,10 +1,7 @@ use crate::server::{Server, SyncOp}; -use crate::storage::{Storage, TaskMap}; +use crate::storage::{ReplicaOp, Storage, TaskMap}; use uuid::Uuid; -#[cfg(test)] -use crate::storage::ReplicaOp; - mod apply; mod snapshot; mod sync; @@ -43,6 +40,13 @@ impl TaskDb { apply::apply(txn.as_mut(), op) } + /// Add an UndoPoint operation to the list of replica operations. + pub fn add_undo_point(&mut self) -> anyhow::Result<()> { + let mut txn = self.storage.txn()?; + txn.add_operation(ReplicaOp::UndoPoint)?; + txn.commit() + } + /// Get all tasks. pub fn all_tasks(&mut self) -> anyhow::Result> { let mut txn = self.storage.txn()?; @@ -171,6 +175,13 @@ mod tests { assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]); } + #[test] + fn test_add_undo_point() { + let mut db = TaskDb::new_inmemory(); + db.add_undo_point().unwrap(); + assert_eq!(db.operations(), vec![ReplicaOp::UndoPoint]); + } + fn newdb() -> TaskDb { TaskDb::new(Box::new(InMemoryStorage::new())) } diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index b6c92f093..6ea11cc6f 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -75,7 +75,7 @@ pub(super) fn sync( let mut local_ops: Vec = txn .operations()? .drain(..) - .map(|op| op.into_sync()) + .filter_map(|op| op.into_sync()) .collect(); // first pull changes and "rebase" on top of them