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

Add is_playing_tv, is_playing_radio, is_playing_line_in properties with tests #225

Merged
merged 9 commits into from Jun 8, 2015

Conversation

Projects
None yet
6 participants
@dajobe
Copy link
Contributor

commented Sep 14, 2014

Also update the docs to note that the switch/play tv are for playbar.

@coveralls

This comment has been minimized.

Copy link

commented Sep 14, 2014

Coverage Status

Coverage increased (+0.1%) when pulling 75671a9 on dajobe:is-playing-tv into 6f318df on SoCo:master.

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

Is this a specific test just for the playbar? (i.e. not line in)

If so, maybe add it to the method comment?

A good addition

+1

Very low risk by the look of it - maybe even one to squeeze into 0.9 (if not certainly 0.10)

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2014

Yes this is for the playbar. I've been using it and it works fine. I'll update the doc

@coveralls

This comment has been minimized.

Copy link

commented Sep 16, 2014

Coverage Status

Coverage increased (+0.1%) when pulling 3bfec51 on dajobe:is-playing-tv into 6f318df on SoCo:master.

@lawrenceakka

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2014

According to the comments at #195 this will also return True if line-in is used. Please can you confirm or deny?!

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2014

@lawrenceakka damn that's annoying. I don't use line in so it works for me.

@stefankoegl

This comment has been minimized.

Copy link
Member

commented Sep 27, 2014

How should we proceed here?

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2014

If I can find the time and a cable I'll try to differentiate line-in and TV on our playbar. Otherwise, this code works for me reliably since I have no ambiguity right now

@coveralls

This comment has been minimized.

Copy link

commented Oct 7, 2014

Coverage Status

Coverage increased (+0.09%) when pulling 484e482 on dajobe:is-playing-tv into 9302fd2 on SoCo:master.

@stefankoegl stefankoegl added this to the 0.11 milestone Nov 16, 2014

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2015

Hi @dajobe - did you manage to do the test you detailed here:
#225 (comment)
To confirm it is different from the "Line-In"

If it is indeed different - then it would be good to get this merged in

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2015

@robwebset the playbar only has one input and intended for TV, so Line-In == TV. I'm not sure what to do with this patch.

"PLAYBAR connects to your TV using a single optical cable and plays all sources connected to the TV, including cable boxes and game consoles." -- http://www.sonos.com/shop/products/playbar

See http://www.sonos.com/shop/i/products/playbar.detail_big.jpg

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2015

@dajobe The question is - does your code is_playing_tv do the same as the check for line-in already in SoCo?

And if you have something that has a line-in - does this is_playing_tv method return "true" when it is using line in?

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2015

@robwebset I can check out the playbar part of it at home, but I don't have any other sonos device with line-in to check the opposite.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

If no one else steps up, I can probably do the test here. In any case, I don't think it should be pulled before it has been tested (and possibly fixed), because the name of the property would then be misleading.

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2015

@robwebset I was looking for "the check for line-in already in SoCo?" you mentioned but I can't see it. There is no is_playing_linein method - maybe we should add that too?

I can see switch_to_line_in that uses x-rincon-stream: with CurrentURI arg to avTransport.SetAVTransportURI

Looks like if that's the difference then TV and Line-In are distinguishable since my patch here looks for TrackURI starting x-sonos-htastream:. I'm curious if my code would make TrackURI return x-rincon-stream... if playing line in. I can't test that since I only have a Playbar. @KennethNielsen can you help ?

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2015

@dajobe Sorry, yes you are correct - got confused - the question I meant to ask - is this the same as line-in, so will the same command check TV and Line in - in which case it should have a generic name to cover both cases - if they are different checks - then they should have different names :)

Edit: the code I use for line-in always switches - it doesn't check to see if it is already using line-in

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

Agree with @robwebset about that the names should reflect the functionality.

@dajobe Yes, I may be able to test it this evening or over the weekend. Can you supply the test code you wich me to run on this branch?

@coveralls

This comment has been minimized.

Copy link

commented Jan 23, 2015

Coverage Status

Coverage increased (+2.76%) to 53.9% when pulling 83d2db8 on dajobe:is-playing-tv into 9302fd2 on SoCo:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 23, 2015

Coverage Status

Coverage increased (+3.04%) to 54.18% when pulling 6c5a262 on dajobe:is-playing-tv into 9302fd2 on SoCo:master.

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

@KennethNielsen I added two new is_playing methods we can try out but more specifically see if is_playing_line_in and is_playing_tv work correctly. I can only test playbar (TV in).

@dajobe dajobe changed the title Add is_playing_tv property and test Add is_playing_tv, is_playing_radio, is_playing_line_in properties with tests Jan 23, 2015

@coveralls

This comment has been minimized.

Copy link

commented Jan 23, 2015

Coverage Status

Coverage increased (+3.04%) to 54.18% when pulling 68a460c on dajobe:is-playing-tv into 9302fd2 on SoCo:master.

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2015

So I think once the is_playing_lin_in has been verified (unfortunately I can't - I don't have anything with a line-in) we can get this into the 0.11 release #278

+1

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2015

When playing tv through my playbar with this code:

In [18]: coord.is_playing_tv
Out[18]: True

In [19]: coord.is_playing_radio
Out[19]: False

In [20]: coord.is_playing_line_in
Out[20]: False
@coveralls

This comment has been minimized.

Copy link

commented Feb 28, 2015

Coverage Status

Coverage increased (+0.25%) to 54.38% when pulling 68e5ee0 on dajobe:is-playing-tv into 843be92 on SoCo:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

commented Feb 28, 2015

Coverage Status

Coverage increased (+0.25%) to 54.38% when pulling 68e5ee0 on dajobe:is-playing-tv into 843be92 on SoCo:master.

@coveralls

This comment has been minimized.

Copy link

commented Feb 28, 2015

Coverage Status

Coverage increased (+0.25%) to 54.38% when pulling 68e5ee0 on dajobe:is-playing-tv into 843be92 on SoCo:master.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Mar 4, 2015

@dajobe I'm very sorry for not reporting back after having promised it. I've been busy with my 5 months old, still that's no excuse for not writing here that I was too busy.

In any case, I finally found a few moments to test the branch. I played something on line-in on my play 5 and it works the way it is supposed to:

>>> kontor.is_playing_tv
False
>>> kontor.is_playing_line_in
True
>>> kontor.is_playing_radio
False

great job

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jun 7, 2015

@robwebset Wow, this has been laying around for a while. I guess this is ready for merge after the test I did, correct?

EDIT Sorry, I meant to refer this comment to @dajobe

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jun 7, 2015

@dajobe ^^

@robwebset

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2015

+1 from me - think everything is OK to go in

@KennethNielsen KennethNielsen referenced this pull request Jun 7, 2015

Closed

Release 0.11 #278

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2015

@KennethNielsen thanks. this took a while :)

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

@dajobe You agree it is good to go in in current form corretc?

@dajobe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2015

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

I'll merge. Please drop a line about the change in the issue with information for the release notes #311

KennethNielsen added a commit that referenced this pull request Jun 8, 2015

Merge pull request #225 from dajobe/is-playing-tv
Add is_playing_tv, is_playing_radio, is_playing_line_in properties with tests

@KennethNielsen KennethNielsen merged commit 8eff40e into SoCo:master Jun 8, 2015

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

@dajobe never mind my comment about added line to the release notes, it seems that @stefankoegl already did it.

@dajobe dajobe deleted the dajobe:is-playing-tv branch Sep 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.