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 Bang & Olufsen media_player grouping #123020

Merged

Conversation

mj23000
Copy link
Contributor

@mj23000 mj23000 commented Aug 1, 2024

Proposed change

Add media_player grouping to the Bang & Olufsen media_player entity. This implementation does not contain any custom services, but does still have some structure from the previous (too big) PR: #113438.

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Add support for media player grouping via beolink
Give media player entity name
simplify Beolink attributes
Add unexpand return value
Set entity name on initialization
Add all_discovered to beolink_expand service
Improve beolink_expand response
Remove unnecessary assignment
Update response typing for updated API
Remove mypy ignore line
Fix jid possibly used before assignment
Fix formatting
Fix remote leader media position bug
Improve remote leader BangOlufsenSource comparison
@home-assistant home-assistant bot marked this pull request as draft August 2, 2024 15:31
@mj23000 mj23000 marked this pull request as ready for review August 23, 2024 14:17
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/strings.json Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft September 4, 2024 13:14
@mj23000 mj23000 marked this pull request as ready for review September 5, 2024 09:57
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of minor remarks on the tests.

assert mock_mozart_client.join_latest_beolink_experience.call_count == 0


async def test_async_unjoin_player(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a negative case to assert what happens when trying to unjoin a player which is not in a group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unjoin service simply calls post_beolink_leave regardless if the device is in a group or not, which leaves the session or does nothing, so I don't think there is anything left to test here.

tests/components/bang_olufsen/test_media_player.py Outdated Show resolved Hide resolved
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mj23000 👍

@emontnemery emontnemery merged commit a8648b7 into home-assistant:dev Sep 16, 2024
29 of 30 checks passed
bealex pushed a commit to DevPocket/homeassistant-core that referenced this pull request Sep 16, 2024
* Add Beolink custom services
Add support for media player grouping via beolink
Give media player entity name

* Fix progress not being set to None as Beolink listener
Revert naming changes

* Update API
simplify Beolink attributes

* Improve beolink custom services

* Fix Beolink expandable source check
Add unexpand return value
Set entity name on initialization

* Handle entity naming as intended

* Fix "null" Beolink self friendly name

* Add regex service input validation
Add all_discovered to beolink_expand service
Improve beolink_expand response

* Add service icons

* Fix merge
Remove unnecessary assignment

* Remove invalid typing
Update response typing for updated API

* Revert to old typed response dict method
Remove mypy ignore line
Fix jid possibly used before assignment

* Re add debugging logging

* Fix coroutine
Fix formatting

* Remove unnecessary update control

* Make tests pass
Fix remote leader media position bug
Improve remote leader BangOlufsenSource comparison

* Fix naming and add callback decorators

* Move regex service check to variable
Suppress KeyError
Update tests

* Re-add hass running check

* Improve comments, naming and type hinting

* Remove old temporary fix

* Convert logged warning to raised exception for invalid media_player
Simplify code using walrus operator

* Fix test for invalid media_player grouping

* Improve method naming

* Improve _beolink_sources explanation

* Improve _beolink_sources explanation

* Add initial media_player grouping

* Convert custom service methods to media_player methods
Fix testing

* Remove beolink JID extra state attribute

* Modify custom services to only work as expected for media_player grouping
Fix tests

* Remove unused dispatch

* Remove wrong comment

* Remove commented out code

* Add config entry mock typing

* Fix beolink listener playback progress
Fix formatting
Add and use get_serial_number_from_jid function

* Fix testing

* Clarify beolink WebSocket notifications

* Further clarify beolink WebSocket notifications

* Convert notification value to enum value

* Improve comments for touch to join

* Fix None being cast to str if leader is not in HA

* Add error messages to devices in Beolink session and not Home Assistant
Rework _get_beolink_jid

* Replace redundant function call

* Show friendly name for unavailable remote leader instead of JID

* Update homeassistant/components/bang_olufsen/media_player.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Remove unneeded typing

* Rework _get_beolink_jid entity check
Clarify invalid entity error message

* Remove redundant "entity" from string

* Fix invalid typing
fix state assertions

* Fix raised error type

---------

Co-authored-by: Erik Montnemery <erik@montnemery.com>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2024
@home-assistant home-assistant unlocked this conversation Sep 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
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.

4 participants