From 20041c120e0da3752fd3d5cdbdb01cb9b98218f8 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Mon, 2 Aug 2021 01:25:16 +0000 Subject: [PATCH] Refactor to store tags as individual attributes Each tag is stored as `tag_: x`. The `x` is required because empty attributes are treated as nonexistent. For compatibility, the `tags` attribute is updated in sync with the per-tag attributes. This compatibility support may be dropped in later versions. Note that synchronization _updates_ use JSON format, which does not change with this patch, and thus no compatibility issues exist. The synchronization _initialization_, however, uses FF4, meaning that a sync server initialized from a version of `task` with this patch will contain `tag_` attributes, which will look like orphaned UDAs to older versions. However, as updates to tasks are synchronized via the sync server, the updates will not contain these attributes and they will show as "deleted" in the `task info` display on the older version. Aside from the noise in the `task info` output, this is harmless. --- src/Task.cpp | 174 ++++++++++++++++++++++++++------------- src/Task.h | 6 +- src/columns/ColTags.cpp | 60 +++++++------- src/commands/CmdEdit.cpp | 2 +- src/commands/CmdInfo.cpp | 1 + test/tag.t | 11 ++- 6 files changed, 163 insertions(+), 91 deletions(-) diff --git a/src/Task.cpp b/src/Task.cpp index 894a07e27..e7c6c377a 100644 --- a/src/Task.cpp +++ b/src/Task.cpp @@ -630,6 +630,15 @@ void Task::parse (const std::string& input) parseLegacy (input); } + // for compatibility, include all tags in `tags` as `tag_..` attributes + if (data.find ("tags") != data.end ()) { + for (auto& tag : split(data["tags"], ',')) { + data[tag2Attr(tag)] = "x"; + } + } + // ..and similarly, update `tags` to match the `tag_..` attributes + fixTagsAttribute(); + recalc_urgency = true; } @@ -692,16 +701,6 @@ void Task::parseJSON (const json::object* root_obj) addTag (tag->_data); } } - // This is a temporary measure to accomodate a malformed JSON message from - // Mirakel sync. - // - // 2016-02-21 Mirakel dropped sync support in late 2015. This can be - // removed in a later release. - else if (i.first == "tags" && i.second->type() == json::j_string) - { - auto tag = (json::string*)i.second; - addTag (tag->_data); - } // Dependencies can be exported as an array of strings. // 2016-02-21: This will be the only option in future releases. @@ -906,6 +905,10 @@ std::string Task::composeJSON (bool decorate /*= false*/) const if (! i.first.compare (0, 11, "annotation_", 11)) continue; + if (i.first == "tags" || isTagAttr (i.first)) + // Tags are handled below + continue; + // If value is an empty string, do not ever output it if (i.second == "") continue; @@ -947,26 +950,6 @@ std::string Task::composeJSON (bool decorate /*= false*/) const ++attributes_written; } - // Tags are converted to an array. - else if (i.first == "tags") - { - auto tags = split (i.second, ','); - - out << "\"tags\":["; - - int count = 0; - for (const auto& i : tags) - { - if (count++) - out << ','; - - out << '"' << i << '"'; - } - - out << ']'; - ++attributes_written; - } - // Dependencies are an array by default. else if (i.first == "depends" #ifdef PRODUCT_TASKWARRIOR @@ -1047,6 +1030,25 @@ std::string Task::composeJSON (bool decorate /*= false*/) const out << ']'; } + auto tags = getTags(); + if (tags.size() > 0) + { + out << ',' + << "\"tags\":["; + + int count = 0; + for (const auto& tag : tags) + { + if (count++) + out << ','; + + out << '"' << tag << '"'; + } + + out << ']'; + ++attributes_written; + } + #ifdef PRODUCT_TASKWARRIOR // Include urgency. if (decorate) @@ -1257,8 +1259,13 @@ std::vector Task::getDependencyTasks () const //////////////////////////////////////////////////////////////////////////////// int Task::getTagCount () const { - auto tags = split (get ("tags"), ','); - return (int) tags.size (); + auto count = 0; + for (auto& attr : data) { + if (isTagAttr (attr.first)) { + count++; + } + } + return count; } //////////////////////////////////////////////////////////////////////////////// @@ -1301,7 +1308,7 @@ bool Task::hasTag (const std::string& tag) const if (tag == "INSTANCE") return has ("template") || has ("parent"); if (tag == "UNTIL") return has ("until"); if (tag == "ANNOTATED") return hasAnnotations (); - if (tag == "TAGGED") return has ("tags"); + if (tag == "TAGGED") return getTagCount() > 0; if (tag == "PARENT") return has ("mask") || has ("last"); // 2017-01-07: Deprecated in 2.6.0 if (tag == "TEMPLATE") return has ("last") || has ("mask"); if (tag == "WAITING") return get ("status") == "waiting"; @@ -1318,9 +1325,7 @@ bool Task::hasTag (const std::string& tag) const } // Concrete tags. - auto tags = split (get ("tags"), ','); - - if (std::find (tags.begin (), tags.end (), tag) != tags.end ()) + if (has (tag2Attr (tag))) return true; return false; @@ -1329,47 +1334,104 @@ bool Task::hasTag (const std::string& tag) const //////////////////////////////////////////////////////////////////////////////// void Task::addTag (const std::string& tag) { - auto tags = split (get ("tags"), ','); - - if (std::find (tags.begin (), tags.end (), tag) == tags.end ()) - { - tags.push_back (tag); - set ("tags", join (",", tags)); - + auto attr = tag2Attr (tag); + if (!has (attr)) { + set (attr, "x"); recalc_urgency = true; + fixTagsAttribute(); } } //////////////////////////////////////////////////////////////////////////////// -void Task::addTags (const std::vector & tags) +void Task::setTags (const std::vector & tags) { - remove ("tags"); + auto existing = getTags(); - for (auto& tag : tags) + // edit in-place, determining which should be + // added and which should be removed + std::vector toAdd; + std::vector toRemove; + + for (auto& tag : tags) { + if (std::find (existing.begin (), existing.end (), tag) == existing.end ()) + toAdd.push_back(tag); + } + + for (auto& tag : getTags ()) { + if (std::find (tags.begin (), tags.end (), tag) == tags.end ()) { + toRemove.push_back (tag); + } + } + + for (auto& tag : toRemove) { + removeTag (tag); + } + for (auto& tag : toAdd) { addTag (tag); + } - recalc_urgency = true; + // (note: addTag / removeTag took care of recalculating urgency) } //////////////////////////////////////////////////////////////////////////////// std::vector Task::getTags () const { - return split (get ("tags"), ','); + std::vector tags; + + for (auto& attr : data) { + if (!isTagAttr (attr.first)) { + continue; + } + auto tag = attr2Tag (attr.first); + tags.push_back (tag); + } + + return tags; } //////////////////////////////////////////////////////////////////////////////// void Task::removeTag (const std::string& tag) { - auto tags = split (get ("tags"), ','); - - auto i = std::find (tags.begin (), tags.end (), tag); - if (i != tags.end ()) - { - tags.erase (i); - set ("tags", join (",", tags)); + auto attr = tag2Attr (tag); + if (has (attr)) { + data.erase (attr); + recalc_urgency = true; + fixTagsAttribute(); } +} - recalc_urgency = true; +//////////////////////////////////////////////////////////////////////////////// +void Task::fixTagsAttribute () +{ + // Fix up the old `tags` attribute to match the `tags_..` attributes (or + // remove it if there are no tags) + auto tags = getTags (); + if (tags.size () > 0) { + set ("tags", join (",", tags)); + } else { + remove ("tags"); + } +} + +//////////////////////////////////////////////////////////////////////////////// +bool Task::isTagAttr(const std::string& attr) const +{ + return attr.compare(0, 5, "tags_") == 0; +} + +//////////////////////////////////////////////////////////////////////////////// +const std::string Task::tag2Attr (const std::string& tag) const +{ + std::stringstream tag_attr; + tag_attr << "tags_" << tag; + return tag_attr.str(); +} + +//////////////////////////////////////////////////////////////////////////////// +const std::string Task::attr2Tag (const std::string& attr) const +{ + assert (isTagAttr (attr)); + return attr.substr(5); } #ifdef PRODUCT_TASKWARRIOR diff --git a/src/Task.h b/src/Task.h index 49bc4e019..5112dc3f1 100644 --- a/src/Task.h +++ b/src/Task.h @@ -125,7 +125,7 @@ public: int getTagCount () const; bool hasTag (const std::string&) const; void addTag (const std::string&); - void addTags (const std::vector &); + void setTags (const std::vector &); std::vector getTags () const; void removeTag (const std::string&); @@ -170,6 +170,10 @@ private: void validate_before (const std::string&, const std::string&); const std::string encode (const std::string&) const; const std::string decode (const std::string&) const; + bool isTagAttr (const std::string&) const; + const std::string tag2Attr (const std::string&) const; + const std::string attr2Tag (const std::string&) const; + void fixTagsAttribute (); public: float urgency_project () const; diff --git a/src/columns/ColTags.cpp b/src/columns/ColTags.cpp index be1336dc7..97dc41d82 100644 --- a/src/columns/ColTags.cpp +++ b/src/columns/ColTags.cpp @@ -115,17 +115,16 @@ void ColumnTags::render ( int width, Color& color) { - if (task.has (_name)) + auto all = task.getTags (); + if (all.size() > 0) { - std::string tags = task.get (_name); if (_style == "default" || _style == "list") { - if (tags.find (',') != std::string::npos) + if (all.size () > 1) { - auto all = split (tags, ','); std::sort (all.begin (), all.end ()); - tags = join (" ", all); + auto tags = join (" ", all); all.clear (); wrapText (all, tags, width, _hyphenate); @@ -134,7 +133,7 @@ void ColumnTags::render ( renderStringLeft (lines, width, color, i); } else - renderStringLeft (lines, width, color, tags); + renderStringLeft (lines, width, color, all[0]); } else if (_style == "indicator") { @@ -142,7 +141,6 @@ void ColumnTags::render ( } else if (_style == "count") { - auto all = split (tags, ','); renderStringRight (lines, width, color, '[' + format (static_cast (all.size ())) + ']'); } } @@ -152,36 +150,36 @@ void ColumnTags::render ( void ColumnTags::modify (Task& task, const std::string& value) { std::string label = " MODIFICATION "; + std::string commasep; + std::vector tags; - // TW-1701 - task.set (_name, ""); - - for (auto& tag : split (value, ',')) + // If it's a DOM ref, eval it first. + Lexer lexer (value); + std::string domRef; + Lexer::Type type; + if (lexer.token (domRef, type) && + type == Lexer::Type::dom) { - // If it's a DOM ref, eval it first. - Lexer lexer (tag); - std::string domRef; - Lexer::Type type; - if (lexer.token (domRef, type) && - type == Lexer::Type::dom) - { - Eval e; - e.addSource (domSource); - contextTask = task; + Eval e; + e.addSource (domSource); + contextTask = task; - Variant v; - e.evaluateInfixExpression (value, v); - task.addTag ((std::string) v); - Context::getContext ().debug (label + "tags <-- '" + (std::string) v + "' <-- '" + tag + '\''); - } - else - { - task.addTag (tag); - Context::getContext ().debug (label + "tags <-- '" + tag + '\''); - } + Variant v; + e.evaluateInfixExpression (value, v); + commasep = (std::string) v; + } else { + commasep = (std::string) value; + } + + for (auto& tag : split (commasep, ',')) + { + tags.push_back ((std::string) tag); + Context::getContext ().debug (label + "tags <-- '" + tag + '\''); feedback_special_tags (task, tag); } + + task.setTags(tags); } //////////////////////////////////////////////////////////////////////////////// diff --git a/src/commands/CmdEdit.cpp b/src/commands/CmdEdit.cpp index 3751b5e6d..0c69e4faa 100644 --- a/src/commands/CmdEdit.cpp +++ b/src/commands/CmdEdit.cpp @@ -366,7 +366,7 @@ void CmdEdit::parseTask (Task& task, const std::string& after, const std::string // tags value = findValue (after, "\n Tags:"); task.remove ("tags"); - task.addTags (split (value, ' ')); + task.setTags (split (value, ' ')); // description. value = findMultilineValue (after, "\n Description:", "\n Created:"); diff --git a/src/commands/CmdInfo.cpp b/src/commands/CmdInfo.cpp index 7b8d7da22..0d719a2d7 100644 --- a/src/commands/CmdInfo.cpp +++ b/src/commands/CmdInfo.cpp @@ -413,6 +413,7 @@ int CmdInfo::execute (std::string& output) for (auto& att : all) { if (att.substr (0, 11) != "annotation_" && + att.substr (0, 5) != "tags_" && Context::getContext ().columns.find (att) == Context::getContext ().columns.end ()) { row = view.addRow (); diff --git a/test/tag.t b/test/tag.t index 39f7ea609..4ff267c1d 100755 --- a/test/tag.t +++ b/test/tag.t @@ -44,11 +44,16 @@ class TestTags(TestCase): def setUp(self): """Executed before each test in the class""" + def split_tags(self, tags): + return sorted(tags.strip().split(',')) + def test_tag_manipulation(self): """Test addition and removal of tags""" self.t("add +one This +two is a test +three") code, out, err = self.t("_get 1.tags") - self.assertEqual("one,two,three\n", out) + self.assertEqual( + sorted(["one", "two", "three"]), + self.split_tags(out)) # Remove tags. self.t("1 modify -three -two -one") @@ -58,7 +63,9 @@ class TestTags(TestCase): # Add tags. self.t("1 modify +four +five +six") code, out, err = self.t("_get 1.tags") - self.assertEqual("four,five,six\n", out) + self.assertEqual( + sorted(["four", "five", "six"]), + self.split_tags(out)) # Remove tags. self.t("1 modify -four -five -six")