-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add feature: Ignore lists #62
Open
push0ret
wants to merge
1
commit into
MusicPlayerDaemon:master
Choose a base branch
from
push0ret:feature_ignore_lists
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// SPDX-License-Identifier: GPL-2.0-or-later | ||
// Copyright The Music Player Daemon Project | ||
|
||
#include "IgnoreList.hxx" | ||
|
||
#include <cassert> | ||
#include <algorithm> | ||
|
||
[[gnu::pure]] | ||
static constexpr bool | ||
MatchIgnoreIfSpecified(std::string_view ignore, std::string_view value) | ||
{ | ||
return ignore.empty() || ignore == value; | ||
} | ||
|
||
bool | ||
IgnoreListEntry::matches_record(const Record& record) const noexcept | ||
{ | ||
/* | ||
The below logic would always return true if the entry is empty. | ||
This condition should never be true, as we don't push empty entries. | ||
*/ | ||
assert(!artist.empty() || !album.empty() || !title.empty()); | ||
|
||
/* | ||
Note the mismatch of 'title' and 'track' field names with the Record structure. | ||
This is not a bug - the Record structure does not use the expected field names. | ||
*/ | ||
return MatchIgnoreIfSpecified(artist, record.artist) && | ||
MatchIgnoreIfSpecified(album, record.album) && | ||
MatchIgnoreIfSpecified(title, record.track) && | ||
MatchIgnoreIfSpecified(track, record.number); | ||
} | ||
|
||
bool | ||
IgnoreList::matches_record(const Record& record) const noexcept | ||
{ | ||
return std::any_of(entries.begin(), | ||
entries.end(), | ||
[&record](const auto& entry) { return entry.matches_record(record); }); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// SPDX-License-Identifier: GPL-2.0-or-later | ||
// Copyright The Music Player Daemon Project | ||
|
||
#ifndef IGNORE_LIST_HXX | ||
#define IGNORE_LIST_HXX | ||
|
||
#include <string> | ||
#include <vector> | ||
|
||
#include "Record.hxx" | ||
|
||
struct IgnoreListEntry { | ||
|
||
std::string artist; | ||
std::string album; | ||
std::string title; | ||
std::string track; | ||
|
||
[[nodiscard]] bool matches_record(const Record& record) const noexcept; | ||
}; | ||
|
||
struct IgnoreList { | ||
std::vector<IgnoreListEntry> entries; | ||
|
||
[[nodiscard]] bool matches_record(const Record& record) const noexcept; | ||
}; | ||
|
||
|
||
#endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need an indirection with another file? This complicates the thing both for the code and for users.
Also this file format looks rather arbitrary and cannot be extended or changed later. One has to remember what those columns are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a separate file for the following reasons:
I came up with the file format to make the parser implementation trivial / easily verifiable. I wanted to avoid having weird audio metadata end up breaking the parser. I considered two options: Very simple easy to parse format and write a small parser myself, or use a standardized machine readable format and implement it using a library. I'm happy to discuss alternatives.
The format also allows easy construction of list entries from audio metadata. In most cases, a simple
mpc current -f "%artist%;%album%;%title%" >> ignorelist
should just work. For the rare cases the metadata itself contains a semicolon, the escaping rules are simple to apply via regex substitution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Max
I understand your concern about the inflexibility of the file format and to address this I would propose a "ignore_file_tags" (naming is not final) option that is global for all scrobblers.
This option would be set to a space delimited string of tag names, which will then correspond to the columns of the ignore file. The current behavior would be represented by the option
ignore_file_tags = artist album title
.At the moment, the
Record
structure only exposesartist
,album
,title
andtrack
tags, so the flexibility is quite limited. However, making the format configurable allows us to add more tags in the future without breaking users existing configurations while also allowing users to only match the tags they care about in their ignore file, avoiding useless extra columns.Please let me know if you have concerns with this approach. I will implement this after your green light.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This idea sounds like it will grow into a complexity monster. And you're going to split the specification between the configuration file and the ignore file - the ignore file will no longer be self-contained, it depends on external data (the tag list specified in the configuration file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback.
I think based on this we could opt for one of the following options:
artist;album;title
as first line would set the format to what is hard coded now. I would avoid an optional format specification line as this would then require some marker to distinguish the specification line from an ignore line. Such a marker in turn requires an escaping mechanism to avoid clashing with metadata.Queen
,Queen;
andQueen;;
identically. Downside is potentially worse user experience if at some point many more tags are supported. Users may have to deal with an annoying number of empty columns in their ignore files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do something like:
... right in the main configuration file. Simpler and super flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dedicated file is useful for:
The reason I did not use the format you proposed in the ignore file is simply parser complexity, having to handle quoting properly. But that is obviously not a blocker.
Considering the aforementioned benefits of the dedicated file, would it be an acceptable compromise to keep the dedicated ignore file, but use a tag="value" format as you suggested?
With the cost of additional complexity, we could also support both methods (in-place in the main config and dedicated ignore files). I'm not positive that that's worth it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting is difficult, of course - but your CSV-like syntax doesn't solve this - look at how you had to implement backslash-quoting. No difference here.
I think it's okay to have this external file. I don't quite like the idea of having two files, but it's a situation where all possible solutions are bad, and maybe you're right and it's the least-bad one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I will implement the tag="value" format for the ignore files then.
Thanks for taking the time to discuss this, it is appreciated.