Skip to content

Add general sound mode support#14729

Merged
balloob merged 6 commits intohome-assistant:devfrom
starkillerOG:patch-6
Jun 7, 2018
Merged

Add general sound mode support#14729
balloob merged 6 commits intohome-assistant:devfrom
starkillerOG:patch-6

Conversation

@starkillerOG
Copy link
Copy Markdown
Contributor

@starkillerOG starkillerOG commented May 31, 2018

Description:

General sound mode support for the Media player component.
See the discussion in the architecture fork: home-assistant/architecture#23

**Developers Docs: home-assistant/developers.home-assistant#17
**Needed as a basis for: #14718

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5495

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

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

  • 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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.


hass.services.call(DOMAIN, SERVICE_SELECT_SOUND_MODE, data)


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

data[ATTR_ENTITY_ID] = entity_id

hass.services.call(DOMAIN, SERVICE_SELECT_SOUND_MODE, data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@rytilahti I made this PR so you can base your Songpal component on it.
This is also needed for the DenonAvr component.
@balloob Do you think this can be merged, or do you see things I need to change?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 1, 2018

Can you add it to the demo media player and make sure it's tested ?

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@balloob I will do that on monday. And also test it on monday.

@fabaff fabaff changed the title Media player: general sound mode support Add general sound mode support Jun 1, 2018


YOUTUBE_COVER_URL_FORMAT = 'https://img.youtube.com/vi/{}/hqdefault.jpg'
SOUND_MODE_LIST = ['Dummy Music','Dummy Movie']
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing whitespace after ','

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@balloob I have updated my HomeAssistant to version 0.70.1 and tested this code.
I can confirm that everything works.
The demo works and it also works with my real Marantz media player (with the aditional code for the denonavr platform).

Can you now merge this PR?

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@balloob could you do a final review of this PR for sound mode support?
And do you know why the cla-bot is stuck?

@balloob balloob merged commit f696331 into home-assistant:dev Jun 7, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 7, 2018

🎉

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
* Media player: general sound mode support

* General sound mode support

* White spaces

* Add sound mode support to demo media player

* white space

* remove unnessesary code
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants