-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
More features for the Bluesound component #11450
Conversation
"""Increases sleep time on player""" | ||
sleep_time = yield from self.send_bluesound_command('/Sleep') | ||
if sleep_time is None: | ||
_LOGGER.error('Error while increasing sleep time on player: %s', self.host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (87 > 79 characters)
def async_remove_slave(self, slave_device): | ||
"""Remove slave to master""" | ||
return self.send_bluesound_command('/RemoveSlave?slave={}&port={}' | ||
.format(slave_device.host, slave_device.port)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (89 > 79 characters)
def async_add_slave(self, slave_device): | ||
"""Add slave to master""" | ||
return self.send_bluesound_command('/AddSlave?slave={}&port={}' | ||
.format(slave_device.host, slave_device.port)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (89 > 79 characters)
@@ -647,6 +778,9 @@ def supported_features(self): | |||
if self._status is None: | |||
return None | |||
|
|||
if self.is_grouped and not self.is_master: | |||
return SUPPORT_VOLUME_STEP | SUPPORT_VOLUME_SET | SUPPORT_VOLUME_MUTE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (81 > 79 characters)
# we will force an update if the player is grouped | ||
# this isn't a foolproof solution. A better solution would be to fetch | ||
# sync_status more often when the device is playing. This would solve | ||
# alot of problems. This change will be done when the communication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (87 > 79 characters)
elif self.is_grouped: | ||
# when player is grouped we need to fetch volume from sync_status. | ||
# we will force an update if the player is grouped | ||
# this isn't a foolproof solution. A better solution would be to fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (90 > 79 characters)
yield from asyncio.sleep(1, loop=self._hass.loop) | ||
yield from self.async_trigger_sync_on_all() | ||
elif self.is_grouped: | ||
# when player is grouped we need to fetch volume from sync_status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (86 > 79 characters)
if group_name != self._group_name: | ||
_LOGGER.debug('Group name change detected on device: %s', self.host) | ||
self._group_name = group_name | ||
# the sleep is needed to make sure that the devices is synced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (81 > 79 characters)
|
||
group_name = self._status.get('groupName', None) | ||
if group_name != self._group_name: | ||
_LOGGER.debug('Group name change detected on device: %s', self.host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (88 > 79 characters)
if master is not None: | ||
self._is_master = False | ||
master_host = master.get('#text') | ||
master_device = [device for device in self._hass.data[DATA_BLUESOUND] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (81 > 79 characters)
Now it's all working. Feels a bit "risky" to rely on 3rd-party API:s to be working for the tests to go through. :( |
if update_tasks: | ||
yield from asyncio.wait(update_tasks, loop=hass.loop) | ||
|
||
descriptions = yield from hass.async_add_job( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do descriptions like this anymore. Please remove.
We fixed the canary issue. Tests should never rely on external APIs. For future PRs, please limit each PR to 1 change. |
Can you resolve the merge conflicts? |
yield from getattr(player, method['method'])(**params) | ||
|
||
for player in target_players: | ||
if player.should_poll: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check this? You know if bluesound entities should be polled or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good question. I can't remember why, I will remove this
sleep = 0 | ||
if self._status is not None: | ||
sleep = self._status.get('sleep', 0) | ||
|
||
return { | ||
ATTR_MODEL: self._model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove model and brand info. That info does not belong in the state machine.
return { | ||
ATTR_MODEL: self._model, | ||
ATTR_MODEL_NAME: self._model_name, | ||
ATTR_BRAND: self._brand, | ||
ATTR_SLEEP: sleep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is sleep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a value that is automatically reduced / increased based on time, it's not allowed in the state attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sleep is a sleep timer. The value is automatically reduced, where am I suppose to place this kind of information? What's the difference from for example 'media_position' ? 'media_position' too is automatically increased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - I haven't changed this yet, even though github seems to think I changed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
media position should not be automatically updated on each update. We store a time once and store a progress_updated_at. The frontend calculates the right value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove sleep from the attributes.
|
||
import aiohttp | ||
from aiohttp.client_exceptions import ClientError | ||
from aiohttp.hdrs import CONNECTION, KEEP_ALIVE | ||
import async_timeout | ||
import voluptuous as vol | ||
|
||
from homeassistant.config import load_yaml_config_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'homeassistant.config.load_yaml_config_file' imported but unused
@@ -8,22 +8,26 @@ | |||
from asyncio.futures import CancelledError | |||
from datetime import timedelta | |||
import logging | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'os' imported but unused
_LOGGER.error('Error while increasing sleep time on player: %s', | ||
self.host) | ||
return 0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line contains whitespace
ATTR_MODEL: self._model, | ||
ATTR_MODEL_NAME: self._model_name, | ||
ATTR_BRAND: self._brand, | ||
ATTR_SLEEP: sleep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove it. We should not have automatic decreasing values in state attributes. Ok values would be a sleep started timestamp and total sleep seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove this
@@ -323,7 +323,7 @@ squeezebox_call_method: | |||
|
|||
yamaha_enable_output: | |||
description: Enable or disable an output port | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this white space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove this, I did it first, but then I noticed that thats how it is in current dev branch and I didn't think it was for me to correct. But I will of course do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🐬 🌮 Awesome! Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a new PR with those fixes.
# communication is moved to a separate library | ||
yield from self.force_update_sync_status() | ||
|
||
self.schedule_update_ha_state() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be self.async_schedule_update_ha_state()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also fix this below in the old code.
Description:
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4340
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
.