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.

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

Add support for media player assumed state #11642

Merged
merged 2 commits into from Feb 11, 2022

Conversation

thecode
Copy link
Member

@thecode thecode commented Feb 10, 2022

Breaking change

The frontend media player card for integrations which sets the assumed_state to True and state to STATE_ON/ STATE_PLAYING/STATE_PAUSED/ STATE_IDLE will change from:
image
to:
assumed_state
The example assumes the following supported features are set: SUPPORT_PREVIOUS_TRACK, SUPPORT_PLAY, SUPPORT_PAUSE, SUPPORT_STOP, SUPPORT_NEXT_TRACK, if one of them is not set the corresponding button will not be displayed.

Currently there is one core integration which sets the assumed_state to True: xiaomi_tv, but it does not set any of the required supported features to cause the media card to change.

Custom integrations which meets these conditions will also have the frontend card changed.

Proposed change

Discussed in home-assistant/architecture#720, Add support for media player integrations which does not have a feedback from the device for the current playing mode (playing/paused/idle), if the integration sets the assumed_state to true the frontend will display all supported features in case the current state is STATE_ON/ STATE_PLAYING/STATE_PAUSED/ STATE_IDLE.

Current behavior:

state supported features assumed state action icon
playing/paused SUPPORT_PREVIOUS_TRACK false media_previous_track mdiSkipPrevious
playing SUPPORT_PAUSE/STOP false media_pause+media_stop mdiPause+mdiStop
paused/idle SUPPORT_PLAY false media_play mdiPlay
on SUPPORT_PLAY/PAUSE false media_play mdiPlayPause
playing/paused SUPPORT_NEXT_TRACK false media_next_track mdiSkipNext

New options when assumed_state is True and state is on/playing/idle/paused:

supported features assumed state action icon
SUPPORT_PREVIOUS_TRACK true media_previous_track mdiSkipPrevious
SUPPORT_PLAY true media_play mdiPlay
SUPPORT_PAUSE true media_pause mdiPause
SUPPORT_STOP true media_stop mdiStop
SUPPORT_NEXT_TRACK true media_next_track mdiSkipNext

Type of change

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

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

src/data/media-player.ts Outdated Show resolved Hide resolved
@spacegaier
Copy link
Member

spacegaier commented Feb 10, 2022

Do we need to show both pause and stop? IIRC the current coding is giving one of the two precedence over the other. Does that have to change with the new assumed_state logic or couldn't we keep that part to reduce screen space usage?

@spacegaier
Copy link
Member

Also, there should be a few more coding places where the logic needs to be changed such as the media player row.

@thecode
Copy link
Member Author

thecode commented Feb 10, 2022

Do we need to show both pause and stop? IIRC the current coding is giving one of the two precedence over the other. Does that have to change with the new assumed_state logic or couldn't we keep that part to reduce screen space usage?

This implementation does add an additional button to the media player card, I tried to think of a way to keep it smaller, but any other option would still require the core integration to implement logic which is not aligned with the device capabilities or just drop one of the options.

@thecode
Copy link
Member Author

thecode commented Feb 10, 2022

Also, there should be a few more coding places where the logic needs to be changed such as the media player row.

Can you point me to the places that needs to be changed? Thanks

balloob
balloob previously approved these changes Feb 10, 2022
@balloob
Copy link
Member

balloob commented Feb 10, 2022

The logic looks good if Elupus comment is integrated.

Spacegaier is right, we should ensure all places are either using this media controls method or are updated. Looks like just 1 spot left:

  • state card state-card-media_player.js: doesn't show controls
  • lovelace entity row hui-media-player-entity-row.ts: needs updating. See line 114
  • ha-bar-media-player ha-bar-media-player.ts: Uses computeMediaControls
  • media player card hui-media-control-card.ts: Uses computeMediaControls

@thecode
Copy link
Member Author

thecode commented Feb 10, 2022

The logic looks good if Elupus comment is integrated.

Spacegaier is right, we should ensure all places are either using this media controls method or are updated. Looks like just 1 spot left:

  • state card state-card-media_player.js: doesn't show controls
  • lovelace entity row hui-media-player-entity-row.ts: needs updating. See line 114
  • ha-bar-media-player ha-bar-media-player.ts: Uses computeMediaControls
  • media player card hui-media-control-card.ts: Uses computeMediaControls

Added assumed state in

  • lovelace entity row hui-media-player-entity-row.ts

And the comment from @elupus (assumed state behave the same on any sate which is not unavailable/unknown/off)

@balloob balloob merged commit 35cc291 into home-assistant:dev Feb 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2022
@thecode thecode deleted the mp-assumed-state branch February 13, 2022 21:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants