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 <artist / album etc.> iterates over all local tracks #913

Closed
joemarshall opened this Issue Dec 16, 2014 · 5 comments

Comments

2 participants
@joemarshall

joemarshall commented Dec 16, 2014

If you call mopidy with a 'list albums' or 'list artists' command, how it is handled is:

  1. List all tracks in the database that fit the query.
  2. Add the the artist for each track into a set
  3. return the set

How it should work:

  1. List all the artists/albums in the database (that fit whatever extra query is applied), possibly via browse URIs or something.

This causes a bug in the sqlite backend, because that backend sanity checks the general 'search' command and limits it to 100 results, and also means that listing artists / albums etc. is very slow if you have even a vaguely large local database. I imagine it also massively slows down the various search things in spotify/google music etc, because presumably if you search for artist it has to return all the tracks by that artist first?

This is not obvious in musicbox, because it doesn't use the list command at all, but breaks most clients (MPDroid, NCMPCPP both support browsing by artist or album).

Description of the sqlite backend bug caused by this here:
https://discuss.mopidy.com/t/browse-artists-albums-only-shows-a-small-number-of-artists/435/3
mopidy/mopidy-local-sqlite#45

Example of the underlying code that needs fixing:

in mopidy/mpd/music_db.py:

    def _list_artist(context, query):
        artists = set()
        results = context.core.library.find_exact(**query).get()
        for track in _get_tracks(results):
            for artist in track.artists:
                if artist.name:
                    artists.add(('Artist', artist.name))
        return artists
@adamcik

This comment has been minimized.

Member

adamcik commented Dec 16, 2014

I've lost track of if I ever opened a bug for the following idea, or just mentioned when talking with jodal. But I suspect we want / need some API in the backends that allows for a more efficient look-up for cases like this. As currently there is no way for backends to provide these listings in an efficient manner.

@joemarshall

This comment has been minimized.

joemarshall commented Dec 17, 2014

One possible neat and tidy way would be for find_exact in library to take a further argument, that said what return type you were looking for.
e.g. with
def find_exact(self, query=None, uris=None, **kwargs)
becomes
find_exact(self,query=None,uris=None,expectedReturnType=TRACK,**kwargs)

If expectedReturnType=TRACK or expectedReturnType=ALBUM or whatever then in the back end, you return models.SearchResult with that type.

Now, obviously this would require the update of backends to support this in the long term. However, you could use inspect on each backend to find out whether it supported expectedReturnType, and drop back to the current behaviour otherwise (or implement it via catching the 'unexpected argument exception', which would work fine given the function call timing itself is hardly a bottleneck in comparison to all the db/filesystem lookups.

It'd be a pretty minor change - would need:
1)library.py changing to pass through expectedReturnType and fallback to current behaviour if that fails.
2)music_db.py changing to pass expectedReturnType in the various queries, and to deal with the result appropriately.
3)Backend find_exact methods updating to handle expectedReturnType as and when their maintainers get round to it.

@joemarshall

This comment has been minimized.

joemarshall commented Dec 19, 2014

Looking in the code comments, I can see that there is a further idea for a tidy up of search and find_exact, so that all code calls 'search', with an argument like 'exact=true' to do what find_exact does.

So this would be an optional argument to search similar to this, and could in fact be implemented at the same time, with the backend base class handling non-updated backends that still use find_exact and don't support return type choice.

Is there any reason I shouldn't implement these optional args to search (plus tests) and do a pull request? ie. might someone else be working on them?

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 23, 2014

Sounds like a reasonable direction to go in. While on the topic I should also mention https://docs.google.com/a/adamcik.no/document/d/1TphZ2dofTvnxk54UxVOQkhIhzVUwChmfWwMHA-vJfaU/edit as one of the future plans and https://docs.google.com/a/adamcik.no/document/d/15hEZkB0_W33vUD-pKsf-gk3BICDgxsa1FC0H_lMXFUU/edit#heading=h.ovemgfx92e2j

Though starting with smaller refactorings like you suggest is likely best.

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 2, 2015

We just merged #1022 which adds a new API for getting these distinct values. Internally the JSON backend still does the same stupid thing, but at least now mopidy-local-sqlite can do a sane distinct query.

In other words I'm considering this solved, as I've opened #1019 to try and hunt down other places relying to "blank" searches to get all the tracks and do stuff with them.

@adamcik adamcik closed this Mar 2, 2015

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