db: fix broken command "list ... group"

Makefile.am

@@ -985,7 +985,6 @@ libtag_a_SOURCES =\
 	src/tag/TagString.cxx src/tag/TagString.hxx \
 	src/tag/TagPool.cxx src/tag/TagPool.hxx \
 	src/tag/TagTable.cxx src/tag/TagTable.hxx \
-	src/tag/Set.cxx src/tag/Set.hxx \
 	src/tag/Format.cxx src/tag/Format.hxx \
 	src/tag/VorbisComment.cxx src/tag/VorbisComment.hxx \
 	src/tag/ReplayGain.cxx src/tag/ReplayGain.hxx \

NEWS

@@ -2,6 +2,7 @@ ver 0.20.22 (not yet released)
 * protocol
   - add tag fallbacks for AlbumArtistSort, ArtistSort
   - "count group ..." can print an empty group
+  - fix broken command "list ... group"
 * storage
   - curl: URL-encode paths
 * Android

configure.ac

@@ -14,7 +14,7 @@ AM_SILENT_RULES
 AC_CONFIG_HEADERS(config.h)
 AC_CONFIG_MACRO_DIR([m4])
 
-AC_DEFINE(PROTOCOL_VERSION, "0.20.0", [The MPD protocol version])
+AC_DEFINE(PROTOCOL_VERSION, "0.20.22", [The MPD protocol version])
 
 GIT_COMMIT=`cd "$srcdir" && git describe --dirty --always 2>/dev/null`
 if test x$GIT_COMMIT != x; then

src/command/DatabaseCommands.cxx

@@ -191,7 +191,7 @@ handle_list(Client &client, Request args, Response &r)
 	}
 
 	std::unique_ptr<SongFilter> filter;
-	tag_mask_t group_mask = 0;
+	TagType group = TAG_NUM_OF_ITEM_TYPES;
 
 	if (args.size == 1) {
 		/* for compatibility with < 0.12.0 */
@@ -206,18 +206,16 @@ handle_list(Client &client, Request args, Response &r)
 					    args.shift()));
 	}
 
-	while (args.size >= 2 &&
-	       StringIsEqual(args[args.size - 2], "group")) {
+	if  (args.size >= 2 &&
+	     StringIsEqual(args[args.size - 2], "group")) {
 		const char *s = args[args.size - 1];
-		TagType gt = tag_name_parse_i(s);
-		if (gt == TAG_NUM_OF_ITEM_TYPES) {
+		group = tag_name_parse_i(s);
+		if (group == TAG_NUM_OF_ITEM_TYPES) {
 			r.FormatError(ACK_ERROR_ARG,
 				      "Unknown tag type: %s", s);
 			return CommandResult::ERROR;
 		}
 
-		group_mask |= tag_mask_t(1) << unsigned(gt);
-
 		args.pop_back();
 		args.pop_back();
 	}
@@ -230,14 +228,13 @@ handle_list(Client &client, Request args, Response &r)
 		}
 	}
 
-	if (tagType < TAG_NUM_OF_ITEM_TYPES &&
-	    group_mask & (tag_mask_t(1) << tagType)) {
+	if (tagType < TAG_NUM_OF_ITEM_TYPES && tagType == group) {
 		r.Error(ACK_ERROR_ARG, "Conflicting group");
 		return CommandResult::ERROR;
 	}
 
 	PrintUniqueTags(r, client.partition,
-			tagType, group_mask, filter.get());
+			tagType, group, filter.get());
 	return CommandResult::OK;
 }
 

src/db/DatabasePrint.cxx

@@ -187,22 +187,34 @@ PrintSongURIVisitor(Response &r, Partition &partition, const LightSong &song)
 }
 
 static void
-PrintUniqueTag(Response &r, TagType tag_type,
-	       const Tag &tag)
+PrintUniqueTags(Response &r, TagType tag_type,
+		const std::set<std::string> &values)
 {
-	const char *value = tag.GetValue(tag_type);
-	assert(value != nullptr);
-	r.Format("%s: %s\n", tag_item_names[tag_type], value);
+	const char *const name = tag_item_names[tag_type];
+	for (const auto &i : values)
+		r.Format("%s: %s\n", name, i.c_str());
+}
 
-	for (const auto &item : tag)
-		if (item.type != tag_type)
-			r.Format("%s: %s\n",
-				 tag_item_names[item.type], item.value);
+static void
+PrintGroupedUniqueTags(Response &r, TagType tag_type, TagType group,
+		       const std::map<std::string, std::set<std::string>> &groups)
+{
+	if (group == TAG_NUM_OF_ITEM_TYPES) {
+		for (const auto &i : groups)
+			PrintUniqueTags(r, tag_type, i.second);
+		return;
+	}
+
+	const char *const group_name = tag_item_names[group];
+	for (const auto &i : groups) {
+		r.Format("%s: %s\n", group_name, i.first.c_str());
+		PrintUniqueTags(r, tag_type, i.second);
+	}
 }
 
 void
 PrintUniqueTags(Response &r, Partition &partition,
-		unsigned type, tag_mask_t group_mask,
+		unsigned type, TagType group,
 		const SongFilter *filter)
 {
 	const Database &db = partition.GetDatabaseOrThrow();
@@ -217,10 +229,9 @@ PrintUniqueTags(Response &r, Partition &partition,
 	} else {
 		assert(type < TAG_NUM_OF_ITEM_TYPES);
 
-		using namespace std::placeholders;
-		const auto f = std::bind(PrintUniqueTag, std::ref(r),
-					 (TagType)type, _1);
-		db.VisitUniqueTags(selection, (TagType)type,
-				   group_mask, f);
+		PrintGroupedUniqueTags(r, TagType(type), group,
+				       db.CollectUniqueTags(selection,
+							    TagType(type),
+							    group));
 	}
 }

src/db/DatabasePrint.hxx

@@ -20,7 +20,7 @@
 #ifndef MPD_DB_PRINT_H
 #define MPD_DB_PRINT_H
 
-#include "tag/Mask.hxx"
+#include "tag/TagType.h"
 
 class SongFilter;
 struct DatabaseSelection;
@@ -44,7 +44,7 @@ db_selection_print(Response &r, Partition &partition,
 
 void
 PrintUniqueTags(Response &r, Partition &partition,
-		unsigned type, tag_mask_t group_mask,
+		unsigned type, TagType group,
 		const SongFilter *filter);
 
 #endif

src/db/Interface.hxx

@@ -22,9 +22,12 @@
 
 #include "Visitor.hxx"
 #include "tag/TagType.h"
-#include "tag/Mask.hxx"
 #include "Compiler.h"
 
+#include <map>
+#include <set>
+#include <string>
+
 #include <time.h>
 
 struct DatabasePlugin;
@@ -99,12 +102,9 @@ public:
 		return Visit(selection, VisitDirectory(), visit_song);
 	}
 
-	/**
-	 * Visit all unique tag values.
-	 */
-	virtual void VisitUniqueTags(const DatabaseSelection &selection,
-				     TagType tag_type, tag_mask_t group_mask,
-				     VisitTag visit_tag) const = 0;
+	virtual std::map<std::string, std::set<std::string>> CollectUniqueTags(const DatabaseSelection &selection,
+									       TagType tag_type,
+									       TagType group=TAG_NUM_OF_ITEM_TYPES) const = 0;
 
 	virtual DatabaseStats GetStats(const DatabaseSelection &selection) const = 0;
 

src/db/UniqueTags.cxx

@@ -20,34 +20,42 @@
 #include "UniqueTags.hxx"
 #include "Interface.hxx"
 #include "LightSong.hxx"
-#include "tag/Set.hxx"
+#include "tag/VisitFallback.hxx"
 
 #include <functional>
 
 #include <assert.h>
 
 static void
-CollectTags(TagSet &set, TagType tag_type, tag_mask_t group_mask,
-	    const LightSong &song)
+CollectTags(std::set<std::string> &result,
+	    const Tag &tag,
+	    TagType tag_type) noexcept
 {
-	assert(song.tag != nullptr);
-	const Tag &tag = *song.tag;
+	VisitTagWithFallbackOrEmpty(tag, tag_type, [&result](const char *value){
+			result.emplace(value);
+		});
+}
 
-	set.InsertUnique(tag, tag_type, group_mask);
+static void
+CollectGroupTags(std::map<std::string, std::set<std::string>> &result,
+		 const Tag &tag,
+		 TagType tag_type,
+		 TagType group) noexcept
+{
+	VisitTagWithFallbackOrEmpty(tag, group, [&](const char *group_name){
+			CollectTags(result[group_name], tag, tag_type);
+		});
 }
 
-void
-VisitUniqueTags(const Database &db, const DatabaseSelection &selection,
-		TagType tag_type, tag_mask_t group_mask,
-		VisitTag visit_tag)
+std::map<std::string, std::set<std::string>>
+CollectUniqueTags(const Database &db, const DatabaseSelection &selection,
+		  TagType tag_type, TagType group)
 {
-	TagSet set;
+	std::map<std::string, std::set<std::string>> result;
 
-	using namespace std::placeholders;
-	const auto f = std::bind(CollectTags, std::ref(set),
-				 tag_type, group_mask, _1);
-	db.Visit(selection, f);
+	db.Visit(selection, [&result, tag_type, group](const LightSong &song){
+			CollectGroupTags(result, *song.tag, tag_type, group);
+		});
 
-	for (const auto &value : set)
-		visit_tag(value);
+	return result;
 }

src/db/UniqueTags.hxx

@@ -20,16 +20,19 @@
 #ifndef MPD_DB_UNIQUE_TAGS_HXX
 #define MPD_DB_UNIQUE_TAGS_HXX
 
-#include "Visitor.hxx"
 #include "tag/TagType.h"
-#include "tag/Mask.hxx"
+#include "Compiler.h"
+
+#include <map>
+#include <set>
+#include <string>
 
 class Database;
 struct DatabaseSelection;
 
-void
-VisitUniqueTags(const Database &db, const DatabaseSelection &selection,
-		TagType tag_type, tag_mask_t group_mask,
-		VisitTag visit_tag);
+gcc_pure
+std::map<std::string, std::set<std::string>>
+CollectUniqueTags(const Database &db, const DatabaseSelection &selection,
+		  TagType tag_type, TagType group);
 
 #endif

src/db/plugins/ProxyDatabasePlugin.cxx

@@ -120,9 +120,9 @@ public:
 		   VisitSong visit_song,
 		   VisitPlaylist visit_playlist) const override;
 
-	void VisitUniqueTags(const DatabaseSelection &selection,
-			     TagType tag_type, tag_mask_t group_mask,
-			     VisitTag visit_tag) const override;
+	std::map<std::string, std::set<std::string>> CollectUniqueTags(const DatabaseSelection &selection,
+								       TagType tag_type,
+								       TagType group) const override;
 
 	DatabaseStats GetStats(const DatabaseSelection &selection) const override;
 
@@ -334,28 +334,19 @@ SendConstraints(mpd_connection *connection, const DatabaseSelection &selection)
 }
 
 static bool
-SendGroupMask(mpd_connection *connection, tag_mask_t mask)
+SendGroup(mpd_connection *connection, TagType group)
 {
-#if LIBMPDCLIENT_CHECK_VERSION(2,12,0)
-	for (unsigned i = 0; i < TAG_NUM_OF_ITEM_TYPES; ++i) {
-		if ((mask & (tag_mask_t(1) << i)) == 0)
-			continue;
-
-		const auto tag = Convert(TagType(i));
-		if (tag == MPD_TAG_COUNT)
-			throw std::runtime_error("Unsupported tag");
+	if (group == TAG_NUM_OF_ITEM_TYPES)
+		return true;
 
-		if (!mpd_search_add_group_tag(connection, tag))
-			return false;
-	}
+#if LIBMPDCLIENT_CHECK_VERSION(2,12,0)
+	const auto tag = Convert(group);
+	if (tag == MPD_TAG_COUNT)
+		throw std::runtime_error("Unsupported tag");
 
-	return true;
+	return mpd_search_add_group_tag(connection, tag);
 #else
 	(void)connection;
-	(void)mask;
-
-	if (mask != 0)
-		throw std::runtime_error("Grouping requires libmpdclient 2.12");
 
 	return true;
 #endif
@@ -799,11 +790,9 @@ ProxyDatabase::Visit(const DatabaseSelection &selection,
 		visit_directory, visit_song, visit_playlist);
 }
 
-void
-ProxyDatabase::VisitUniqueTags(const DatabaseSelection &selection,
-			       TagType tag_type,
-			       tag_mask_t group_mask,
-			       VisitTag visit_tag) const
+std::map<std::string, std::set<std::string>>
+ProxyDatabase::CollectUniqueTags(const DatabaseSelection &selection,
+				 TagType tag_type, TagType group) const
 try {
 	// TODO: eliminate the const_cast
 	const_cast<ProxyDatabase *>(this)->EnsureConnected();
@@ -814,54 +803,56 @@ try {
 
 	if (!mpd_search_db_tags(connection, tag_type2) ||
 	    !SendConstraints(connection, selection) ||
-	    !SendGroupMask(connection, group_mask))
+	    !SendGroup(connection, group))
 		ThrowError(connection);
 
 	if (!mpd_search_commit(connection))
 		ThrowError(connection);
 
-	TagBuilder builder;
+	std::map<std::string, std::set<std::string>> result;
 
-	while (auto *pair = mpd_recv_pair(connection)) {
-		AtScopeExit(this, pair) {
-			mpd_return_pair(connection, pair);
-		};
+	if (group == TAG_NUM_OF_ITEM_TYPES) {
+		auto &values = result[std::string()];
 
-		const auto current_type = tag_name_parse_i(pair->name);
-		if (current_type == TAG_NUM_OF_ITEM_TYPES)
-			continue;
+		while (auto *pair = mpd_recv_pair(connection)) {
+			AtScopeExit(this, pair) {
+				mpd_return_pair(connection, pair);
+			};
 
-		if (current_type == tag_type && !builder.IsEmpty()) {
-			try {
-				visit_tag(builder.Commit());
-			} catch (...) {
-				mpd_response_finish(connection);
-				throw;
-			}
+			const auto current_type = tag_name_parse_i(pair->name);
+			if (current_type == TAG_NUM_OF_ITEM_TYPES)
+				continue;
+
+			if (current_type == tag_type)
+				values.emplace(pair->value);
 		}
+	} else {
+		std::set<std::string> *current_group = nullptr;
 
-		builder.AddItem(current_type, pair->value);
+		while (auto *pair = mpd_recv_pair(connection)) {
+			AtScopeExit(this, pair) {
+				mpd_return_pair(connection, pair);
+			};
 
-		if (!builder.HasType(current_type))
-			/* if no tag item has been added, then the
-			   given value was not acceptable
-			   (e.g. empty); forcefully insert an empty
-			   tag in this case, as the caller expects the
-			   given tag type to be present */
-			builder.AddEmptyItem(current_type);
-	}
+			const auto current_type = tag_name_parse_i(pair->name);
+			if (current_type == TAG_NUM_OF_ITEM_TYPES)
+				continue;
 
-	if (!builder.IsEmpty()) {
-		try {
-			visit_tag(builder.Commit());
-		} catch (...) {
-			mpd_response_finish(connection);
-			throw;
+			if (current_type == tag_type) {
+				if (current_group == nullptr)
+					current_group = &result[std::string()];
+
+				current_group->emplace(pair->value);
+			} else if (current_type == group) {
+				current_group = &result[pair->value];
+			}
 		}
 	}
 
 	if (!mpd_response_finish(connection))
 		ThrowError(connection);
+
+	return result;
 } catch (...) {
 	if (connection != nullptr)
 		mpd_search_cancel(connection);

src/db/plugins/simple/SimpleDatabasePlugin.cxx

@@ -312,12 +312,11 @@ SimpleDatabase::Visit(const DatabaseSelection &selection,
 			    "No such directory");
 }
 
-void
-SimpleDatabase::VisitUniqueTags(const DatabaseSelection &selection,
-				TagType tag_type, tag_mask_t group_mask,
-				VisitTag visit_tag) const
+std::map<std::string, std::set<std::string>>
+SimpleDatabase::CollectUniqueTags(const DatabaseSelection &selection,
+				  TagType tag_type, TagType group) const
 {
-	::VisitUniqueTags(*this, selection, tag_type, group_mask, visit_tag);
+	return ::CollectUniqueTags(*this, selection, tag_type, group);
 }
 
 DatabaseStats

src/db/plugins/simple/SimpleDatabasePlugin.hxx

@@ -119,9 +119,9 @@ public:
 		   VisitSong visit_song,
 		   VisitPlaylist visit_playlist) const override;
 
-	void VisitUniqueTags(const DatabaseSelection &selection,
-			     TagType tag_type, tag_mask_t group_mask,
-			     VisitTag visit_tag) const override;
+	std::map<std::string, std::set<std::string>> CollectUniqueTags(const DatabaseSelection &selection,
+								       TagType tag_type,
+								       TagType group) const override;
 
 	DatabaseStats GetStats(const DatabaseSelection &selection) const override;
 

src/db/plugins/upnp/UpnpDatabasePlugin.cxx

@@ -87,9 +87,9 @@ public:
 		   VisitSong visit_song,
 		   VisitPlaylist visit_playlist) const override;
 
-	void VisitUniqueTags(const DatabaseSelection &selection,
-			     TagType tag_type, tag_mask_t group_mask,
-			     VisitTag visit_tag) const override;
+	std::map<std::string, std::set<std::string>> CollectUniqueTags(const DatabaseSelection &selection,
+								       TagType tag_type,
+								       TagType group) const override;
 
 	DatabaseStats GetStats(const DatabaseSelection &selection) const override;
 
@@ -603,17 +603,15 @@ UpnpDatabase::Visit(const DatabaseSelection &selection,
 		    visit_directory, visit_song, visit_playlist);
 }
 
-void
-UpnpDatabase::VisitUniqueTags(const DatabaseSelection &selection,
-			      TagType tag, gcc_unused tag_mask_t group_mask,
-			      VisitTag visit_tag) const
+std::map<std::string, std::set<std::string>>
+UpnpDatabase::CollectUniqueTags(const DatabaseSelection &selection,
+				TagType tag, TagType group) const
 {
-	// TODO: use group_mask
+	(void)group; // TODO: use group
 
-	if (!visit_tag)
-		return;
+	std::map<std::string, std::set<std::string>> result;
+	auto &values = result[std::string()];
 
-	std::set<std::string> values;
 	for (auto& server : discovery->GetDirectories()) {
 		const auto dirbuf = SearchSongs(server, rootid, selection);
 
@@ -633,11 +631,7 @@ UpnpDatabase::VisitUniqueTags(const DatabaseSelection &selection,
 		}
 	}
 
-	for (const auto& value : values) {
-		TagBuilder builder;
-		builder.AddItem(tag, value.c_str());
-		visit_tag(builder.Commit());
-	}
+	return result;
 }
 
 DatabaseStats

src/tag/Set.cxx

-/*
- * Copyright 2003-2017 The Music Player Daemon Project
- * http://www.musicpd.org
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-#include "Set.hxx"
-#include "Fallback.hxx"
-#include "TagBuilder.hxx"
-#include "util/StringView.hxx"
-
-#include <functional>
-
-#include <assert.h>
-
-/**
- * Copy all tag items of the specified type.
- */
-static bool
-CopyTagItem2(TagBuilder &dest, TagType dest_type,
-	     const Tag &src, TagType src_type)
-{
-	bool found = false;
-
-	for (const auto &item : src) {
-		if (item.type == src_type) {
-			dest.AddItem(dest_type, item.value);
-			found = true;
-		}
-	}
-
-	return found;
-}
-
-/**
- * Copy all tag items of the specified type.  Fall back to "Artist" if
- * there is no "AlbumArtist".
- */
-static void
-CopyTagItem(TagBuilder &dest, const Tag &src, TagType type)
-{
-	ApplyTagWithFallback(type,
-			     std::bind(CopyTagItem2, std::ref(dest), type,
-				       std::cref(src), std::placeholders::_1));
-}
-
-/**
- * Copy all tag items of the types in the mask.
- */
-static void
-CopyTagMask(TagBuilder &dest, const Tag &src, tag_mask_t mask)
-{
-	for (unsigned i = 0; i < TAG_NUM_OF_ITEM_TYPES; ++i)
-		if ((mask & (tag_mask_t(1) << i)) != 0)
-			CopyTagItem(dest, src, TagType(i));
-}
-
-void
-TagSet::InsertUnique(const Tag &src, TagType type, const char *value,
-		     tag_mask_t group_mask) noexcept
-{
-	TagBuilder builder;
-	builder.AddItemUnchecked(type, value);
-	CopyTagMask(builder, src, group_mask);
-#if CLANG_OR_GCC_VERSION(4,8)
-	emplace(builder.Commit());
-#else
-	insert(builder.Commit());
-#endif
-}
-
-bool
-TagSet::CheckUnique(TagType dest_type,
-		    const Tag &tag, TagType src_type,
-		    tag_mask_t group_mask) noexcept
-{
-	bool found = false;
-
-	for (const auto &item : tag) {
-		if (item.type == src_type) {
-			InsertUnique(tag, dest_type, item.value, group_mask);
-			found = true;
-		}
-	}
-
-	return found;
-}
-
-void
-TagSet::InsertUnique(const Tag &tag,
-		     TagType type, tag_mask_t group_mask) noexcept
-{
-	static_assert(sizeof(group_mask) * 8 >= TAG_NUM_OF_ITEM_TYPES,
-		      "Mask is too small");
-
-	assert((group_mask & (tag_mask_t(1) << unsigned(type))) == 0);
-
-	if (!ApplyTagWithFallback(type,
-				  std::bind(&TagSet::CheckUnique, this,
-					    type, std::cref(tag),
-					    std::placeholders::_1,
-					    group_mask)))
-		InsertUnique(tag, type, "", group_mask);
-}

src/tag/Set.hxx

-/*
- * Copyright 2003-2017 The Music Player Daemon Project
- * http://www.musicpd.org
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-#ifndef MPD_TAG_SET_HXX
-#define MPD_TAG_SET_HXX
-
-#include "Compiler.h"
-#include "Tag.hxx"
-#include "Mask.hxx"
-
-#include <set>
-
-#include <string.h>
-
-/**
- * Helper class for #TagSet which compares two #Tag objects.
- */
-struct TagLess {
-	gcc_pure
-	bool operator()(const Tag &a, const Tag &b) const noexcept {
-		if (a.num_items != b.num_items)
-			return a.num_items < b.num_items;
-
-		const unsigned n = a.num_items;
-		for (unsigned i = 0; i < n; ++i) {
-			const TagItem &ai = *a.items[i];
-			const TagItem &bi = *b.items[i];
-			if (ai.type != bi.type)
-				return unsigned(ai.type) < unsigned(bi.type);
-
-			const int cmp = strcmp(ai.value, bi.value);
-			if (cmp != 0)
-				return cmp < 0;
-		}
-
-		return false;
-	}
-};
-
-/**
- * A set of #Tag objects.
- */
-class TagSet : public std::set<Tag, TagLess> {
-public:
-	void InsertUnique(const Tag &tag,
-			  TagType type, tag_mask_t group_mask) noexcept;
-
-private:
-	void InsertUnique(const Tag &src, TagType type, const char *value,
-			  tag_mask_t group_mask) noexcept;
-
-	bool CheckUnique(TagType dest_type,
-			 const Tag &tag, TagType src_type,
-			 tag_mask_t group_mask) noexcept;
-};
-
-#endif