Skip to content

Added play media to squeezebox - #3306

Merged
balloob merged 4 commits into
home-assistant:devfrom
sytone:squeezebox-playmedia
Sep 23, 2016
Merged

Added play media to squeezebox#3306
balloob merged 4 commits into
home-assistant:devfrom
sytone:squeezebox-playmedia

Conversation

@sytone

@sytone sytone commented Sep 10, 2016

Copy link
Copy Markdown
Contributor

Description:
Updated the squeezebox component to handle play commands.

  • You can now specify a URI in the play and it will play it immediately.
  • You can specify enqueuer and it adds it to the end of the current play list.

Related issue (if applicable): NA

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
NA - adding support for existing features, no change in documentation.

Example entry for configuration.yaml (if applicable):
NA - No configuration changes.

Checklist:

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

If code communicates with devices, web services, or a:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

The squeezebox component can now add a URI to an existing playlist or just over write it to force a stream to play.

The squeezebox component can now add a URI to an existing playlist or just over write it to force a stream to play.
Spacing... The end of the world! Fixed. Once day the tools will fix this on the fly, one day...

[x] ./homeassistant/components/media_player/squeezebox.py:307:1: W293 blank line contains whitespace
[x] ./homeassistant/components/media_player/squeezebox.py:366:1: W391 blank line at end of file
[x] ./homeassistant/components/media_player/squeezebox.py:366:1: W293 blank line contains whitespace

Updated SUPPORT_SQUEEZEBOX to add SUPPORT_PLAY_MEDIA

[x] ./homeassistant/components/media_player/squeezebox.py:13:1: F401 'homeassistant.components.media_player.SUPPORT_PLAY_MEDIA' imported but unused

<playerid> playlist play <item> <title> <fadeInSecs>

The "playlist play" command puts the specified song URL,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please move some of this documentation to a PR against homeassistant.io? Lets try to keep this short and to the point

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This documentation is not for the user, it is for any future developer. I has always tried to add as much information in the code so future developers can work better with it. Is this not a stance for Home Assistant?

Updated the comments to indicate they are developer / API comments and not for end users.
Marked the private functions with a leading underscore (_)
202ERROR: InvocationError: '/home/travis/build/home-assistant/home-assistant/.tox/lint/bin/flake8'

203lint runtests: commands[1] | pylint homeassistant

204************* Module homeassistant.components.media_player.squeezebox

205C:322, 0: Trailing whitespace (trailing-whitespace)
@kellerza

Copy link
Copy Markdown
Member

Let's try to keep comments as short as possible, only the core HASS methods are documented in detail. How play and enque work fits well in user documentation. You can keep the telnet command sructures showing additional features (Like fadeinsec). Examples and returns feels a bit redundant.

"Overloading" methods really depends on the component and services it exposes (see here for the play service)). Not all media players will support all functions and we have to try model them in a way (part of the reason I asked to make new functions private/even remove them). You are welcome to suggest enhancements to the media player model, but preferably in a separate PR.

@balloob
balloob merged commit d5912f4 into home-assistant:dev Sep 23, 2016
@Phigital-Inc

Copy link
Copy Markdown

Hi, to support media_content_type: playlist, I would recommend not just using the "playlist play" CLI command but rather then "playlistcontrol" CLI command -- the former does not support playing imported iTunes playlists nor specifying "playlist_name". Ideally, the "playlistcontrol" command would support URIs, but unfortunately it does not...

@Phigital-Inc

Copy link
Copy Markdown

CORRECTION: Turns out the "playlistcontrol" does support URIs (albeit a little slowly since it has to load the playlist), so it could replace "playlist play" as the default

@home-assistant home-assistant locked and limited conversation to collaborators Apr 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants