-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Fix grouping feature for MusicCast #95958
Conversation
Hey there @vigonotion, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have been discussing this, mostly the original PR, so making this review to block this one for now.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Yes, it's that PR that we are concerned about. We know it's been released. We're discussing reverting it. |
Okay - if you are considering reverting it because of the ability to use translations in state attributes: I would love to use translations instead of renaming sources. But for sources, it makes no sense to use static translations. On MusicCast devices, you can rename sources in the MusicCast app. For example, my hdmi1 is renamed to Apple-TV, and that is the only thing that shows up in the device display and the MusicCast app. But for other users, hdmi1 might be renamed to XBOX, etc. If it were possible to let the device return translations/labels for sources, I would also prefer to go that way, but to my knowledge this is not the case, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is good to go as these are user defined labels, and we cannot offer translations for those.
check the current source for grouping using the source ID instead of the label
check the current source for grouping using the source ID instead of the label
Proposed change
Due to the new source names from #74954 we were checking the current source by using the source names instead of the source ids. Long story short: We weren't taking care of the breaking change, we introduced there. This will be fixed by this PR so that the grouping features of MusicCast should now work again.
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: