Tags: implement multi values, change .set to .add

src/ogg.cc

@@ -121,7 +121,7 @@ void ogg::Stream::parse_opustags(const ogg_packet &op)
         uint32_t comment_length = le32toh(*reinterpret_cast<uint32_t*>(data));
         if (remaining - 4 < comment_length)
             throw std::runtime_error("no space for comment contents");
-        tags.set(std::string(data + 4, comment_length));
+        tags.add(std::string(data + 4, comment_length));
         data += 4 + comment_length;
         remaining -= 4 + comment_length;
     }
@@ -290,12 +290,12 @@ std::string ogg::Encoder::render_opustags(const Tags &tags)
     length = htole32(assocs.size());
     s.sputn(reinterpret_cast<char*>(&length), 4);
 
-    for (auto assoc : assocs) {
-        length = htole32(assoc.first.size() + 1 + assoc.second.size());
+    for (const auto assoc : assocs) {
+        length = htole32(assoc.key.size() + 1 + assoc.value.size());
         s.sputn(reinterpret_cast<char*>(&length), 4);
-        s.sputn(assoc.first.data(), assoc.first.size());
+        s.sputn(assoc.key.data(), assoc.key.size());
         s.sputc('=');
-        s.sputn(assoc.second.data(), assoc.second.size());
+        s.sputn(assoc.value.data(), assoc.value.size());
     }
 
     s.sputn(tags.extra.data(), tags.extra.size());

src/tags.cc

@@ -3,63 +3,54 @@
 
 using namespace opustags;
 
-Tags::Tags() : max_index(0)
+const std::vector<Tag> Tags::get_all() const
 {
-}
-
-const std::vector<std::pair<std::string, std::string>> Tags::get_all() const
-{
-    std::vector<std::string> keys;
-    for (const auto &kv : key_to_value)
-        keys.push_back(kv.first);
-
-    std::sort(
-        keys.begin(),
-        keys.end(),
-        [&](const std::string &a, const std::string &b)
-        {
-            return key_to_index.at(a) < key_to_index.at(b);
-        });
-
-    std::vector<std::pair<std::string, std::string>> result;
-    for (const auto &key : keys) {
-        result.push_back(
-            std::make_pair(
-                key, key_to_value.at(key)));
-    }
-
-    return result;
+    return tags;
 }
 
 std::string Tags::get(const std::string &key) const
 {
-    return key_to_value.at(key);
+    for (auto &tag : tags)
+        if (tag.key == key)
+            return tag.value;
+    throw std::runtime_error("Tag '" + key + "' not found.");
 }
 
-void Tags::set(const std::string &key, const std::string &value)
+void Tags::add(const std::string &key, const std::string &value)
 {
-    key_to_value[key] = value;
-    key_to_index[key] = max_index;
-    max_index++;
+    tags.push_back({key, value});
 }
 
-void Tags::set(const std::string &assoc)
+void Tags::clear()
+{
+    tags.clear();
+}
+
+void Tags::add(const std::string &assoc)
 {
     size_t eq = assoc.find_first_of('=');
     if (eq == std::string::npos)
         throw std::runtime_error("misconstructed tag");
     std::string name = assoc.substr(0, eq);
     std::string value = assoc.substr(eq + 1);
-    set(name, value);
+    add(name, value);
 }
 
 void Tags::remove(const std::string &key)
 {
-    key_to_value.erase(key);
-    key_to_index.erase(key);
+    std::vector<Tag> new_tags;
+    std::copy_if(
+        tags.begin(),
+        tags.end(),
+        std::back_inserter(new_tags),
+        [&](const Tag &tag) { return tag.key != key; });
+    tags = new_tags;
 }
 
 bool Tags::contains(const std::string &key) const
 {
-    return key_to_value.find(key) != key_to_value.end();
+    return std::count_if(
+        tags.begin(),
+        tags.end(),
+        [&](const Tag &tag) { return tag.key == key; }) > 0;
 }

src/tags.h

@@ -6,19 +6,24 @@
 
 namespace opustags {
 
+    struct Tag final
+    {
+        std::string key;
+        std::string value;
+    };
+
     // A std::map adapter that keeps the order of insertion.
     class Tags final
     {
     public:
-        Tags();
-
-        const std::vector<std::pair<std::string, std::string>> get_all() const;
+        const std::vector<Tag> get_all() const;
 
         std::string get(const std::string &key) const;
-        void set(const std::string &key, const std::string &value);
-        void set(const std::string &assoc); // KEY=value
+        void add(const std::string &key, const std::string &value);
+        void add(const std::string &assoc); // KEY=value
         void remove(const std::string &key);
         bool contains(const std::string &key) const;
+        void clear();
 
         // Additional fields are required to match the specs:
         // https://tools.ietf.org/html/draft-ietf-codec-oggopus-14#section-5.2
@@ -27,9 +32,7 @@ namespace opustags {
         std::string extra;
 
     private:
-        std::map<std::string, std::string> key_to_value;
-        std::map<std::string, size_t> key_to_index;
-        size_t max_index;
+        std::vector<Tag> tags;
     };
 
 }

src/tags_handlers/insertion_tags_handler.cc

@@ -26,6 +26,6 @@ bool InsertionTagsHandler::edit_impl(Tags &tags)
     if (tags.contains(tag_key))
         throw TagAlreadyExistsError(tag_key);
 
-    tags.set(tag_key, tag_value);
+    tags.add(tag_key, tag_value);
     return true;
 }

src/tags_handlers/listing_tags_handler.cc

@@ -11,6 +11,6 @@ ListingTagsHandler::ListingTagsHandler(
 
 void ListingTagsHandler::list_impl(const Tags &tags)
 {
-    for (const auto &kv : tags.get_all())
-        output_stream << std::get<0>(kv) << "=" << std::get<1>(kv) << "\n";
+    for (const auto &tag : tags.get_all())
+        output_stream << tag.key << "=" << tag.value << "\n";
 }

src/tags_handlers/modification_tags_handler.cc

@@ -22,9 +22,13 @@ std::string ModificationTagsHandler::get_tag_value() const
 
 bool ModificationTagsHandler::edit_impl(Tags &tags)
 {
-    if (tags.contains(tag_key) && tags.get(tag_key) == tag_value)
-        return false;
+    if (tags.contains(tag_key))
+    {
+        if (tags.get(tag_key) == tag_value)
+            return false;
+        tags.remove(tag_key);
+    }
 
-    tags.set(tag_key, tag_value);
+    tags.add(tag_key, tag_value);
     return true;
 }

src/tags_handlers/removal_tags_handler.cc

@@ -23,10 +23,9 @@ bool RemovalTagsHandler::edit_impl(Tags &tags)
 {
     if (tag_key.empty())
     {
-        const auto all_tags = tags.get_all();
-        for (const auto &kv : all_tags)
-            tags.remove(std::get<0>(kv));
-        return !all_tags.empty();
+        const auto anything_removed = tags.get_all().size() > 0;
+        tags.clear();
+        return anything_removed;
     }
     else
     {

tests/tags_handlers/listing_tags_handler_test.cc

@@ -9,10 +9,10 @@ TEST_CASE("Listing tags handler test")
     const auto streamno = 1;
 
     Tags tags;
-    tags.set("z", "value1");
-    tags.set("a", "value2");
-    tags.set("y", "value3");
-    tags.set("c", "value4");
+    tags.add("z", "value1");
+    tags.add("a", "value2");
+    tags.add("y", "value3");
+    tags.add("c", "value4");
 
     std::stringstream ss;
     ListingTagsHandler handler(streamno, ss);

tests/tags_handlers/modification_tags_handler_test.cc

@@ -12,7 +12,7 @@ TEST_CASE("Modification tags handler test")
     const auto new_value = "dummy 2";
 
     Tags tags;
-    tags.set(first_tag_key, dummy_value);
+    tags.add(first_tag_key, dummy_value);
 
     REQUIRE(tags.get_all().size() == 1);
 

tests/tags_handlers/removal_tags_handler_test.cc

@@ -15,8 +15,8 @@ TEST_CASE("Removal tags handler test")
         RemovalTagsHandler handler(streamno, expected_tag_key);
 
         Tags tags;
-        tags.set(expected_tag_key, dummy_value);
-        tags.set(other_tag_key, dummy_value);
+        tags.add(expected_tag_key, dummy_value);
+        tags.add(other_tag_key, dummy_value);
 
         REQUIRE(tags.get_all().size() == 2);
         REQUIRE(handler.edit(streamno, tags));
@@ -30,10 +30,10 @@ TEST_CASE("Removal tags handler test")
         RemovalTagsHandler handler(streamno);
 
         Tags tags;
-        tags.set("z", "value1");
-        tags.set("a", "value2");
-        tags.set("y", "value3");
-        tags.set("c", "value4");
+        tags.add("z", "value1");
+        tags.add("a", "value2");
+        tags.add("y", "value3");
+        tags.add("c", "value4");
 
         REQUIRE(tags.get_all().size() == 4);
         REQUIRE(handler.edit(streamno, tags));

tests/tags_test.cc

@@ -8,7 +8,7 @@ TEST_CASE("Tag manipulation test", "[tags]")
     SECTION("Basic operations") {
         Tags tags;
         REQUIRE(!tags.contains("a"));
-        tags.set("a", "1");
+        tags.add("a", "1");
         REQUIRE(tags.get("a") == "1");
         REQUIRE(tags.contains("a"));
         tags.remove("a");
@@ -16,48 +16,68 @@ TEST_CASE("Tag manipulation test", "[tags]")
         REQUIRE_THROWS(tags.get("a"));
     }
 
+    SECTION("Clearing") {
+        Tags tags;
+        tags.add("a", "1");
+        tags.add("b", "2");
+        REQUIRE(tags.get_all().size() == 2);
+        tags.clear();
+        REQUIRE(tags.get_all().size() == 0);
+    }
+
     SECTION("Maintaing order of insertions") {
         Tags tags;
-        tags.set("z", "1");
-        tags.set("y", "2");
-        tags.set("x", "3");
-        tags.set("y", "4");
+        tags.add("z", "1");
+        tags.add("y", "2");
+        tags.add("x", "3");
+        tags.add("y", "4");
 
-        REQUIRE(tags.get_all().size() == 3);
-        REQUIRE(std::get<0>(tags.get_all()[0]) == "z");
-        REQUIRE(std::get<0>(tags.get_all()[1]) == "x");
-        REQUIRE(std::get<0>(tags.get_all()[2]) == "y");
+        REQUIRE(tags.get_all().size() == 4);
+        REQUIRE(tags.get_all()[0].key == "z");
+        REQUIRE(tags.get_all()[1].key == "y");
+        REQUIRE(tags.get_all()[2].key == "x");
+        REQUIRE(tags.get_all()[3].key == "y");
 
         tags.remove("z");
-        REQUIRE(tags.get_all().size() == 2);
-        REQUIRE(std::get<0>(tags.get_all()[0]) == "x");
-        REQUIRE(std::get<0>(tags.get_all()[1]) == "y");
-
-        tags.set("gamma", "5");
         REQUIRE(tags.get_all().size() == 3);
-        REQUIRE(std::get<0>(tags.get_all()[0]) == "x");
-        REQUIRE(std::get<0>(tags.get_all()[1]) == "y");
-        REQUIRE(std::get<0>(tags.get_all()[2]) == "gamma");
+        REQUIRE(tags.get_all()[0].key == "y");
+        REQUIRE(tags.get_all()[1].key == "x");
+        REQUIRE(tags.get_all()[2].key == "y");
+
+        tags.add("gamma", "5");
+        REQUIRE(tags.get_all().size() == 4);
+        REQUIRE(tags.get_all()[0].key == "y");
+        REQUIRE(tags.get_all()[1].key == "x");
+        REQUIRE(tags.get_all()[2].key == "y");
+        REQUIRE(tags.get_all()[3].key == "gamma");
     }
 
     SECTION("Key to multiple values") {
         // ARTIST is set once per artist.
         // https://www.xiph.org/vorbis/doc/v-comment.html
         Tags tags;
-        tags.set("ARTIST", "You");
-        tags.set("ARTIST", "Me");
+        tags.add("ARTIST", "You");
+        tags.add("ARTIST", "Me");
         REQUIRE(tags.get_all().size() == 2);
     }
 
+    SECTION("Removing multivalues should remove all of them") {
+        Tags tags;
+        tags.add("ARTIST", "You");
+        tags.add("ARTIST", "Me");
+        tags.remove("ARTIST");
+        REQUIRE(tags.get_all().size() == 0);
+    }
+
     SECTION("Raw set") {
         Tags tags;
-        tags.set("TITLE=Foo=Bar");
+        tags.add("TITLE=Foo=Bar");
         REQUIRE(tags.get("TITLE") == "Foo=Bar");
     }
 
     SECTION("Case insensitiveness for keys") {
         Tags tags;
-        tags.set("TiTlE=Boop");
+        tags.add("TiTlE=Boop");
         REQUIRE(tags.get("tiTLE") == "Boop");
     }
 }