Add media player device actions

[chmielowiec]

Proposed change

Add media player actions without extra arguments

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

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

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[frenck]

Hi there @chmielowiec,

This PR is changing our entity model, which requires a discussion in our architecture repository first. As per:

https://developers.home-assistant.io/docs/entity_index/#changing-the-entity-model

You can find the architecture repository here: https://github.com/home-assistant/architecture

🙈 I need coffee... Good morning! ☕

[elupus]

Unclear what you want to check here. Given the the supported_features can be a bitwise combination like "SUPPORT_TURN_ON | SUPPORT_TURN_OFF" this is somewhat unclear. The check now will allow TURN_ON or TURN_OFF. It's enough with one of the feature. Is this what is wanted, or are both required? I suggest changing this to:

if ((supported_features & action_config[CONF_SUPPORTED_FEATURES]) == action_config[CONF_SUPPORTED_FEATURES])

And if the OR condition is also required, allow a list of features for the conf:

if (any((x & supported_feature) == x for x in action_config[CONF_SUPPORTED_FEATURES]))

That way it becomes clearer what is expected.

[chmielowiec]

I tried to do it same way services are registered, see here: https://github.com/home-assistant/core/blob/dev/homeassistant/components/media_player/__init__.py#L218
After digging a bit more I found that core helper is broken in the same way: https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/service.py#L434

# Skip entities that don't have the required feature.
    if required_features is not None and not any(
        entity.supported_features & feature_set for feature_set in required_features
    ):
        continue

It accepts list of required features, currently there is no difference whether you pass
[SUPPORT_A | SUPPORT_B] or [SUPPORT_A, SUPPORT_B].

Should I fix this too? Impact of that change could be huge. Created #35718

I will adjust my code to match service registration, changing volumes check to list.

[elupus]

Small note added. I think this mostly looks nice, but need more core review.

[balloob]

The problem we have with media players is that it is the only integration that treats supported features as a dynamic attribute where it describes the current capabilities of the media and not what the player can do.

That's why this feature will not work as intended, because the returned triggers will be based on what media is playing !

[chmielowiec]

It's based on service registration code here:

core/homeassistant/components/media_player/__init__.py

Lines 212 to 327 in f841648

	component.async_register_entity_service(
	SERVICE_TURN_ON, {}, "async_turn_on", [SUPPORT_TURN_ON]
	)
	component.async_register_entity_service(
	SERVICE_TURN_OFF, {}, "async_turn_off", [SUPPORT_TURN_OFF]
	)
	component.async_register_entity_service(
	SERVICE_TOGGLE, {}, "async_toggle", [SUPPORT_TURN_OFF | SUPPORT_TURN_ON]
	)
	component.async_register_entity_service(
	SERVICE_VOLUME_UP,
	{},
	"async_volume_up",
	[SUPPORT_VOLUME_SET, SUPPORT_VOLUME_STEP],
	)
	component.async_register_entity_service(
	SERVICE_VOLUME_DOWN,
	{},
	"async_volume_down",
	[SUPPORT_VOLUME_SET, SUPPORT_VOLUME_STEP],
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_PLAY_PAUSE,
	{},
	"async_media_play_pause",
	[SUPPORT_PLAY | SUPPORT_PAUSE],
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_PLAY, {}, "async_media_play", [SUPPORT_PLAY]
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_PAUSE, {}, "async_media_pause", [SUPPORT_PAUSE]
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_STOP, {}, "async_media_stop", [SUPPORT_STOP]
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_NEXT_TRACK, {}, "async_media_next_track", [SUPPORT_NEXT_TRACK]
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_PREVIOUS_TRACK,
	{},
	"async_media_previous_track",
	[SUPPORT_PREVIOUS_TRACK],
	)
	component.async_register_entity_service(
	SERVICE_CLEAR_PLAYLIST, {}, "async_clear_playlist", [SUPPORT_CLEAR_PLAYLIST]
	)
	component.async_register_entity_service(
	SERVICE_VOLUME_SET,
	vol.All(
	cv.make_entity_service_schema(
	{vol.Required(ATTR_MEDIA_VOLUME_LEVEL): cv.small_float}
	),
	_rename_keys(volume=ATTR_MEDIA_VOLUME_LEVEL),
	),
	"async_set_volume_level",
	[SUPPORT_VOLUME_SET],
	)
	component.async_register_entity_service(
	SERVICE_VOLUME_MUTE,
	vol.All(
	cv.make_entity_service_schema(
	{vol.Required(ATTR_MEDIA_VOLUME_MUTED): cv.boolean}
	),
	_rename_keys(mute=ATTR_MEDIA_VOLUME_MUTED),
	),
	"async_mute_volume",
	[SUPPORT_VOLUME_MUTE],
	)
	component.async_register_entity_service(
	SERVICE_MEDIA_SEEK,
	vol.All(
	cv.make_entity_service_schema(
	{
	vol.Required(ATTR_MEDIA_SEEK_POSITION): vol.All(
	vol.Coerce(float), vol.Range(min=0)
	)
	}
	),
	_rename_keys(position=ATTR_MEDIA_SEEK_POSITION),
	),
	"async_media_seek",
	[SUPPORT_SEEK],
	)
	component.async_register_entity_service(
	SERVICE_SELECT_SOURCE,
	{vol.Required(ATTR_INPUT_SOURCE): cv.string},
	"async_select_source",
	[SUPPORT_SELECT_SOURCE],
	)
	component.async_register_entity_service(
	SERVICE_SELECT_SOUND_MODE,
	{vol.Required(ATTR_SOUND_MODE): cv.string},
	"async_select_sound_mode",
	[SUPPORT_SELECT_SOUND_MODE],
	)
	component.async_register_entity_service(
	SERVICE_PLAY_MEDIA,
	vol.All(
	cv.make_entity_service_schema(MEDIA_PLAYER_PLAY_MEDIA_SCHEMA),
	_rename_keys(
	media_type=ATTR_MEDIA_CONTENT_TYPE,
	media_id=ATTR_MEDIA_CONTENT_ID,
	enqueue=ATTR_MEDIA_ENQUEUE,
	),
	),
	"async_play_media",
	[SUPPORT_PLAY_MEDIA],
	)
	component.async_register_entity_service(
	SERVICE_SHUFFLE_SET,
	{vol.Required(ATTR_MEDIA_SHUFFLE): cv.boolean},
	"async_set_shuffle",
	[SUPPORT_SHUFFLE_SET],
	)

How is it dynamic? Or maybe it shouldn't be based on services?
Or should I remove getting supported_features from state and base it only on entity?

core/homeassistant/components/media_player/device_action.py

Lines 128 to 131 in da7ba72

	if state:
	supported_features = state.attributes.get(CONF_SUPPORTED_FEATURES, 0)
	else:
	supported_features = entry.supported_features

[balloob]

Services are checked against supported features at the moment they are called. So if an entity doesn't support pause because nothing is playing, it won't do anything, and that's fine.

However, device automations can be configured at any time. So checking the supported features will not return the right set of capabilities.

I also think that we should not just wrap all services, as uh, then people can just use services.

I think that we should close this PR for now, and re-address this issue once we have figured out a way to split out current_media_features out of supported_features.