kalkih / mini-media-player

Minimalistic media card for Home Assistant Lovelace UI
MIT License
1.51k stars 206 forks source link

Grouping of media_player domain (and HEOS) #604

Closed tmjo closed 2 years ago

tmjo commented 2 years ago

Hi @kalkih and thanks for your awesome card! I've used it for a long time, and also did a custom integration for HEOS to enable grouping. In HA 2021.12 grouping was added to the official integration of HEOS after a really long time and a architectural discussion on how to organize grouping of media players in HA.

However, we see some issues with the card in combination with your card. Basic grouping (1+1 player) works just fine, but trying to add a second player to the group it replaces the first player with the second one. According to the official HEOS integration docs all members should be part of the _groupmembers call, not only the recently added. I do not know if this is the case for HEOS only, but I would believe that after such a long architectural discussion it had been decided that this is the way once the HEOS feature was added. Would you happen to know what is correct?

See the forum for discussions on the subject. Below an animation of what happens taken from the forum: heos-mini-media-card

I did a tweak to my fork of your card in handleGroupChange:

        case PLATFORM.MEDIAPLAYER:
          let members = this.group;
          members.shift() // remove master from list
          members.push(entity); // add new entity to group
          return this.callService(e, 'join', {
            entity_id: this.entityId,
            group_members: members,
          }, platform);

and this seems to achieve what I want it to. I would be happy to propose a PR on this, but I would like to hear your opinion on how to do it first.

PS! Once done, HEOS should be added to the supported players list :)

kalkih commented 2 years ago

Hello,

Thanks for the detailed explanation, and great work on the custom integration!

Do you know if this new "standard" way of implementation is adopted by the other media player integrations with media_player.join/media_player.unjoin support as well? I think e.g. musicCast https://github.com/kalkih/mini-media-player/issues/528 & Roon https://github.com/kalkih/mini-media-player/issues/519 already used these services and the current service call in this card seems to work there, or at least it did...

If we think this is the way forward in HA, I think we should make the change and a PR would be appreciated! And if other integrations break because of the change we could just add special cases for those until they've adjusted to the new changes.

tmjo commented 2 years ago

Yeah, that's the thing. I'm really not sure if this goes for all integrations or if it is something that was agreed for HEOS. I believe it was agreed as a common ground for HA integrations, but not sure and also not sure if it is has been implemented for all integrations yet if it is the case.

I see the arch issue is still open, so perhaps it is not decided. On the other hand, the HEOS grouping feature was blocked from merging for a long time mainly due to these discussions, but finally found it's way in HA 2021.12.

If it turns out that it is not adopted (or even agreed) yet, would it be possible to add a case HEOS and let users specify HEOS as a platform for the card, but still call the media_player domain in a way similar to what I proposed? Or would that just make a even bigger mess? :)

kalkih commented 2 years ago

Alright, Yes, We could add a heos case for now, and leave the media_player case as is for now until we see these changes in other integrations.

tmjo commented 2 years ago

Great, I'll propose a PR. Thought I had it ready, but ran into some issues with the Ungroup button (when ungrouping whole group), but it may be a temporary problem in my installation. I'll do some more testing before presenting the PR.

tmjo commented 2 years ago

Please see if you find PR #607 to your liking and let me know if you prefer I change something.

It seems to be working fine in my installation now (one of my units needed rebooting after too much 'unhealthy' testing, that was what gave me issues on the ungroup all command previously).

tmjo commented 2 years ago

Hmm, I see the checks failed. Will look into that later, I couldn't see exactly what it is failing on, but it's getting late :)

tmjo commented 2 years ago

Was some stupid linting issues from my side, should be ok now. I get some errors on my local installation on lint (on code I didn't change), but I guess it is something with my nvm/npm installation, but at least it is passing lint tests on the server so it should be good to go.

kalkih commented 2 years ago

https://github.com/kalkih/mini-media-player/pull/607 now available in latest release, great stuff @tmjo 👍🏼