OpenVoiceOS / ovos-shell

Apache License 2.0
2 stars 4 forks source link

feat: allow user to override blop sound on first boot volume change #39

Closed mikejgray closed 10 months ago

mikejgray commented 10 months ago

https://github.com/OpenVoiceOS/ovos-PHAL-plugin-alsa/blob/da06817ddd9d03c0dcf611f05d5fa6fa36cd294f/ovos_PHAL_plugin_alsa/__init__.py#L54

set_volume() does have a play_sound option but there is no way to set it in the code as it is now

Create a config option, defaulting to true, that allows users to disable the initial blop sound when booting

Per Matrix chat: https://matrix.to/#/!ZhEZYNzKBfpEAhtQIz:matrix.org/$279GVdCG20Gv3dG-3fZ449YGnQlulOyS0g3-eMgpWH8?via=matrix.org&via=nitro.chat&via=strugee.net

JarbasAl commented 10 months ago

this only happens on very first boot, should not really be configurable, note that first_boot turns itself off and saves settings

i think the source of your issue is not the place you linked, but a bus message emitted from somewhere else

mikejgray commented 10 months ago

@JarbasAl You're right... if the user wants to configure it, they can edit ~/.config/OpenVoiceOS/ovos-PHAL-plugin-alsa.json, although that's kind of a weird place to do it.

I think the source of the Neon-reported issue is when Neon updates the Mark 2, it may be removing or overwriting that file, causing it to play the blop sound every time it updates.

j1nx commented 10 months ago

Although closed, if indeed the NEON update overwrites user config files, that might be a proper bug to report to that update system.

I believe a system update should not overwrite user config files?

JarbasAl commented 10 months ago

issue comes from https://github.com/OpenVoiceOS/ovos-shell/blob/master/application/qml/panel/quicksettings/VolumeSlider.qml

when shell loads it requests volume value (for the slider)

Connections {
        target: Mycroft.MycroftController
        onSocketStatusChanged: {
            if (Mycroft.MycroftController.status == Mycroft.MycroftController.Open) {
                Mycroft.MycroftController.sendRequest("mycroft.volume.get", {},
                    {"session": {"session_id": "default"}});
            }
        }
        onIntentRecevied: {
            if (type == "mycroft.volume.get.response") {
                slider.value = Math.round(data.percent * 100);
            }
        }
    }
}

when value changes in the slider, it emits a bus event for the alsa plugin

 onChangeValueChanged: {
        Mycroft.MycroftController.sendRequest("mycroft.volume.set", {"percent": (changeValue / 100)},
            {"session": {"session_id": "default"}});
    }

whenever something "reads" the volume from the alsa plugin the shell "sets" alsa volume to that same value again

the listener for the .get.response should not trigger the "mycroft.volume.set" , the shell needs better event handling

mikejgray commented 10 months ago

Although closed, if indeed the NEON update overwrites user config files, that might be a proper bug to report to that update system.

I believe a system update should not overwrite user config files?

A good callout for sure, and I did reach out to Neon. They confirmed this isn't the case as the settings file is in an XDG path, which isn't changed on update.

j1nx commented 10 months ago

Although closed, if indeed the NEON update overwrites user config files, that might be a proper bug to report to that update system. I believe a system update should not overwrite user config files?

A good callout for sure, and I did reach out to Neon. They confirmed this isn't the case as the settings file is in an XDG path, which isn't changed on update.

https://matrix.to/#/!ZhEZYNzKBfpEAhtQIz%3Amatrix.org/%240U7kS0ZCvKwDat6PF1JMK2DUpZgBT41FI_EIEwFZ8Ew 🤦‍♂️🤣