sbryfcz / harmony-card

A Home Assistant Lovelace Care for Harmony Integration
MIT License
104 stars 12 forks source link

Using volume_entity at activity level does not override card volume_entity as per order of precedence #46

Closed kalhimeo closed 3 years ago

kalhimeo commented 4 years ago

Dear developer,

I experience a bug with the volume control of the harmony card. I have a very basic setup : One card to switch between 2 activities and control the volume of one of them via 'volume_entity' (Marantz media player).

type: 'custom:harmony-card'
entity: remote.salon
tap_action: none
activities:
  - device: Console de jeu NVIDIA
    name: Télévision
    volume_entity: media_player.marantz_sr5011
  - device: Ampli Marantz
    name: Musique`

With this configuration, the volume of the Marantz receiver is correctly displayed, but any change from the card would not be applied (neither mute, slider, or volume up/down).

If I change my config and add the volume_entity on the "card level" in addition of the "activity level", the volume selection is working fine from the card.

Could you please have a look ?

Thank you very much !

sbryfcz commented 4 years ago

Hmmmm. Thats odd. I'll try to take a look this weekend when I get a moment.

kalhimeo commented 4 years ago

Thank you very much !

kalhimeo commented 4 years ago

I did some more testing today, and I think that the card is not giving the priority to the volume_entity defined at the "activity level" for volume control (it only shows the correct volume level of the entity in the slider, but modifying it does not work).

The following tests describe the behavior when the activity is activated : 1) volume_entity only on "activity level" -> the correct volume is shown on the slider but moving it does not modify the volume (neither do the button press). However the slider moves if I modify the volume with an external device. 2) volume_entity only on "card level" -> the volume is shown and moving the slider modifies the volume as intended 3) volume_entity on the "card level", and a different "dummy" volume_entity on the "activity level" -> the volume shown is the one from the "dummy" volume_entity which is set on the "activity level", but moving the slider will change the volume of the volume_entity defined on "card level"

So I think that in case 1) it does not work because the card tries to modify the volume of a non-existing entity, as it is not defined on the "card level".

Hope this helps you to find the source of the bug ;-)

kalhimeo commented 3 years ago

any chance to have this fixed ? thanks in advance !

kalhimeo commented 3 years ago

Some more debugging today, the order of precedence works as expected for the "device_volume" setting (card level / activity level), only the "entity_volume" is not working as described by the documentation

github-actions[bot] commented 3 years ago

This issue is being marked as stale due to lack of activity

kalhimeo commented 3 years ago

bump to avoid closing as the problem is still there :/

github-actions[bot] commented 3 years ago

This issue is being marked as stale due to lack of activity

kalhimeo commented 3 years ago

So I am not experienced with this complex coding, but as I understand it, the problem must come from one of those 2 functions :

    protected volumeCommand(e, command: string, attributes?: any) {
        this.preventBubbling(e);

        if (this._config?.volume_entity) {

            var baseAttributes = { entity_id: this._config?.volume_entity };

            this.hass?.callService("media_player", command, Object.assign(baseAttributes, attributes || {}));
        }
    }

or

    private renderMediaPlayerVolumeControls(hass: HomeAssistant, volumeMediaPlayer: string, buttonConfig: { [key: string]: HarmonyButtonConfig }) {
        var volume_state = hass.states[volumeMediaPlayer];

        var volume = volume_state.attributes.volume_level;
        var muted = volume_state.attributes.is_volume_muted;

        var volumeDownStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_down'].color });
        var volumeUpStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_up'].color });
        var volumeMuteStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_mute'].color });

        return html`
            <div class="volume-controls">
                <ha-icon-button style="${styleMap(volumeDownStyle)}" icon="${buttonConfig['volume_down'].icon}" @click="${e => this.volumeCommand(e, 'volume_down')}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
                <ha-icon-button style="${styleMap(volumeUpStyle)}" icon="${buttonConfig['volume_up'].icon}" @click="${e => this.volumeCommand(e, 'volume_up')}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
                <paper-slider           
                    @change=${e => this.volumeCommand(e, 'volume_set', { volume_level: e.target.value / 100 })}
                    @click=${e => e.stopPropagation()}
                    @touchstart="${e => this.preventBubbling(e)}"
                    ?disabled=${muted}
                    min=0 max=100
                    value=${volume * 100}
                    dir=${'ltr'}
                    ignore-bar-touch pin>
                </paper-slider>

                <ha-icon-button style="${styleMap(volumeMuteStyle)}" icon="${buttonConfig['volume_mute'].icon}" @click="${e => this.volumeCommand(e, 'volume_mute', { is_volume_muted: true })}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
            </div>`;
    }

From my tests with the card, the "volumeCommand" function is not able to assign the correct entity_id when the "volume_entity" is defined on the "currentActivityConfig" level (it will always use the one defined on global config level, if any).

So it may either come from the "renderMediaPlayerVolumeControls" which is not sending the right parameters, or from the "volumeCommand" itself which does not retrieve the right entity_id.

Hopefully someone with better coding knowledge will be able to debug this.

Thanks in advance !

kalhimeo commented 3 years ago

alright I dig more into this code and I think that I have understood the problem. The "volumeCommand" function should receive the proper "volumeMediaPlayer" on which the change should be applied (at the moment it only takes the global one at card level). As far as I can see the function is only called from the "renderMediaPlayerVolumeControls" part, so I propose this change :

    protected volumeCommand(e, volumeMediaPlayer: string, command: string, attributes?: any) {
        this.preventBubbling(e);

        var baseAttributes = { entity_id: volumeMediaPlayer };

        this.hass?.callService("media_player", command, Object.assign(baseAttributes, attributes || {}));

    }

and

  private renderMediaPlayerVolumeControls(hass: HomeAssistant, volumeMediaPlayer: string, buttonConfig: { [key: string]: HarmonyButtonConfig }) {
        var volume_state = hass.states[volumeMediaPlayer];

        var volume = volume_state.attributes.volume_level;
        var muted = volume_state.attributes.is_volume_muted;

        var volumeDownStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_down'].color });
        var volumeUpStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_up'].color });
        var volumeMuteStyle = Object.assign({} as StyleInfo, { color: buttonConfig['volume_mute'].color });

        return html`
            <div class="volume-controls">
                <ha-icon-button style="${styleMap(volumeDownStyle)}" icon="${buttonConfig['volume_down'].icon}" @click="${e => this.volumeCommand(e, volumeMediaPlayer, 'volume_down')}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
                <ha-icon-button style="${styleMap(volumeUpStyle)}" icon="${buttonConfig['volume_up'].icon}" @click="${e => this.volumeCommand(e, volumeMediaPlayer, 'volume_up')}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
                <paper-slider           
                    @change=${e => this.volumeCommand(e, volumeMediaPlayer, 'volume_set', { volume_level: e.target.value / 100 })}
                    @click=${e => e.stopPropagation()}
                    @touchstart="${e => this.preventBubbling(e)}"
                    ?disabled=${muted}
                    min=0 max=100
                    value=${volume * 100}
                    dir=${'ltr'}
                    ignore-bar-touch pin>
                </paper-slider>

                <ha-icon-button style="${styleMap(volumeMuteStyle)}" icon="${buttonConfig['volume_mute'].icon}" @click="${e => this.volumeCommand(e, volumeMediaPlayer, 'volume_mute', { is_volume_muted: true })}" @touchstart="${e => this.preventBubbling(e)}"></ha-icon-button>
            </div>`;
    }

I don't know how to test or submit this as a pull request as I never did that before, but hopefully someone can check the code an submit that :-) Does that sounds good to you @sbryfcz ?

github-actions[bot] commented 3 years ago

This issue is being marked as stale due to lack of activity