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

fix: One media entity for all; minor refactorings; support RestIot in rest_media_entity.py along with favorites + specific sounds from app #121

Closed
wants to merge 4 commits into from
Closed

fix: One media entity for all; minor refactorings; support RestIot in rest_media_entity.py along with favorites + specific sounds from app #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5eb8290
One media entity for all; minor refactorings; support RestIot in rest…
ChrisCarini Mar 25, 2024
7e6b4e2
Merge remote-tracking branch 'origin/main' into ChrisCarini/changes
ChrisCarini Apr 6, 2024
deeae23
bump deps including HA 2024.4.1
ChrisCarini Apr 6, 2024
e3e8f19
Removing commented out test code
ChrisCarini Apr 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "ludeeus/integration_blueprint",
"image": "mcr.microsoft.com/vscode/devcontainers/python:0-3.10-bullseye",
"image": "mcr.microsoft.com/vscode/devcontainers/python:0-3.12-bullseye",
"postCreateCommand": "scripts/setup",
"forwardPorts": [
8123
Expand Down
2 changes: 1 addition & 1 deletion .ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# The contents of this file is based on https://github.com/home-assistant/core/blob/dev/pyproject.toml

target-version = "py310"
target-version = "py312"

select = [
"B007", # Loop control variable {name} not used within loop body
Expand Down
2 changes: 1 addition & 1 deletion custom_components/ha_hatch/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@
"documentation": "https://github.com/dahlb/ha_hatch",
"iot_class": "cloud_push",
"issue_tracker": "https://github.com/dahlb/ha_hatch/issues",
"requirements": ["hatch_rest_api==1.21.0"],
"requirements": ["hatch_rest_api==1.24.0"],
"version": "1.17.1"
}
6 changes: 1 addition & 5 deletions custom_components/ha_hatch/media_player.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
)
from hatch_rest_api import RestIot, RestoreIot
from .rest_media_entity import RestMediaEntity
from .riot_media_entity import RiotMediaEntity

_LOGGER = logging.getLogger(__name__)

Expand All @@ -26,10 +25,7 @@ def choose_media_entity(
CONFIG_TURN_ON_MEDIA, CONFIG_TURN_ON_DEFAULT
)

if isinstance(rest_device, RestIot | RestoreIot):
return RiotMediaEntity(rest_device)
elif not isinstance(rest_device, RestoreIot):
return RestMediaEntity(rest_device, config_turn_on_media)
return RestMediaEntity(rest_device, config_turn_on_media)


async def async_setup_entry(
Expand Down
88 changes: 57 additions & 31 deletions custom_components/ha_hatch/rest_media_entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
RestMini,
RestMiniAudioTrack,
REST_MINI_AUDIO_TRACKS,
RestIot,
RestoreIot,
RIoTAudioTrack,
REST_IOT_AUDIO_TRACKS,
)
from .rest_entity import RestEntity

Expand All @@ -32,30 +36,30 @@ class RestMediaEntity(RestEntity, MediaPlayerEntity):
_attr_media_content_type = MediaType.MUSIC
_attr_device_class = MediaPlayerDeviceClass.SPEAKER

def __init__(self, rest_device: RestMini | RestPlus, config_turn_on_media):
def __init__(self, rest_device: RestIot | RestMini | RestPlus, config_turn_on_media):
super().__init__(rest_device, "Media Player")
self.config_turn_on_media = config_turn_on_media
if isinstance(rest_device, RestMini):
self._attr_sound_mode_list = [x.name for x in REST_MINI_AUDIO_TRACKS[1:]]
self.none_track = RestMiniAudioTrack.NONE
self._attr_supported_features = (

tracks = self._get_tracks()
self.none_track = tracks[0]
self.default_tracks = [x.name for x in tracks[1:]]
self.favorites = self.rest_device.favorite_names()
self._attr_sound_mode_list = self.default_tracks + self.favorites

# Default supported features; RestMini only has these features
self._attr_supported_features = (
MediaPlayerEntityFeature.PAUSE
| MediaPlayerEntityFeature.PLAY
| MediaPlayerEntityFeature.STOP
| MediaPlayerEntityFeature.SELECT_SOUND_MODE
| MediaPlayerEntityFeature.VOLUME_SET
| MediaPlayerEntityFeature.VOLUME_STEP
)
else:
self._attr_sound_mode_list = [x.name for x in REST_PLUS_AUDIO_TRACKS[1:]]
self.none_track = RestPlusAudioTrack.NONE
)

# RestIot and RestPlus have additional features
if isinstance(rest_device, RestIot) or isinstance(rest_device, RestPlus):
self._attr_supported_features = (
MediaPlayerEntityFeature.PAUSE
| MediaPlayerEntityFeature.PLAY
| MediaPlayerEntityFeature.STOP
| MediaPlayerEntityFeature.SELECT_SOUND_MODE
| MediaPlayerEntityFeature.VOLUME_SET
| MediaPlayerEntityFeature.VOLUME_STEP
self._attr_supported_features
| MediaPlayerEntityFeature.TURN_ON
| MediaPlayerEntityFeature.TURN_OFF
)
Expand All @@ -79,36 +83,58 @@ def _update_local_state(self):
def _find_track(self, sound_mode=None):
if sound_mode is None:
sound_mode = self._attr_sound_mode

tracks = self._get_tracks()

return next(
(track for track in tracks if track.name == sound_mode),
None,
)

def _get_tracks(self) -> list[RestMiniAudioTrack] | list[RestPlusAudioTrack] | list[RIoTAudioTrack]:
if isinstance(self.rest_device, RestMini):
return next(
(track for track in REST_MINI_AUDIO_TRACKS if track.name == sound_mode),
None,
)
else:
return next(
(track for track in REST_PLUS_AUDIO_TRACKS if track.name == sound_mode),
Copy link
Owner

Choose a reason for hiding this comment

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

this appears to remove support for gen1 rest plus. why are you merging the gen1 and gen2 entities? As discussed in the api review, history has shown you will not get users to test a gen2 change against their gen1 devices. Please leave gen2 with their riot prefixes like the other gen2 entities.

Copy link
Author

Choose a reason for hiding this comment

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

this appears to remove support for gen1 rest plus. why are you merging the gen1 and gen2 entities?

This change should still support gen1 rest plus - its captured by the implicit else return statement here. I initially merged these two media entities because of the sheer amount of duplicate logic/code between the two of them.

As discussed in the api review, history has shown you will not get users to test a gen2 change against their gen1 devices.

Yup - that was not mentioned prior to me implementing this change (I did both the lib and this change concurrently). 😅

Please leave gen2 with their riot prefixes like the other gen2 entities.

Sure - do you mind if I clean up the code in both of these media entities? The way the current code (that is, the code in main branch, not in this PR) is, is a bit funky, IMO; specifically:

  1. the code in media_player.py: 
        if isinstance(rest_device, RestIot | RestoreIot):
        return RiotMediaEntity(rest_device)
    elif not isinstance(rest_device, RestoreIot):
        return RestMediaEntity(rest_device, config_turn_on_media)
    Why have the elif statement here as opposed to a generic else? The two conditions are a bit confusing, because it says "if restiot or restore iot, create gen2 device, otherwise if not restore iot (aka restiot would possibly be valid, if not already returned), create gen1 device". This is a bit confusing. and IMO should be fixed for correctness (i.e. if restiot or restoreiot, create gen2, else create gen1); something like: 
        if isinstance(rest_device, RestIot | RestoreIot):
        return RiotMediaEntity(rest_device)
    return RestMediaEntity(rest_device, config_turn_on_media)
  2. the base class in rest_entity.py has leakage of RestPlus specific implementation for turning on - if we want to keep gen1 and gen2 separate, then that should be moved into the specific media entity to prevent leakage into the base class.
  3. can we move the methods (or shared code within) that are identical between the two entities into the base class RestEntity, or are you opposed to that entirely? I'm specifically thinking:
    1. The self._attr_supported_features features that are applicable across both gen1 and gen2 devices - only gen1 enetity for restplus would need overriding
    2. set_volume_level() method
    3. media_pause() method
    4. the common code in _update_local_state() method

@dahlb let me know your thoughts please! :)

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I missed the rest plus return before you pointed it out, looks like an extra line break within the function confused the GitHub diff so that the return was listed after another function was listed as removed.

  1. yeah that else is ugly and should be updated, thanks

  2. we can move the turn on method out of the rest entity and into each of it's sub classes, note though you should check references as rest entity is used for more then the media player, one example being the rest light entity.

  3. you could make another parent class for identical methods between rest plus and iot entity, but since rest entity is shared between different kinds of entities, things like supported features don't belong there.

None,
)
return REST_MINI_AUDIO_TRACKS
elif isinstance(self.rest_device, RestIot):
return REST_IOT_AUDIO_TRACKS

def set_volume_level(self, volume):
self.rest_device.set_volume(volume * 100)
return REST_PLUS_AUDIO_TRACKS

def set_volume_level(self, volume: float):
percentage = int(volume * 100)
_LOGGER.debug(f"Setting volume to {percentage}% on {self.rest_device.device_name}")
self.rest_device.set_volume(percentage)

def media_play(self):
_LOGGER.debug(f"Playing audio track [{self.rest_device.audio_track}] on {self.rest_device.device_name}")
self.rest_device.set_audio_track(self.rest_device.audio_track)
if self.config_turn_on_media:
self.turn_on()

def media_pause(self):
_LOGGER.debug(f"Pausing audio track on {self.rest_device.device_name}")
self.rest_device.set_audio_track(self.none_track)

def media_stop(self):
self.rest_device.set_audio_track(self.none_track)
_LOGGER.debug(f"Stopping audio track on {self.rest_device.device_name}")
if isinstance(self.rest_device, RestIot) or isinstance(self.rest_device, RestoreIot):
self.rest_device.turn_off()
else:
self.rest_device.set_audio_track(self.none_track)

def select_sound_mode(self, sound_mode: str):
track = self._find_track(sound_mode=sound_mode)
if track is None:
track = self.none_track
self.rest_device.set_audio_track(track)
_LOGGER.debug(f'Setting sound mode [{sound_mode}] on {self.rest_device.device_name}')

if sound_mode in self.favorites:
_LOGGER.debug(f'Setting favorite [{sound_mode}] on {self.rest_device.device_name}')
self.rest_device.set_favorite(sound_mode)

elif sound_mode in self.default_tracks:
track = self._find_track(sound_mode=sound_mode)
_LOGGER.debug(f'Setting track [{track}] for sound_mode [{sound_mode}] on {self.rest_device.device_name}')
if track is None:
track = self.none_track
self.rest_device.set_audio_track(track)

if self.config_turn_on_media:
self.turn_on()

Expand Down
61 changes: 0 additions & 61 deletions custom_components/ha_hatch/riot_media_entity.py
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
colorlog==6.8.2
homeassistant==2023.5.1
pip>=8.0.3,<24.1
ruff==0.3.4
hatch-rest-api==1.22.0
homeassistant==2024.4.1
pip>=24
ruff>=0.3.5
hatch-rest-api==1.24.0