diff --git a/cli/src/invocation/cmd/add.rs b/cli/src/invocation/cmd/add.rs index 104a6357c..1175b587e 100644 --- a/cli/src/invocation/cmd/add.rs +++ b/cli/src/invocation/cmd/add.rs @@ -33,7 +33,11 @@ mod test { execute(&mut w, &mut replica, modification).unwrap(); // check that the task appeared.. - let task = replica.get_working_set_task(1).unwrap().unwrap(); + let working_set = replica.working_set().unwrap(); + let task = replica + .get_task(working_set.by_index(1).unwrap()) + .unwrap() + .unwrap(); assert_eq!(task.get_description(), "my description"); assert_eq!(task.get_status(), Status::Pending); diff --git a/cli/src/invocation/cmd/info.rs b/cli/src/invocation/cmd/info.rs index 5d70163c7..603093315 100644 --- a/cli/src/invocation/cmd/info.rs +++ b/cli/src/invocation/cmd/info.rs @@ -12,6 +12,8 @@ pub(crate) fn execute( filter: Filter, debug: bool, ) -> Fallible<()> { + let working_set = replica.working_set()?; + for task in filtered_tasks(replica, &filter)? { let uuid = task.get_uuid(); @@ -24,7 +26,7 @@ pub(crate) fn execute( } } else { t.add_row(row![b->"Uuid", uuid]); - if let Some(i) = replica.get_working_set_index(uuid)? { + if let Some(i) = working_set.by_uuid(uuid) { t.add_row(row![b->"Id", i]); } t.add_row(row![b->"Description", task.get_description()]); diff --git a/cli/src/invocation/filter.rs b/cli/src/invocation/filter.rs index f9440dcd9..28e25c238 100644 --- a/cli/src/invocation/filter.rs +++ b/cli/src/invocation/filter.rs @@ -2,9 +2,9 @@ use crate::argparse::{Condition, Filter, TaskId}; use failure::Fallible; use std::collections::HashSet; use std::convert::TryInto; -use taskchampion::{Replica, Status, Tag, Task, Uuid}; +use taskchampion::{Replica, Status, Tag, Task, Uuid, WorkingSet}; -fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option) -> bool { +fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set: &WorkingSet) -> bool { for cond in &filter.conditions { match cond { Condition::HasTag(ref tag) => { @@ -29,6 +29,8 @@ fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option { let uuid_str = uuid.to_string(); let mut found = false; + let working_set_id = working_set.by_uuid(uuid); + for id in ids { if match id { TaskId::WorkingSetId(i) => Some(*i) == working_set_id, @@ -111,6 +113,8 @@ pub(super) fn filtered_tasks( log::debug!("Applying filter {:?}", filter); + let working_set = replica.working_set()?; + // We will enumerate the universe of tasks for this filter, checking // each resulting task with match_task match universe_for_filter(filter) { @@ -123,7 +127,11 @@ pub(super) fn filtered_tasks( let mut seen = HashSet::new(); for id in ids { let task = match id { - TaskId::WorkingSetId(id) => replica.get_working_set_task(*id)?, + TaskId::WorkingSetId(id) => working_set + .by_index(*id) + .map(|uuid| replica.get_task(uuid)) + .transpose()? + .flatten(), TaskId::PartialUuid(_) => unreachable!(), // not present in absolute id list TaskId::Uuid(id) => replica.get_task(*id)?, }; @@ -136,9 +144,7 @@ pub(super) fn filtered_tasks( } seen.insert(uuid); - let working_set_id = replica.get_working_set_index(uuid)?; - - if match_task(filter, &task, uuid, working_set_id) { + if match_task(filter, &task, uuid, &working_set) { res.push(task); } } @@ -149,19 +155,16 @@ pub(super) fn filtered_tasks( Universe::AllTasks => { log::debug!("Scanning all tasks in the task database"); for (uuid, task) in replica.all_tasks()?.drain() { - // Yikes, slow! https://github.com/djmitche/taskchampion/issues/108 - let working_set_id = replica.get_working_set_index(uuid)?; - if match_task(filter, &task, uuid, working_set_id) { + if match_task(filter, &task, uuid, &working_set) { res.push(task); } } } Universe::WorkingSet => { log::debug!("Scanning only the working set (pending tasks)"); - for (i, task) in replica.working_set()?.drain(..).enumerate() { - if let Some(task) = task { - let uuid = task.get_uuid(); - if match_task(filter, &task, uuid, Some(i)) { + for (_, uuid) in working_set.iter() { + if let Some(task) = replica.get_task(uuid)? { + if match_task(filter, &task, uuid, &working_set) { res.push(task); } } diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index 12497553c..185eaa4b3 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -6,35 +6,9 @@ use config::Config; use failure::{format_err, Fallible}; use prettytable::{Row, Table}; use std::cmp::Ordering; -use taskchampion::{Replica, Task, Uuid}; +use taskchampion::{Replica, Task, WorkingSet}; use termcolor::WriteColor; -// pending #123, this is a non-fallible way of looking up a task's working set index -struct WorkingSet(Vec>); - -impl WorkingSet { - fn new(replica: &mut Replica) -> Fallible { - let working_set = replica.working_set()?; - Ok(Self( - working_set - .iter() - .map(|opt| opt.as_ref().map(|t| t.get_uuid())) - .collect(), - )) - } - - fn index(&self, target: Uuid) -> Option { - for (i, uuid) in self.0.iter().enumerate() { - if let Some(uuid) = uuid { - if *uuid == target { - return Some(i); - } - } - } - None - } -} - /// Sort tasks for the given report. fn sort_tasks(tasks: &mut Vec, report: &Report, working_set: &WorkingSet) { tasks.sort_by(|a, b| { @@ -43,8 +17,8 @@ fn sort_tasks(tasks: &mut Vec, report: &Report, working_set: &WorkingSet) SortBy::Id => { let a_uuid = a.get_uuid(); let b_uuid = b.get_uuid(); - let a_id = working_set.index(a_uuid); - let b_id = working_set.index(b_uuid); + let a_id = working_set.by_uuid(a_uuid); + let b_id = working_set.by_uuid(b_uuid); println!("a_uuid {} -> a_id {:?}", a_uuid, a_id); println!("b_uuid {} -> b_id {:?}", b_uuid, b_id); match (a_id, b_id) { @@ -78,7 +52,7 @@ fn task_column(task: &Task, column: &Column, working_set: &WorkingSet) -> String Property::Id => { let uuid = task.get_uuid(); let mut id = uuid.to_string(); - if let Some(i) = working_set.index(uuid) { + if let Some(i) = working_set.by_uuid(uuid) { id = i.to_string(); } id @@ -111,7 +85,7 @@ pub(super) fn display_report( filter: Filter, ) -> Fallible<()> { let mut t = Table::new(); - let working_set = WorkingSet::new(replica)?; + let working_set = replica.working_set()?; // Get the report from settings let mut report = Report::from_config(settings.get(&format!("reports.{}", report_name))?) @@ -151,7 +125,7 @@ mod test { use crate::invocation::test::*; use crate::report::Sort; use std::convert::TryInto; - use taskchampion::Status; + use taskchampion::{Status, Uuid}; fn create_tasks(replica: &mut Replica) -> [Uuid; 3] { let t1 = replica.new_task(Status::Pending, s!("A")).unwrap(); @@ -172,7 +146,7 @@ mod test { fn sorting_by_descr() { let mut replica = test_replica(); create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); let mut report = Report { sort: vec![Sort { ascending: true, @@ -199,7 +173,7 @@ mod test { fn sorting_by_id() { let mut replica = test_replica(); create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); let mut report = Report { sort: vec![Sort { ascending: true, @@ -226,7 +200,7 @@ mod test { fn sorting_by_uuid() { let mut replica = test_replica(); let uuids = create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); let report = Report { sort: vec![Sort { ascending: true, @@ -254,7 +228,7 @@ mod test { .add_tag(&("second".try_into().unwrap())) .unwrap(); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); let report = Report { sort: vec![ Sort { @@ -280,9 +254,9 @@ mod test { fn task_column_id() { let mut replica = test_replica(); let uuids = create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); - let task = replica.get_working_set_task(1).unwrap().unwrap(); + let task = replica.get_task(uuids[0]).unwrap().unwrap(); let column = Column { label: s!(""), property: Property::Id, @@ -301,10 +275,10 @@ mod test { #[test] fn task_column_uuid() { let mut replica = test_replica(); - create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let uuids = create_tasks(&mut replica); + let working_set = replica.working_set().unwrap(); - let task = replica.get_working_set_task(1).unwrap().unwrap(); + let task = replica.get_task(uuids[0]).unwrap().unwrap(); let column = Column { label: s!(""), property: Property::Uuid, @@ -319,7 +293,7 @@ mod test { fn task_column_active() { let mut replica = test_replica(); let uuids = create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); // make task A active replica @@ -335,19 +309,19 @@ mod test { property: Property::Active, }; - let task = replica.get_working_set_task(1).unwrap().unwrap(); + let task = replica.get_task(uuids[0]).unwrap().unwrap(); assert_eq!(task_column(&task, &column, &working_set), s!("*")); - let task = replica.get_working_set_task(2).unwrap().unwrap(); + let task = replica.get_task(uuids[2]).unwrap().unwrap(); assert_eq!(task_column(&task, &column, &working_set), s!("")); } #[test] fn task_column_description() { let mut replica = test_replica(); - create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let uuids = create_tasks(&mut replica); + let working_set = replica.working_set().unwrap(); - let task = replica.get_working_set_task(2).unwrap().unwrap(); + let task = replica.get_task(uuids[2]).unwrap().unwrap(); let column = Column { label: s!(""), property: Property::Description, @@ -359,7 +333,7 @@ mod test { fn task_column_tags() { let mut replica = test_replica(); let uuids = create_tasks(&mut replica); - let working_set = WorkingSet::new(&mut replica).unwrap(); + let working_set = replica.working_set().unwrap(); // add some tags to task A let mut t1 = replica @@ -375,9 +349,9 @@ mod test { property: Property::Tags, }; - let task = replica.get_working_set_task(1).unwrap().unwrap(); + let task = replica.get_task(uuids[0]).unwrap().unwrap(); assert_eq!(task_column(&task, &column, &working_set), s!("+bar +foo")); - let task = replica.get_working_set_task(2).unwrap().unwrap(); + let task = replica.get_task(uuids[2]).unwrap().unwrap(); assert_eq!(task_column(&task, &column, &working_set), s!("")); } } diff --git a/taskchampion/src/lib.rs b/taskchampion/src/lib.rs index fb91f36e7..cdb9e8cc1 100644 --- a/taskchampion/src/lib.rs +++ b/taskchampion/src/lib.rs @@ -32,10 +32,12 @@ mod task; mod taskdb; pub mod taskstorage; mod utils; +mod workingset; pub use config::{ReplicaConfig, ServerConfig}; pub use replica::Replica; pub use task::{Priority, Status, Tag, Task, TaskMut}; +pub use workingset::WorkingSet; /// Re-exported type from the `uuid` crate, for ease of compatibility for consumers of this crate. pub use uuid::Uuid; diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index fa20c7c3b..95cbaf576 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -4,6 +4,7 @@ use crate::server::Server; use crate::task::{Status, Task}; use crate::taskdb::TaskDB; use crate::taskstorage::{KVStorage, Operation, TaskMap, TaskStorage}; +use crate::workingset::WorkingSet; use chrono::Utc; use failure::Fallible; use log::trace; @@ -87,21 +88,10 @@ impl Replica { self.taskdb.all_task_uuids() } - /// Get the "working set" for this replica -- the set of pending tasks, as indexed by small - /// integers - pub fn working_set(&mut self) -> Fallible>> { - let working_set = self.taskdb.working_set()?; - let mut res = Vec::with_capacity(working_set.len()); - for item in working_set.iter() { - res.push(match item { - Some(u) => match self.taskdb.get_task(*u)? { - Some(tm) => Some(Task::new(*u, tm)), - None => None, - }, - None => None, - }) - } - Ok(res) + /// Get the "working set" for this replica. This is a snapshot of the current state, + /// and it is up to the caller to decide how long to store this value. + pub fn working_set(&mut self) -> Fallible { + Ok(WorkingSet::new(self.taskdb.working_set()?)) } /// Get an existing task by its UUID @@ -112,33 +102,6 @@ impl Replica { .map(move |tm| Task::new(uuid, tm))) } - /// Get an existing task by its working set index - pub fn get_working_set_task(&mut self, i: usize) -> Fallible> { - let working_set = self.taskdb.working_set()?; - if (i as usize) < working_set.len() { - if let Some(uuid) = working_set[i as usize] { - return Ok(self - .taskdb - .get_task(uuid)? - .map(move |tm| Task::new(uuid, tm))); - } - } - Ok(None) - } - - /// Get the working set index for the given task uuid - pub fn get_working_set_index(&mut self, uuid: Uuid) -> Fallible> { - let working_set = self.taskdb.working_set()?; - for (i, u) in working_set.iter().enumerate() { - if let Some(u) = u { - if *u == uuid { - return Ok(Some(i)); - } - } - } - Ok(None) - } - /// Create a new task. The task must not already exist. pub fn new_task(&mut self, status: Status, description: String) -> Fallible { let uuid = Uuid::new_v4(); @@ -258,7 +221,8 @@ mod tests { rep.rebuild_working_set(true).unwrap(); - assert!(rep.get_working_set_index(t.get_uuid()).unwrap().is_none()); + let ws = rep.working_set().unwrap(); + assert!(ws.by_uuid(t.get_uuid()).is_none()); } #[test] @@ -270,16 +234,13 @@ mod tests { .unwrap(); let uuid = t.get_uuid(); - let t = rep.get_working_set_task(1).unwrap().unwrap(); - assert_eq!(t.get_status(), Status::Pending); - assert_eq!(t.get_description(), "to-be-pending"); + let ws = rep.working_set().unwrap(); + assert_eq!(ws.len(), 1); // only one non-none value + assert!(ws.by_index(0).is_none()); + assert_eq!(ws.by_index(1), Some(uuid)); let ws = rep.working_set().unwrap(); - assert_eq!(ws.len(), 2); - assert!(ws[0].is_none()); - assert_eq!(ws[1].as_ref().unwrap().get_uuid(), uuid); - - assert_eq!(rep.get_working_set_index(t.get_uuid()).unwrap().unwrap(), 1); + assert_eq!(ws.by_uuid(t.get_uuid()), Some(1)); } #[test] diff --git a/taskchampion/src/workingset.rs b/taskchampion/src/workingset.rs new file mode 100644 index 000000000..5bdc3696b --- /dev/null +++ b/taskchampion/src/workingset.rs @@ -0,0 +1,132 @@ +use std::collections::HashMap; +use uuid::Uuid; + +/// A WorkingSet represents a snapshot of the working set from a replica. +/// +/// A replica's working set is a mapping from small integers to task uuids for all pending tasks. +/// The small integers are meant to be stable, easily-typed identifiers for users to interact with +/// important tasks. +/// +/// IMPORTANT: the content of the working set may change at any time that a DB transaction is not +/// in progress, and the data in this type will not be updated automatically. It is up to the +/// caller to decide how long to keep this value, and how much to trust the accuracy of its +/// contents. In practice, the answers are usually "a few milliseconds" and treating unexpected +/// results as non-fatal. +pub struct WorkingSet { + by_index: Vec>, + by_uuid: HashMap, +} + +impl WorkingSet { + /// Create a new WorkingSet. Typically this is acquired via `replica.working_set()` + pub(crate) fn new(by_index: Vec>) -> Self { + let mut by_uuid = HashMap::new(); + for (index, uuid) in by_index.iter().enumerate() { + if let Some(uuid) = uuid { + by_uuid.insert(*uuid, index); + } + } + Self { by_index, by_uuid } + } + + /// Get the "length" of the working set: the total number of uuids in the set. + pub fn len(&self) -> usize { + self.by_index.iter().filter(|e| e.is_some()).count() + } + + /// True if the length is zero + pub fn is_empty(&self) -> bool { + self.by_index.iter().all(|e| e.is_none()) + } + + /// Get the uuid with the given index, if any exists. + pub fn by_index(&self, index: usize) -> Option { + if let Some(Some(uuid)) = self.by_index.get(index) { + Some(*uuid) + } else { + None + } + } + + /// Get the index for the given uuid, if any + pub fn by_uuid(&self, uuid: Uuid) -> Option { + self.by_uuid.get(&uuid).copied() + } + + /// Iterate over pairs (index, uuid), in order by index. + pub fn iter(&self) -> impl Iterator + '_ { + self.by_index + .iter() + .enumerate() + .filter_map(|(index, uuid)| { + if let Some(uuid) = uuid { + Some((index, *uuid)) + } else { + None + } + }) + } +} + +#[cfg(test)] +mod test { + use super::*; + + fn make() -> (Uuid, Uuid, WorkingSet) { + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + ( + uuid1, + uuid2, + WorkingSet::new(vec![None, Some(uuid1), None, Some(uuid2), None]), + ) + } + + #[test] + fn test_new() { + let (_, uuid2, ws) = make(); + assert_eq!(ws.by_index[3], Some(uuid2)); + assert_eq!(ws.by_uuid.get(&uuid2), Some(&3)); + } + + #[test] + fn test_len_and_is_empty() { + let (_, _, ws) = make(); + assert_eq!(ws.len(), 2); + assert_eq!(ws.is_empty(), false); + + let ws = WorkingSet::new(vec![]); + assert_eq!(ws.len(), 0); + assert_eq!(ws.is_empty(), true); + + let ws = WorkingSet::new(vec![None, None, None]); + assert_eq!(ws.len(), 0); + assert_eq!(ws.is_empty(), true); + } + + #[test] + fn test_by_index() { + let (uuid1, uuid2, ws) = make(); + assert_eq!(ws.by_index(0), None); + assert_eq!(ws.by_index(1), Some(uuid1)); + assert_eq!(ws.by_index(2), None); + assert_eq!(ws.by_index(3), Some(uuid2)); + assert_eq!(ws.by_index(4), None); + assert_eq!(ws.by_index(100), None); // past the end of the vector + } + + #[test] + fn test_by_uuid() { + let (uuid1, uuid2, ws) = make(); + let nosuch = Uuid::new_v4(); + assert_eq!(ws.by_uuid(uuid1), Some(1)); + assert_eq!(ws.by_uuid(uuid2), Some(3)); + assert_eq!(ws.by_uuid(nosuch), None); + } + + #[test] + fn test_iter() { + let (uuid1, uuid2, ws) = make(); + assert_eq!(ws.iter().collect::>(), vec![(1, uuid1), (3, uuid2),]); + } +}