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

Local json library ignores "limit" and "offset" search parameters #917

Closed
tkem opened this Issue Dec 21, 2014 · 3 comments

Comments

5 participants
@tkem
Member

tkem commented Dec 21, 2014

AFAICS, the local json library doesn't handle limit and offset when searching: https://github.com/mopidy/mopidy/blob/develop/mopidy/local/json.py#L160
This seems to be the reason that the json and sqlite libraries behave differently with regard to MPD's list artist, list album, etc. commands, which caused bug reports for Mopidy-Local-SQLite, which handles this correctly, I think: mopidy/mopidy-local-sqlite#45.
I suggest fixing this in the json library, so users may no longer believe this to be an SQLite-related issue.

@joemarshall

This comment has been minimized.

joemarshall commented Dec 21, 2014

Looking at it, the limit and offset parameters are not passed through from anywhere, and they default to very low. So they either:
a)Shouldn't be there,
or
b)Should be passed through all the way from the core.

and probably should also be not so low as default (or perhaps should be disabled as default)

Until that is fixed, there is no way to list more than 100 albums with mopidy-local-sqlite. If this limit is enabled in json, then that breaks also. It's also a pain to implement because of the way that json does search. The json backend really needs retiring.

It's shows up most obviously in part because of
#913

which means that the list [blah] commands all completely fail to work in mopidy-local-sqlite once you have >100 songs, but even if album listing is implemented, I expect people won't be happy if it fails for anyone who has >100 albums.

@jodal jodal added this to the v0.20 - Audio cleanup part 1 milestone Feb 5, 2015

@jodal jodal added 1 - Ready and removed 1 - Ready labels Feb 5, 2015

@ali

This comment has been minimized.

Contributor

ali commented Feb 20, 2015

cc @adamcik

Three options I'm considering:

  1. Remove the limit and offset parameters. Is there a reason to limit or offset the results? Why is pagination a concern? (re: #949 (comment))
  2. Make the limit and offset parameters optional (default to None, can be an int).
  3. Find a way to use pagination from local.library's search functions.

I'm uncertain about both option 3 and paginating local libraries, especially if these parameters are unused.

adamcik added a commit that referenced this issue Mar 2, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 2, 2015

Fixed by #947 now that #1022 has been merged. There is still no way to do pagination though, which should be a new issue IMO.

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