Create separate discovery crate

[Johannesd3]

This PR splits out a new librespot-discovery crate out of librespot-connect. The original rationale was the fact that discovery and spirc don't have many dependencies in common. Probably they only belong to the same crate because Spotify sells them as one feature.

However, after the split was done, I wondered how the new crate could be improved and extended.

There might be other endpoints that we (at least I) don't know of, or they might be added in future, so my first aim was to make it easy to add them. I also thought of the possibility to add a client functionality to this crate. Therefore, I decided to create slightly overengineered Serialize and Deserialize implementations for arguments and responses. Those can be found in the modules request and response. I added unit tests to handle the additional complexity.

I tried to create a neat API surface, and added documentation for all public items.

I don't want this to get merged before #665 is merged into dev, so I keep it as a draft for now. But I'd really like to hear you feedback before I continue working on it. Do you think it is a worthwhile change, or is it getting too complicated?

This change is not breaking: The module librespot_connect::discovery remains as a compatibility layer for the new crate and will be deprecated. This module could be removed in a later breaking release.

PS: There's one concrete improvement: The server does not panic anymore on certain malformed requests.

Future possibilites

• Generalize the API, so that users of the crate are able to use all information given to the addUser endpoint or to send custom error messages as response.
• Add client functionality, possibly behind a feature flag

EDIT: After removing commit e3e60bb, most things don't apply anymore. It would be just a step into this direction.

[sashahilton00]

I left a more detailed comment in #648 w.r.t discovery/mdns being split out. I think this makes sense if we do go ahead and strim down librespot into a leaner library.

[Johannesd3]

(Just rebased it on top of #668 because they're somehow related and I don't want them to diverge)

[Johannesd3]

The change is not breaking anymore, so it would be eligible for a 0.2.x release.

The de-/serializing of requests and responses became (too?) elaborate, but if you want so, I could restore the old parsing functions.

While I find it pleasing to run cargo doc for librespot-discovery, the docs will certainly contain mistakes due to my english, so please look over it too.

What do you say?

[Johannesd3]

Would you feel more comfortable if I would split off e3e60bb from this PR? It would be much smaller then.

[roderickvd]

+1 if I had anything to say about it.

1. If fits within ongoing work to improve decoupling: decoder to playback (Move decoder to playback crate #692), volume control to playback (Optimize volume control logic #685). This fits nicely within that theme and seems like a no regret move.

2. I feel like we're on the brink of a major undertaking to move to the new API and CDN. If this eases development accessing new or updated endpoints, then all the better.

3. I'm not afraid of breaking changes. You could say it's nice that 0.1.x has been pretty stable, at the same time you could say it's been slow moving. As development pace picks up, and we're doing some major revamping of playback and hopefully the API/CDN, it's only logical that we're disturbing the waters. It's up to downstream packages to decide whether they'll hang out on 0.1.x and wait for it to settle, or hopefully keep up pace. We've certainly made it easier now the changelog is in.

4. Additionally from Optimize volume control logic #685 you also know I'm not afraid of opportunistic refactoring, so I say keep e3e60bb in.

[Johannesd3]

4. Additionally from Optimize volume control logic #685 you also know I'm not afraid of opportunistic refactoring, so I say keep e3e60bb in.

This is more than opportunistic refactoring (-115 +1,037), and it's not strictly necessary. It would just make it easier if new functions are discovered in this small protocol, or if someone wants to reverse the functionality and use it as client to discover other devices. It might be good to keep it for later, but for now I'm going to remove this commit.

[roderickvd]

I left the tiniest review. Other than that, looks good to me and a good move to reduce coupling.

[Johannesd3]

You left no review.

[roderickvd]

You left no review.

Forgot to push a big green button 🤦

[roderickvd]

So this may be nitpicking but I think you should update the version field a few lines up to do the same. Or more specifically I think the version field should be the librespot version and libraryVersion the discovery crate version.

For librespot this will likely be the same. If we want to be 100% goody two shoes about it, downstream applications should set the version field to their version and use the librespot version for libraryVersion.

Again this is really nitpicking so unnecessary to spend too much time on it.

For reference a couple of strings I found from official Connect devices:

Denon HEOS 1:

{
   "status":101,
   "statusString":"OK",
   "spotifyError":0,
   "version":"2.3.0",
   "deviceID":"c55c5eac1425934ea557ac129df7ad0fa00011a2",
   "remoteName":"Sypialnia",
   "activeUser":"xxx",
   "publicKey":"gDPJ1G71jRxqxZHET/t081EF5cQ4IR/wE0WCeqTRtAb72OAooAqn8RdUnVTfxGPAI7eph69CDoamD15i91TutfrDvw7ALOUplwg7bjM3MAuWSwjimOoiPDNRdDPz8n0y",
   "brandDisplayName":"HEOS by Denon",
   "modelDisplayName":"HEOS 5",
   "libraryVersion":"2.8.71-g9919e10f",
   "resolverVersion":"1"
}

Autonomic:

{
   "status":101,
   "statusString":"ERROR-OK",
   "spotifyError":0,
   "version":"2.6.1",
   "deviceID":"11224DB33B1B",
   "deviceType":"SPEAKER",
   "remoteName":"Kök",
   "activeUser":null,
   "publicKey":null,
   "accountReq":null,
   "brandDisplayName":"AC",
   "modelDisplayName":"Media Control Server",
   "libraryVersion":"5.35.20191025.0",
   "resolverVersion":null,
   "groupStatus":null,
   "tokenType":null,
   "clientID":null,
   "scope":null,
   "availability":null,
   "autonomic_fqn":null
}

Asus Clique R100:

{
   "status":101,
   "statusString":"OK",
   "spotifyError":0,
   "version":"2.1.0",
   "deviceID":"255720ef-e852-419f-a2f5-6443905b8cfd",
   "remoteName":"Wohnzimmer",
   "publicKey":"bTBzujQi9/JfWvZZIlhMnFy7MHwzTrHH1DC2B7Xtl3VJhFgZTywLD26VlsrqSv5jO5f6sLgZ01wrotShQpNPJ7varaZ9VJ/7Ch/tHegbeObCCAMMOAQkpWIp0/aPlI/g",
   "activeUser":"xxx",
   "accountReq":"PREMIUM",
   "deviceType":"SPEAKER",
   "brandDisplayName":"ASUS",
   "modelDisplayName":"Clique_R100",
   "libraryVersion":"1.20.0-g594175d4"
}

[Johannesd3]

No, I don't think so. I'm really no expert in this regard, but 2.x.y could very well indicate the version of the protocol used.

[roderickvd]

No, I don't think so. I'm really no expert in this regard, but 2.x.y could very well indicate the version of the protocol used.

Neither am I so let's try it this way then.