From: Maxim Raznatovski Date: Sat, 27 Sep 2025 01:12:20 +0000 (+0200) Subject: More grouping fixes after the player refactor (#2429) X-Git-Url: https://git.kitaultman.com/?a=commitdiff_plain;h=901f3b03caa3d3b2ebc8c4a320806d37d7163ad3;p=music-assistant-server.git More grouping fixes after the player refactor (#2429) --- diff --git a/music_assistant/controllers/players.py b/music_assistant/controllers/players.py index b9c7fded..f81b91d9 100644 --- a/music_assistant/controllers/players.py +++ b/music_assistant/controllers/players.py @@ -1010,16 +1010,16 @@ class PlayerController(CoreController): child_player.name, child_player.active_group, ) - try: - await other_group.set_members(player_ids_to_remove=[child_player.player_id]) - except UnsupportedFeaturedException as err: + if child_player.player_id in other_group.static_group_members: self.logger.warning( - "Failed to remove player %s from group %s: %s, powering it off instead", + "Player %s is a static member of group %s: removing is not possible, " + "powering the group off instead", child_player.name, child_player.active_group, - err, ) await self.cmd_power(child_player.active_group, False) + else: + await other_group.set_members(player_ids_to_remove=[child_player.player_id]) else: self.logger.warning( "Player %s is already part of another group (%s), powering it off first", @@ -1040,10 +1040,28 @@ class PlayerController(CoreController): # if we reach here, all checks passed final_player_ids_to_add.append(child_player_id) + final_player_ids_to_remove: list[str] = [] + if player_ids_to_remove: + static_members = set(parent_player.static_group_members) + for child_player_id in player_ids_to_remove: + if child_player_id == target_player: + raise UnsupportedFeaturedException( + f"Cannot remove {parent_player.name} from itself as a member!" + ) + if child_player_id not in parent_player.group_members: + continue + if child_player_id in static_members: + raise UnsupportedFeaturedException( + f"Cannot remove {child_player_id} from {parent_player.name} " + "as it is a static member of this group" + ) + final_player_ids_to_remove.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=final_player_ids_to_add, player_ids_to_remove=player_ids_to_remove + player_ids_to_add=final_player_ids_to_add or None, + player_ids_to_remove=final_player_ids_to_remove or None, ) @api_command("players/cmd/group") @@ -1090,20 +1108,25 @@ class PlayerController(CoreController): 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)) and (PlayerFeature.SET_MEMBERS in group_player.supported_features) ): # the player is part of a (permanent) groupplayer and the user tries to ungroup + if player_id in group_player.static_group_members: + raise UnsupportedFeaturedException( + f"Player {player.name} is a static member of group {group_player.name} " + "and cannot be removed from that group!" + ) await group_player.set_members(player_ids_to_remove=[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 not (player.synced_to or player.group_members): return # nothing to do @@ -1259,8 +1282,8 @@ class PlayerController(CoreController): # ensure we fetch and set the latest/full config for the player player_config = await self.mass.config.get_player_config(player_id) player.set_config(player_config) - # call on_registered hook after the player is registered and config is set - await player.on_registered() + # call hook after the player is registered and config is set + await player.on_config_updated() # always call update to fix special attributes like display name, group volume etc. player.update_state() @@ -1312,9 +1335,11 @@ class PlayerController(CoreController): If the player is not registered, this will silently be ignored. """ - player = self._players.pop(player_id, None) + player = self._players.get(player_id) if player is None: return + await self._cleanup_player_memberships(player_id) + del self._players[player_id] self.logger.info("Player removed: %s", player.name) self.mass.player_queues.on_player_remove(player_id, permanent=permanent) await player.on_unload() @@ -1400,49 +1425,8 @@ class PlayerController(CoreController): # handle DSP reload of the leader when grouping/ungrouping if ATTR_GROUP_MEMBERS in changed_values: - new_group_members: list[str] = changed_values[ATTR_GROUP_MEMBERS][1] - prev_group_members: list[str] = changed_values[ATTR_GROUP_MEMBERS][0] or [] - prev_child_count = len(prev_group_members) - new_child_count = len(new_group_members) - is_player_group = player.type == PlayerType.GROUP - - # handle special case for PlayerGroups: since there are no leaders, - # DSP still always work with a single player in the group. - multi_device_dsp_threshold = 1 if is_player_group else 0 - - prev_is_multiple_devices = prev_child_count > multi_device_dsp_threshold - new_is_multiple_devices = new_child_count > multi_device_dsp_threshold - - if prev_is_multiple_devices != new_is_multiple_devices: - supports_multi_device_dsp = ( - PlayerFeature.MULTI_DEVICE_DSP in player.supported_features - ) - dsp_enabled: bool - if player.type == PlayerType.GROUP: - # Since player groups do not have leaders, we will use the only child - # that was in the group before and after the change - if prev_is_multiple_devices: - if childs := new_group_members: - # We shrank the group from multiple players to a single player - # So the now only child will control the DSP - dsp_enabled = self.mass.config.get_player_dsp_config(childs[0]).enabled - else: - dsp_enabled = False - elif childs := prev_group_members: - # We grew the group from a single player to multiple players, - # let's see if the previous single player had DSP enabled - dsp_enabled = self.mass.config.get_player_dsp_config(childs[0]).enabled - else: - dsp_enabled = False - else: - dsp_enabled = self.mass.config.get_player_dsp_config(player_id).enabled - if dsp_enabled and not supports_multi_device_dsp: - # We now know that that the group configuration has changed so: - # - multi-device DSP is not supported - # - we switched from a group with multiple players to a single player - # (or vice versa) - # - the leader has DSP enabled - self.mass.create_task(self.mass.players.on_player_dsp_change(player_id)) + prev_group_members, new_group_members = changed_values[ATTR_GROUP_MEMBERS] + self._handle_group_dsp_change(player, prev_group_members or [], new_group_members) if ATTR_GROUP_MEMBERS in changed_values: # Removed group members also need to be updated since they are no longer part @@ -1454,6 +1438,14 @@ class PlayerController(CoreController): if removed_player := self.get(player_id): removed_player.update_state() + became_inactive = False + if "available" in changed_values: + became_inactive = changed_values["available"][1] is False + if not became_inactive and "enabled" in changed_values: + became_inactive = changed_values["enabled"][1] is False + if became_inactive and (player.active_group or player.synced_to): + self.mass.create_task(self._cleanup_player_memberships(player.player_id)) + # signal player update on the eventbus self.mass.signal_event(EventType.PLAYER_UPDATED, object_id=player_id, data=player) @@ -1690,6 +1682,7 @@ class PlayerController(CoreController): if not (player := self.get(config.player_id)): return # guard against player not being registered (yet) player.set_config(config) + await player.on_config_updated() player.update_state() resume_queue: PlayerQueue | None = ( self.mass.player_queues.get(player.active_source) if player.active_source else None @@ -1707,13 +1700,6 @@ class PlayerController(CoreController): # always stop first to ensure the player uses the new config await self.mass.player_queues.stop(resume_queue.queue_id) self.mass.call_later(1, self.mass.player_queues.resume, resume_queue.queue_id, False) - # check for group memberships that need to be updated - if player_disabled and player.active_group and player_provider: - # try to remove from the group - group_player = self.get(player.active_group) - assert group_player is not None # for type checking - with suppress(UnsupportedFeaturedException, PlayerCommandFailed): - await group_player.set_members(player_ids_to_remove=[player.player_id]) async def on_player_dsp_change(self, player_id: str) -> None: """Call (by config manager) when the DSP settings of a player change.""" @@ -1730,6 +1716,26 @@ class PlayerController(CoreController): await self.cmd_stop(player_id) await self.cmd_play(player_id) + async def _cleanup_player_memberships(self, player_id: str) -> None: + """Ensure a player is detached from any groups or syncgroups.""" + if not (player := self.get(player_id)): + return + + if ( + player.active_group + and (group := self.get(player.active_group)) + and group.supports_feature(PlayerFeature.SET_MEMBERS) + ): + # Ungroup the player if its part of an active group, this will ignore + # static_group_members since that is only checked when using cmd_set_members + with suppress(UnsupportedFeaturedException, PlayerCommandFailed): + await group.set_members(player_ids_to_remove=[player_id]) + elif player.synced_to and player.supports_feature(PlayerFeature.SET_MEMBERS): + # Remove the player if it was synced, otherwise it will still show as + # synced to the other player after it gets registered again + with suppress(UnsupportedFeaturedException, PlayerCommandFailed): + await player.ungroup() + def _get_player_with_redirect(self, player_id: str) -> Player: """Get player with check if playback related command should be redirected.""" player = self.get(player_id, True) @@ -2001,6 +2007,54 @@ class PlayerController(CoreController): # trigger player update to ensure the source is set self.trigger_player_update(player.player_id) + def _handle_group_dsp_change( + self, player: Player, prev_group_members: list[str], new_group_members: list[str] + ) -> None: + """Handle DSP reload when group membership changes.""" + prev_child_count = len(prev_group_members) + new_child_count = len(new_group_members) + is_player_group = player.type == PlayerType.GROUP + + # handle special case for PlayerGroups: since there are no leaders, + # DSP still always work with a single player in the group. + multi_device_dsp_threshold = 1 if is_player_group else 0 + + prev_is_multiple_devices = prev_child_count > multi_device_dsp_threshold + new_is_multiple_devices = new_child_count > multi_device_dsp_threshold + + if prev_is_multiple_devices == new_is_multiple_devices: + return # no change in multi-device status + + supports_multi_device_dsp = PlayerFeature.MULTI_DEVICE_DSP in player.supported_features + + dsp_enabled: bool + if player.type == PlayerType.GROUP: + # Since player groups do not have leaders, we will use the only child + # that was in the group before and after the change + if prev_is_multiple_devices: + if childs := new_group_members: + # We shrank the group from multiple players to a single player + # So the now only child will control the DSP + dsp_enabled = self.mass.config.get_player_dsp_config(childs[0]).enabled + else: + dsp_enabled = False + elif childs := prev_group_members: + # We grew the group from a single player to multiple players, + # let's see if the previous single player had DSP enabled + dsp_enabled = self.mass.config.get_player_dsp_config(childs[0]).enabled + else: + dsp_enabled = False + else: + dsp_enabled = self.mass.config.get_player_dsp_config(player.player_id).enabled + + if dsp_enabled and not supports_multi_device_dsp: + # We now know that the group configuration has changed so: + # - multi-device DSP is not supported + # - we switched from a group with multiple players to a single player + # (or vice versa) + # - the leader has DSP enabled + self.mass.create_task(self.mass.players.on_player_dsp_change(player.player_id)) + def __iter__(self) -> Iterator[Player]: """Iterate over all players.""" return iter(self._players.values()) diff --git a/music_assistant/models/player.py b/music_assistant/models/player.py index fee599cc..8f560d5e 100644 --- a/music_assistant/models/player.py +++ b/music_assistant/models/player.py @@ -132,6 +132,7 @@ class Player(ABC): _attr_type: PlayerType = PlayerType.PLAYER _attr_supported_features: set[PlayerFeature] _attr_group_members: list[str] + _attr_static_group_members: list[str] _attr_device_info: DeviceInfo _attr_can_group_with: set[str] _attr_source_list: list[PlayerSource] @@ -159,6 +160,7 @@ class Player(ABC): # initialize mutable attributes self._attr_supported_features = set() self._attr_group_members = [] + self._attr_static_group_members = [] self._attr_device_info = DeviceInfo() self._attr_can_group_with = set() self._attr_source_list = [] @@ -317,6 +319,17 @@ class Player(ABC): return [] return self._attr_group_members + @property + def static_group_members(self) -> list[str]: + """ + Return the static group members for a player group. + + For PlayerType.GROUP return the player_ids of members that must not be removed by + the user. + For all other player types return an empty list. + """ + return self._attr_static_group_members + @property def can_group_with(self) -> set[str]: """ @@ -606,13 +619,13 @@ class Player(ABC): ), ] - async def on_registered(self) -> None: + async def on_config_updated(self) -> None: """ - Handle logic when the player is registered and config is set. + Handle logic when the player is loaded or updated. Override this method in your player implementation if you need to perform any additional setup logic after the player is registered and - the self.config was loaded. + the self.config was loaded, and whenever the config changes. """ return @@ -1233,6 +1246,7 @@ class Player(ABC): self._state.volume_level = self.volume_state self._state.volume_muted = self.volume_muted_state self._state.group_members = UniqueList(self.group_members) + self._state.static_group_members = UniqueList(self.static_group_members) self._state.can_group_with = self.can_group_with self._state.synced_to = self.synced_to self._state.active_source = self.active_source_state @@ -1399,16 +1413,17 @@ class SyncGroupPlayer(GroupPlayer): PlayerFeature.VOLUME_SET, } - async def on_registered(self) -> None: - """Complete the initialization once the player was registered.""" + async def on_config_updated(self) -> None: + """Handle logic when the player is loaded or updated.""" # Config is only available after the player was registered - # Copy the list so not every added player becomes a static member - self._attr_group_members = list( - cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) - ) - # Uses self.config + static_members = cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) + self._attr_static_group_members = static_members.copy() + if not self.powered: + self._attr_group_members = static_members.copy() if self.is_dynamic: self._attr_supported_features.add(PlayerFeature.SET_MEMBERS) + else: + self._attr_supported_features.discard(PlayerFeature.SET_MEMBERS) @property def supported_features(self) -> set[PlayerFeature]: @@ -1538,10 +1553,12 @@ class SyncGroupPlayer(GroupPlayer): # collision: child player is part another group that is already active ! # solve this by trying to leave the group first if other_group := self.mass.players.get(group): - try: - other_group.check_feature(PlayerFeature.SET_MEMBERS) + if ( + other_group.supports_feature(PlayerFeature.SET_MEMBERS) + and member.player_id not in other_group.static_group_members + ): await other_group.set_members(player_ids_to_remove=[member.player_id]) - except UnsupportedFeaturedException: + else: # if the other group does not support SET_MEMBERS or it is a static # member, we need to power it off to leave the group await other_group.power(False) @@ -1567,6 +1584,15 @@ class SyncGroupPlayer(GroupPlayer): self.update_state() if powered: + # reset the group members to the available static members when powering on + self._attr_group_members = [] + for static_group_member in self._attr_static_group_members: + if ( + (member_player := self.mass.players.get(static_group_member)) + and member_player.available + and member_player.enabled + ): + self._attr_group_members.append(static_group_member) # Select sync leader and handle turn on new_leader = self._select_sync_leader() # handle TURN_ON of the group player by turning on all members @@ -1589,10 +1615,10 @@ class SyncGroupPlayer(GroupPlayer): await member.power(False) if not powered: - # reset the original group members when powered off and clear leader - self._attr_group_members = list( - cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) - ) + # Reset to unfiltered static members list when powered off + # (the frontend will hide unavailable members) + self._attr_group_members = self._attr_static_group_members.copy() + # and clear the sync leader self.sync_leader = None async def _dissolve_syncgroup(self) -> None: @@ -1675,15 +1701,9 @@ class SyncGroupPlayer(GroupPlayer): final_players_to_add.append(player_id) # handle removals final_players_to_remove: list[str] = [] - static_members = cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) for player_id in player_ids_to_remove or []: if player_id not in self._attr_group_members: continue - if player_id in static_members: - raise UnsupportedFeaturedException( - f"Cannot remove {player_id} from {self.display_name} " - "as it is a static member of this group" - ) if player_id == self.player_id: raise UnsupportedFeaturedException( f"Cannot remove {self.display_name} from itself as a member!" diff --git a/music_assistant/providers/_demo_player_provider/player.py b/music_assistant/providers/_demo_player_provider/player.py index df050f71..788b1006 100644 --- a/music_assistant/providers/_demo_player_provider/player.py +++ b/music_assistant/providers/_demo_player_provider/player.py @@ -31,12 +31,13 @@ class DemoPlayer(Player): } self._set_attributes() - async def on_registered(self) -> None: - """Complete the initialization once the player was registered.""" + async def on_config_updated(self) -> None: + """Handle logic when the player is loaded or updated.""" # OPTIONAL # This method is optional and should be implemented if you need to handle - # any initialization logic after the player was registered with the Player controller. + # any initialization logic after the config was initially loaded or updated. # This is called after the player is registered and self.config was loaded. + # And also when the config was updated. # You don't need to call update_state() here. @property diff --git a/music_assistant/providers/squeezelite/player.py b/music_assistant/providers/squeezelite/player.py index f50bcf00..0148cb1d 100644 --- a/music_assistant/providers/squeezelite/player.py +++ b/music_assistant/providers/squeezelite/player.py @@ -14,7 +14,7 @@ from aioslimproto.models import PlayerState as SlimPlayerState from aioslimproto.models import Preset as SlimPreset from aioslimproto.models import SlimEvent from aioslimproto.models import VisualisationType as SlimVisualisationType -from music_assistant_models.config_entries import ConfigEntry, ConfigValueOption, PlayerConfig +from music_assistant_models.config_entries import ConfigEntry, ConfigValueOption from music_assistant_models.enums import ( ConfigEntryType, ContentType, @@ -97,13 +97,16 @@ class SqueezelitePlayer(Player): self._sync_playpoints: deque[SyncPlayPoint] = deque(maxlen=MIN_REQ_PLAYPOINTS) self._do_not_resync_before: float = 0.0 + async def on_config_updated(self) -> None: + """Handle logic when the player is registered or the config was updated.""" + # set presets and display + await self._set_preset_items() + await self._set_display() + async def setup(self) -> None: """Set up the player.""" player_id = self.client.player_id self.logger.info("Player %s connected", self.client.name or player_id) - # set presets and display - await self._set_preset_items() - await self._set_display() # update all dynamic attributes self.update_attributes() # restore volume and power state @@ -349,13 +352,6 @@ class SqueezelitePlayer(Player): # for now, we dont support late joining into an existing stream self.mass.create_task(self.play_media(self.current_media)) - def set_config(self, config: PlayerConfig) -> None: - """Set/update the player config.""" - super().set_config(config) - # update preset and display when config changes - self.mass.create_task(self._set_preset_items()) - self.mass.create_task(self._set_display()) - def handle_slim_event(self, event: SlimEvent) -> None: """Handle player event from slimproto server.""" if event.type == SlimEventType.PLAYER_BUFFER_READY: diff --git a/music_assistant/providers/universal_group/player.py b/music_assistant/providers/universal_group/player.py index 0e1d94ae..42c748d1 100644 --- a/music_assistant/providers/universal_group/player.py +++ b/music_assistant/providers/universal_group/player.py @@ -81,12 +81,16 @@ class UniversalGroupPlayer(GroupPlayer): } self._set_attributes() - async def on_registered(self) -> None: - """Complete the initialization once the player was registered.""" - # Config entries are only fully available after the player was registered - self._attr_group_members = list( - cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) - ) + async def on_config_updated(self) -> None: + """Handle logic when the player is loaded or updated.""" + static_members = cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) + self._attr_static_group_members = static_members.copy() + if not self.powered: + self._attr_group_members = static_members.copy() + if self.is_dynamic: + self._attr_supported_features.add(PlayerFeature.SET_MEMBERS) + elif PlayerFeature.SET_MEMBERS in self._attr_supported_features: + self._attr_supported_features.remove(PlayerFeature.SET_MEMBERS) @cached_property def is_dynamic(self) -> bool: @@ -148,6 +152,15 @@ class UniversalGroupPlayer(GroupPlayer): self._attr_powered = powered if powered: + # reset the group members to the available static members when powering on + self._attr_group_members = [] + for static_group_member in self._attr_static_group_members: + if ( + (member_player := self.mass.players.get(static_group_member)) + and member_player.available + and member_player.enabled + ): + self._attr_group_members.append(static_group_member) # handle TURN_ON of the group player by turning on all members for member in self.mass.players.iter_group_members( self, only_powered=False, active_only=False @@ -162,15 +175,17 @@ class UniversalGroupPlayer(GroupPlayer): # collision: child player is part of multiple groups # and another group already active ! # solve this by trying to leave the group first - if ( - other_group := self.mass.players.get(member.active_group) - ) and PlayerFeature.SET_MEMBERS in other_group.supported_features: - await other_group.set_members(player_ids_to_remove=[member.player_id]) - else: - # if the other group does not support SET_MEMBERS, - # we need to power it off to leave the group - await self.mass.players.cmd_power(member.active_group, False) - await asyncio.sleep(1) + if other_group := self.mass.players.get(member.active_group): + if ( + other_group.supports_feature(PlayerFeature.SET_MEMBERS) + and member.player_id not in other_group.static_group_members + ): + await other_group.set_members(player_ids_to_remove=[member.player_id]) + else: + # if the other group does not support SET_MEMBERS or it is a static + # member, we need to power it off to leave the group + await other_group.power(False) + await asyncio.sleep(1) await asyncio.sleep(1) if member.synced_to: # edge case: the member is part of a syncgroup - ungroup it first @@ -188,9 +203,7 @@ class UniversalGroupPlayer(GroupPlayer): if not powered: # reset the original group members when powered off - self._attr_group_members = cast( - "list[str]", self.config.get_value(CONF_GROUP_MEMBERS, []) - ) + self._attr_group_members = self._attr_static_group_members.copy() self.update_state() async def volume_set(self, volume_level: int) -> None: @@ -308,15 +321,9 @@ class UniversalGroupPlayer(GroupPlayer): ), ) # handle removals - static_members = cast("list[str]", self.config.get_value(CONF_GROUP_MEMBERS, [])) for player_id in player_ids_to_remove or []: if player_id not in self._attr_group_members: continue - if player_id in static_members: - raise UnsupportedFeaturedException( - f"Cannot remove {player_id} from {self.display_name} " - "as it is a static member of this group" - ) if player_id == self.player_id: raise UnsupportedFeaturedException( f"Cannot remove {self.display_name} from itself as a member!" diff --git a/pyproject.toml b/pyproject.toml index b5fd71cf..c70e512d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ dependencies = [ "ifaddr==0.2.0", "mashumaro==3.16", "music-assistant-frontend==2.16.1", - "music-assistant-models==1.1.59", + "music-assistant-models==1.1.60", "mutagen==1.47.0", "orjson==3.11.3", "pillow==11.3.0", diff --git a/requirements_all.txt b/requirements_all.txt index 670889c6..6018eb49 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -33,7 +33,7 @@ liblistenbrainz==0.6.0 lyricsgenius==3.7.2 mashumaro==3.16 music-assistant-frontend==2.16.1 -music-assistant-models==1.1.59 +music-assistant-models==1.1.60 mutagen==1.47.0 orjson==3.11.3 pillow==11.3.0