by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
Michael Guntsche
@maru-sama
Let me try and get a xml dump of that
Kenneth Nielsen
@KennethNielsen
In any case, if the code markup is too difficult, I can review it in a PR
Michael Guntsche
@maru-sama
Ok, so this is the xml that's returned
b'<res xmlns="urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/" duration="0:03:01">x-sonos-spotify:spotify:track:3sfLdQneX6bJ4IokTpibMf?sid=9&amp;flags=0&amp;sn=9</res>' b'<res xmlns="urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/" />' b'<res xmlns="urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/" duration="0:03:01">x-sonos-spotify:spotify:track:3sfLdQneX6bJ4IokTpibMf?sid=9&amp;flags=0&amp;sn=9</res>'
So yes, why write a protocolInfo in there at all with spotify direct. This used to be different but a recent version apparently changed this.
and yes they are sending it twice.... no comment
Michael Guntsche
@maru-sama
And right now I cannot think of a better solution than parsing the text if the protocolInfo is missing, checking if it is sonos related and if yes, manually set protocolInfo. Do you have a better idea?
Kenneth Nielsen
@KennethNielsen
No. Not with my current understanding of the problem. I will be easier to see in context. Just make sure to mark these music service non-spec compliant hacks clearly with comments in the code (with an explanation of what the code was changed from an to) so it can be reverted later if the problem is fixed
Michael Guntsche
@maru-sama
I wonder if we should put these exceptions in a separate file to better track them and keep them separate from the rest of the code. On the other hand this might make it more complex
Kenneth Nielsen
@KennethNielsen
Yeah. I actually though about that. I remember reading once that for like graphics cards drivers in the Linux kernel they have files containing "Quirks", which provides the logic for exceptions to the expected behavior and it would be tempting to something like that. I do however see two problems with it. One is that I find i difficult to figure where in the structure to put it and the other challenge that even at this point it would be a pretty big task to try and track down the exceptions that are already there.
Actually now thinking about it. I think the way that I would do it, if I were to do it, would probably be to manipulate the XML before being parsed ... hmm ... that could work, not sure it is worth it... or maybe it is.
Will think a little more about the design of it on the bikecride home from work. I think if you are interested, it might be cool to try and do it for this problem. That will also pretty clearly show which music services are the sinners ;)
Kenneth Nielsen
@KennethNielsen

@maru-sama ok, the more I think about this the more I'm convinced it is an idea worth persuing :D What I would propose is something along the lines of this:

  • Create a data_structure_quirks module whose interface is just a single pure function apply_object_quirks (we could later do a apply_ressource_quirksas well)
  • In data_structures.DidlObject.from_element, as the very first thing pass element though that apply quirks function
  • Then in the apply quirks function, make some system to check which quirks should be applied. Like basically doing a text search for the service id of something in an text exported version of the element and then if it matches, call a specific internal that applies quirks for that service

What do you think?

Michael Guntsche
@maru-sama

So in the spotify direct case the following would happen.

  • Inside quirks call a function that checks for a missing protocolInfo
  • In the case it is missing call a fixup function which itself checks in the text if it is coming from spotify direct
  • If yes add the protocolInfo for spotify and return the element

We could also put logging in the function to inform the user if any quirks were applied. So if in the future sonos decides to fix this we can disable the quirk again.
Right now I think the worst thing that can happen is that we build this up just for this specific case and nothing else will use it, but on the other hand we keep the standard code clean and move all the special case handling to a separate module.

Kenneth Nielsen
@KennethNielsen
Oh. I think I would do it the other way around. Inside the quirks function, check whether the content is spotify direct, it if is call a function that does all corrections for spotify direct, like check for that missing item and fix it. That way you will end up with n+1 functions, where n is the number of music services that have broken implementations and a translation dictionary with service search term to correction function
Don't worry about adding this just for the one case. There are more already. Besides from that there is a PR for fixing Alexa which is huge. I don't know whether that service is still that broken, but if it is, even for that it would be worth it.
And yeah, logging is a good idea
I also have another idea which might help in bringing the data structure modules back closer to spec, so this will fit in nicely with some restructuring there
Kenneth Nielsen
@KennethNielsen
@DPH hi. It's been a while since I was active with SoCo, so I'm trying to touch base with everyone. Are you interested and have time to do a little bit of testing of various PR. I have very limited time, so if possible I would like to try and do more reviews and bug squashing.
David Harding
@DPH
Hi @KennethNielsen I can make some contribution but have less time than I used to. Feel free to ask, or I will spot changes and jump in where I can. Cheers David
Kenneth Nielsen
@KennethNielsen
@DPH ok, what I will do is tag you once in a while in PRs and ask if you can test and if you do not have time to spend on SoCo, just ignore it ;)
Kenneth Nielsen
@KennethNielsen
@stefankoegl I was considering updating the code example on the io pages, but they seem to contain a lot of html. Did you use some sort of tool to generate the html?
Stefan Kögl
@stefankoegl
@KennethNielsen I apparently created it using the "Automatic Page Generator", which has been discontinued since
maybe I can find the time during the weekend to re-create it with today's tools
Kenneth Nielsen
@KennethNielsen
@stefankoegl if you can, that would be great. I think the page content wise is good enough as it is except the code example. The code example is Python 2 only and I also don't think shows of the best parts of our API. I would, humbly, suggest the code example in the frontpage of read the docs
Kenneth Nielsen
@KennethNielsen
Ok. So it has been quite in here for a while. For once SoCo is not late due to real life taking all the time but shear forgetfulness, probably about time I add SoCo deadlines to my real world calendar. I preparing to feature freeze 0.19 shortly and want to get a few more changes in. Most significantly is #697 for adding a Python 2.7 deprecation warning, which will allow us to start removing Python 2 support immediately after release.
@stefankoegl regarding 1.0 I've run into an API design "issue" regarding group handling, which I would prefer to have sorted out before we do a 1.0, so if you don't mind I'd prefer to keep this one 0.19
Kenneth Nielsen
@KennethNielsen
@maru-sama I will start the quirks work with the missing protocol info being the first and only thing in there in time for 0.19, unless you already started it.
Kenneth Nielsen
@KennethNielsen
@maru-sama please see #698 and check that the fix for Sprotify Direct is correct
Kenneth Nielsen
@KennethNielsen
0.19 is feature frozen and we now enter a 2 week testing period. At this link is a (not necessarily complete) check list for the testing period: https://github.com/SoCo/SoCo/issues/673#issuecomment-570391917
Kenneth Nielsen
@KennethNielsen
Gentle reminder for testing 0.19. I have only limited time and we shouldn't release without proper testing.
Michael Guntsche
@maru-sama
@KennethNielsen I just checked this and made some additional comments. It does not fix the issue completely but I explained how I fixed it in #698
Michael Guntsche
@maru-sama
I found a way to do it in quirks and commented in #698
Michael Guntsche
@maru-sama
@KennethNielsen As you might have seen I did a pull request with the most straightforward fix I could find.
Kenneth Nielsen
@KennethNielsen
@maru-sama thanks a lot for testing it. I will have a look at it as soon as I get the chance.
pwt
@pwt
What's left to do for the 0.19 release? Anything specific I can help with?
pwt
@pwt
Anyone up for reviewing PR #704 for merge onto master? It's been system tested, is documented, and comes with unit tests.
Kenneth Nielsen
@KennethNielsen
@pwt I need testing as per https://github.com/SoCo/SoCo/issues/673#issuecomment-570391917, but it seems that amidst the spotty maintainer coverage we lost active developers as well, but that is understandable
I will have a look at that PR
Kenneth Nielsen
@KennethNielsen
I finally got around to finishing the PR to paint SoCo black, please have a look: SoCo/SoCo#706
pwt
@pwt
I've done some 0.19 integration (and unit) testing. I'll accumulate results and notes at: https://github.com/SoCo/SoCo/issues/673#issuecomment-599095679
The Black changes #706 look good to me. No issues for me with flake8, pylint, or the unit and integration tests (or the Black check).
Kenneth Nielsen
@KennethNielsen
@pwt thanks a lot
Kenneth Nielsen
@KennethNielsen
If anyone else have a second, please have a look at #706 as I want to get that in before starting to merge other stuff
pwt
@pwt
I've now successfully run all v0.19 unit and integration tests on:
Python 3.9.0a4+ on Raspbian Buster
Python 3.8.2+ on Raspbian Buster
Python 3.7.7 on macOS 10.14.6
Python 3.7.3 on Raspbian Buster
Python 3.6.10+ on Raspbian Buster
Python 2.7.16 on Raspbian Buster
pwt
@pwt
... also Python 3.5.9+ on Raspbian Buster
Kenneth Nielsen
@KennethNielsen
Wow, ok ok, more than enough. I was just looking for one more python and OS. I will do the release as soon as I have a little extra time. @pwt thanks a lot.
Kenneth Nielsen
@KennethNielsen
:fireworks: