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

'list "album" "albumartist" "foo"' command not supported #468

Closed
henne49 opened this Issue Jun 10, 2013 · 12 comments

Comments

4 participants
@henne49

henne49 commented Jun 10, 2013

Hi,

get this error using MPD client MPDroid on my tablet accessing and spotify by using mopidy.

DEBUG    2013-06-10 16:47:23,743 [5334:MpdSession-31] mopidy.frontends.mpd
  Request from [192.168.0.179]:59698: list "album" "albumartist" "Biffy Clyro"
DEBUG    2013-06-10 16:47:23,761 [5334:MpdSession-31] mopidy.frontends.mpd
  Response to [192.168.0.179]:59698: ACK [2@0] {list} not able to parse args

it seems that the keyword albumartist is not valid.

Can I add this keyword somewhere, sorry I'm new to python and was not really able to help myself.

Many thanks

@henne49 henne49 closed this Jun 10, 2013

@henne49 henne49 reopened this Jun 10, 2013

@jodal

This comment has been minimized.

Member

jodal commented Jul 22, 2013

There is no mention of a four-token version of list in the MPD protocol docs at http://www.musicpd.org/doc/protocol/ch03s06.html. We'll have to look at how the original MPD server responds to this.

Using a very small music DB:

list "album"
Album: Going Out in Style
Album: The Understanding
Album: Versus
Album: Junior
Album: Playing Live in a Room
Album: Supermodified
Album: Riot on an Empty Street
Album: 4 Ton Mantis
Album: Melody A.M.
Album: Quiet Is the New Loud
OK

list "albumartist"
AlbumArtist: Kings of Convenience
AlbumArtist: 
AlbumArtist: Röyksopp
AlbumArtist: Dropkick Murphys
AlbumArtist: Amon Tobin
OK

According to the docs, at third token is only allowed if the second is album. This is matched by MPD's implementation:

list "album" "Amon Tobin"
Album: Supermodified
Album: 4 Ton Mantis
OK

list "albumartist" "Amon Tobin"
ACK [2@0] {list} should be "Album" for 3 arguments

According to the MPD source code, when the number of tokens is greater than 3 and even the tokens from the third location and on is used as filter pairs:

list "album" "albumartist" "Kings of Convenience"
Album: Versus
Album: Playing Live in a Room
Album: Riot on an Empty Street
Album: Quiet Is the New Loud
OK

list "album" "albumartist" "Kings of Convenience" "album" "Versus"
Album: Versus
OK

@ghost ghost assigned jodal Jul 30, 2013

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Sep 28, 2013

There are a few of these things that seems to have broken?

I cannot do either 'list "albumartist"' nor 'list "album" "albumartist" "Daft Punk"':

$ telnet localhost 6600
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
OK MPD 0.17.0
list "album"
Album: Random Access Memories
Album: Discovery
OK
list "albumartist"
ACK [2@0] {list} incorrect arguments
list "album" "albumartist" "Daft Punk"
ACK [2@0] {list} not able to parse args

I am not yet adept enough with the Mopidy code to figure out where things are breaking down, though.

@txomon

This comment has been minimized.

Member

txomon commented Sep 30, 2013

@ZenithDK ATM, mopidy only supports album, artist, date and genre.

Here the lines:
https://github.com/mopidy/mopidy/blob/develop/mopidy/frontends/mpd/protocol/music_db.py#L210

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Sep 30, 2013

@txomon: Yep, I've been working on fixing that :)
I have a branch where I have it working in, working on implementing "track" as a filter specifier also, and I want to get "count" working too, then I'll submit it.

@txomon

This comment has been minimized.

Member

txomon commented Sep 30, 2013

Can you reference it here?

Javier Domingo

2013/9/30 Lasse Bigum notifications@github.com

@txomon https://github.com/txomon: Yep, I've been working on fixing
that :)
I have a branch where I have it working in, working on implementing
"track" as a filter specifier also, and I want to get "count" working too,
then I'll submit it.


Reply to this email directly or view it on GitHubhttps://github.com//issues/468#issuecomment-25339788
.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Sep 30, 2013

https://github.com/ZenithDK/mopidy/tree/albumartist
Feel free to review and comment, albumartist filtering is working, still left is the track and count I mentioned.
Also I need to clean it up a bit, and I would like to refactor the regexps since that threw me a bit off since they are in 2 (3?) files.

@jodal

This comment has been minimized.

Member

jodal commented Oct 1, 2013

I have to admit that I have an old 90% complete private branch fixing this. I'll try to get around to look at your branch and get something merged soon.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Oct 2, 2013

Jodal: Right now the branch is not in a mergeable state as I worked some more on it, trying to get the count command to work (works now!) and I added support for 'list "albumartist" "album" "something" "track" "1" which MPDroid likes to throw in to sort the albums by year.
Last, I started hacking on getting the search results sorted better when we now have both artist and albumartist.
I have a lot of debug to remove - but if you could review all the non-debug related stuff, that would be great.

Too bad if I have duplicated some work you already did - but hey, it was a learning experience for me if nothing else :-)
Can you link your private branch so I can get inspiration?

@jodal

This comment has been minimized.

Member

jodal commented Oct 11, 2013

@ZenithDK I've now solved the list "album" "albumartist" "something" case in pull request #532. I haven't looked at filtering by track, implementing the MPD command count, or result sorting. If you can separate out those parts as individual commits/pull requests with tests, I'd love to merge them. You can look at the #532 pull request for examples of how to test it.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Oct 11, 2013

@jodal: I don't agree with how albumartist has been implemented in #532. Please try to look up the difference between AlbumArtist and Artist - they are not the same.
I have not implemented them as the same in my PR.
http://forums.ilounge.com/itunes-related-mac-pc-applications/218581-artist-vs-album-artist-im-confused.html
http://forums.mp3tag.de/index.php?showtopic=14780

Example: You have an album where there is a main artist, I have the album Daft Punk - Random Access Memories, where the MP3 tag AlbumArtist is "Daft Punk", but for the individual tracks the Artist has been labelled with "Pharell" and other people.
My changes extract the AlbumArtist in the scanner and extends Album to have a new AlbumArtist property and works off that.

I'm on a business trip to Korea right now and the working hours are long, so I won't be able to split it out and make a PR until after the 18th, but I will do so at that time.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Oct 11, 2013

My only concern/puzzle was if I should merge Artist and AlbumArtist when you receive a query for just AlbumArtist, as you would otherwise leave a lot of artists out, if they only have tracks where they are listed as the Artist, and not AlbumArtist - e.g. just about everything out there?
But I tested with MPD, and it does not, so I didn't either.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Oct 13, 2013

Oh, and another argument for not merging artist+albumartist is that artist would normally be a superset of albumartist.
You can have an album called "Herbie Hancock Tribute" where the actual track artists are "Dude1", "Dude2", etc, but the albumartist is "Herbie Hancock".
If you want "Dude1" and "Dude2", just use the artist tag to search for them.

@adamcik adamcik closed this in #532 Oct 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment