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

core.playlists.create returns "null" playlist #1162

Closed
tkem opened this Issue May 5, 2015 · 12 comments

Comments

3 participants
@tkem
Member

tkem commented May 5, 2015

Mopidy v1.0.4:

"websocket:outgoingMessage" Object { method: "core.playlists.create", params: Object, jsonrpc: "2.0", id: 2 } localhost:8100:30:31
"websocket:incomingMessage" message { target: WebSocket, isTrusted: true, data: "{"jsonrpc": "2.0", "id": 2, "result": null}", origin: "ws://127.0.0.1:6680", lastEventId: "", eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, timeStamp: 1430845313203906, originalTarget: WebSocket } localhost:8100:30:31
"websocket:incomingMessage" message { target: WebSocket, isTrusted: true, data: "{"playlist": null, "event": "playlist_changed"}", origin: "ws://127.0.0.1:6680", lastEventId: "", eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, timeStamp: 1430845313207905, originalTarget: WebSocket } localhost:8100:30:31
"event:playlistChanged" Object { playlist: null } localhost:8100:30:31
"websocket:outgoingMessage" Object { method: "core.playlists.as_list", jsonrpc: "2.0", id: 3 }

EDIT: For debug output, see below.

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Nevermind.

@tkem tkem closed this May 5, 2015

@tkem tkem reopened this May 5, 2015

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Sorry for the confusion...
Real debug output is:

DEBUG    2015-05-05 19:07:13,336 [3323:HttpServer] mopidy.http.handlers
  Received WebSocket message from 127.0.0.1: u'{"method":"core.playlists.create","params":{"name":"tkem"},"jsonrpc":"2.0","id":4}'
DEBUG    2015-05-05 19:07:13,337 [3323:MainThread] mopidy.listener
  Sending playlist_changed to CoreListener: {'playlist': None}

So a playlist.create({name: "tkem"}) apparently returns a null playlist.

@adamcik

This comment has been minimized.

Member

adamcik commented May 5, 2015

So re-reading our docs we claim that if URI is none we will pick the "first" one that supports playlists. So that would likely be based on the order the entry points get loaded I would presume.

So looking at the code you would like hit this code path https://github.com/mopidy/mopidy/blob/develop/mopidy/core/playlists.py#L128 which is already marked as possibly fishy. I would suspect the "first" one ended up being RO playlist provider, where we need to be looking for the ones that support RW.

I also suspect this code will fail if there is no backend with playlists :/

@adamcik adamcik added this to the v1.1 - Robust core milestone May 5, 2015

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Ahem, sorry, could have thought of that myself, but it was a long day...
Removing the spotify extension (which reports a "Spotify login error: Needs a premium account", so it shouldn't have been active, anyway) fixes this.

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

However, this leads us back to the question whether "read-only" playlist providers should be supported (and marked as such) at all, or if it wouldn't be better to use the browse interface for RO playlists...

@jodal

This comment has been minimized.

Member

jodal commented May 5, 2015

We still haven't decided on anything here, but using browse for RO playlists and only implementing the playlist provider for RW playlists is something that has been discussed. I can't remember any huge drawbacks, except that it's not something we can enforce until 2.0, but we can decide on which direction to go in and update extensions accordingly.

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Sure, no need to rush. Still, wearing my "client developer hat", this has been bugging me since starting with playlist support for Mopidy Mobile, so I think it's time to define what we really mean when calling something a "playlist".

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Focusing back on the original issue, for an intermediate fix, I wonder if a "null" playlist result could mean "try the next playlist provider"?

@adamcik

This comment has been minimized.

Member

adamcik commented May 5, 2015

That sounds like a reasonable workaround, and something for the 1.0.x branch. Dp you time for a quick PR or are you too busy with Mopidy-Mobile? :-)

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

Current list of priorities:
a) work
b) MoMo
c) new slotcar racecourse
d) see what I'll can do

;-)

@jodal

This comment has been minimized.

Member

jodal commented May 5, 2015

The "first one" is fishy. It made sense back when we had a BACKENDS configuration that was a list you could reorder yourself. It's been fishy and ready for rethinking since extension support and the new config system landed in Mopidy 0.14 two years ago.

@tkem tkem self-assigned this May 5, 2015

@tkem

This comment has been minimized.

Member

tkem commented May 5, 2015

As stated before by @adamcik, the best thing we'll probably come up with short-term is loop through back ends until one returns a valid playlist (and doesn't throw). Will provide a PR along these lines in the next couple of days. Would love to see this in 1.0.5, since otherwise MoMo's new "create playlist" feature might break on any Mopidy installation with Spotify enabled. .

tkem added a commit to tkem/mopidy that referenced this issue May 6, 2015

jodal added a commit that referenced this issue May 6, 2015

Merge pull request #1165 from tkem/fix/1162-v1.0.x
Fix #1162: Ignore None results and exceptions from PlaylistsProvider.create().

@tkem tkem closed this in 636639a May 6, 2015

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