Subsonic: Implement queue syncing

[jplitza]

This (ab)uses the playlist mechanism to save the play queue as a playlist named "Queue", putting the extra data of which song is playing at which position in its comment as a JSON.

This fixes #1103

[paulijar]

Thanks for the PR. Here are a few thoughts:

1. Returning an error code on unimplemented API endpoints may break some clients. If this wasn't the case, we wouldn't need the stub methods for unimplemented endpoints in the first place because the dispatcher function handleRequest already returns an error response if no implementation is found. See e.g. Add nop getNowPlaying route for SubsonicController #1079.

2. This "Queue" playlist now becomes visible as normal playlist on all clients and the web UI which is probably not what the user expects.

3. The Subsonic play queue may contain songs or podcast episodes but this implementation now supports only the songs. The queue may even be a mixed set of songs and episodes. Obviously, our playlist system supports only songs which is another reason, why this queue probably shouldn't be saved as a playlist.

4. In getPlayQueue, the name used for the items of the queue should be entry and not songs. See the example in http://www.subsonic.org/pages/api.jsp#getPlayQueue.

5. In the schema, the PlayQueue is defined to have three required attributes which are missing from the proposed solution
   
   At least DSub client depends on the changed attribute since it apparently checks if the version on the server is newer than its own locally saved queue.

6. DSub seems to choke on the restored play queue if it has too many songs. We might need to limit the maximum amount of entries returned by getPlayQueue. Some more testing would be needed to determine the suitable limit but at least 280 entries seemed to be already too much for DSub.

BTW, what's your reference client for this feature? Personally, I haven't noticed any other client except DSub using this part of the API.

[jplitza]

Thanks for the quick feedback! All in all, I doubt I'll find the time to implement a proper implementation, given the nuances you point out.

1. Returning an error code on unimplemented API endpoints may break some clients. If this wasn't the case, we wouldn't need the stub methods for unimplemented endpoints in the first place because the dispatcher function [`handleRequest`](https://github.com/owncloud/music/blob/a7c824f17e48d3b6a46d4a46de99c47f808644bf/lib/Controller/SubsonicController.php#L159) already returns an error response if no implementation is found. See e.g. #1079.

Fair enough. That commit basically became obsolete by implementing getPlayQueue, where returning an empty queue could arguably be a problem (had it been according to schema).

2. This "Queue" playlist now becomes visible as normal playlist on all clients and the web UI which is probably not what the user expects.

I expected criticism for that decision, but actually like the "graceful fallback" it provides for clients that don't support the feature. I expected it to be easier to reuse the playlist implementation, however...

3. The Subsonic play queue may contain songs or podcast episodes but this implementation now supports only the songs. The queue may even be a mixed set of songs and episodes. Obviously, our playlist system supports only songs which is another reason, why this queue probably shouldn't be saved as a playlist.

...this completely destroys that idea then. (I have no idea about Subsonic itself, and don't use episodes.) It sounds quite inconsistent to allow episodes here but not in playlists. Then again, the whole API seems rather ad-hoc. (For example, when a song is contained multiple times in the play queue, it's unclear which instance the "current" id refers to, since they have the same id. Also, GET for saving possibly huge amounts of data?!)

4. In `getPlayQueue`, the name used for the items of the queue should be `entry` and not `songs`. See the example in http://www.subsonic.org/pages/api.jsp#getPlayQueue.

Huh, it did work in [Sublime](https://sublimemusic.app/) when I tested it (to answer that question as well ;) ). But maybe they implemented it wrong.

5. In the [schema](http://www.subsonic.org/pages/inc/api/schema/subsonic-rest-api-1.16.1.xsd), the PlayQueue is defined to have three required attributes which are missing from the proposed solution  At least DSub client depends on the `changed` attribute since it apparently checks if the version on the server is newer than its own locally saved queue. 6. DSub seems to choke on the restored play queue if it has too many songs. We might need to limit the maximum amount of entries returned by `getPlayQueue`. Some more testing would be needed to determine the suitable limit but at least 280 entries seemed to be already too much for DSub.

See above: Saving via GET already implies a limit.

[paulijar]

Regarding the GET limits, all the methods in the Subsonic API may be called either with GET or POST. It's up to the client which one it wants to use. But yes, some of the methods in the Subsonic API are ill-suited to be used with GET.

[scrutinizer-notifier]

The inspection completed: No new issues

[jplitza]

I fixed remarks 1, 4 and 5.

Regarding the GET limits, all the methods in the Subsonic API may be called either with GET or POST. It's up to the client which one it wants to use. But yes, some of the methods in the Subsonic API are ill-suited to be used with GET.

Ah, I see. Then Sublime is to blame for using GET for that endpoint (or maybe it even switches to POST when the data gets too large, dunno).

6. DSub seems to choke on the restored play queue if it has too many songs. We might need to limit the maximum amount of entries returned by getPlayQueue. Some more testing would be needed to determine the suitable limit but at least 280 entries seemed to be already too much for DSub.

That's not many songs. My play queue routinely has several hundred songs, and I'd be quite disappointed to have only the first X of them synced. Also, what if the currently played song isn't part of what is returned?

In general, this sounds like a client-side issue that the server shouldn't care about.

[paulijar]

I fixed remarks 1, 4 and 5.

Okay, great. The changedBy property should actually be the name of the client app which saved the queue. This name should be available in the parameter c. The $this->userId should go to the property username.

That's not many songs. My play queue routinely has several hundred songs, and I'd be quite disappointed to have only the first X of them synced. Also, what if the currently played song isn't part of what is returned?

I agree that it's quite low limit and a bit surprising that DSub handles it this badly. The cropping logic should probably be made smart enough to prefer to crop items before the current song rather than after it, to avoid cropping away the current song.

In general, this sounds like a client-side issue that the server shouldn't care about.

In general, yes. But the practical point of view is that DSub doesn't seem to be very actively maintained nowadays, with the latest release being 18 months old. But it's not dead yet, either, since the latest PR has been merged this August. Also, DSub is one of the most feature-complete Subsonic clients and this makes it a valuable reference client for me, and I wouldn't want to break the compatibility with it.

Maybe we should make DSub-specific hack here and crop the queue only if the argument c=DSub. I don't like at all making client-specific exceptions but this time it might be the least bad option.

---

What comes to storing the queue, one option might be to store it in JSON format using the Nextcloud configuration manager. This would require injecting \OCP\IConfig to the SubsonicController, after which it could be used like this:

$this->configManager->setUserValue($this->userId, $this->appName, 'play_queue', $json);
...
$json = $this->configManager->getUserValue($this->userId, $this->appName, 'play_queue');

This stores the value in the database table oc_preferences. Another quite similar solution would be to store the JSON to our own DB table oc_music_cache by using the Cache class. The benefit would be keeping this data in our own table instead of a shared one. The downside would be that Music always clears the cache on version update.