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

Feature/mpd protocol extensions #1230

Merged
merged 4 commits into from Jul 27, 2015

Conversation

3 participants
@fatg3erman
Contributor

fatg3erman commented Jul 23, 2015

Here's an Initial proof-of-concept for some MPD protocol extensions.

Adds a new mpd command:
x_protocolextensions {BOOL}
which switches on the new functionality. This setting is stored on
a session-by-session basis, which makes slightly more work for the
client but means that the behaviour is entirely sandboxed and only
visible to clients that want it, even in a multi-client setup.

There is a new config item:
mpd/proto_ext_allowed {BOOL}
which defaults to true but, if set to false, disables the above
command, thus making mopidy revert to 'standard' MPD behaviour

Enabling x_protocolextensions then provides 2 new pieces of extra
functionality to the client:
When translating mopidy tracks to MPD output, album URIs and album
Images are output along with the standard information, eg:
X-AlbumUri: spotify:album:fhwfhewfihi38
X-AlbumImage: http://some.path.to.an/image.jpg

'search' and 'find' commands permit an extra parameter to specify
which backends to search:
search artist "someartist" domains "local:,spotify:"
Trying to use 'domains' with protocol extensions disabled will return
an error just like standard mpd.

I updated most of the existing tests so they pass, but wanted to get this up here for discussion before going on with any further new tests or documentation

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2015

#1235 added support for the tagtype command, which lets clients introspect what fields are available. With this in place I'm comfortable just adding the fields without needing the opt in command or config option. Which hopefully also makes things easier for your :-)

As for find and search, could we make this work with file/filename instead of adding a new query field? Also if we do add something using the name source would be a better match with one of the other PRs we have underway now.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2015

Oh, and ideally the find/search changes and the X-... changes would be better as separate PRs in this case :-)

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 26, 2015

Yeah OK so you're suggesting I add the X- tags to the new tagtypes list and just output them all the time? That actually means I can back out a large amount of the extra messy stuff, which makes life a whole lot easier :)

Search using file would be better. I'm not sure exactly how this would work though. It currently kind of does for spotify but not with any other backend. But yes these should be separate PRs, I got carried away with experimenting.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2015

Correct, just keep it simple for now. And if we see any reports of this breaking anything we can go back to the idea of making it opt-in.

I think we can make core + the MPD find/search commands handle this right with a few changes, and ideally without having to change backends.

fatg3erman added some commits Jul 26, 2015

Simplify the whole thing by using taglist types and not bothering wit…
…h the

config option or command to switch it on
@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 26, 2015

OK I'm not sure exactly what I did with git there but some commit history seems to have been lost which makes a lot of the comments make no sense now. What is git? It's a VCS but not as we know it.. :)
Anyway, this is now updated so that it just outputs the new fields.

if track.album and track.album.uri:
result.append(('X-AlbumUri', track.album.uri))
if track.album and track.album.images:
images = ';'.join(i for i in track.album.images if i is not '')

This comment has been minimized.

@adamcik

adamcik Jul 26, 2015

Member

This is technically correct, but https://docs.mopidy.com/en/latest/api/core/?highlight=get_images#mopidy.core.LibraryController.get_images is replacing it. So if you want to continue getting image data your probably want to use that instead.

I think we can merge this as is, but I would do a followup PR or bug with adding and images argument to this function which takes the results from a get_images call.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2015

Only missing a quick changelog entry to be ready to merge, assuming we postpone the get_images part :-)

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 26, 2015

OK added a changelog update. I'll look at updating this to use get_images in due course.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2015

Test seems to fail as the order isn't deterministic.

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 26, 2015

Ho-Hum. Not much I can do about that then except make it a list of 1 item.

@jodal jodal added this to the v1.1 - Robust core milestone Jul 27, 2015

adamcik added a commit that referenced this pull request Jul 27, 2015

Merge pull request #1230 from fatg3erman/feature/mpd-protocol-extensions
mpd: Add additional metadata fields for album URIs and image URIs

@adamcik adamcik merged commit 2faf668 into mopidy:develop Jul 27, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fatg3erman fatg3erman deleted the fatg3erman:feature/mpd-protocol-extensions branch Jul 28, 2015

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