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.

Sign up for GitHub

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

Jump to bottom

Rework Sonos media player platform #12126

Merged
merged 9 commits into from
Feb 18, 2018
Merged

Rework Sonos media player platform #12126

merged 9 commits into from
Feb 18, 2018

Conversation

amelchio
Copy link
Contributor

@amelchio amelchio commented Feb 2, 2018 •
edited
Loading

Description:

This is an updated version of the huge Sonos rework originally proposed in #10419. It decreases code complexity and moves to a push model based on Sonos event subscriptions.

I have integrated all changes made to the Sonos platform since my original PR but I do not have the hardware to test the Night Sound support; maybe @rbdixon can help out?

Related issue (if applicable): fixes #11822, fixes #9570

Checklist:

  • The code change is tested and works locally.

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully.
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

amelchio
amelchio commented Feb 2, 2018
View reviewed changes
homeassistant/components/media_player/sonos.py Outdated
REQUIREMENTS = ['SoCo==0.13']
REQUIREMENTS = [
'https://github.com/SoCo/SoCo/archive/'
'9f848e7a2c73aebb0f6d0b09008b17990f4ffabc.zip#SoCo==0.14a5']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A needed third party fix is on track to be released in SoCo 0.14 on February 17th. So feel free to review now, even though the REQUIREMENTS are not completely ready yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is okay, we had this before also some times :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm told it is no longer allowed to introduce Github requirements so I will hold off merging this PR until SoCo 0.14 ships.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay. That is only for packages they want produce a packages. But in this case we need this version and it will go into mainstream later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that is a policy that makes sense to me but I think some consensus is needed here. This PR has been lingering for two months because I did ask about just this policy in Discord and IIRC both @balloob and @armills said that my only options were to a) fork SoCo with my single patch added, or b) wait it out for a new SoCo release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had the same situation on last update of this platform and we had do it like now. First we point to commit and after the release was out, we update to new release. I see no problem, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One technical question I have is how to format the string so the package is not downloaded on each restart. The SoCo Github currently has version "0.12+" and we are using "0.13" in current HA code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need look how pip does install it: pip list --format=freeze

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that helped me figure it out. But that apparently points to a disadvantage of a Github requirement: two different commits can have the same version and pip will not know to update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

pvizeli
pvizeli approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing 👍 Thanks a lot for this hard work

@pvizeli
Copy link
Member

pvizeli commented Feb 6, 2018 •
edited
Loading

https://github.com/SoCo/SoCo/blob/master/soco/__init__.py#L22 You are sure with "12-"? But the goal is that he don't try to reinstall it self every startup and not the correct version :)

@andrey-git andrey-git mentioned this pull request Feb 6, 2018
Closed
@amelchio
Copy link
Contributor Author

amelchio commented Feb 6, 2018

Yes, "0.12-" is what works to stop the reinstall on startup. But I think we should await SoCo 0.14 or a resolution of home-assistant/architecture#3.

@balloob balloob added the waiting-for-upstream We're waiting for a change upstream label Feb 8, 2018
@balloob
Copy link
Member

balloob commented Feb 8, 2018

We should not add new requirements that point at GitHub. What is the release cycle of SoCo ?

@amelchio
Copy link
Contributor Author

amelchio commented Feb 8, 2018

@balloob That is just a placeholder, see here: #12126 (comment)

@amelchio amelchio removed the waiting-for-upstream We're waiting for a change upstream label Feb 18, 2018
@amelchio
Copy link
Contributor Author

Upstream was finally updated. This has waited for so long, I will merge it in a few hours if nobody objects.

Some fallout is expected so I have set myself as codeowner for the time being.

@balloob
Copy link
Member

balloob commented Feb 18, 2018 •
edited
Loading

Sounds good 👍 go for it.

@amelchio amelchio merged commit 635d36c into home-assistant:dev Feb 18, 2018
@rbdixon
Copy link
Contributor

rbdixon commented Feb 19, 2018

@amelchio: Sorry I'm late to the party. Just confirmed that the set_option service works as expected with a Sonos Soundbar. Tested against currently dev branch.

@balloob balloob mentioned this pull request Feb 22, 2018
Merged
@ranathan14
Copy link

With version 0.64 there are problems. No picture of the album and there is no possibility to start or stop music playback

@home-assistant home-assistant locked as off-topic and limited conversation to collaborators Feb 26, 2018
@balloob
Copy link
Member

balloob commented Feb 26, 2018

Open an issue if something is broken. Don't spam old PRs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@emlove emlove emlove left review comments

@pvizeli pvizeli pvizeli approved these changes

@andrey-git andrey-git Awaiting requested review from andrey-git

Assignees
No one assigned
Labels
cla-signed integration: sonos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sonos One - Exception Sonos Play 1 issues
8 participants
@amelchio @pvizeli @balloob @rbdixon @ranathan14 @emlove @andrey-git @homeassistant