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

Make backend.playlists.lookup() return list of track URIs #527

Closed
adamcik opened this Issue Oct 6, 2013 · 6 comments

Comments

3 participants
@adamcik
Member

adamcik commented Oct 6, 2013

The lookup of tracks is currently only done against the local backend, so other URI types be it http or file won't work even if the stream backend can handle them.

We have to options so solve this:

  1. Simply add a Track(uri=uri) for cases where lookup fails and it's not a local uri.
  2. Update the API between core and backends to only return playlist tracks uris, which implies core needing to look them all up afterwards.

First option is obviously the least intrusive, the second is in my opinion the correct one.

@txomon

This comment has been minimized.

Member

txomon commented Oct 7, 2013

IMO, the core should be responsible for all this stuff. If we switch from Objects method to URI one, we would have this hereby fixed.

@adamcik

This comment has been minimized.

Member

adamcik commented Oct 7, 2013

Agreed, but we probably want to do just the first fix for now as an immediate, and wait with the larger rewrite until we get to it in our roadmap instead of starting to shave yaks right away.

@txomon

This comment has been minimized.

Member

txomon commented Oct 7, 2013

Ok, so I would go for solution 1, as it is the most closed to the next solution =)

adamcik added a commit to adamcik/mopidy that referenced this issue Oct 20, 2013

local: Temporary workaround for issue mopidy#527
Adds a fallback to `Track(uri=uri` when track lookup fails for playlists. This
means we can at least load metadata less tracks giving users functioning
playlists, instead of only supporting `local:track:...` style URIs.

Issue is not fixed, but this is sufficient to reduce priority until we get to
the larger planed refactor for this and other core API issues.

@ghost ghost assigned adamcik Oct 20, 2013

jodal added a commit that referenced this issue Oct 20, 2013

Merge pull request #539 from adamcik/fix/bug-527-temp-workaround
local: Temporary workaround for issue #527

adamcik added a commit to adamcik/mopidy that referenced this issue Nov 27, 2013

local: Always return track with just uri for local playlist tracks
This is related to mopidy#527, but is only a stop gap until we fix it right. Note
that this actually causes a regression, as not playlist tracks will have any
metadata after this change.

@adamcik adamcik referenced this issue Nov 27, 2013

Merged

Switch to json based library for local. #595

6 of 7 tasks complete
@adamcik

This comment has been minimized.

Member

adamcik commented Feb 25, 2014

I'm actually wondering if we just want to kill this. #704 would imply that playlist providers could simply return refs, and then the library backend's lookup would convert them to tracks. A down side is that I'm not sure if I want to mix library and playlist responsibilities, though this could make sense.

@txomon

This comment has been minimized.

Member

txomon commented Feb 25, 2014

SGTM... I can't nevertheless say much about this because I haven't got up-to-date with mopidy since 0.17... But unifying the place to convert uris, making it more centralized, into refs seems reasonable

@jodal jodal modified the milestones: v0.20, v0.19 - MPD playlist mgmt and other MPD improvements Jun 13, 2014

@jodal jodal removed this from the MPD playlist mgmt milestone Feb 5, 2015

@jodal

This comment has been minimized.

Member

jodal commented Feb 5, 2015

Option 2 and #704 is part of the Core API v1.0 plans. Closing this issue, as this is tracked elsewhere.

@jodal jodal closed this Feb 5, 2015

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