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

Bang & Olufsen add beolink grouping #113438

Merged

Conversation

mj23000
Copy link
Contributor

@mj23000 mj23000 commented Mar 14, 2024

Proposed change

Add additional media player grouping to the bang_olufsen media_player entity, using Beolink custom actions.

Beolink is not limited to Mozart devices, which means that devices that can't to be added to Home Assistant (with this integration) can be expanded to using the custom actions, which reference the device's unique JID, instead of entity_id.

Additionally extra_state_attributes show Beolink self (name and JID), peers, listeners and leader.

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

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.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Please split this PR. One for bumping the library (and all changes needed to support that new version (not the changes needed for new features made possible for that version)). Then try to split the services into a separate PR if that makes sense. I haven't checked the media_player diff, but I don't believe adding services would add that much code to media_player.py

@home-assistant home-assistant bot marked this pull request as draft March 16, 2024 14:54
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@mj23000
Copy link
Contributor Author

mj23000 commented Mar 18, 2024

Please split this PR. One for bumping the library (and all changes needed to support that new version (not the changes needed for new features made possible for that version)). Then try to split the services into a separate PR if that makes sense. I haven't checked the media_player diff, but I don't believe adding services would add that much code to media_player.py

Of course.
I've made a new PR for just the API update. This PR depends on the API update, so I'll just keep this as a draft for now, and merge any changes that may be made when the time comes.

As for the custom services, there are 5 in total and they are all "required" for a smooth Beolink experience. These include "join", "expand", "unexpand", "leave" and "allstandby".

I could reduce this down to only the "expand" service and remove the extra_state_attributes for the Beolink session, which would retain support for media_player grouping and remove a sizable amount of code, if that would make this PR more digestable.

@mj23000 mj23000 changed the title Bang & Olufsen add beolink grouping and update API Bang & Olufsen add beolink grouping Mar 18, 2024
@mj23000 mj23000 marked this pull request as ready for review May 16, 2024 11:11
@home-assistant home-assistant bot requested a review from joostlek May 16, 2024 11:11
@emontnemery
Copy link
Contributor

@mj23000 please don't forget to click the "Ready for review" button when you want the PR to be reviewed

@mj23000 mj23000 marked this pull request as ready for review September 25, 2024 11:04
@emontnemery
Copy link
Contributor

emontnemery commented Sep 25, 2024

Please fix the merge conflicts

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.

A few comments. I've not reviewed the tests.

homeassistant/components/bang_olufsen/strings.json 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
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.

I think there's another rebase mistake to clear up

homeassistant/components/bang_olufsen/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/bang_olufsen/services.yaml 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 November 8, 2024 09:24
@mj23000 mj23000 marked this pull request as ready for review November 8, 2024 10:01
@home-assistant home-assistant bot requested a review from emontnemery November 8, 2024 10:01
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.

Thanks, @mj23000 👍

@emontnemery emontnemery merged commit e3dfa84 into home-assistant:dev Nov 8, 2024
31 of 32 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 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.

3 participants