WIP: MPD-0.21 Search/Find Expressions #52
Conversation
girst
force-pushed
the
pr-expressions
branch
5 times, most recently
from
998e563
to
ab7309e
Compare
These will be implemented in the following commit(s). Descriptions are quoted from the MPD Protocol documentation, v0.21.4. https://web.archive.org/web/20190104200400/https://www.musicpd.org/doc/html/protocol.html
Notes: - file(name) only has the '==' operator documented, but mpd(1) accepts all tag-filter-operators for it. - AudioFormat only documents '==' and '=~', which is not a regex, but accepts '*' wildcards. We don't support this tag type. - in '(...AND...)', MPD enforces at least two subexpressions, we are laxer and allow a single one as well (i.e., just an extra pair of parentheses around an expression). - where possible, exact error messages from MPD 0.23 have been used.
Mopidy can't handle not-matching, so no need to implement this. This is a seperate commit, so if in the future Mopidy gains this ability, it can be easily reintroduced.
| @@ -0,0 +1,128 @@ | |||
| from mopidy_mpd import exceptions | |||
|
|
|||
| class peekable: | |||
There was a problem hiding this comment.
The code in this file almost looks like it was copy+pasted from somewhere else, and definitely doesn't match the code style conventions of the codebase it's being inserted into.
There was a problem hiding this comment.
my implementation of peekable was modeled after the interface provided by more_itertools, but stripped down for the use case here. the rest of the code is original, but written outside mopidy first.
for a more general take, i'm writing a 'real' comment outside the review box.
Note that we support OR-ing multiple base URIS, which MPD doesn't.
|
i agree that my parser doesn't match the style of the rest of mopidy-mpd. the problem as i see it is that filter expressions require parsing of recursive structures, something that also doesn't fit with the rest of mpd commands. i wrote the parser from 36985e0 first, with the intention of fully matching the spec. but while integrating it into mopidy, i noticed that we can't support everything anyways, and stripped out the constructs mopidy-core doesn't understand in ecc666c. that stripped-down version actually has the interesting side effect of not really requiring recursive parsing any more (if we don't care about descriptive error reporting and limit parsing to non-nested expressions). that way we could get rid of the parser entirely and replace it with a regex based one, similar to how tokenize.py does it. before i throw everything away (which i'd be fine with) and start over with a different approach, i wanted to check in with core devs on which strategy you prefer (which i didn't until now because i got distracted with work, but now that you asked, it's as good a time as any other). a downside with a regex based parser is that it can't fully support the spec, and if a client sends us a nested expression we can't parse, we can't really report to the user why we don't support this query. thoughts? |
|
another thing i wanted to mention, unrelated to the specific implementation: i was unable to find a mpd client that actually uses the new expression syntax. mpc, ncmpc, ncmpcpp and cantata all use the 'legacy' search parameters, that we already support (mpc is a bit of a special case, since it allows the user to manually type a filter expression, but it won't assemble one out of a simple search). |
|
It sounds like https://github.com/jcorporation/myMPD/ supports search expressions. But one (relatively small) client doesn't make it worth supporting if there's a lot of work involved. EDIT: And maybe this is a useful indicator: https://github.com/search?q=mpd_search_add_expression&type=code |
|
Would it be difficult to implement support for "not" matching in Mopidy? I'm guessing it would need changes to all backends as well as the Mopidy core.
It's difficult for clients to submit it until servers support it first. If it's brand new syntax, I'd expect client support to improve over the next couple of years. I personally don't think the current state of play should be a blocker if someone is generous enough to put in the effort 🙏 |
Today I've been testing different newer MPD clients for the albumart feature I contributed to, and most of them have issues with Mopidy because of the new search filters, sorting and grouping. Supersonic and MaxiumMPD for example are new good looking Android clients, but they're unusable because of protocol issues. |
TODO:
(TYPE == 'VALUE'),(...AND...)and(!...)_query_from_mpd_search_parametersto return(base ...)[sort {TYPE}] [window {START:END}],[group {GROUPTYPE}]