search

kellnerd / harmony

Music Metadata Aggregator and MusicBrainz Importer
MIT License
40 stars 6 forks source link

Tidal provider #11

Closed phw closed 3 months ago

phw commented 3 months ago

This implements a Tidal provider. The provider supports lookup by album URL or by GTIN search.

In my tests so far this working well. A few things to note:

  1. For now this expects the API credentials (client ID + client secret) for the Tidal API to be set in the environment variables HARMONY_TIDAL_CLIENT_ID and HARMONY_TIDAL_CLIENT_SECRET. API credentials can be registered on the Tidal developer portal on https://developer.tidal.com/dashboard/ , see also https://developer.tidal.com/documentation/api-sdk/api-sdk-quick-start

    As discussed in chat we can add functionality to hide the provider if the pre-requisites are not given. But this is something I'd do in a separate PR.

  2. I implemented filling the label from the providerInfo field as documented in the API. I could however not find a release using this, it might be Tidal stopped supporting it.

  3. While Tidal is available only in a limited list of markets and releases may or may not be available in specific markets (see this list), the API does not indicate the availability. Instead each API request must include the country code, and the request might fail. Right now the provider tries the first given region or falls back to US.

  4. Releases might contain video tracks, such as for example https://tidal.com/browse/album/130201923 . Right now those are treated like any other track. They could be filtered out, but probably shouldn't. I think in the future it would be good if Harmony would have explicit support for marking tracks as video (even if unfortunately this can currently not be seeded).

Here is a screenshot of a lookup:

grafik

phw commented 3 months ago

I added 0d6a203 to show a warning message if the track list contains video tracks:

grafik

phw commented 3 months ago

@kellnerd I realized I wasn't using the regions as supposed and changed it to actually try all the regions set in provider.options. Not completely happy with the code though, maybe you have some suggestions on 3736729d4ee8403e2f3133ad09d2829fbe77e476

I first tried to handle this in the query method of the provider, as iTunes does it. But this doesn't fit well, as response structure varies and query is used for GTIN lookups as well as for release and track list requests.

phw commented 3 months ago

While this would also work for tracklist requests in theory, these should not try all regions again, so we would need two separate query methods: One which just uses the given API URL as is and one which tries all regions until it succeeds. Not sure whether it's worth to implement this.

I thought about this. I think it makes in general more sense to keep the region try logic in the ReleaseLookup class. The query method on the provider is pretty generic, and I think it is good so. Unless all requests behave the same determining whether a region call is required and whether it succeeded should be the responsibility of the caller.

But a separate helper queryAllRegions with either a data extractor function as you suggested or maybe a validator function could be helpful. Let's get this merged first and I will look into this.