Skip to content
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

Return valid db_update date/time in MPD stats response #686

Closed
CDrummond opened this issue Feb 3, 2014 · 19 comments
Closed

Return valid db_update date/time in MPD stats response #686

CDrummond opened this issue Feb 3, 2014 · 19 comments

Comments

@CDrummond
Copy link

I'm the author of Cantata, a Qt4/5 MPD client. To populate the music listings I use the listallinfo command - which is now supported by Mopidy as of 0.18. Cantata caches the music listing, to save calling listallinfo at each start-up. To ensure the cache is valid, Cantata stores MPD's db_update date/time in its cache file - and compares this with that returned by a stats call.

Currently, Mopidy returns 0 for all values in the stats response. Could mopdiy not return the date/time of the local backend's tag cache instead?

(I'm ok with leaving the other fields as all 0 - as this is currently how Cantata detects that it is connected to Mopidy :-) )

@kingosticks
Copy link
Member

would extensions like spotify, soundcloud, gmusic etc still be supported
with a cache?
On 3 Feb 2014 21:23, "CraigD" notifications@github.com wrote:

I'm the author of Cantata, a Qt4/5 MPD client. To populate the music
listings I use the listallinfo command - which is now supported by Mopidy
as of 0.18. Cantata caches the music listing, to save calling listallinfo
at each start-up. To ensure the cache is valid, Cantata stores MPD's
db_update date/time in its cache file - and compares this with that
returned by a stats call.

Currently, Mopidy returns 0 for all values in the stats response. Could
mopdiy not return the date/time of the local backend's tag cache instead?

(I'm ok with leaving the other fields as all 0 - as this is currently how
Cantata detects that it is connected to Mopidy :-) )

Reply to this email directly or view it on GitHubhttps://github.com//issues/686
.

@CDrummond
Copy link
Author

But listallinfo only returns the local music, so spotify, etc, would not be in the cache. Cantata (in trunk) has an MPD search tab that would work with spotify, etc.

As stated above, Cantata (trunk) seems to work fine with mopidy 0.18 - its just that the cache does not update correctly, due to Mopidy not returning a valid date/time to a MPD stats call.

@CDrummond CDrummond reopened this Feb 3, 2014
@CDrummond
Copy link
Author

D'oh! Sorry, did not mean to close the issue - thought 'Close and Comment' was to close the edit field! :-(

@kingosticks
Copy link
Member

Listallinfo can return results from any backend, not just local. But maybe
that's fine. Providing search actually does a search and not just a cache
lookup.
On 3 Feb 2014 21:49, "CraigD" notifications@github.com wrote:

Reopened #686 #686.

Reply to this email directly or view it on GitHubhttps://github.com//issues/686
.

@CDrummond
Copy link
Author

Ah, I thought it only worked for the local backend. Must admit that I'm new to mopidy, and only tried it with the local and subsonic backends - and listallinfo only returned the local music.

Yeah, the search tab performs its search via the mpd api on the server. So, in my example above it also found music via subsonic.

@adamcik
Copy link
Member

adamcik commented Feb 3, 2014

As Nick already pointed out we do indeed expose all kinds of backends in browse. An example would be spotify where we have the toplists for the current user, all the spotify countries etc, which of course are not static. So tying this to just the local backend isn't an option for us.

We could perhaps instead set the time to the current timestamp at startup and then reset it when ever we get a "rescan" or refresh call?

@kingosticks
Copy link
Member

That seems reasonable. By refresh do you include any backend doing a
refresh?

Mpdroid recently added a cache so if we could get something working it'd
help them out too.
On 3 Feb 2014 22:01, "Thomas Adamcik" notifications@github.com wrote:

Ask Nick already pointed out we do indeed expose all kinds of backends in
browse. An example would be spotify where we have the toplists for the
current user, all the spotify countries etc, which of course are not
static. So tying this to just the local backend isn't an option for us.

We could perhaps instead set the time to the current timestamp at startup
and then reset it when ever we get a "rescan" or refresh call?

Reply to this email directly or view it on GitHubhttps://github.com//issues/686#issuecomment-34005595
.

@CDrummond
Copy link
Author

But is listallinfo really useable for the non-local backends?

@kingosticks
Copy link
Member

File style navigation is really nice (and for some backends it's preferable) so we don't want to lose it. You're proposing that listall and listallinfo (they must be consistent) only return from the local backend. That would leave lsinfo and listall outputting inconsistent results. In the case where you only have remote backends it's particularly odd. Do you think we could rely on all mpd clients handling this?

It probably is very usable for the beets backend and probably others too. I see what you're saying but I don't know if it's workable.

@CDrummond
Copy link
Author

I'm not really proposing anything - I don't have enough Mopidy experience for that :-) I was merely querying whether listallinfo makes sense if the output is not 'static'. listallinfo/listall response from MPD can be huge, so if you add onto this non-static data, then it does not (IMHO) make much sense for a client to use it. If the client cannot cache the response (as the data can change), then it means the client needs to call listallinfo each time - which could be slow.

To be honest, I'm not sure how many clients actually use listallinfo. Cantata uses it to work around MPD's Artist/AlbumArtist tag handling, soring albums by year, durations, etc.

What would be nice is if there was a way for client to know it is connected to Mopidy (perhaps add a "is_mopidy" API call, MPD would simply return with an error to this, so no real harm caused). Then a client can act accordingly. So, for example, in a listallinfo call it could perhaps call "listallinfo " - then Mopdiy would only return local files.

@kingosticks
Copy link
Member

Apologies @CDrummond, I didn't mean to put words in your mouth, only to hash out the idea a bit more (for my benefit if nobody elses).

An additional mopidy-only MPD command is an idea but then you're supporting two code paths in your client. There was a very similar discussion here abarisain/dmix#436 (comment).

@CDrummond
Copy link
Author

To be honest, I already need to know if Cantata is connected to Mopidy or not. Cantata has a basic tag-editor, and file organizer. After these are used, Cantata calls "update" on MPD, but Mopidy does not support this. Therefore, if connected to Mopidy I display a warning note about needing to manually update the DB.

So, some official way of detecting Mopidy would be great. At the moment, I just use the fact that Mopidy sets the response to stat as all 0 to detect Mopidy.

I personally have no issue with having 2 separate paths - afterall, it is quite different.

@jodal jodal added the question label Feb 7, 2014
@adamcik
Copy link
Member

adamcik commented Feb 16, 2014

Personally I prefer clients not know or care about if they are talking to Mopidy. Though, currently I see that this likely isn't always a realistic option.

Given some of the musings from one of the guys involved in mpd development, it sounds like they are working on extending their plugin support, which might mean that the "special" things that need to be done to account for Mopidy might also apply to MPD in the future. Just depends on what they have in mind.

@flying-sheep
Copy link

Cantata doesn’t work with my Pi Musicbox:

While it shows “now playing” information and the queue just fine, it only shows “Updating…” in the Artist list.

@tkem
Copy link
Member

tkem commented Nov 22, 2014

Maybe Mopidy should just drop support for "listall" and "listallinfo" alltogether, and just return with an error code that forces MPD clients to drop their local caching and use the server-side interface like it's meant to be? I'm aware that this would render many MPD clients unusable (or "broken", depending on your point of view), but I guess they'd catch up sometime, and "listallinfo" to me sounds a bit like the infamous "download the whole Internet" button, given Mopidy's range of extensions. I also wonder if these clients handle circular references, which might be perfectly legitimate for some extensions...

Just my 0.02$

@tkem
Copy link
Member

tkem commented Nov 22, 2014

Even if my last comment may sound too radical to some, "feature detection" is (almost) always a better choice than "product/vendor detection". Ask any frontend developer who coded an

if (isNetscape) {
...
}

a few decades ago.

@adamcik
Copy link
Member

adamcik commented Dec 16, 2014

+1 for just turning of those features or hiding them behind a config flag or something.

@tkem
Copy link
Member

tkem commented Dec 16, 2014

To reiterate (since it just came up again on discuss.mopidy.com and in mopidy/mopidy-local-sqlite#45), IMHO, for any MPD server that has the ability to provide substantial amounts of music from "the cloud", MPDs listall will never work as used by -- it seems -- most of today's clients, i.e. as a client-local cache/storage/backup of each and every track.
So instead of having to deal with bug reports/user discussions reporting a behavior "half working" -- either taking forever, or, in the case of the local extension, "working" up to an arbitrary limit of tracks, it might be less confusing to simply turn this off.

@adamcik
Copy link
Member

adamcik commented Feb 18, 2015

Since the MPD stats don't really make sense in Mopidy, and we just merged #990 which blacklists listall and listallinfo info I don't think there is anything that can be done with this bug. So going ahead and closing it.

@adamcik adamcik closed this as completed Feb 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants