Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preliminary speaker info with more information #335

Merged
merged 1 commit into from
Aug 28, 2015

Conversation

the01
Copy link

@the01 the01 commented Aug 24, 2015

Updated the get_speaker_info() function to parse xml/device_description.xml as described in #320

  • changed zone_icon to player_icon
  • added model_number
  • added model_name
  • added display_version

No unit tests yet

@the01
Copy link
Author

the01 commented Aug 25, 2015

My guess is that dom.findtext('.//{urn:schemas-upnp-org:device-1-0}device')returns None and that is the reason the test fails. Any ideas why this only happens for python 2.6?

@dundeemt
Copy link
Contributor

It would be nice to also have, memory, flash, ampOnTime. Also an option to get the full url to the icon i.e. full_urls=True.

Failing on 2.6 - soco/core.py line 1099 -- I am doing something similar in my helper library but I encode the response from requests using .encode('UTF-8'). Translating to your code...

dom = XML.fromstring(response.content.encode('UTF-8'))

This was cannibalized from the commented out code @ soco/core.py def uid

my helper lib is https://bitbucket.org/dundeemt/chilledsoco/src/66be490323f863eabd6ac3e4fdbd8419b4a12372/chilled.py?at=default class DeviceProperties

@the01
Copy link
Author

the01 commented Aug 25, 2015

The question is whether memory, flash,..etc. is really all that important or if we are starting to pollute the speaker_info-dict. For now you can always build the full url with the .ip_addressproperty.
Sadly your proposed fix did not work. I can't get my python2.6 installation up and running properly, so maybe someone with a working env could investigate?

@lawrenceakka
Copy link
Contributor

There is a bug in path support in python 2.6. We had some previous discussion about it here: #154 (You may need to view comments on outdated diffs to see it):

This seems to be a bug in Elementtree < 1.3 (i.e. the version in Python 2.6). Your fix is too widely targeted though, since it testing for xml.etree.ElementTree' in sys.modules will be true for Python 3.

You are right however that the .// is not in fact needed at all here, since the item element is always an immediate child of the top level element, in this particular case.

So in fact, the best solution is for you to remove the xml_find_prefix code, and remove the ".//" in all cases where you have otherwise used it. i.e. in your patch, replace

for item in metadata.findall(self.xml_find_prefix +
'{urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/}item'):
with

for item in metadata.findall('{urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/}item'):

One way to handle it might be to find the device element, and then conduct a search of its immediate children (i.e. avoid ".//" as much as possible). I haven't tested this though.

I don't mind including memory, flash etc (though I can't image why anyone would need them), but I'm not keen on the full_urls idea. I agree with @the01 that this can easily be done by the user by combining with the ip address if necessary.

Lawrence

@dundeemt
Copy link
Contributor

full_urls @lawrenceakka @the01 is a common idiom in soco. The argument against it here is the same that could be made else where browse/ search/ and variants have a parameter to specify the full url is returned. I bring it up for consistency.

As for, those other things in device_description.xml, I can see the point about not polluting the get_speaker_info return structure, but offering access to all of that information, somewhere would be a good thing. Perhaps, that c/sh ould be a different feature request and pr.
-Jeff

@the01
Copy link
Author

the01 commented Aug 25, 2015

Maybe I will have another go at the whole 2.6 tests later, though my only way to test this is to do a commit and let travis to its thing. Which is not really optimal for debuging purposes (and makes for an interesting git history 😄 ). So it would be greatly appreciated if someone with a working 2.6 installation could have a look.
Though is it actually necessary to support a platform (2.6) that hasn't seen an update since, I think, 2013? Are there any usage statistics on pypi on which version a package gets installed for?

Maybe a get_speaker_info_all() whitch does the actual request and returns a dict with all possible entries and get_speaker_info() uses the all-method instead of requests/lxml and just filters the dict for the values we want?

@the01
Copy link
Author

the01 commented Aug 26, 2015

Alrigth, the tests finally surrendered, we are good to go 😉

@lawrenceakka
Copy link
Contributor

+1 from me. Please squash a few of the commits, and I'll commit.

Separate PRs for absolute URIs, and for additional info (which I think should be in the same dict), are welcome

…player_icon; added model_number; added model_name; added display_version;
@the01
Copy link
Author

the01 commented Aug 28, 2015

Down to only a single commit

lawrenceakka added a commit that referenced this pull request Aug 28, 2015
Preliminary speaker info with more information
@lawrenceakka lawrenceakka merged commit 69325ab into SoCo:master Aug 28, 2015
@lawrenceakka
Copy link
Contributor

Thanks very much for this. Please make sure to add a comment on #332 which we will use to create release notes

@lawrenceakka lawrenceakka added this to the 0.12 milestone Aug 28, 2015
lawrenceakka added a commit that referenced this pull request Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants