mopidy / mopidy-local-sqlite

DEPRECATED (Mopidy SQLite local library extension)
https://mopidy.com
Apache License 2.0
30 stars 10 forks source link

'list artists' / 'list albums' are broken #45

Closed joemarshall closed 9 years ago

joemarshall commented 9 years ago

There's a limit of 100 on search results in schema.py.

Unfortunately, the way that mopidy core responds to a 'list artists' command (in mopidy/mpd/music_db.py ) is to retrieve all tracks. This means that if you have more than 100 tracks, calling list artists or albums only returns the artists or albums for the first 100 tracks.

The fix is probably for mopidy to update to some kind of library API that knows about tracks and albums, because the current one is a bit limited. You can also up the limit, although that makes doing these queries slow if you have a lot of artists or albums.

https://github.com/mopidy/mopidy/issues/913

It isn't obvious in pi-musicbox, because that doesn't use list artists ever, and only ever uses the file browser interface, but it is obvious in pretty much any MPD client which supports listing artists or albums (e.g. mpdroid, ncmpcpp).

joemarshall commented 9 years ago

As an example of how this is inefficient, I've upped the limit and timed query of artist on 1700 tracks, with the browse files interface, it takes about 0.5 seconds, with 'list artists', 8.5 seconds,

tkem commented 9 years ago

The limits come from Mopidy core, so this is nothing Mopidy-Local-SQLite can do about it. As for support for MPDs listall and listallinfo etc., I think these are broken by design (at least on an extensible platform like Mopidy), and shouldn't be supported at all; see mopidy/mopidy#686 for my little rant about that ;-)

joemarshall commented 9 years ago

This is kind of true. Although the current limit of 100 tracks means that the sqlite backend is totally unusable on most MPD clients, because it doesn't support listing the artists or albums if you have more than 100 tracks.

One possible temporary patch at least until mopidy updates to pass artist / album queries through correctly, would be to detect the particular searches generated by list and respond to them with the full list. i.e. detecting: list all tracks search (used by list albums, list artists, list albumartist ) list all tracks with 'artist=' query (used by list albums artist = blah) and dropping the sql limit for those queries.

That way the sqlite backend wouldn't be broken which would be nice.

joemarshall commented 9 years ago

Or, alternative solution, which would make you happy because it would break things that relied on 'list all' which you are right is a stupid thing to do.

Detect these queries, return the same thing you return from the browse interface, without all the tracks, ie. list all tracks would need to return 1 track per artist, 1 track per album.

That way, it would still be broken for clients who download the full listing, which is fair enough, but basic functionality like list the artists in my music library would work, which frankly are pretty fundamental to the browsing and playback of music.

tkem commented 9 years ago

As I said before, I don't consider Mopidy-Local-SQLite broken, so there's nothing to fix. I also happen to successfully use several MPD clients with this, so "totally unusable" is a wild exaggeration. 'Nuff said.

joemarshall commented 9 years ago

Hi, can you tell me which clients work with it, so I can use one of them?

All I want is something that is as functional as a 10 year old iPad, ie. lets me easily go straight to artist / albums, and play and pause, without having to go through layers of 'local storage, artist name, album name...)

e.g. I want something where when I go to the 'artists' tab, it lists all artists, not just the first 10, and similarly when you go to the albums tab, you get an alphabetical list of your albums, not the first 10.

Oh, and lets me choose what to add, and whether to add it to the current playlist, or clear playlist and play it.

And doesn't have any obvious bugs, like the musicbox web client in which search is broken for all local backends (try search for artist name with any local backend, it only lists tracks)

So far I've tried: 1)MPDroid on android (broken because of this issue, works fine on json backend though) 2)ncmpcpp on ssh (broken because of this issue) 3)Musicbox web client (search broken, no way to get quickly to artist except going down into local media/artists ) 4)Rompr (works okay with json, but only because it caches all the data, which I agree with you is wrong)

As for whether it is broken, I know it's an interaction between how Mopidy is written and how you've implemented local-sqlite, but it is still the case that for a very typical use case of 'list what artists are in my music library', mopidy responds correctly with the json backend, and incorrectly with the sqlite backend. It only doesn't fail in testing because none of the sqlite backend tests use a library of >100 tracks and because mopidy doesn't test against the sqlite backend in integration testing.

tkem commented 9 years ago

Since you insist on flogging a dead horse (i.e. a closed bug report ;-)...

  1. Using MPDroid, I can browse artists via "Library -> Files -> Local media -> Artists" (I'm running the German version, so the translation may not be 100% correct). That's why M-L-SQlite has the Artists/Albums/Genre/etc. top-level browse categories in the first place, since these are usable by both Web and MPD clients. Yes, you are right, Mopidy's MPD support is far from perfect, and this isn't very intuitive for new users, but how to solve this is something that belongs in the MPD frontend and/or Mopidy core, but not in M-L-SQLite, which is happily unware of the idiosyncrasies of the MPD protocol (and will definitely stay that way). Anyway, "totally unusable" is a wild exaggeration (to put it nicely).
  2. The limit=100 comes from https://docs.mopidy.com/en/latest/modules/local/#mopidy.local.Library.search. The - AFAICS - only reason MPD's list artist command works with the josn library is given in https://github.com/mopidy/mopidy/blob/develop/mopidy/local/json.py#L160 Apparently, the json library completely ignores the limit and offset parameters passed to search(). I consider this a bug in the json library. If Mopidy-Local changes defaults or adds new APIs for listing artists/albums, I'm more than willing to adapt M-L-SQLite (although I don't think this would be a smart idea, at least not in the way you suggested). Until then, it will stay as-is.
  3. As for your 10 year old iPad (hey, I didn't know they were making these back then ;-)... Well, things have changed a bit in the mean time. Any changes to Mopidy's core and backend APIs must also take into account Web-based and streaming services, for which MPD's list artist simply doesn't fit -- listing all albums available from a Spotify account would (a) take forever and (b) probably exhaust your mobile device's memory/storage space.
  4. That's my last word on this. I appreciate if you want to help improve Mopidy's support for existing MPD clients, but please continue on mopidy/mopidy#913 where this discussion really belongs.

Regards, Thomas