From 249571269a3346898a4d05120c99b633796649a5 Mon Sep 17 00:00:00 2001 From: Marcel van der Veldt Date: Sat, 16 Aug 2025 02:05:00 +0200 Subject: [PATCH] Some follow-up fixes (esp. for groups) after the Player refactor (#2335) --- music_assistant/controllers/config.py | 58 +++---- music_assistant/controllers/players.py | 152 +++++++++--------- music_assistant/providers/airplay/provider.py | 7 +- .../providers/musiccast/provider.py | 13 +- .../providers/snapcast/provider.py | 8 +- music_assistant/providers/sonos/provider.py | 7 +- .../providers/sonos_s1/provider.py | 7 +- .../providers/squeezelite/provider.py | 7 +- 8 files changed, 146 insertions(+), 113 deletions(-) diff --git a/music_assistant/controllers/config.py b/music_assistant/controllers/config.py index cb540592..250f3c8d 100644 --- a/music_assistant/controllers/config.py +++ b/music_assistant/controllers/config.py @@ -5,7 +5,6 @@ from __future__ import annotations import base64 import logging import os -from contextlib import suppress from typing import TYPE_CHECKING, Any from uuid import uuid4 @@ -23,11 +22,10 @@ from music_assistant_models.config_entries import ( ProviderConfig, ) from music_assistant_models.dsp import DSPConfig, DSPConfigPreset, ToneControlFilter -from music_assistant_models.enums import EventType, ProviderFeature, ProviderType +from music_assistant_models.enums import EventType, ProviderType from music_assistant_models.errors import ( ActionUnavailable, InvalidDataError, - PlayerCommandFailed, UnsupportedFeaturedException, ) from music_assistant_models.helpers import get_global_cache_value @@ -431,33 +429,20 @@ class ConfigController: """Remove PlayerConfig.""" conf_key = f"{CONF_PLAYERS}/{player_id}" dsp_conf_key = f"{CONF_PLAYER_DSP}/{player_id}" - existing = self.get(conf_key) - if not existing: + player_config = self.get(conf_key) + if not player_config: msg = f"Player configuration for {player_id} does not exist" raise KeyError(msg) - player = self.mass.players.get(player_id) - player_provider = player.provider - if player_provider and ProviderFeature.REMOVE_PLAYER in player_provider.supported_features: - # provider supports removal of player (e.g. group player) - await player_provider.remove_player(player_id) - elif player and player_provider and player.available: - # removing a player config while it is active is not allowed - # unless the provider reports it has the remove_player feature (e.g. group player) - raise ActionUnavailable("Can not remove config for an active player!") - # check for group memberships that need to be updated - if ( - player - and player.active_group - and (group_player := self.mass.players.get(player.active_group)) - ): - # try to remove from the group - with suppress(UnsupportedFeaturedException, PlayerCommandFailed): - await group_player.set_members( - player_ids_to_remove=[player_id], - ) - # tell the player manager to remove the player if its lingering around - # set cleanup_flag to false otherwise we end up in an infinite loop - self.mass.players.remove(player_id, cleanup_config=False) + if self.mass.players.get(player_id): + try: + await self.mass.players.remove(player_id) + except UnsupportedFeaturedException: + # removing a player config while it is active is not allowed + # unless the provider reports it has the remove_player feature + raise ActionUnavailable("Can not remove config for an active player!") + # tell the player manager to remove the player if its lingering around + # set permanent to false otherwise we end up in an infinite loop + self.mass.players.unregister(player_id, permanent=False) # remove the actual config if all of the above passed self.remove(conf_key) # Also remove the DSP config if it exists @@ -607,6 +592,7 @@ class ConfigController: provider=provider, player_id=player_id, enabled=enabled, + name=name, default_name=name, ) default_conf_raw = default_conf.to_raw() @@ -856,7 +842,7 @@ class ConfigController: LOGGER.exception("Error while reading persistent storage file %s", filename) LOGGER.debug("Started with empty storage: No persistent storage file found.") - async def _migrate(self) -> None: + async def _migrate(self) -> None: # noqa: PLR0915 changed = False # some type hints to help with the code below @@ -909,6 +895,7 @@ class ConfigController: break # migrate player_group entries + ugp_found = False for player_config in self._data.get(CONF_PLAYERS, {}).values(): if not player_config.get("provider").startswith("player_group"): continue @@ -920,8 +907,21 @@ class ConfigController: changed = True if group_type == "universal": player_config["provider"] = "universal_group" + ugp_found = True else: player_config["provider"] = group_type + for provider_config in list(self._data.get(CONF_PROVIDERS, {}).values()): + instance_id = provider_config["instance_id"] + if not instance_id.startswith("player_group"): + continue + # this is the legacy player_group provider, migrate into 'universal_group' + changed = True + self._data[CONF_PROVIDERS].pop(instance_id, None) + if not ugp_found: + continue + provider_config["domain"] = "universal_group" + provider_config["instance_id"] = "universal_group" + self._data[CONF_PROVIDERS]["universal_group"] = provider_config if changed: await self._async_save() diff --git a/music_assistant/controllers/players.py b/music_assistant/controllers/players.py index efe09ebd..91301a00 100644 --- a/music_assistant/controllers/players.py +++ b/music_assistant/controllers/players.py @@ -44,7 +44,6 @@ from music_assistant_models.errors import ( ProviderUnavailableError, UnsupportedFeaturedException, ) -from music_assistant_models.media_items import UniqueList from music_assistant_models.player_control import PlayerControl # noqa: TC002 from music_assistant.constants import ( @@ -62,6 +61,8 @@ from music_assistant.constants import ( CONF_ENTRY_ANNOUNCE_VOLUME_MAX, CONF_ENTRY_ANNOUNCE_VOLUME_MIN, CONF_ENTRY_ANNOUNCE_VOLUME_STRATEGY, + CONF_PLAYER_DSP, + CONF_PLAYERS, CONF_TTS_PRE_ANNOUNCE, ) from music_assistant.helpers.api import api_command @@ -898,37 +899,23 @@ class PlayerController(CoreController): async with self._player_throttlers[player_id]: await player.enqueue_next_media(media) - @api_command("players/cmd/group") - @handle_player_command - async def cmd_group(self, player_id: str, target_player: str) -> None: - """Handle GROUP command for given player. - - Join/add the given player(id) to the given (leader) player/sync group. - If the target player itself is already synced to another player, this may fail. - If the player can not be synced with the given target player, this may fail. - - :param player_id: player_id of the player to handle the command. - :param target_player: player_id of the syncgroup leader or group player. - - :raises UnsupportedFeaturedException: if the target player does not support grouping. - :raises PlayerCommandFailed: if the target player is already synced to another player. - :raises PlayerUnavailableError: if the target player is not available. - :raises PlayerCommandFailed: if the player is already grouped to another player. - """ - await self.cmd_group_many(target_player, [player_id]) - - @api_command("players/cmd/group_many") - async def cmd_group_many(self, target_player: str, child_player_ids: list[str]) -> None: + @api_command("players/cmd/set_members") + async def cmd_set_members( + self, + target_player: str, + player_ids_to_add: list[str] | None = None, + player_ids_to_remove: list[str] | None = None, + ) -> None: """ - Join given player(s) to target player. + Join/unjoin given player(s) to/from target player. Will add the given player(s) to the target player (sync leader or group player). :param target_player: player_id of the syncgroup leader or group player. - :param child_player_ids: list of player_ids to add to the target player. + :param player_ids_to_add: List of player_id's to add to the target player. + :param player_ids_to_remove: List of player_id's to remove from the target player. :raises UnsupportedFeaturedException: if the target player does not support grouping. - :raises PlayerCommandFailed: if the target player is already synced to another player. :raises PlayerUnavailableError: if the target player is not available. """ parent_player: Player | None = self.get(target_player, True) @@ -945,10 +932,12 @@ class PlayerController(CoreController): ) # filter all player ids on compatibility and availability - final_player_ids: UniqueList[str] = UniqueList() - for child_player_id in child_player_ids: + final_player_ids_to_add: list[str] = [] + for child_player_id in player_ids_to_add or []: if child_player_id == target_player: continue + if child_player_id in final_player_ids_to_add: + continue if not (child_player := self.get(child_player_id)) or not child_player.available: self.logger.warning("Player %s is not available", child_player_id) continue @@ -970,33 +959,47 @@ class PlayerController(CoreController): ): continue # already synced to this target - # perform some sanity checks on the child player - # if we're not joining a group player - if ( - parent_player.type == PlayerType.PLAYER - and child_player.group_members - and child_player.playback_state != PlaybackState.IDLE - ): - # guard edge case: childplayer is already a sync leader on its own - raise PlayerCommandFailed( - f"Player {child_player.name} is already synced with other players, " - "you need to ungroup it first before you can join it to another player.", - ) - # power on the player if needed if not child_player.powered and child_player.power_control != PLAYER_CONTROL_NONE: await self.cmd_power(child_player.player_id, True, skip_update=True) # if we reach here, all checks passed - final_player_ids.append(child_player_id) + final_player_ids_to_add.append(child_player_id) # forward command to the player after all (base) sanity checks async with self._player_throttlers[target_player]: await parent_player.set_members( - player_ids_to_add=[ - x for x in final_player_ids if x not in parent_player.group_members - ] + player_ids_to_add=final_player_ids_to_add, player_ids_to_remove=player_ids_to_remove ) + @api_command("players/cmd/group") + @handle_player_command + async def cmd_group(self, player_id: str, target_player: str) -> None: + """Handle GROUP command for given player. + + Join/add the given player(id) to the given (leader) player/sync group. + If the target player itself is already synced to another player, this may fail. + If the player can not be synced with the given target player, this may fail. + + :param player_id: player_id of the player to handle the command. + :param target_player: player_id of the syncgroup leader or group player. + + :raises UnsupportedFeaturedException: if the target player does not support grouping. + :raises PlayerCommandFailed: if the target player is already synced to another player. + :raises PlayerUnavailableError: if the target player is not available. + :raises PlayerCommandFailed: if the player is already grouped to another player. + """ + await self.cmd_set_members(target_player, player_ids_to_add=[player_id]) + + @api_command("players/cmd/group_many") + async def cmd_group_many(self, target_player: str, child_player_ids: list[str]) -> None: + """ + Join given player(s) to target player. + + Will add the given player(s) to the target player (sync leader or group player). + NOTE: This is a (deprecated) alias for cmd_set_members. + """ + await self.cmd_set_members(target_player, player_ids_to_add=child_player_ids) + @api_command("players/cmd/ungroup") @handle_player_command async def cmd_ungroup(self, player_id: str) -> None: @@ -1006,12 +1009,17 @@ class PlayerController(CoreController): If the player is not currently grouped to any other player, this will silently be ignored. - - player_id: player_id of the player to handle the command. + NOTE: This is a (deprecated) alias for cmd_set_members. """ if not (player := self.get(player_id)): self.logger.warning("Player %s is not available", player_id) return + if player.synced_to and (synced_player := self.get(player.synced_to)): + # player is a sync member + await synced_player.set_members(player_ids_to_remove=[player_id]) + return + if ( player.active_group and (group_player := self.get(player.active_group)) @@ -1028,23 +1036,6 @@ class PlayerController(CoreController): self.logger.warning("Player %s does not support (un)group commands", player.name) return - # handle (edge)case where un ungroup command is sent to a sync leader; - # we dissolve the entire syncgroup in this case. - # while maybe not strictly needed to do this for all player providers, - # we do this to keep the functionality consistent across all providers - if player.group_members: - self.logger.warning( - "Detected ungroup command to player %s which is a sync(group) leader, " - "all sync members will be ungrouped!", - player.name, - ) - async with TaskManager(self.mass) as tg: - for group_child_id in player.group_members: - if group_child_id == player_id: - continue - tg.create_task(self.cmd_ungroup(group_child_id)) - return - # forward command to the player once all checks passed await player.ungroup() @@ -1081,15 +1072,15 @@ class PlayerController(CoreController): :param player_id: ID of the group player to remove. """ if not (player := self.get(player_id)): - raise PlayerUnavailableError(f"Player {player_id} not found") + # we simply permanently delete the player by wiping its config + self.mass.config.remove(f"players/{player_id}") + return if player.type != PlayerType.GROUP: raise UnsupportedFeaturedException( f"Player {player.display_name} is not a group player" ) - provider = self.mass.get_provider(player.provider.instance_id) - provider.check_feature(ProviderFeature.REMOVE_GROUP_PLAYER) - provider = cast("PlayerProvider", provider) - await provider.remove_group_player(player_id) + player.provider.check_feature(ProviderFeature.REMOVE_GROUP_PLAYER) + await player.provider.remove_group_player(player_id) @api_command("players/add_currently_playing_to_favorites") async def add_currently_playing_to_favorites(self, player_id: str) -> None: @@ -1252,19 +1243,34 @@ class PlayerController(CoreController): self.mass.config.remove(f"players/{player_id}") self.mass.signal_event(EventType.PLAYER_REMOVED, player_id) + @api_command("players/remove") async def remove(self, player_id: str) -> None: """ Remove a player from a provider. Can only be called when a PlayerProvider supports ProviderFeature.REMOVE_PLAYER. """ - player = self.get(player_id, True) - assert player is not None # for type checker - if ProviderFeature.REMOVE_PLAYER not in player.provider.supported_features: - raise UnsupportedFeaturedException( - f"Provider {player.provider.name} does not support removing players" - ) + player = self.get(player_id) + if player is None: + # we simply permanently delete the player by wiping its config + conf_key = f"{CONF_PLAYERS}/{player_id}" + dsp_conf_key = f"{CONF_PLAYER_DSP}/{player_id}" + for key in (conf_key, dsp_conf_key): + self.mass.config.remove(key) + return + if player.type == PlayerType.GROUP: + # Handle group player removal + await player.provider.remove_group_player(player_id) + return + player.provider.check_feature(ProviderFeature.REMOVE_PLAYER) await player.provider.remove_player(player_id) + # check for group memberships that need to be updated + if player.active_group and (group_player := self.mass.players.get(player.active_group)): + # try to remove from the group + with suppress(UnsupportedFeaturedException, PlayerCommandFailed): + await group_player.set_members( + player_ids_to_remove=[player_id], + ) def signal_player_state_update( self, diff --git a/music_assistant/providers/airplay/provider.py b/music_assistant/providers/airplay/provider.py index 5b5a1864..176a3674 100644 --- a/music_assistant/providers/airplay/provider.py +++ b/music_assistant/providers/airplay/provider.py @@ -43,7 +43,12 @@ class AirPlayProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS} + return { + ProviderFeature.SYNC_PLAYERS, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" diff --git a/music_assistant/providers/musiccast/provider.py b/music_assistant/providers/musiccast/provider.py index ccf9fe8b..05bb0c40 100644 --- a/music_assistant/providers/musiccast/provider.py +++ b/music_assistant/providers/musiccast/provider.py @@ -23,11 +23,7 @@ from music_assistant.providers.musiccast.constants import ( ) from music_assistant.providers.sonos.helpers import get_primary_ip_address -from .musiccast import ( - MusicCastController, - MusicCastPhysicalDevice, - MusicCastZoneDevice, -) +from .musiccast import MusicCastController, MusicCastPhysicalDevice, MusicCastZoneDevice from .player import MusicCastPlayer, UpnpUpdateHelper @@ -103,7 +99,12 @@ class MusicCastProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS} + return { + ProviderFeature.SYNC_PLAYERS, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Async init.""" diff --git a/music_assistant/providers/snapcast/provider.py b/music_assistant/providers/snapcast/provider.py index 736b5ce9..05e08b53 100644 --- a/music_assistant/providers/snapcast/provider.py +++ b/music_assistant/providers/snapcast/provider.py @@ -50,7 +50,13 @@ class SnapCastProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS, ProviderFeature.REMOVE_PLAYER} + return { + ProviderFeature.SYNC_PLAYERS, + ProviderFeature.REMOVE_PLAYER, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" diff --git a/music_assistant/providers/sonos/provider.py b/music_assistant/providers/sonos/provider.py index 70a6833b..d3ada68a 100644 --- a/music_assistant/providers/sonos/provider.py +++ b/music_assistant/providers/sonos/provider.py @@ -38,7 +38,12 @@ class SonosPlayerProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS} + return { + ProviderFeature.SYNC_PLAYERS, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" diff --git a/music_assistant/providers/sonos_s1/provider.py b/music_assistant/providers/sonos_s1/provider.py index 56768149..bf687bbd 100644 --- a/music_assistant/providers/sonos_s1/provider.py +++ b/music_assistant/providers/sonos_s1/provider.py @@ -39,7 +39,12 @@ class SonosPlayerProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS} + return { + ProviderFeature.SYNC_PLAYERS, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" diff --git a/music_assistant/providers/squeezelite/provider.py b/music_assistant/providers/squeezelite/provider.py index 8f844d9c..f5afd6b8 100644 --- a/music_assistant/providers/squeezelite/provider.py +++ b/music_assistant/providers/squeezelite/provider.py @@ -45,7 +45,12 @@ class SqueezelitePlayerProvider(PlayerProvider): @property def supported_features(self) -> set[ProviderFeature]: """Return the features supported by this Provider.""" - return {ProviderFeature.SYNC_PLAYERS} + return { + ProviderFeature.SYNC_PLAYERS, + # support sync groups by reporting create/remove player group support + ProviderFeature.CREATE_GROUP_PLAYER, + ProviderFeature.REMOVE_GROUP_PLAYER, + } async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" -- 2.34.1