From 1881b0e975b4be8f01c798ef7c0a9e061583b815 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:06:56 +0100 Subject: [PATCH 01/21] song/TagSongFilter: rename MatchNN() to Match() The "NN" suffix used to mean "no negation", but that's not how it's implemented today. --- src/song/TagSongFilter.cxx | 8 ++++---- src/song/TagSongFilter.hxx | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index 547172a4c..b04a38437 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -35,14 +35,14 @@ TagSongFilter::ToExpression() const noexcept } bool -TagSongFilter::MatchNN(const TagItem &item) const noexcept +TagSongFilter::Match(const TagItem &item) const noexcept { return (type == TAG_NUM_OF_ITEM_TYPES || item.type == type) && filter.Match(item.value); } bool -TagSongFilter::MatchNN(const Tag &tag) const noexcept +TagSongFilter::Match(const Tag &tag) const noexcept { bool visited_types[TAG_NUM_OF_ITEM_TYPES]; std::fill_n(visited_types, size_t(TAG_NUM_OF_ITEM_TYPES), false); @@ -50,7 +50,7 @@ TagSongFilter::MatchNN(const Tag &tag) const noexcept for (const auto &i : tag) { visited_types[i.type] = true; - if (MatchNN(i)) + if (Match(i)) return true; } @@ -89,5 +89,5 @@ TagSongFilter::MatchNN(const Tag &tag) const noexcept bool TagSongFilter::Match(const LightSong &song) const noexcept { - return MatchNN(song.tag); + return Match(song.tag); } diff --git a/src/song/TagSongFilter.hxx b/src/song/TagSongFilter.hxx index e60910934..6d0449d61 100644 --- a/src/song/TagSongFilter.hxx +++ b/src/song/TagSongFilter.hxx @@ -68,8 +68,8 @@ public: bool Match(const LightSong &song) const noexcept override; private: - bool MatchNN(const Tag &tag) const noexcept; - bool MatchNN(const TagItem &tag) const noexcept; + bool Match(const Tag &tag) const noexcept; + bool Match(const TagItem &tag) const noexcept; }; #endif From cb71f6dd04343dacdef6909c4f64821c1b9f2377 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:09:55 +0100 Subject: [PATCH 02/21] doc/protocol.rst: clarify the meaning of the `any` tag type --- doc/protocol.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/protocol.rst b/doc/protocol.rst index 41bb2a861..ea8c9c13d 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -147,7 +147,7 @@ of: - ``(TAG == 'VALUE')``: match a tag value. ``(TAG != 'VALUE')``: mismatch a tag value. The special tag "*any*" checks all - tag values. + tag types. *albumartist* looks for ``VALUE`` in ``AlbumArtist`` and falls back to ``Artist`` tags if From 8777737861ca2c01003e03b0f90b78feb2a2895a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:11:30 +0100 Subject: [PATCH 03/21] doc/protocol.rst: use double backticks for tag names --- doc/protocol.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/protocol.rst b/doc/protocol.rst index ea8c9c13d..c51db6dd3 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -146,9 +146,9 @@ of: - ``(TAG == 'VALUE')``: match a tag value. ``(TAG != 'VALUE')``: mismatch a tag value. - The special tag "*any*" checks all + The special tag ``any`` checks all tag types. - *albumartist* looks for + ``AlbumArtist`` looks for ``VALUE`` in ``AlbumArtist`` and falls back to ``Artist`` tags if ``AlbumArtist`` does not exist. @@ -178,7 +178,7 @@ of: - ``(AudioFormat =~ 'SAMPLERATE:BITS:CHANNELS')``: matches the audio format with the given mask (i.e. one - or more attributes may be "*"). + or more attributes may be ``*``). - ``(!EXPRESSION)``: negate an expression. Note that each expression must be enclosed in parantheses, e.g. :code:`(!(artist == 'VALUE'))` From ddd2b604890db0c7afcdd8377e551d34fae0d03d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:14:06 +0100 Subject: [PATCH 04/21] doc/protocol.rst: add missing operators to example expressions --- doc/protocol.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/protocol.rst b/doc/protocol.rst index c51db6dd3..cc23c14aa 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -207,11 +207,11 @@ backslash. Example expression which matches an artist named ``foo'bar"``:: - (artist "foo\'bar\"") + (Artist == "foo\'bar\"") At the protocol level, the command must look like this:: - find "(artist \"foo\\'bar\\\"\")" + find "(Artist == \"foo\\'bar\\\"\")" The double quotes enclosing the artist name must be escaped because they are inside a double-quoted ``find`` parameter. The single quote From 8d1f30e55b107b1b96eedf26b2df0cd3622da0b1 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:23:10 +0100 Subject: [PATCH 05/21] tag/Fallback: add API documentation --- src/tag/Fallback.hxx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/tag/Fallback.hxx b/src/tag/Fallback.hxx index 08e4dc80a..ecbe0d439 100644 --- a/src/tag/Fallback.hxx +++ b/src/tag/Fallback.hxx @@ -22,6 +22,11 @@ #include +/** + * Invoke the given function for all fallback tags of the given + * #TagType, until the function returns true (or until there are no + * more fallback tags). + */ template bool ApplyTagFallback(TagType type, F &&f) noexcept @@ -43,6 +48,11 @@ ApplyTagFallback(TagType type, F &&f) noexcept return false; } +/** + * Invoke the given function for the given #TagType and all of its + * fallback tags, until the function returns true (or until there are + * no more fallback tags). + */ template bool ApplyTagWithFallback(TagType type, F &&f) noexcept From fde9a470dd889243b6d26a753cf172f894a6bc18 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 14 Mar 2019 20:31:22 +0100 Subject: [PATCH 06/21] song/TagSongFilter: eliminate the std::fill_n() call --- src/song/TagSongFilter.cxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index b04a38437..2a084de5e 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -44,8 +44,7 @@ TagSongFilter::Match(const TagItem &item) const noexcept bool TagSongFilter::Match(const Tag &tag) const noexcept { - bool visited_types[TAG_NUM_OF_ITEM_TYPES]; - std::fill_n(visited_types, size_t(TAG_NUM_OF_ITEM_TYPES), false); + bool visited_types[TAG_NUM_OF_ITEM_TYPES]{}; for (const auto &i : tag) { visited_types[i.type] = true; From 67d73a2aee0af836ae62c129feb41e5d60cd658f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:54:16 +0100 Subject: [PATCH 07/21] song/TagSongFilter: improve lambda indent --- src/song/TagSongFilter.cxx | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index 2a084de5e..7f8c5d7e1 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -55,21 +55,20 @@ TagSongFilter::Match(const Tag &tag) const noexcept if (type < TAG_NUM_OF_ITEM_TYPES && !visited_types[type]) { bool result = false; - if (ApplyTagFallback(type, - [&](TagType tag2) { - if (!visited_types[tag2]) - return false; + if (ApplyTagFallback(type, [&](TagType tag2) { + if (!visited_types[tag2]) + return false; - for (const auto &item : tag) { - if (item.type == tag2 && - filter.Match(item.value)) { - result = true; - break; - } - } + for (const auto &item : tag) { + if (item.type == tag2 && + filter.Match(item.value)) { + result = true; + break; + } + } - return true; - })) + return true; + })) return result; /* If the search critieron was not visited during the From b850eb74b761797da2b7028be4dbcd085f0e234a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 19:52:54 +0100 Subject: [PATCH 08/21] song/TagSongFilter: add code comments --- src/song/TagSongFilter.cxx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index 7f8c5d7e1..4b587c920 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -54,9 +54,15 @@ TagSongFilter::Match(const Tag &tag) const noexcept } if (type < TAG_NUM_OF_ITEM_TYPES && !visited_types[type]) { + /* if the specified tag is not present, try the + fallback tags */ + bool result = false; if (ApplyTagFallback(type, [&](TagType tag2) { if (!visited_types[tag2]) + /* we already know that this tag type + isn't present, so let's bail out + without checking again */ return false; for (const auto &item : tag) { From 9e9418294a69bfcb27b8cc631ad32b1e6507f3c6 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:28:27 +0100 Subject: [PATCH 09/21] song/TagSongFilter: eliminate Match(TagItem) --- src/song/TagSongFilter.cxx | 10 ++-------- src/song/TagSongFilter.hxx | 1 - 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index 4b587c920..df92ed435 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -34,13 +34,6 @@ TagSongFilter::ToExpression() const noexcept + " \"" + EscapeFilterString(filter.GetValue()) + "\")"; } -bool -TagSongFilter::Match(const TagItem &item) const noexcept -{ - return (type == TAG_NUM_OF_ITEM_TYPES || item.type == type) && - filter.Match(item.value); -} - bool TagSongFilter::Match(const Tag &tag) const noexcept { @@ -49,7 +42,8 @@ TagSongFilter::Match(const Tag &tag) const noexcept for (const auto &i : tag) { visited_types[i.type] = true; - if (Match(i)) + if ((type == TAG_NUM_OF_ITEM_TYPES || i.type == type) && + filter.Match(i.value)) return true; } diff --git a/src/song/TagSongFilter.hxx b/src/song/TagSongFilter.hxx index 6d0449d61..3f9f7051b 100644 --- a/src/song/TagSongFilter.hxx +++ b/src/song/TagSongFilter.hxx @@ -69,7 +69,6 @@ public: private: bool Match(const Tag &tag) const noexcept; - bool Match(const TagItem &tag) const noexcept; }; #endif From 5c5dc1b7c0f98aefd549e25810f5f8163456bbb5 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 16 Mar 2019 13:22:01 +0100 Subject: [PATCH 10/21] meson.build: increase protocol version to 0.21.6 There is a minor new feature (commit 713c1f2ba9c) and clients might be interested in detecting it by the protocol version. --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 8e092a4c6..723b72295 100644 --- a/meson.build +++ b/meson.build @@ -20,7 +20,7 @@ conf.set_quoted('PACKAGE', meson.project_name()) conf.set_quoted('PACKAGE_NAME', meson.project_name()) conf.set_quoted('PACKAGE_VERSION', meson.project_version()) conf.set_quoted('VERSION', meson.project_version()) -conf.set_quoted('PROTOCOL_VERSION', '0.21.4') +conf.set_quoted('PROTOCOL_VERSION', '0.21.6') conf.set_quoted('SYSTEM_CONFIG_FILE_LOCATION', join_paths(get_option('prefix'), get_option('sysconfdir'), 'mpd.conf')) common_cppflags = [ From 137ffba1b43ef54bc2056c5760bf9e3785e03a01 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:41:55 +0100 Subject: [PATCH 11/21] test/test_translate_song: move MakeTag() to header --- test/MakeTag.hxx | 44 ++++++++++++++++++++++++++++++++++++ test/test_translate_song.cxx | 23 +------------------ 2 files changed, 45 insertions(+), 22 deletions(-) create mode 100644 test/MakeTag.hxx diff --git a/test/MakeTag.hxx b/test/MakeTag.hxx new file mode 100644 index 000000000..55147f4e8 --- /dev/null +++ b/test/MakeTag.hxx @@ -0,0 +1,44 @@ +/* + * Copyright 2003-2019 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 "tag/Builder.hxx" +#include "tag/Tag.hxx" +#include "util/Compiler.h" + +static void +BuildTag(gcc_unused TagBuilder &tag) +{ +} + +template +static void +BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) +{ + tag.AddItem(type, value); + BuildTag(tag, std::forward(args)...); +} + +template +static Tag +MakeTag(Args&&... args) +{ + TagBuilder tag; + BuildTag(tag, std::forward(args)...); + return tag.Commit(); +} diff --git a/test/test_translate_song.cxx b/test/test_translate_song.cxx index 2419b1bac..cd4e39a5c 100644 --- a/test/test_translate_song.cxx +++ b/test/test_translate_song.cxx @@ -2,6 +2,7 @@ * Unit tests for playlist_check_translate_song(). */ +#include "MakeTag.hxx" #include "playlist/PlaylistSong.hxx" #include "song/DetachedSong.hxx" #include "SongLoader.hxx" @@ -38,28 +39,6 @@ uri_supported_scheme(const char *uri) noexcept static constexpr auto music_directory = PATH_LITERAL("/music"); static Storage *storage; -static void -BuildTag(gcc_unused TagBuilder &tag) -{ -} - -template -static void -BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) -{ - tag.AddItem(type, value); - BuildTag(tag, std::forward(args)...); -} - -template -static Tag -MakeTag(Args&&... args) -{ - TagBuilder tag; - BuildTag(tag, std::forward(args)...); - return tag.Commit(); -} - static Tag MakeTag1a() { From 9b26d451e462b83d25c07f6461b3641160df0662 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:43:21 +0100 Subject: [PATCH 12/21] test/MakeTag: remove `static` --- test/MakeTag.hxx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/MakeTag.hxx b/test/MakeTag.hxx index 55147f4e8..4861583e5 100644 --- a/test/MakeTag.hxx +++ b/test/MakeTag.hxx @@ -21,13 +21,13 @@ #include "tag/Tag.hxx" #include "util/Compiler.h" -static void +inline void BuildTag(gcc_unused TagBuilder &tag) { } template -static void +inline void BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) { tag.AddItem(type, value); @@ -35,7 +35,7 @@ BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) } template -static Tag +inline Tag MakeTag(Args&&... args) { TagBuilder tag; From cf66a60c604198ba9c33f2dd5c0b764207a7cec7 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:43:50 +0100 Subject: [PATCH 13/21] test/MakeTag: add `noexcept` --- test/MakeTag.hxx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/MakeTag.hxx b/test/MakeTag.hxx index 4861583e5..49e221b35 100644 --- a/test/MakeTag.hxx +++ b/test/MakeTag.hxx @@ -22,13 +22,14 @@ #include "util/Compiler.h" inline void -BuildTag(gcc_unused TagBuilder &tag) +BuildTag(gcc_unused TagBuilder &tag) noexcept { } template inline void -BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) +BuildTag(TagBuilder &tag, TagType type, const char *value, + Args&&... args) noexcept { tag.AddItem(type, value); BuildTag(tag, std::forward(args)...); @@ -36,7 +37,7 @@ BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) template inline Tag -MakeTag(Args&&... args) +MakeTag(Args&&... args) noexcept { TagBuilder tag; BuildTag(tag, std::forward(args)...); From 7a3e15d8e55e61f3c57f33ce471c3f9ecf6a4217 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:34:14 +0100 Subject: [PATCH 14/21] test/meson.build: add section for filter tests --- test/meson.build | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/test/meson.build b/test/meson.build index 628e5cbfd..eac099dec 100644 --- a/test/meson.build +++ b/test/meson.build @@ -20,16 +20,6 @@ gtest_dep = declare_dependency( subdir('net') -executable( - 'ParseSongFilter', - 'ParseSongFilter.cxx', - include_directories: inc, - dependencies: [ - song_dep, - pcm_dep, - ], -) - executable( 'read_conf', 'read_conf.cxx', @@ -211,6 +201,20 @@ if zlib_dep.found() ) endif +# +# Filter +# + +executable( + 'ParseSongFilter', + 'ParseSongFilter.cxx', + include_directories: inc, + dependencies: [ + song_dep, + pcm_dep, + ], +) + # # Neighbor # From 52ce39dc3e695b224d4a9e57cde35aebded358c9 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:34:52 +0100 Subject: [PATCH 15/21] test/TestSongFilter: unit test for song filters A few of those tests fail due to bugs. --- test/TestTagSongFilter.cxx | 163 +++++++++++++++++++++++++++++++++++++ test/meson.build | 13 +++ 2 files changed, 176 insertions(+) create mode 100644 test/TestTagSongFilter.cxx diff --git a/test/TestTagSongFilter.cxx b/test/TestTagSongFilter.cxx new file mode 100644 index 000000000..230409631 --- /dev/null +++ b/test/TestTagSongFilter.cxx @@ -0,0 +1,163 @@ +/* + * Copyright 2003-2019 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 "MakeTag.hxx" +#include "song/TagSongFilter.hxx" +#include "song/LightSong.hxx" +#include "tag/Type.h" + +#include + +static bool +InvokeFilter(const TagSongFilter &f, const Tag &tag) noexcept +{ + return f.Match(LightSong("dummy", tag)); +} + +TEST(TagSongFilter, Basic) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle", TAG_TITLE, "foo"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo", TAG_TITLE, "needle", TAG_ALBUM, "bar"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_ARTIST, "needle"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedleBAR"))); +} + +/** + * Test with empty string. This matches tags where the given tag type + * does not exist. + */ +TEST(TagSongFilter, Empty) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); +} + +TEST(TagSongFilter, Substring) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, true, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needleBAR"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedleBAR"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "eedle"))); +} + +TEST(TagSongFilter, Negated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); +} + +/** + * Combine the "Empty" and "Negated" tests. + */ +TEST(TagSongFilter, EmptyNegated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("", false, false, true)); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); +} + +/** + * Negation with multiple tag values. + */ +TEST(TagSongFilter, MultiNegated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "needle"))); +} + +/** + * Check whether fallback tags work, e.g. AlbumArtist falls back to + * just Artist if there is no AlbumArtist. + */ +TEST(TagSongFilter, Fallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("needle", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); + + /* no fallback, thus the Artist tag isn't used and this must + be a mismatch */ + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle", TAG_ALBUM_ARTIST, "foo"))); +} + +/** + * Combine the "Empty" and "Fallback" tests. + */ +TEST(TagSongFilter, EmptyFallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); +} + +/** + * Combine the "Negated" and "Fallback" tests. + */ +TEST(TagSongFilter, NegatedFallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle", TAG_ALBUM_ARTIST, "foo"))); +} diff --git a/test/meson.build b/test/meson.build index eac099dec..7345283d9 100644 --- a/test/meson.build +++ b/test/meson.build @@ -215,6 +215,19 @@ executable( ], ) +test( + 'TestSongFilter', + executable( + 'TestSongFilter', + 'TestTagSongFilter.cxx', + include_directories: inc, + dependencies: [ + song_dep, + gtest_dep, + ], + ) +) + # # Neighbor # From 6b89fd61006d345b41440f091b1e2e527784cf03 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:30:23 +0100 Subject: [PATCH 16/21] song/StringFilter: make MatchWithoutNegation() public --- src/song/StringFilter.cxx | 2 +- src/song/StringFilter.hxx | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/song/StringFilter.cxx b/src/song/StringFilter.cxx index 2f9c26039..9131595ae 100644 --- a/src/song/StringFilter.cxx +++ b/src/song/StringFilter.cxx @@ -22,7 +22,7 @@ #include -inline bool +bool StringFilter::MatchWithoutNegation(const char *s) const noexcept { #if !CLANG_CHECK_VERSION(3,6) diff --git a/src/song/StringFilter.hxx b/src/song/StringFilter.hxx index a1a281f3c..588e34bdb 100644 --- a/src/song/StringFilter.hxx +++ b/src/song/StringFilter.hxx @@ -105,7 +105,9 @@ public: gcc_pure bool Match(const char *s) const noexcept; -private: + /** + * Like Match(), but ignore the "negated" flag. + */ gcc_pure bool MatchWithoutNegation(const char *s) const noexcept; }; From 0acb55cde5565b63f5b4289437436186d56071be Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:31:05 +0100 Subject: [PATCH 17/21] song/StringFilter: remove obsolete #if --- src/song/StringFilter.cxx | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/song/StringFilter.cxx b/src/song/StringFilter.cxx index 9131595ae..4a72f8216 100644 --- a/src/song/StringFilter.cxx +++ b/src/song/StringFilter.cxx @@ -25,10 +25,7 @@ bool StringFilter::MatchWithoutNegation(const char *s) const noexcept { -#if !CLANG_CHECK_VERSION(3,6) - /* disabled on clang due to -Wtautological-pointer-compare */ assert(s != nullptr); -#endif #ifdef HAVE_PCRE if (regex) From 3bf521d5caadb395bd26e0df82d8d8d3de78b599 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 15 Mar 2019 20:32:03 +0100 Subject: [PATCH 18/21] song/TagSongFilter: apply negation properly to multiple tag values The old implementation didn't make a lot of sense; the "!=" operator was not actually the opposite of "==". Closes https://github.com/MusicPlayerDaemon/MPD/issues/505 --- NEWS | 1 + src/song/TagSongFilter.cxx | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index af3e6e142..41f9169a9 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,7 @@ ver 0.21.6 (not yet released) * protocol - allow loading playlists specified as absolute filesystem paths + - fix negated filter expressions with multiple tag values - fix "list" with filter expression * input - cdio_paranoia: fix build failure due to missing #include diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index df92ed435..00f2021aa 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -43,8 +43,8 @@ TagSongFilter::Match(const Tag &tag) const noexcept visited_types[i.type] = true; if ((type == TAG_NUM_OF_ITEM_TYPES || i.type == type) && - filter.Match(i.value)) - return true; + filter.MatchWithoutNegation(i.value)) + return !filter.IsNegated(); } if (type < TAG_NUM_OF_ITEM_TYPES && !visited_types[type]) { @@ -61,7 +61,7 @@ TagSongFilter::Match(const Tag &tag) const noexcept for (const auto &item : tag) { if (item.type == tag2 && - filter.Match(item.value)) { + filter.MatchWithoutNegation(item.value)) { result = true; break; } @@ -69,7 +69,7 @@ TagSongFilter::Match(const Tag &tag) const noexcept return true; })) - return result; + return result != filter.IsNegated(); /* If the search critieron was not visited during the sweep through the song's tag, it means this field @@ -78,10 +78,10 @@ TagSongFilter::Match(const Tag &tag) const noexcept then it's a match as well and we should return true. */ if (filter.empty()) - return true; + return !filter.IsNegated(); } - return false; + return filter.IsNegated(); } bool From a4b8a0d801ab86a1798402942b566657ab2ccdeb Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 16 Mar 2019 13:21:22 +0100 Subject: [PATCH 19/21] doc/protocol.rst: clarify filter expressions with multiple tag values Clarification for https://github.com/MusicPlayerDaemon/MPD/issues/505 --- doc/protocol.rst | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/doc/protocol.rst b/doc/protocol.rst index cc23c14aa..bbe1a655b 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -144,8 +144,10 @@ syntax:: ``EXPRESSION`` is a string enclosed in parantheses which can be one of: -- ``(TAG == 'VALUE')``: match a tag value. - ``(TAG != 'VALUE')``: mismatch a tag value. +- ``(TAG == 'VALUE')``: match a tag value; if there are multiple + values of the given type, at least one must match. + ``(TAG != 'VALUE')``: mismatch a tag value; if there are multiple + values of the given type, none of them must match. The special tag ``any`` checks all tag types. ``AlbumArtist`` looks for @@ -153,6 +155,9 @@ of: and falls back to ``Artist`` tags if ``AlbumArtist`` does not exist. ``VALUE`` is what to find. + An empty value string means: match only if the given tag type does + not exist at all; this implies that negation with an empty value + checks for the existence of the given tag type. - ``(TAG contains 'VALUE')`` checks if the given value is a substring of the tag value. From 1aa7cdd602ad7e7e2b2fe04a829a0d59c6c07f9a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 16 Mar 2019 13:55:19 +0100 Subject: [PATCH 20/21] decoder/opus: fix replay gain when there are no other tags The `tag_builder.empty()` check was wrong for the SubmitReplayGain() call. Closes https://github.com/MusicPlayerDaemon/MPD/issues/497 --- NEWS | 2 ++ src/decoder/plugins/OpusDecoderPlugin.cxx | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 41f9169a9..de0802376 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ ver 0.21.6 (not yet released) - fix "list" with filter expression * input - cdio_paranoia: fix build failure due to missing #include +* decoder + - opus: fix replay gain when there are no other tags * playlist - flac: fix use-after-free bug * support abstract sockets on Linux diff --git a/src/decoder/plugins/OpusDecoderPlugin.cxx b/src/decoder/plugins/OpusDecoderPlugin.cxx index 542f71540..3b3c312af 100644 --- a/src/decoder/plugins/OpusDecoderPlugin.cxx +++ b/src/decoder/plugins/OpusDecoderPlugin.cxx @@ -208,10 +208,12 @@ MPDOpusDecoder::HandleTags(const ogg_packet &packet) TagBuilder tag_builder; AddTagHandler h(tag_builder); - if (ScanOpusTags(packet.packet, packet.bytes, &rgi, h) && - !tag_builder.empty()) { - client.SubmitReplayGain(&rgi); + if (!ScanOpusTags(packet.packet, packet.bytes, &rgi, h)) + return; + client.SubmitReplayGain(&rgi); + + if (!tag_builder.empty()) { Tag tag = tag_builder.Commit(); auto cmd = client.SubmitTag(input_stream, std::move(tag)); if (cmd != DecoderCommand::NONE) From 0bb71f1f20096811d3a49d1c7e58ba2c353ac62d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 16 Mar 2019 14:03:10 +0100 Subject: [PATCH 21/21] output/pulse: use pa_channel_map_init_extend() instead of _auto() Unlike pa_channel_map_init_auto(), pa_channel_map_init_extend() does not fail if there is no valid mapping for the given channel count, but instead maps additional "AUX" channels. Closes https://github.com/MusicPlayerDaemon/MPD/issues/493 --- NEWS | 2 ++ src/output/plugins/PulseOutputPlugin.cxx | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index de0802376..76523c37c 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ ver 0.21.6 (not yet released) - cdio_paranoia: fix build failure due to missing #include * decoder - opus: fix replay gain when there are no other tags +* output + - pulse: work around error with unusual channel count * playlist - flac: fix use-after-free bug * support abstract sockets on Linux diff --git a/src/output/plugins/PulseOutputPlugin.cxx b/src/output/plugins/PulseOutputPlugin.cxx index 59fe9fb54..fa1b06da9 100644 --- a/src/output/plugins/PulseOutputPlugin.cxx +++ b/src/output/plugins/PulseOutputPlugin.cxx @@ -581,8 +581,8 @@ PulseOutput::SetupStream(const pa_sample_spec &ss) /* WAVE-EX is been adopted as the speaker map for most media files */ pa_channel_map chan_map; - pa_channel_map_init_auto(&chan_map, ss.channels, - PA_CHANNEL_MAP_WAVEEX); + pa_channel_map_init_extend(&chan_map, ss.channels, + PA_CHANNEL_MAP_WAVEEX); stream = pa_stream_new(context, name, &ss, &chan_map); if (stream == nullptr) throw MakePulseError(context,