New Model Mapping #149
New Model Mapping #149
Conversation
|
@quodrum-glas Thanks, your contribution is much appreciated, even if it is not a complete PR. Of course, this will require quite a bit of work to implement your changes but it looks like it could be worth it in the long run. Perhaps @2e0byo or @blacklight could be interested in having a look. Otherwise, I'd take a look when the latest HiRes features are added to this release of mopidy-tidal
Feel free to link to this other fork, if you have more relevant additions that you think are relevant to add to mopidy-tidal
Perhaps it might be a good idea to add a similar PR to python-tidal so your bugfixes can be added there so I can get them merged in. |
That indeed is awaited! I tried myself in the past, having PKCE implemented for a while now, but just didn't want to. Probably the identification was not allowed even thought taken from the latest android app. The WebUI credentials showed more promise but got 401 and did not know what to do with it. Now I know it was the player...
Other items may have become a duplicated at this point. It will will be clearer what's left after the new model and other current effort from you at the moment.
Yes, I will start with this. Hopefully starting again using soon to use the master at |
| from mopidy_tidal.uri import URI, URIType | ||
| from mopidy_tidal.workers import paginated | ||
|
|
||
| DEFAULT_IMAGE = "https://tidal.com/browse/assets/images/defaultImages/default{0.__class__.__name__}Image.png".format |
There was a problem hiding this comment.
binding a callable (.format) to a constant is a little unexpected.
|
@quodrum-glas thanks for this, it's really good to see. Couple of high level comments:
Functionality-wise caching is definitely a good idea, although we need to handle stale state (if there was handling for that I missed it, sorry). [sorry for the early post: hit enter by mistake] |
| @@ -167,272 +49,79 @@ def get_distinct(self, field, query=None) -> set[str]: | |||
| apply_watermark(a.name) | |||
| for a in self._get_artist_albums(session, artist_id) | |||
| ] | |||
| elif field in {"track", "track_name"}: | |||
| elif field == "track": | |||
There was a problem hiding this comment.
Duplicate condition check?
There was a problem hiding this comment.
This might be a consequence of trying to merge something I did not change but remained as part of much earlier fork.
Effectively is a "roll back" :)
| from mopidy_tidal.search import tidal_search | ||
|
|
||
| logger.debug("Browsing distinct %s with query %r", field, query) | ||
| session = self._session | ||
| logger.info(f"Browsing distinct {field!s} with query {query!r}") |
There was a problem hiding this comment.
Nitpicking, but I think (at least according to Black's style conventions) the log.info("msg: %s", var) notation is preferred over log.info(f"msg: {msg}") - at least for logging functions.
There was a problem hiding this comment.
(the reason for this btw is that f-strings are eagerly evaluated, but a logging function might never produce any output. Using % pushes the formatting down the stack and (in theory) avoids doing it if the log level suppresses the output)
There was a problem hiding this comment.
That is correct. Don't consider this change. I was trying to extract only the model mapping change and other things were included. And I was wrong to change the logging arguments, as it turns out.
@quodrum-glas Thanks for the suggestion. I ended up implemented it as you suggested, using a local HTTP server based on your code. It works rather well! I had to rewrite most of the relevant backend code but it works and it can also be used for OAuth login. |
|
@2e0byo The pull request is only to raise awareness on this model mapping that is quite responsive. Also this model mapping adds more items. It adds pages as per native app browsing, etc. The cache is defined centrally in one place and can be adjusted easily to best suited balance. There were a lot of re-requests for data. Especially for track data. Quite expensive. I welcome the point about I believe the value here is that I tried to match the need for server data with browsing behaviour and frontend structure (as in only request what is needed at the time and no further content without explicit request) This has lived with me for a while and naturally can be improved already. |
|
To test this implementation and see the actual outcome, one can go to this ReadMe and use the command as stated there: As you know... ...must be done beforehand Or use
Another login is required (web server) but won't overwrite the stored session from master |
|
@quodrum-glas minor point on the uri: it shouldn't change, but tidal do actually change the uris of resources (perhaps when the rights are granted via a different deal?). I think (but haven't confirmed) that this already bites us. This looks great and I look forward to testing it. Speedup are very welcome. |
This is not ready to merge, as it requires more typing and testing must be integrated.
But it is worth implementing as is faster and easier to maintain
I don't have the time to do more, I stripped down testing and typing and use it from my fork (or this without merging many commits).
I would not part from it. I tested it extensively from phone app.
This is stripped down from my fork where I have more changes, in order to keep it just to the new model mapping.
The lack of time to invest to make this a true pull request, means I keep my changes obscure in my own space.
If you think is worth it, maybe there are other contributors willing to do the necessary testing code, typing, login_hack decoration, etc
If this gets integrated, then I will contribute more to the master
PS There might be some bugs, but that would come from the tidalapi I have a update many commits behind here
At least the one on page recursion is still happening