diff --git a/cli/src/argparse/args/colon.rs b/cli/src/argparse/args/colon.rs index a0eec90f8..bca014081 100644 --- a/cli/src/argparse/args/colon.rs +++ b/cli/src/argparse/args/colon.rs @@ -1,4 +1,4 @@ -use super::{any, timestamp}; +use super::{any, id_list, timestamp, TaskId}; use crate::argparse::NOW; use nom::bytes::complete::tag as nomtag; use nom::{branch::*, character::complete::*, combinator::*, sequence::*, IResult}; @@ -48,6 +48,17 @@ pub(crate) fn wait_colon(input: &str) -> IResult<&str, Option>> { )(input) } +/// Recognizes `depends:` to `(true, )` and `depends:-` to `(false, )`. +pub(crate) fn depends_colon(input: &str) -> IResult<&str, (bool, Vec)> { + fn to_bool(maybe_minus: Option) -> Result { + Ok(maybe_minus.is_none()) // None -> true, Some -> false + } + preceded( + nomtag("depends:"), + pair(map_res(opt(char('-')), to_bool), id_list), + )(input) +} + #[cfg(test)] mod test { use super::*; diff --git a/cli/src/argparse/args/idlist.rs b/cli/src/argparse/args/idlist.rs index 095dd1cee..a7ea71e0e 100644 --- a/cli/src/argparse/args/idlist.rs +++ b/cli/src/argparse/args/idlist.rs @@ -2,7 +2,7 @@ use nom::{branch::*, character::complete::*, combinator::*, multi::*, sequence:: use taskchampion::Uuid; /// A task identifier, as given in a filter command-line expression -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Hash, Clone)] pub(crate) enum TaskId { /// A small integer identifying a working-set task WorkingSetId(usize), diff --git a/cli/src/argparse/args/mod.rs b/cli/src/argparse/args/mod.rs index f7124fa29..b9c20e9cd 100644 --- a/cli/src/argparse/args/mod.rs +++ b/cli/src/argparse/args/mod.rs @@ -8,7 +8,7 @@ mod tags; mod time; pub(crate) use arg_matching::arg_matching; -pub(crate) use colon::{status_colon, wait_colon}; +pub(crate) use colon::{depends_colon, status_colon, wait_colon}; pub(crate) use idlist::{id_list, TaskId}; pub(crate) use misc::{any, literal, report_name}; pub(crate) use tags::{minus_tag, plus_tag}; diff --git a/cli/src/argparse/modification.rs b/cli/src/argparse/modification.rs index ed597b2e0..21ddeef8e 100644 --- a/cli/src/argparse/modification.rs +++ b/cli/src/argparse/modification.rs @@ -1,4 +1,4 @@ -use super::args::{any, arg_matching, minus_tag, plus_tag, wait_colon}; +use super::args::{any, arg_matching, depends_colon, minus_tag, plus_tag, wait_colon, TaskId}; use super::ArgList; use crate::usage; use nom::{branch::alt, combinator::*, multi::fold_many0, IResult}; @@ -30,27 +30,33 @@ impl Default for DescriptionMod { /// A modification represents a change to a task: adding or removing tags, setting the /// description, and so on. #[derive(Debug, PartialEq, Clone, Default)] -pub struct Modification { +pub(crate) struct Modification { /// Change the description - pub description: DescriptionMod, + pub(crate) description: DescriptionMod, /// Set the status - pub status: Option, + pub(crate) status: Option, /// Set (or, with `Some(None)`, clear) the wait timestamp - pub wait: Option>>, + pub(crate) wait: Option>>, /// Set the "active" state, that is, start (true) or stop (false) the task. - pub active: Option, + pub(crate) active: Option, /// Add tags - pub add_tags: HashSet, + pub(crate) add_tags: HashSet, /// Remove tags - pub remove_tags: HashSet, + pub(crate) remove_tags: HashSet, + + /// Add dependencies + pub(crate) add_dependencies: HashSet, + + /// Remove dependencies + pub(crate) remove_dependencies: HashSet, /// Add annotation - pub annotate: Option, + pub(crate) annotate: Option, } /// A single argument that is part of a modification, used internally to this module @@ -59,6 +65,8 @@ enum ModArg<'a> { PlusTag(Tag), MinusTag(Tag), Wait(Option>), + AddDependencies(Vec), + RemoveDependencies(Vec), } impl Modification { @@ -82,6 +90,16 @@ impl Modification { ModArg::Wait(wait) => { acc.wait = Some(wait); } + ModArg::AddDependencies(task_ids) => { + for tid in task_ids { + acc.add_dependencies.insert(tid); + } + } + ModArg::RemoveDependencies(task_ids) => { + for tid in task_ids { + acc.remove_dependencies.insert(tid); + } + } } acc } @@ -90,6 +108,7 @@ impl Modification { Self::plus_tag, Self::minus_tag, Self::wait, + Self::dependencies, // this must come last Self::description, )), @@ -128,6 +147,17 @@ impl Modification { map_res(arg_matching(wait_colon), to_modarg)(input) } + fn dependencies(input: ArgList) -> IResult { + fn to_modarg(input: (bool, Vec)) -> Result, ()> { + Ok(if input.0 { + ModArg::AddDependencies(input.1) + } else { + ModArg::RemoveDependencies(input.1) + }) + } + map_res(arg_matching(depends_colon), to_modarg)(input) + } + pub(super) fn get_usage(u: &mut usage::Usage) { u.modifications.push(usage::Modification { syntax: "DESCRIPTION", @@ -161,6 +191,19 @@ impl Modification { reports, e.g., `wait:3day` to wait for three days. With `wait:`, the time is un-set. See the documentation for the timestamp syntax.", }); + u.modifications.push(usage::Modification { + syntax: "depends:", + summary: "Add task dependencies", + description: " + Add a dependency of this task on the given tasks. The tasks can be specified + in the same syntax as for filters, e.g., `depends:13,94500c95`.", + }); + u.modifications.push(usage::Modification { + syntax: "depends:-", + summary: "Remove task dependencies", + description: " + Remove the dependency of this task on the given tasks.", + }); } } @@ -222,6 +265,39 @@ mod test { ); } + #[test] + fn test_add_deps() { + let (input, modification) = Modification::parse(argv!["depends:13,e72b73d1-9e88"]).unwrap(); + assert_eq!(input.len(), 0); + let mut deps = HashSet::new(); + deps.insert(TaskId::WorkingSetId(13)); + deps.insert(TaskId::PartialUuid("e72b73d1-9e88".into())); + assert_eq!( + modification, + Modification { + add_dependencies: deps, + ..Default::default() + } + ); + } + + #[test] + fn test_remove_deps() { + let (input, modification) = + Modification::parse(argv!["depends:-13,e72b73d1-9e88"]).unwrap(); + assert_eq!(input.len(), 0); + let mut deps = HashSet::new(); + deps.insert(TaskId::WorkingSetId(13)); + deps.insert(TaskId::PartialUuid("e72b73d1-9e88".into())); + assert_eq!( + modification, + Modification { + remove_dependencies: deps, + ..Default::default() + } + ); + } + #[test] fn test_unset_wait() { let (input, modification) = Modification::parse(argv!["wait:"]).unwrap(); diff --git a/cli/src/invocation/cmd/add.rs b/cli/src/invocation/cmd/add.rs index 1957e6dfc..5dfd5c215 100644 --- a/cli/src/invocation/cmd/add.rs +++ b/cli/src/invocation/cmd/add.rs @@ -1,19 +1,19 @@ -use crate::argparse::{DescriptionMod, Modification}; -use crate::invocation::apply_modification; +use crate::argparse::DescriptionMod; +use crate::invocation::{apply_modification, ResolvedModification}; use taskchampion::{Replica, Status}; use termcolor::WriteColor; -pub(crate) fn execute( +pub(in crate::invocation) fn execute( w: &mut W, replica: &mut Replica, - mut modification: Modification, + mut modification: ResolvedModification, ) -> Result<(), crate::Error> { // extract the description from the modification to handle it specially - let description = match modification.description { + let description = match modification.0.description { DescriptionMod::Set(ref s) => s.clone(), _ => "(no description)".to_owned(), }; - modification.description = DescriptionMod::None; + modification.0.description = DescriptionMod::None; let task = replica.new_task(Status::Pending, description).unwrap(); let mut task = task.into_mut(replica); @@ -25,6 +25,7 @@ pub(crate) fn execute( #[cfg(test)] mod test { use super::*; + use crate::argparse::Modification; use crate::invocation::test::*; use pretty_assertions::assert_eq; @@ -32,10 +33,10 @@ mod test { fn test_add() { let mut w = test_writer(); let mut replica = test_replica(); - let modification = Modification { + let modification = ResolvedModification(Modification { description: DescriptionMod::Set(s!("my description")), ..Default::default() - }; + }); execute(&mut w, &mut replica, modification).unwrap(); // check that the task appeared.. @@ -54,11 +55,11 @@ mod test { fn test_add_with_tags() { let mut w = test_writer(); let mut replica = test_replica(); - let modification = Modification { + let modification = ResolvedModification(Modification { description: DescriptionMod::Set(s!("my description")), add_tags: vec![tag!("tag1")].drain(..).collect(), ..Default::default() - }; + }); execute(&mut w, &mut replica, modification).unwrap(); // check that the task appeared.. diff --git a/cli/src/invocation/cmd/info.rs b/cli/src/invocation/cmd/info.rs index bd7e6e269..2766e6267 100644 --- a/cli/src/invocation/cmd/info.rs +++ b/cli/src/invocation/cmd/info.rs @@ -2,7 +2,7 @@ use crate::argparse::Filter; use crate::invocation::filtered_tasks; use crate::table; use prettytable::{cell, row, Table}; -use taskchampion::Replica; +use taskchampion::{Replica, Status}; use termcolor::WriteColor; pub(crate) fn execute( @@ -44,6 +44,25 @@ pub(crate) fn execute( for ann in annotations { t.add_row(row![b->"Annotation", format!("{}: {}", ann.entry, ann.description)]); } + + let mut deps: Vec<_> = task.get_dependencies().collect(); + deps.sort(); + for dep in deps { + let mut descr = None; + if let Some(task) = replica.get_task(dep)? { + if task.get_status() == Status::Pending { + if let Some(i) = working_set.by_uuid(dep) { + descr = Some(format!("{} - {}", i, task.get_description())) + } else { + descr = Some(format!("{} - {}", dep, task.get_description())) + } + } + } + + if let Some(descr) = descr { + t.add_row(row![b->"Depends On", descr]); + } + } } t.print(w)?; } @@ -54,6 +73,7 @@ pub(crate) fn execute( #[cfg(test)] mod test { use super::*; + use crate::argparse::{Condition, TaskId}; use crate::invocation::test::*; use taskchampion::Status; @@ -71,4 +91,27 @@ mod test { execute(&mut w, &mut replica, filter, debug).unwrap(); assert!(w.into_string().contains("my task")); } + + #[test] + fn test_deps() { + let mut w = test_writer(); + let mut replica = test_replica(); + let t1 = replica.new_task(Status::Pending, s!("my task")).unwrap(); + let t2 = replica + .new_task(Status::Pending, s!("dunno, depends")) + .unwrap(); + let mut t2 = t2.into_mut(&mut replica); + t2.add_dependency(t1.get_uuid()).unwrap(); + let t2 = t2.into_immut(); + + let filter = Filter { + conditions: vec![Condition::IdList(vec![TaskId::Uuid(t2.get_uuid())])], + }; + let debug = false; + execute(&mut w, &mut replica, filter, debug).unwrap(); + let s = w.into_string(); + // length of whitespace between these two strings is not important + assert!(s.contains("Depends On")); + assert!(s.contains("1 - my task")); + } } diff --git a/cli/src/invocation/cmd/modify.rs b/cli/src/invocation/cmd/modify.rs index 3bf7e5b41..cb8eaf5b1 100644 --- a/cli/src/invocation/cmd/modify.rs +++ b/cli/src/invocation/cmd/modify.rs @@ -1,6 +1,6 @@ -use crate::argparse::{Filter, Modification}; +use crate::argparse::Filter; use crate::invocation::util::{confirm, summarize_task}; -use crate::invocation::{apply_modification, filtered_tasks}; +use crate::invocation::{apply_modification, filtered_tasks, ResolvedModification}; use crate::settings::Settings; use taskchampion::Replica; use termcolor::WriteColor; @@ -39,12 +39,12 @@ fn check_modification( Ok(false) } -pub(crate) fn execute( +pub(in crate::invocation) fn execute( w: &mut W, replica: &mut Replica, settings: &Settings, filter: Filter, - modification: Modification, + modification: ResolvedModification, ) -> Result<(), crate::Error> { let tasks = filtered_tasks(replica, &filter)?; @@ -68,7 +68,7 @@ pub(crate) fn execute( #[cfg(test)] mod test { use super::*; - use crate::argparse::DescriptionMod; + use crate::argparse::{DescriptionMod, Modification}; use crate::invocation::test::test_replica; use crate::invocation::test::*; use pretty_assertions::assert_eq; @@ -87,10 +87,10 @@ mod test { let filter = Filter { ..Default::default() }; - let modification = Modification { + let modification = ResolvedModification(Modification { description: DescriptionMod::Set(s!("new description")), ..Default::default() - }; + }); execute(&mut w, &mut replica, &settings, filter, modification).unwrap(); // check that the task appeared.. diff --git a/cli/src/invocation/mod.rs b/cli/src/invocation/mod.rs index 0ae3f44e0..0b75799f4 100644 --- a/cli/src/invocation/mod.rs +++ b/cli/src/invocation/mod.rs @@ -15,7 +15,7 @@ mod util; mod test; use filter::filtered_tasks; -use modify::apply_modification; +use modify::{apply_modification, resolve_modification, ResolvedModification}; use report::display_report; /// Invoke the given Command in the context of the given settings @@ -52,7 +52,10 @@ pub(crate) fn invoke(command: Command, settings: Settings) -> Result<(), crate:: Command { subcommand: Subcommand::Add { modification }, .. - } => return cmd::add::execute(&mut w, &mut replica, modification), + } => { + let modification = resolve_modification(modification, &mut replica)?; + return cmd::add::execute(&mut w, &mut replica, modification); + } Command { subcommand: @@ -61,7 +64,10 @@ pub(crate) fn invoke(command: Command, settings: Settings) -> Result<(), crate:: modification, }, .. - } => return cmd::modify::execute(&mut w, &mut replica, &settings, filter, modification), + } => { + let modification = resolve_modification(modification, &mut replica)?; + return cmd::modify::execute(&mut w, &mut replica, &settings, filter, modification); + } Command { subcommand: diff --git a/cli/src/invocation/modify.rs b/cli/src/invocation/modify.rs index 7a6896565..934ab1bc2 100644 --- a/cli/src/invocation/modify.rs +++ b/cli/src/invocation/modify.rs @@ -1,12 +1,97 @@ -use crate::argparse::{DescriptionMod, Modification}; +use crate::argparse::{DescriptionMod, Modification, TaskId}; +use std::collections::HashSet; use taskchampion::chrono::Utc; -use taskchampion::{Annotation, TaskMut}; +use taskchampion::{Annotation, Replica, TaskMut}; + +/// A wrapper for Modification, promising that all TaskId instances are of variant TaskId::Uuid. +pub(super) struct ResolvedModification(pub(super) Modification); + +/// Resolve a Modification to a ResolvedModification, based on access to a Replica. +/// +/// This is not automatically done in `apply_modification` because, by that time, the TaskMut being +/// modified has an exclusive reference to the Replica, so it is impossible to search for matching +/// tasks. +pub(super) fn resolve_modification( + unres: Modification, + replica: &mut Replica, +) -> anyhow::Result { + Ok(ResolvedModification(Modification { + description: unres.description, + status: unres.status, + wait: unres.wait, + active: unres.active, + add_tags: unres.add_tags, + remove_tags: unres.remove_tags, + add_dependencies: resolve_task_ids(replica, unres.add_dependencies)?, + remove_dependencies: resolve_task_ids(replica, unres.remove_dependencies)?, + annotate: unres.annotate, + })) +} + +/// Convert a set of arbitrary TaskId's into TaskIds containing only TaskId::Uuid. +fn resolve_task_ids( + replica: &mut Replica, + task_ids: HashSet, +) -> anyhow::Result> { + // already all UUIDs (or empty)? + if task_ids.iter().all(|tid| matches!(tid, TaskId::Uuid(_))) { + return Ok(task_ids); + } + + let mut result = HashSet::new(); + let mut working_set = None; + let mut all_tasks = None; + for tid in task_ids { + match tid { + TaskId::WorkingSetId(i) => { + let ws = match working_set { + Some(ref ws) => ws, + None => { + working_set = Some(replica.working_set()?); + working_set.as_ref().unwrap() + } + }; + if let Some(u) = ws.by_index(i) { + result.insert(TaskId::Uuid(u)); + } + } + TaskId::PartialUuid(partial) => { + let ts = match all_tasks { + Some(ref ts) => ts, + None => { + all_tasks = Some( + replica + .all_task_uuids()? + .drain(..) + .map(|u| (u, u.to_string())) + .collect::>(), + ); + all_tasks.as_ref().unwrap() + } + }; + for (u, ustr) in ts { + if ustr.starts_with(&partial) { + result.insert(TaskId::Uuid(*u)); + } + } + } + TaskId::Uuid(u) => { + result.insert(TaskId::Uuid(u)); + } + } + } + + Ok(result) +} /// Apply the given modification pub(super) fn apply_modification( task: &mut TaskMut, - modification: &Modification, + modification: &ResolvedModification, ) -> anyhow::Result<()> { + // unwrap the "Resolved" promise + let modification = &modification.0; + match modification.description { DescriptionMod::Set(ref description) => task.set_description(description.clone())?, DescriptionMod::Prepend(ref description) => { @@ -49,5 +134,114 @@ pub(super) fn apply_modification( })?; } + for tid in &modification.add_dependencies { + if let TaskId::Uuid(u) = tid { + task.add_dependency(*u)?; + } else { + // this Modification is resolved, so all TaskIds should + // be the Uuid variant. + unreachable!(); + } + } + + for tid in &modification.remove_dependencies { + if let TaskId::Uuid(u) = tid { + task.remove_dependency(*u)?; + } else { + // this Modification is resolved, so all TaskIds should + // be the Uuid variant. + unreachable!(); + } + } + Ok(()) } + +#[cfg(test)] +mod test { + use super::*; + use crate::invocation::test::*; + use pretty_assertions::assert_eq; + use taskchampion::{Status, Uuid}; + + #[test] + fn test_resolve_modifications() { + let mut replica = test_replica(); + let u1 = Uuid::new_v4(); + let t1 = replica.new_task(Status::Pending, "a task".into()).unwrap(); + replica.rebuild_working_set(true).unwrap(); + + let modi = Modification { + add_dependencies: set![TaskId::Uuid(u1), TaskId::WorkingSetId(1)], + ..Default::default() + }; + + let res = resolve_modification(modi, &mut replica).unwrap(); + + assert_eq!( + res.0.add_dependencies, + set![TaskId::Uuid(u1), TaskId::Uuid(t1.get_uuid())], + ); + } + + #[test] + fn test_resolve_task_ids_empty() { + let mut replica = test_replica(); + + assert_eq!( + resolve_task_ids(&mut replica, HashSet::new()).unwrap(), + HashSet::new() + ); + } + + #[test] + fn test_resolve_task_ids_all_uuids() { + let mut replica = test_replica(); + let uuid = Uuid::new_v4(); + let tids = set![TaskId::Uuid(uuid)]; + assert_eq!(resolve_task_ids(&mut replica, tids.clone()).unwrap(), tids); + } + + #[test] + fn test_resolve_task_ids_working_set_not_found() { + let mut replica = test_replica(); + let tids = set![TaskId::WorkingSetId(13)]; + assert_eq!( + resolve_task_ids(&mut replica, tids.clone()).unwrap(), + HashSet::new() + ); + } + + #[test] + fn test_resolve_task_ids_working_set() { + let mut replica = test_replica(); + let t1 = replica.new_task(Status::Pending, "a task".into()).unwrap(); + let t2 = replica + .new_task(Status::Pending, "another task".into()) + .unwrap(); + replica.rebuild_working_set(true).unwrap(); + let tids = set![TaskId::WorkingSetId(1), TaskId::WorkingSetId(2)]; + let resolved = set![TaskId::Uuid(t1.get_uuid()), TaskId::Uuid(t2.get_uuid())]; + assert_eq!(resolve_task_ids(&mut replica, tids).unwrap(), resolved); + } + + #[test] + fn test_resolve_task_ids_partial_not_found() { + let mut replica = test_replica(); + let tids = set![TaskId::PartialUuid("abcd".into())]; + assert_eq!( + resolve_task_ids(&mut replica, tids.clone()).unwrap(), + HashSet::new() + ); + } + + #[test] + fn test_resolve_task_ids_partial() { + let mut replica = test_replica(); + let t1 = replica.new_task(Status::Pending, "a task".into()).unwrap(); + let uuid_str = t1.get_uuid().to_string(); + let tids = set![TaskId::PartialUuid(uuid_str[..8].into())]; + let resolved = set![TaskId::Uuid(t1.get_uuid())]; + assert_eq!(resolve_task_ids(&mut replica, tids).unwrap(), resolved); + } +} diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index 5c75f191e..81b4c8622 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -406,9 +406,12 @@ mod test { let task = replica.get_task(uuids[0]).unwrap().unwrap(); assert_eq!( task_column(&task, &column, &working_set), - s!("+PENDING +bar +foo") + s!("+PENDING +UNBLOCKED +bar +foo") ); let task = replica.get_task(uuids[2]).unwrap().unwrap(); - assert_eq!(task_column(&task, &column, &working_set), s!("+PENDING")); + assert_eq!( + task_column(&task, &column, &working_set), + s!("+PENDING +UNBLOCKED") + ); } } diff --git a/cli/src/macros.rs b/cli/src/macros.rs index 1a3024c13..f2cbe803b 100644 --- a/cli/src/macros.rs +++ b/cli/src/macros.rs @@ -12,14 +12,16 @@ macro_rules! argv { } /// Create a hashset, similar to vec! +// NOTE: in Rust 1.56.0, this can be changed to HashSet::from([..]) #[cfg(test)] macro_rules! set( - { $($key:expr),+ } => { + { $($key:expr),* $(,)? } => { { + #[allow(unused_mut)] let mut s = ::std::collections::HashSet::new(); $( s.insert($key); - )+ + )* s } }; diff --git a/integration-tests/src/bindings_tests/task.c b/integration-tests/src/bindings_tests/task.c index 4e20e52b3..4a30f8e1c 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/integration-tests/src/bindings_tests/task.c @@ -520,6 +520,43 @@ static void test_task_udas(void) { tc_replica_free(rep); } +// dependency manipulation +static void test_task_dependencies(void) { + TCReplica *rep = tc_replica_new_in_memory(); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); + + TCTask *task1 = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("task 1")); + TEST_ASSERT_NOT_NULL(task1); + TCTask *task2 = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("task 2")); + TEST_ASSERT_NOT_NULL(task2); + + TCUuidList deps; + + deps = tc_task_get_dependencies(task1); + TEST_ASSERT_EQUAL(0, deps.len); + tc_uuid_list_free(&deps); + + tc_task_to_mut(task1, rep); + TEST_ASSERT_EQUAL(TC_RESULT_OK, + tc_task_add_dependency(task1, tc_task_get_uuid(task2))); + + deps = tc_task_get_dependencies(task1); + TEST_ASSERT_EQUAL(1, deps.len); + TEST_ASSERT_EQUAL_MEMORY(tc_task_get_uuid(task2).bytes, deps.items[0].bytes, 16); + tc_uuid_list_free(&deps); + + TEST_ASSERT_EQUAL(TC_RESULT_OK, + tc_task_remove_dependency(task1, tc_task_get_uuid(task2))); + + deps = tc_task_get_dependencies(task1); + TEST_ASSERT_EQUAL(0, deps.len); + tc_uuid_list_free(&deps); + + tc_task_free(task1); + tc_task_free(task2); + tc_replica_free(rep); +} + static void tckvlist_assert_key(TCKVList *list, char *key, char *value) { TEST_ASSERT_NOT_NULL(list); for (size_t i = 0; i < list->len; i++) { @@ -624,6 +661,7 @@ int task_tests(void) { RUN_TEST(test_task_get_tags); RUN_TEST(test_task_annotations); RUN_TEST(test_task_udas); + RUN_TEST(test_task_dependencies); RUN_TEST(test_task_taskmap); RUN_TEST(test_task_list_take); return UNITY_END(); diff --git a/lib/src/task.rs b/lib/src/task.rs index ee4709a38..dce84d7aa 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -6,7 +6,7 @@ use std::ops::Deref; use std::ptr::NonNull; use std::str::FromStr; use taskchampion::chrono::{TimeZone, Utc}; -use taskchampion::{Annotation, Tag, Task, TaskMut}; +use taskchampion::{Annotation, Tag, Task, TaskMut, Uuid}; /// A task, as publicly exposed by this library. /// @@ -790,6 +790,56 @@ pub unsafe extern "C" fn tc_task_remove_legacy_uda(task: *mut TCTask, key: TCStr ) } +/// Get all dependencies for a task. +#[no_mangle] +pub unsafe extern "C" fn tc_task_get_dependencies(task: *mut TCTask) -> TCUuidList { + wrap(task, |task| { + let vec: Vec = task + .get_dependencies() + .map(|u| { + // SAFETY: + // - value is not allocated + unsafe { TCUuid::return_val(u) } + }) + .collect(); + // SAFETY: + // - caller will free this list + unsafe { TCUuidList::return_val(vec) } + }) +} + +/// Add a dependency. +#[no_mangle] +pub unsafe extern "C" fn tc_task_add_dependency(task: *mut TCTask, dep: TCUuid) -> TCResult { + // SAFETY: + // - tcuuid is a valid TCUuid (all byte patterns are valid) + let dep: Uuid = unsafe { TCUuid::val_from_arg(dep) }; + wrap_mut( + task, + |task| { + task.add_dependency(dep)?; + Ok(TCResult::Ok) + }, + TCResult::Error, + ) +} + +/// Remove a dependency. +#[no_mangle] +pub unsafe extern "C" fn tc_task_remove_dependency(task: *mut TCTask, dep: TCUuid) -> TCResult { + // SAFETY: + // - tcuuid is a valid TCUuid (all byte patterns are valid) + let dep: Uuid = unsafe { TCUuid::val_from_arg(dep) }; + wrap_mut( + task, + |task| { + task.remove_dependency(dep)?; + Ok(TCResult::Ok) + }, + TCResult::Error, + ) +} + /// Get the latest error for a task, or a string NULL ptr field if the last operation succeeded. /// Subsequent calls to this function will return NULL. The task pointer must not be NULL. The /// caller must free the returned string. diff --git a/lib/taskchampion.h b/lib/taskchampion.h index 8c5f02575..a4ec0061e 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -911,6 +911,21 @@ TCResult tc_task_set_legacy_uda(struct TCTask *task, struct TCString key, struct */ TCResult tc_task_remove_legacy_uda(struct TCTask *task, struct TCString key); +/** + * Get all dependencies for a task. + */ +struct TCUuidList tc_task_get_dependencies(struct TCTask *task); + +/** + * Add a dependency. + */ +TCResult tc_task_add_dependency(struct TCTask *task, struct TCUuid dep); + +/** + * Remove a dependency. + */ +TCResult tc_task_remove_dependency(struct TCTask *task, struct TCUuid dep); + /** * Get the latest error for a task, or a string NULL ptr field if the last operation succeeded. * Subsequent calls to this function will return NULL. The task pointer must not be NULL. The diff --git a/taskchampion/src/depmap.rs b/taskchampion/src/depmap.rs new file mode 100644 index 000000000..d2f6225bf --- /dev/null +++ b/taskchampion/src/depmap.rs @@ -0,0 +1,81 @@ +use uuid::Uuid; + +/// DependencyMap stores information on task dependencies between pending tasks. +/// +/// This information requires a scan of the working set to generate, so it is +/// typically calculated once and re-used. +#[derive(Debug, PartialEq)] +pub struct DependencyMap { + /// Edges of the dependency graph. If (a, b) is in this array, then task a depends on tsak b. + edges: Vec<(Uuid, Uuid)>, +} + +impl DependencyMap { + /// Create a new, empty DependencyMap. + pub(super) fn new() -> Self { + Self { edges: Vec::new() } + } + + /// Add a dependency of a on b. + pub(super) fn add_dependency(&mut self, a: Uuid, b: Uuid) { + self.edges.push((a, b)); + } + + /// Return an iterator of Uuids on which task `deps_of` depends. This is equivalent to + /// `task.get_dependencies()`. + pub fn dependencies(&self, dep_of: Uuid) -> impl Iterator + '_ { + self.edges + .iter() + .filter_map(move |(a, b)| if a == &dep_of { Some(*b) } else { None }) + } + + /// Return an iterator of Uuids of tasks that depend on `dep_on` + /// `task.get_dependencies()`. + pub fn dependents(&self, dep_on: Uuid) -> impl Iterator + '_ { + self.edges + .iter() + .filter_map(move |(a, b)| if b == &dep_on { Some(*a) } else { None }) + } +} + +#[cfg(test)] +mod test { + use super::*; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + + #[test] + fn dependencies() { + let t = Uuid::new_v4(); + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + let mut dm = DependencyMap::new(); + + dm.add_dependency(t, uuid1); + dm.add_dependency(t, uuid2); + dm.add_dependency(Uuid::new_v4(), t); + dm.add_dependency(Uuid::new_v4(), uuid1); + dm.add_dependency(uuid2, Uuid::new_v4()); + + assert_eq!( + dm.dependencies(t).collect::>(), + set![uuid1, uuid2] + ); + } + + #[test] + fn dependents() { + let t = Uuid::new_v4(); + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + let mut dm = DependencyMap::new(); + + dm.add_dependency(uuid1, t); + dm.add_dependency(uuid2, t); + dm.add_dependency(t, Uuid::new_v4()); + dm.add_dependency(Uuid::new_v4(), uuid1); + dm.add_dependency(uuid2, Uuid::new_v4()); + + assert_eq!(dm.dependents(t).collect::>(), set![uuid1, uuid2]); + } +} diff --git a/taskchampion/src/lib.rs b/taskchampion/src/lib.rs index 24dcd852d..2e22b992e 100644 --- a/taskchampion/src/lib.rs +++ b/taskchampion/src/lib.rs @@ -45,6 +45,10 @@ This crate supports Rust version 1.47 and higher. */ +// NOTE: it's important that this 'mod' comes first so that the macros can be used in other modules +mod macros; + +mod depmap; mod errors; mod replica; pub mod server; @@ -54,6 +58,7 @@ mod taskdb; mod utils; mod workingset; +pub use depmap::DependencyMap; pub use errors::Error; pub use replica::Replica; pub use server::{Server, ServerConfig}; diff --git a/taskchampion/src/macros.rs b/taskchampion/src/macros.rs new file mode 100644 index 000000000..eb34b4640 --- /dev/null +++ b/taskchampion/src/macros.rs @@ -0,0 +1,17 @@ +#![macro_use] + +/// Create a hashset, similar to vec! +// NOTE: in Rust 1.56.0, this can be changed to HashSet::from([..]) +#[cfg(test)] +macro_rules! set( + { $($key:expr),* $(,)? } => { + { + #[allow(unused_mut)] + let mut s = ::std::collections::HashSet::new(); + $( + s.insert($key); + )* + s + } + }; +); diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 686b34842..3787f3b25 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -1,3 +1,4 @@ +use crate::depmap::DependencyMap; use crate::server::{Server, SyncOp}; use crate::storage::{Storage, TaskMap}; use crate::task::{Status, Task}; @@ -7,6 +8,7 @@ use anyhow::Context; use chrono::{Duration, Utc}; use log::trace; use std::collections::HashMap; +use std::rc::Rc; use uuid::Uuid; /// A replica represents an instance of a user's task data, providing an easy interface @@ -28,7 +30,12 @@ use uuid::Uuid; /// during the garbage-collection process. pub struct Replica { taskdb: TaskDb, + + /// If true, this replica has already added an undo point. added_undo_point: bool, + + /// The dependency map for this replica, if it has been calculated. + depmap: Option>, } impl Replica { @@ -36,6 +43,7 @@ impl Replica { Replica { taskdb: TaskDb::new(storage), added_undo_point: false, + depmap: None, } } @@ -76,9 +84,10 @@ impl Replica { /// Get all tasks represented as a map keyed by UUID pub fn all_tasks(&mut self) -> anyhow::Result> { + let depmap = self.dependency_map(false)?; let mut res = HashMap::new(); for (uuid, tm) in self.taskdb.all_tasks()?.drain(..) { - res.insert(uuid, Task::new(uuid, tm)); + res.insert(uuid, Task::new(uuid, tm, depmap.clone())); } Ok(res) } @@ -94,12 +103,47 @@ impl Replica { Ok(WorkingSet::new(self.taskdb.working_set()?)) } + /// Get the dependency map for all pending tasks. + /// + /// The data in this map is cached when it is first requested and may not contain modifications + /// made locally in this Replica instance. The result is reference-counted and may + /// outlive the Replica. + /// + /// If `force` is true, then the result is re-calculated from the current state of the replica, + /// although previously-returned dependency maps are not updated. + pub fn dependency_map(&mut self, force: bool) -> anyhow::Result> { + if force || self.depmap.is_none() { + let mut dm = DependencyMap::new(); + let ws = self.working_set()?; + for i in 1..=ws.largest_index() { + if let Some(u) = ws.by_index(i) { + // note: we can't use self.get_task here, as that depends on a + // DependencyMap + if let Some(taskmap) = self.taskdb.get_task(u)? { + for p in taskmap.keys() { + if let Some(dep_str) = p.strip_prefix("dep_") { + if let Ok(dep) = Uuid::parse_str(dep_str) { + dm.add_dependency(u, dep); + } + } + } + } + } + } + self.depmap = Some(Rc::new(dm)); + } + + // at this point self.depmap is guaranteed to be Some(_) + Ok(self.depmap.as_ref().unwrap().clone()) + } + /// Get an existing task by its UUID pub fn get_task(&mut self, uuid: Uuid) -> anyhow::Result> { + let depmap = self.dependency_map(false)?; Ok(self .taskdb .get_task(uuid)? - .map(move |tm| Task::new(uuid, tm))) + .map(move |tm| Task::new(uuid, tm, depmap))) } /// Create a new task. @@ -107,7 +151,8 @@ impl Replica { let uuid = Uuid::new_v4(); self.add_undo_point(false)?; let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?; - let mut task = Task::new(uuid, taskmap).into_mut(self); + let depmap = self.dependency_map(false)?; + let mut task = Task::new(uuid, taskmap, depmap).into_mut(self); task.set_description(description)?; task.set_status(status)?; task.set_entry(Some(Utc::now()))?; @@ -121,7 +166,8 @@ impl Replica { pub fn import_task_with_uuid(&mut self, uuid: Uuid) -> anyhow::Result { self.add_undo_point(false)?; let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?; - Ok(Task::new(uuid, taskmap)) + let depmap = self.dependency_map(false)?; + Ok(Task::new(uuid, taskmap, depmap)) } /// Delete a task. The task must exist. Note that this is different from setting status to @@ -217,6 +263,7 @@ mod tests { use crate::task::Status; use chrono::TimeZone; use pretty_assertions::assert_eq; + use std::collections::HashSet; use uuid::Uuid; #[test] @@ -445,4 +492,67 @@ mod tests { assert!(t.get_description().starts_with("keeper")); } } + + #[test] + fn dependency_map() { + let mut rep = Replica::new_inmemory(); + + let mut tasks = vec![]; + for _ in 0..4 { + tasks.push(rep.new_task(Status::Pending, "t".into()).unwrap()); + } + + let uuids: Vec<_> = tasks.iter().map(|t| t.get_uuid()).collect(); + + // t[3] depends on t[2], and t[1] + { + let mut t = tasks.pop().unwrap().into_mut(&mut rep); + t.add_dependency(uuids[2]).unwrap(); + t.add_dependency(uuids[1]).unwrap(); + } + + // t[2] depends on t[0] + { + let mut t = tasks.pop().unwrap().into_mut(&mut rep); + t.add_dependency(uuids[0]).unwrap(); + } + + // t[1] depends on t[0] + { + let mut t = tasks.pop().unwrap().into_mut(&mut rep); + t.add_dependency(uuids[0]).unwrap(); + } + + // generate the dependency map, forcing an update based on the newly-added + // dependencies + let dm = rep.dependency_map(true).unwrap(); + + assert_eq!( + dm.dependencies(uuids[3]).collect::>(), + set![uuids[1], uuids[2]] + ); + assert_eq!( + dm.dependencies(uuids[2]).collect::>(), + set![uuids[0]] + ); + assert_eq!( + dm.dependencies(uuids[1]).collect::>(), + set![uuids[0]] + ); + assert_eq!(dm.dependencies(uuids[0]).collect::>(), set![]); + + assert_eq!(dm.dependents(uuids[3]).collect::>(), set![]); + assert_eq!( + dm.dependents(uuids[2]).collect::>(), + set![uuids[3]] + ); + assert_eq!( + dm.dependents(uuids[1]).collect::>(), + set![uuids[3]] + ); + assert_eq!( + dm.dependents(uuids[0]).collect::>(), + set![uuids[1], uuids[2]] + ); + } } diff --git a/taskchampion/src/task/tag.rs b/taskchampion/src/task/tag.rs index bb8361f80..a4f1e677c 100644 --- a/taskchampion/src/task/tag.rs +++ b/taskchampion/src/task/tag.rs @@ -134,6 +134,9 @@ pub(super) enum SyntheticTag { Pending, Completed, Deleted, + Blocked, + Unblocked, + Blocking, } #[cfg(test)] diff --git a/taskchampion/src/task/task.rs b/taskchampion/src/task/task.rs index cd8a12f85..2a8464837 100644 --- a/taskchampion/src/task/task.rs +++ b/taskchampion/src/task/task.rs @@ -1,11 +1,13 @@ use super::tag::{SyntheticTag, TagInner}; use super::{Annotation, Priority, Status, Tag, Timestamp}; +use crate::depmap::DependencyMap; use crate::replica::Replica; use crate::storage::TaskMap; use chrono::prelude::*; use log::trace; use std::convert::AsRef; use std::convert::TryInto; +use std::rc::Rc; use std::str::FromStr; use uuid::Uuid; @@ -29,10 +31,18 @@ use uuid::Uuid; /// This struct contains only getters for various values on the task. The /// [`into_mut`](Task::into_mut) method /// returns a TaskMut which can be used to modify the task. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub struct Task { uuid: Uuid, taskmap: TaskMap, + depmap: Rc, +} + +impl PartialEq for Task { + fn eq(&self, other: &Task) -> bool { + // compare only the taskmap and uuid; depmap is just present for reference + self.uuid == other.uuid && self.taskmap == other.taskmap + } } /// A mutable task, with setter methods. @@ -84,8 +94,12 @@ fn uda_tuple_to_string(namespace: impl AsRef, key: impl AsRef) -> Stri } impl Task { - pub(crate) fn new(uuid: Uuid, taskmap: TaskMap) -> Task { - Task { uuid, taskmap } + pub(crate) fn new(uuid: Uuid, taskmap: TaskMap, depmap: Rc) -> Task { + Task { + uuid, + taskmap, + depmap, + } } pub fn get_uuid(&self) -> Uuid { @@ -151,6 +165,16 @@ impl Task { self.taskmap.contains_key(Prop::Start.as_ref()) } + /// Determine whether this task is blocked -- that is, has at least one unresolved dependency. + pub fn is_blocked(&self) -> bool { + self.depmap.dependencies(self.uuid).next().is_some() + } + + /// Determine whether this task is blocking -- that is, has at least one unresolved dependent. + pub fn is_blocking(&self) -> bool { + self.depmap.dependents(self.uuid).next().is_some() + } + /// Determine whether a given synthetic tag is present on this task. All other /// synthetic tag calculations are based on this one. fn has_synthetic_tag(&self, synth: &SyntheticTag) -> bool { @@ -160,6 +184,9 @@ impl Task { SyntheticTag::Pending => self.get_status() == Status::Pending, SyntheticTag::Completed => self.get_status() == Status::Completed, SyntheticTag::Deleted => self.get_status() == Status::Deleted, + SyntheticTag::Blocked => self.is_blocked(), + SyntheticTag::Unblocked => !self.is_blocked(), + SyntheticTag::Blocking => self.is_blocking(), } } @@ -243,10 +270,27 @@ impl Task { .map(|(p, v)| (p.as_ref(), v.as_ref())) } + /// Get the modification time for this task. pub fn get_modified(&self) -> Option> { self.get_timestamp(Prop::Modified.as_ref()) } + /// Get the UUIDs of tasks on which this task depends. + /// + /// This includes all dependencies, regardless of their status. In fact, it may include + /// dependencies that do not exist. + pub fn get_dependencies(&self) -> impl Iterator + '_ { + self.taskmap.iter().filter_map(|(p, _)| { + if let Some(dep_str) = p.strip_prefix("dep_") { + if let Ok(u) = Uuid::parse_str(dep_str) { + return Some(u); + } + // (un-parseable dep_.. properties are ignored) + } + None + }) + } + // -- utility functions fn is_known_key(key: &str) -> bool { @@ -423,6 +467,18 @@ impl<'r> TaskMut<'r> { self.set_string(key, None) } + /// Add a dependency. + pub fn add_dependency(&mut self, dep: Uuid) -> anyhow::Result<()> { + let key = format!("dep_{}", dep); + self.set_string(key, Some("".to_string())) + } + + /// Remove a dependency. + pub fn remove_dependency(&mut self, dep: Uuid) -> anyhow::Result<()> { + let key = format!("dep_{}", dep); + self.set_string(key, None) + } + // -- utility functions fn update_modified(&mut self) -> anyhow::Result<()> { @@ -491,6 +547,11 @@ impl<'r> std::ops::Deref for TaskMut<'r> { mod test { use super::*; use pretty_assertions::assert_eq; + use std::collections::HashSet; + + fn dm() -> Rc { + Rc::new(DependencyMap::new()) + } fn with_mut_task(f: F) { let mut replica = Replica::new_inmemory(); @@ -511,7 +572,7 @@ mod test { #[test] fn test_is_active_never_started() { - let task = Task::new(Uuid::new_v4(), TaskMap::new()); + let task = Task::new(Uuid::new_v4(), TaskMap::new(), dm()); assert!(!task.is_active()); } @@ -522,6 +583,7 @@ mod test { vec![(String::from("start"), String::from("1234"))] .drain(..) .collect(), + dm(), ); assert!(task.is_active()); @@ -529,13 +591,13 @@ mod test { #[test] fn test_is_active_inactive() { - let task = Task::new(Uuid::new_v4(), Default::default()); + let task = Task::new(Uuid::new_v4(), Default::default(), dm()); assert!(!task.is_active()); } #[test] fn test_entry_not_set() { - let task = Task::new(Uuid::new_v4(), TaskMap::new()); + let task = Task::new(Uuid::new_v4(), TaskMap::new(), dm()); assert_eq!(task.get_entry(), None); } @@ -547,13 +609,14 @@ mod test { vec![(String::from("entry"), format!("{}", ts.timestamp()))] .drain(..) .collect(), + dm(), ); assert_eq!(task.get_entry(), Some(ts)); } #[test] fn test_wait_not_set() { - let task = Task::new(Uuid::new_v4(), TaskMap::new()); + let task = Task::new(Uuid::new_v4(), TaskMap::new(), dm()); assert!(!task.is_waiting()); assert_eq!(task.get_wait(), None); @@ -567,6 +630,7 @@ mod test { vec![(String::from("wait"), format!("{}", ts.timestamp()))] .drain(..) .collect(), + dm(), ); assert!(!task.is_waiting()); @@ -581,6 +645,7 @@ mod test { vec![(String::from("wait"), format!("{}", ts.timestamp()))] .drain(..) .collect(), + dm(), ); assert!(task.is_waiting()); @@ -597,6 +662,7 @@ mod test { ] .drain(..) .collect(), + dm(), ); assert!(task.has_tag(&utag("abc"))); @@ -618,17 +684,17 @@ mod test { ] .drain(..) .collect(), + dm(), ); - let mut tags: Vec<_> = task.get_tags().collect(); - tags.sort(); - let mut exp = vec![ + let tags: HashSet<_> = task.get_tags().collect(); + let exp = set![ utag("abc"), utag("def"), stag(SyntheticTag::Pending), stag(SyntheticTag::Waiting), + stag(SyntheticTag::Unblocked), ]; - exp.sort(); assert_eq!(tags, exp); } @@ -644,11 +710,19 @@ mod test { ] .drain(..) .collect(), + dm(), ); // only "ok" is OK - let tags: Vec<_> = task.get_tags().collect(); - assert_eq!(tags, vec![utag("ok"), stag(SyntheticTag::Pending)]); + let tags: HashSet<_> = task.get_tags().collect(); + assert_eq!( + tags, + set![ + utag("ok"), + stag(SyntheticTag::Pending), + stag(SyntheticTag::Unblocked) + ] + ); } #[test] @@ -669,6 +743,7 @@ mod test { ] .drain(..) .collect(), + dm(), ); let mut anns: Vec<_> = task.get_annotations().collect(); @@ -884,6 +959,7 @@ mod test { ] .drain(..) .collect(), + dm(), ); let mut udas: Vec<_> = task.get_udas().collect(); @@ -905,6 +981,7 @@ mod test { ] .drain(..) .collect(), + dm(), ); assert_eq!(task.get_uda("", "description"), None); // invalid UDA @@ -925,6 +1002,7 @@ mod test { ] .drain(..) .collect(), + dm(), ); assert_eq!(task.get_legacy_uda("description"), None); // invalid UDA @@ -1011,4 +1089,53 @@ mod test { assert!(task.remove_legacy_uda("tag_abc").is_err()); }) } + + #[test] + fn test_dependencies() { + with_mut_task(|mut task| { + assert_eq!(task.get_dependencies().collect::>(), vec![]); + let dep1 = Uuid::new_v4(); + let dep2 = Uuid::new_v4(); + + task.add_dependency(dep1).unwrap(); + assert_eq!(task.get_dependencies().collect::>(), vec![dep1]); + + task.add_dependency(dep1).unwrap(); // add twice is ok + task.add_dependency(dep2).unwrap(); + let deps = task.get_dependencies().collect::>(); + assert!(deps.contains(&dep1)); + assert!(deps.contains(&dep2)); + + task.remove_dependency(dep1).unwrap(); + assert_eq!(task.get_dependencies().collect::>(), vec![dep2]); + }) + } + + #[test] + fn dependencies_tags() { + let mut rep = Replica::new_inmemory(); + let uuid1; + let uuid2; + { + let t1 = rep.new_task(Status::Pending, "1".into()).unwrap(); + uuid1 = t1.get_uuid(); + let t2 = rep.new_task(Status::Pending, "2".into()).unwrap(); + uuid2 = t2.get_uuid(); + + let mut t1 = t1.into_mut(&mut rep); + t1.add_dependency(t2.get_uuid()).unwrap(); + } + + // force-refresh depmap + rep.dependency_map(true).unwrap(); + + let t1 = rep.get_task(uuid1).unwrap().unwrap(); + let t2 = rep.get_task(uuid2).unwrap().unwrap(); + assert!(t1.has_tag(&stag(SyntheticTag::Blocked))); + assert!(!t1.has_tag(&stag(SyntheticTag::Unblocked))); + assert!(!t1.has_tag(&stag(SyntheticTag::Blocking))); + assert!(!t2.has_tag(&stag(SyntheticTag::Blocked))); + assert!(t2.has_tag(&stag(SyntheticTag::Unblocked))); + assert!(t2.has_tag(&stag(SyntheticTag::Blocking))); + } }