Refactor task diffs to handle new attributes

This refactors task(Info)Differences to be methods of Task and to
correctly handle the `annotation_`, `tags_`, and `dep_` attributes.

src/Task.cpp

@@ -273,7 +273,7 @@ void Task::set (const std::string& name, const std::string& value)
 {
   data[name] = value;
 
-  if (! name.compare (0, 11, "annotation_", 11))
+  if (isAnnotationAttr (name))
     ++annotation_count;
 
   recalc_urgency = true;
@@ -293,7 +293,7 @@ void Task::remove (const std::string& name)
   if (data.erase (name))
     recalc_urgency = true;
 
-  if (! name.compare (0, 11, "annotation_", 11))
+  if (isAnnotationAttr (name))
     --annotation_count;
 }
 
@@ -637,7 +637,7 @@ void Task::parse (const std::string& input)
             legacyAttributeMap (name);
 #endif
 
-            if (! name.compare (0, 11, "annotation_", 11))
+            if (isAnnotationAttr (name))
               ++annotation_count;
 
             data[name] = decode (json::decode (value));
@@ -1529,6 +1529,12 @@ const std::string Task::attr2Dep (const std::string& attr) const
   return attr.substr(4);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+bool Task::isAnnotationAttr(const std::string& attr) const
+{
+  return attr.compare(0, 11, "annotation_") == 0;
+}
+
 #ifdef PRODUCT_TASKWARRIOR
 ////////////////////////////////////////////////////////////////////////////////
 // A UDA Orphan is an attribute that is not represented in context.columns.
@@ -2391,3 +2397,248 @@ void Task::modify (modType type, bool text_required /* = false */)
 #endif
 
 ////////////////////////////////////////////////////////////////////////////////
+// Compare this task to another and summarize the differences for display
+std::string Task::diff (const Task& after) const
+{
+  // Attributes are all there is, so figure the different attribute names
+  // between this (before) and after.
+  std::vector <std::string> beforeAtts;
+  for (auto& att : data)
+    beforeAtts.push_back (att.first);
+
+  std::vector <std::string> afterAtts;
+  for (auto& att : after.data)
+    afterAtts.push_back (att.first);
+
+  std::vector <std::string> beforeOnly;
+  std::vector <std::string> afterOnly;
+  listDiff (beforeAtts, afterAtts, beforeOnly, afterOnly);
+
+  // Now start generating a description of the differences.
+  std::stringstream out;
+  for (auto& name : beforeOnly)
+  {
+    if (isAnnotationAttr (name))
+    {
+      out << "  - "
+          << format ("Annotation {1} will be removed.", name)
+          << "\n";
+    }
+    else if (isTagAttr (name))
+    {
+      out << "  - "
+          << format ("Tag {1} will be removed.", attr2Tag (name))
+          << "\n";
+    }
+    else if (isDepAttr (name))
+    {
+      out << "  - "
+          << format ("Depenency on {1} will be removed.", attr2Dep (name))
+          << "\n";
+    }
+    else if (name == "depends" || name == "tags")
+    {
+      // do nothing for legacy attributes
+    }
+    else
+    {
+      out << "  - "
+          << format ("{1} will be deleted.", Lexer::ucFirst (name))
+          << "\n";
+    }
+  }
+
+  for (auto& name : afterOnly)
+  {
+    if (isAnnotationAttr (name))
+    {
+      out << format ("Annotation of {1} will be added.\n", after.get (name));
+    }
+    else if (isTagAttr (name))
+    {
+      out << format ("Tag {1} will be added.\n", attr2Tag (name));
+    }
+    else if (isDepAttr (name))
+    {
+      out << format ("Dependency on {1} will be added.\n", attr2Dep (name));
+    }
+    else if (name == "depends" || name == "tags")
+    {
+      // do nothing for legacy attributes
+    }
+    else
+      out << "  - "
+          << format ("{1} will be set to '{2}'.",
+                     Lexer::ucFirst (name),
+                     renderAttribute (name, after.get (name)))
+          << "\n";
+  }
+
+  for (auto& name : beforeAtts)
+  {
+    // Ignore UUID differences, and find values that changed, but are not also
+    // in the beforeOnly and afterOnly lists, which have been handled above..
+    if (name              != "uuid" &&
+        get (name)        != after.get (name) &&
+        std::find (beforeOnly.begin (), beforeOnly.end (), name) == beforeOnly.end () &&
+        std::find (afterOnly.begin (),  afterOnly.end (),  name) == afterOnly.end ())
+    {
+      if (name == "depends" || name == "tags")
+      {
+        // do nothing for legacy attributes
+      }
+      else if (isTagAttr (name) || isDepAttr (name))
+      {
+        // ignore new attributes
+      }
+      else if (isAnnotationAttr (name))
+      {
+        out << format ("Annotation will be changed to {1}.\n", after.get (name));
+      }
+      else
+        out << "  - "
+            << format ("{1} will be changed from '{2}' to '{3}'.",
+                       Lexer::ucFirst (name),
+                       renderAttribute (name, get (name)),
+                       renderAttribute (name, after.get (name)))
+            << "\n";
+    }
+  }
+
+  // Shouldn't just say nothing.
+  if (out.str ().length () == 0)
+    out << "  - No changes will be made.\n";
+
+  return out.str ();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Similar to diff, but formatted for inclusion in the output of the info command
+std::string Task::diffForInfo (
+  const Task& after,
+  const std::string& dateformat,
+  long& last_timestamp,
+  const long current_timestamp) const
+{
+  // Attributes are all there is, so figure the different attribute names
+  // between before and after.
+  std::vector <std::string> beforeAtts;
+  for (auto& att : data)
+    beforeAtts.push_back (att.first);
+
+  std::vector <std::string> afterAtts;
+  for (auto& att : after.data)
+    afterAtts.push_back (att.first);
+
+  std::vector <std::string> beforeOnly;
+  std::vector <std::string> afterOnly;
+  listDiff (beforeAtts, afterAtts, beforeOnly, afterOnly);
+
+  // Now start generating a description of the differences.
+  std::stringstream out;
+  for (auto& name : beforeOnly)
+  {
+    if (isAnnotationAttr (name))
+    {
+      out << format ("Annotation '{1}' deleted.\n", get (name));
+    }
+    else if (isTagAttr (name))
+    {
+      out << format ("Tag '{1}' deleted.\n", attr2Tag(name));
+    }
+    else if (isDepAttr (name))
+    {
+      out << format ("Dependency on '{1}' deleted.\n", attr2Dep(name));
+    }
+    else if (name == "depends" || name == "tags")
+    {
+      // do nothing for legacy attributes
+    }
+    else if (name == "start")
+    {
+      Datetime started (get ("start"));
+      Datetime stopped;
+
+      if (after.has ("end"))
+        // Task was marked as finished, use end time
+        stopped = Datetime (after.get ("end"));
+      else
+        // Start attribute was removed, use modification time
+        stopped = Datetime (current_timestamp);
+
+      out << format ("{1} deleted (duration: {2}).",
+                     Lexer::ucFirst (name),
+                     Duration (stopped - started).format ())
+          << "\n";
+    }
+    else
+    {
+      out << format ("{1} deleted.\n", Lexer::ucFirst (name));
+    }
+  }
+
+  for (auto& name : afterOnly)
+  {
+    if (isAnnotationAttr (name))
+    {
+      out << format ("Annotation of '{1}' added.\n", after.get (name));
+    }
+    else if (isTagAttr (name))
+    {
+      out << format ("Tag '{1}' added.\n", attr2Tag (name));
+    }
+    else if (isDepAttr (name))
+    {
+      out << format ("Dependency on '{1}' added.\n", attr2Dep (name));
+    }
+    else if (name == "depends" || name == "tags")
+    {
+      // do nothing for legacy attributes
+    }
+    else
+    {
+      if (name == "start")
+          last_timestamp = current_timestamp;
+
+      out << format ("{1} set to '{2}'.",
+                     Lexer::ucFirst (name),
+                     renderAttribute (name, after.get (name), dateformat))
+          << "\n";
+    }
+  }
+
+  for (auto& name : beforeAtts)
+    if (name              != "uuid" &&
+        name              != "modified" &&
+        get (name)        != after.get (name) &&
+        get (name)        != "" &&
+        after.get (name)  != "")
+    {
+      if (name == "depends" || name == "tags")
+      {
+        // do nothing for legacy attributes
+      }
+      else if (isTagAttr (name) || isDepAttr (name))
+      {
+        // ignore new attributes
+      }
+      else if (isAnnotationAttr (name))
+      {
+        out << format ("Annotation changed to '{1}'.\n", after.get (name));
+      }
+      else
+        out << format ("{1} changed from '{2}' to '{3}'.",
+                       Lexer::ucFirst (name),
+                       renderAttribute (name, get (name), dateformat),
+                       renderAttribute (name, after.get (name), dateformat))
+            << "\n";
+    }
+
+  // Shouldn't just say nothing.
+  if (out.str ().length () == 0)
+    out << "No changes made.\n";
+
+  return out.str ();
+}
+
+////////////////////////////////////////////////////////////////////////////////

src/Task.h

@@ -165,6 +165,9 @@ public:
   void modify (modType, bool text_required = false);
 #endif
 
+  std::string diff (const Task& after) const;
+  std::string diffForInfo (const Task& after, const std::string& dateformat, long& last_timestamp, const long current_timestamp) const; 
+
 private:
   int determineVersion (const std::string&);
   void parseJSON (const std::string&);
@@ -179,6 +182,7 @@ private:
   bool isDepAttr (const std::string&) const;
   const std::string dep2Attr (const std::string&) const;
   const std::string attr2Dep (const std::string&) const;
+  bool isAnnotationAttr (const std::string&) const;
   void fixDependsAttribute ();
   void fixTagsAttribute ();
 

src/commands/CmdAnnotate.cpp

@@ -84,7 +84,7 @@ int CmdAnnotate::execute (std::string&)
 
     task.modify (Task::modAnnotate, true);
 
-    if (permission (taskDifferences (before, task) + question, filtered.size ()))
+    if (permission (before.diff (task) + question, filtered.size ()))
     {
       Context::getContext ().tdb2.modify (task);
       ++count;

src/commands/CmdAppend.cpp

@@ -84,7 +84,7 @@ int CmdAppend::execute (std::string&)
 
     task.modify (Task::modAppend, true);
 
-    if (permission (taskDifferences (before, task) + question, filtered.size ()))
+    if (permission (before.diff (task) + question, filtered.size ()))
     {
       Context::getContext ().tdb2.modify (task);
       ++count;

src/commands/CmdDenotate.cpp

@@ -134,7 +134,7 @@ int CmdDenotate::execute (std::string&)
                               task.identifier (true),
                               task.get ("description"));
 
-      if (permission (taskDifferences (before, task) + question, filtered.size ()))
+      if (permission (before.diff (task) + question, filtered.size ()))
       {
         ++count;
         Context::getContext ().tdb2.modify (task);

src/commands/CmdDone.cpp

@@ -98,7 +98,7 @@ int CmdDone::execute (std::string&)
           task.addAnnotation (Context::getContext ().config.get ("journal.time.stop.annotation"));
       }
 
-      if (permission (taskDifferences (before, task) + question, filtered.size ()))
+      if (permission (before.diff (task) + question, filtered.size ()))
       {
         updateRecurrenceMask (task);
         Context::getContext ().tdb2.modify (task);

src/commands/CmdInfo.cpp

@@ -571,7 +571,7 @@ int CmdInfo::execute (std::string& output)
 
             Task before (undo[previous].substr (4));
             Task after (undo[current].substr (4));
-            journal.set (row, 1, taskInfoDifferences (before, after, dateformat, last_timestamp, Datetime(after.get("modified")).toEpoch()));
+            journal.set (row, 1, before.diffForInfo (after, dateformat, last_timestamp, Datetime(after.get("modified")).toEpoch()));
           }
         }
       }

src/commands/CmdModify.cpp

@@ -88,7 +88,7 @@ int CmdModify::execute (std::string&)
                               task.identifier (true),
                               task.get ("description"));
 
-      if (permission (taskDifferences (before, task) + question, filtered.size ()))
+      if (permission (before.diff (task) + question, filtered.size ()))
       {
         count += modifyAndUpdate (before, task, &projectChanges);
       }

src/commands/CmdPrepend.cpp

@@ -84,7 +84,7 @@ int CmdPrepend::execute (std::string&)
 
     task.modify (Task::modPrepend, true);
 
-    if (permission (taskDifferences (before, task) + question, filtered.size ()))
+    if (permission (before.diff (task) + question, filtered.size ()))
     {
       Context::getContext ().tdb2.modify (task);
       ++count;

src/commands/CmdStart.cpp

@@ -96,7 +96,7 @@ int CmdStart::execute (std::string&)
       if (Context::getContext ().config.getBoolean ("journal.time"))
         task.addAnnotation (Context::getContext ().config.get ("journal.time.start.annotation"));
 
-      if (permission (taskDifferences (before, task) + question, filtered.size ()))
+      if (permission (before.diff (task) + question, filtered.size ()))
       {
         updateRecurrenceMask (task);
         Context::getContext ().tdb2.modify (task);

src/commands/CmdStop.cpp

@@ -87,7 +87,7 @@ int CmdStop::execute (std::string&)
       if (Context::getContext ().config.getBoolean ("journal.time"))
         task.addAnnotation (Context::getContext ().config.get ("journal.time.stop.annotation"));
 
-      if (permission (taskDifferences (before, task) + question, filtered.size ()))
+      if (permission (before.diff (task) + question, filtered.size ()))
       {
         updateRecurrenceMask (task);
         Context::getContext ().tdb2.modify (task);

src/feedback.cpp

@@ -42,213 +42,6 @@
 
 static void countTasks (const std::vector <Task>&, const std::string&, int&, int&);
 
-////////////////////////////////////////////////////////////////////////////////
-// Converts a vector of tasks to a human-readable string that represents the tasks.
-std::string taskIdentifiers (const std::vector <Task>& tasks)
-{
-  std::vector <std::string> identifiers;
-  identifiers.reserve(tasks.size());
-  for (const auto& task: tasks)
-    identifiers.push_back (task.identifier (true));
-
-  return join (", ", identifiers);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-std::string taskDifferences (const Task& before, const Task& after)
-{
-  // Attributes are all there is, so figure the different attribute names
-  // between before and after.
-  std::vector <std::string> beforeAtts;
-  for (auto& att : before.data)
-    beforeAtts.push_back (att.first);
-
-  std::vector <std::string> afterAtts;
-  for (auto& att : after.data)
-    afterAtts.push_back (att.first);
-
-  std::vector <std::string> beforeOnly;
-  std::vector <std::string> afterOnly;
-  listDiff (beforeAtts, afterAtts, beforeOnly, afterOnly);
-
-  // Now start generating a description of the differences.
-  std::stringstream out;
-  for (auto& name : beforeOnly)
-    out << "  - "
-        << format ("{1} will be deleted.", Lexer::ucFirst (name))
-        << "\n";
-
-  // TODO: #2572 - rewrite to look at dep_ and tag_
-  for (auto& name : afterOnly)
-  {
-    if (name == "depends")
-    {
-      auto deps_after = after.getDependencyTasks ();
-
-      out << "  - "
-          << format ("Dependencies will be set to '{1}'.", taskIdentifiers (deps_after))
-          << "\n";
-    }
-    else
-      out << "  - "
-          << format ("{1} will be set to '{2}'.",
-                     Lexer::ucFirst (name),
-                     renderAttribute (name, after.get (name)))
-          << "\n";
-  }
-
-  for (auto& name : beforeAtts)
-  {
-    // Ignore UUID differences, and find values that changed, but are not also
-    // in the beforeOnly and afterOnly lists, which have been handled above..
-    if (name              != "uuid" &&
-        before.get (name) != after.get (name) &&
-        std::find (beforeOnly.begin (), beforeOnly.end (), name) == beforeOnly.end () &&
-        std::find (afterOnly.begin (),  afterOnly.end (),  name) == afterOnly.end ())
-    {
-      if (name == "depends")
-      {
-        auto deps_before = before.getDependencyTasks ();
-        std::string from = taskIdentifiers (deps_before);
-
-        auto deps_after = after.getDependencyTasks ();
-        std::string to = taskIdentifiers (deps_after);
-
-        out << "  - "
-            << format ("Dependencies will be changed from '{1}' to '{2}'.", from, to)
-            << "\n";
-      }
-      else
-        out << "  - "
-            << format ("{1} will be changed from '{2}' to '{3}'.",
-                       Lexer::ucFirst (name),
-                       renderAttribute (name, before.get (name)),
-                       renderAttribute (name, after.get (name)))
-            << "\n";
-    }
-  }
-
-  // Shouldn't just say nothing.
-  if (out.str ().length () == 0)
-    out << "  - No changes will be made.\n";
-
-  return out.str ();
-}
-
-////////////////////////////////////////////////////////////////////////////////
-std::string taskInfoDifferences (
-  const Task& before,
-  const Task& after,
-  const std::string& dateformat,
-  long& last_timestamp,
-  const long current_timestamp)
-{
-  // Attributes are all there is, so figure the different attribute names
-  // between before and after.
-  std::vector <std::string> beforeAtts;
-  for (auto& att : before.data)
-    beforeAtts.push_back (att.first);
-
-  std::vector <std::string> afterAtts;
-  for (auto& att : after.data)
-    afterAtts.push_back (att.first);
-
-  std::vector <std::string> beforeOnly;
-  std::vector <std::string> afterOnly;
-  listDiff (beforeAtts, afterAtts, beforeOnly, afterOnly);
-
-  // Now start generating a description of the differences.
-  std::stringstream out;
-  for (auto& name : beforeOnly)
-  {
-    if (name == "depends")
-    {
-        out << format ("Dependencies '{1}' deleted.", taskIdentifiers (before.getDependencyTasks ()))
-            << "\n";
-    }
-    else if (name.substr (0, 11) == "annotation_")
-    {
-      out << format ("Annotation '{1}' deleted.\n", before.get (name));
-    }
-    else if (name == "start")
-    {
-      Datetime started (before.get ("start"));
-      Datetime stopped;
-
-      if (after.has ("end"))
-        // Task was marked as finished, use end time
-        stopped = Datetime (after.get ("end"));
-      else
-        // Start attribute was removed, use modification time
-        stopped = Datetime (current_timestamp);
-
-      out << format ("{1} deleted (duration: {2}).",
-                     Lexer::ucFirst (name),
-                     Duration (stopped - started).format ())
-          << "\n";
-    }
-    else
-    {
-      out << format ("{1} deleted.\n", Lexer::ucFirst (name));
-    }
-  }
-
-  for (auto& name : afterOnly)
-  {
-    if (name == "depends")
-    {
-      out << format ("Dependencies set to '{1}'.", taskIdentifiers (after.getDependencyTasks ()))
-          << "\n";
-    }
-    else if (name.substr (0, 11) == "annotation_")
-    {
-      out << format ("Annotation of '{1}' added.\n", after.get (name));
-    }
-    else
-    {
-      if (name == "start")
-          last_timestamp = current_timestamp;
-
-      out << format ("{1} set to '{2}'.",
-                     Lexer::ucFirst (name),
-                     renderAttribute (name, after.get (name), dateformat))
-          << "\n";
-    }
-  }
-
-  for (auto& name : beforeAtts)
-    if (name              != "uuid" &&
-        name              != "modified" &&
-        before.get (name) != after.get (name) &&
-        before.get (name) != "" &&
-        after.get (name)  != "")
-    {
-      if (name == "depends")
-      {
-        auto from = taskIdentifiers (before.getDependencyTasks ());
-        auto to   = taskIdentifiers (after.getDependencyTasks ());
-
-        out << format ("Dependencies changed from '{1}' to '{2}'.\n", from, to);
-      }
-      else if (name.substr (0, 11) == "annotation_")
-      {
-        out << format ("Annotation changed to '{1}'.\n", after.get (name));
-      }
-      else
-        out << format ("{1} changed from '{2}' to '{3}'.",
-                       Lexer::ucFirst (name),
-                       renderAttribute (name, before.get (name), dateformat),
-                       renderAttribute (name, after.get (name), dateformat))
-            << "\n";
-    }
-
-  // Shouldn't just say nothing.
-  if (out.str ().length () == 0)
-    out << "No changes made.\n";
-
-  return out.str ();
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 std::string renderAttribute (const std::string& name, const std::string& value, const std::string& format /* = "" */)
 {

src/main.h

@@ -64,8 +64,6 @@ void dependencyChainOnComplete (Task&);
 void dependencyChainOnStart (Task&);
 
 // feedback.cpp
-std::string taskDifferences (const Task&, const Task&);
-std::string taskInfoDifferences (const Task&, const Task&, const std::string&, long&, const long);
 std::string renderAttribute (const std::string&, const std::string&, const std::string& format = "");
 void feedback_affected (const std::string&);
 void feedback_affected (const std::string&, int);

test/util.t.cpp

@@ -57,14 +57,14 @@ int main (int, char**)
 
   Task rightAgain (right);
 
-  std::string output = taskDifferences (left, right);
+  std::string output = left.diff (right);
   t.ok (left.data != right.data,                                                              "Detected changes");
   t.ok (output.find ("Zero will be changed from '0' to '00'") != std::string::npos, "Detected change zero:0 -> zero:00");
   t.ok (output.find ("One will be deleted")                   != std::string::npos, "Detected deletion one:1 ->");
   t.ok (output.find ("Two")                                   == std::string::npos, "Detected no change two:2 -> two:2");
   t.ok (output.find ("Three will be set to '3'")              != std::string::npos, "Detected addition -> three:3");
 
-  output = taskDifferences (right, rightAgain);
+  output = right.diff (rightAgain);
   t.ok (output.find ("No changes will be made")               != std::string::npos, "No changes detected");
 
   // std::vector<std::string> indentProject (const std::string&, const std::string whitespace="  ", char delimiter='.');