From 208d83ce22c5ad35aef1682505b6b939d347c388 Mon Sep 17 00:00:00 2001 From: Hartmnt Date: Wed, 11 Oct 2023 16:13:04 +0000 Subject: [PATCH] FIX(client): Fix using the keyboad to change local volume adjustment Previously, when using keyboard arrows or the mouse wheel, the new slider widget action applied the new local volume but did not save it. That was because it used the sliderReleased event to save the value, which is not triggered by keyboard or mouse wheel updates. We could simply save the local volume on every sliderChange instead of on sliderRelease, but that would cause many writes to the disk in a short period of time and possibly lag the client. Additionally, the ListenerVolumeSlider would spam packets to the server, as the listener volume is saved server-side. This commit introduces event filters in addition to the sliderChanged event, which are observing key releases and mouse wheel events. It also introduces a delay timer for packets send to the server from the ListenerVolumeSlider. Fixes #6211 --- src/mumble/CMakeLists.txt | 2 + src/mumble/ListenerVolumeSlider.cpp | 48 ++++++++++++---- src/mumble/ListenerVolumeSlider.h | 15 ++++- src/mumble/UserLocalVolumeSlider.cpp | 2 +- src/mumble/UserLocalVolumeSlider.h | 4 +- src/mumble/VolumeSliderWidgetAction.cpp | 22 ++++++- src/mumble/VolumeSliderWidgetAction.h | 2 +- src/mumble/widgets/EventFilters.cpp | 76 +++++++++++++++++++++++++ src/mumble/widgets/EventFilters.h | 50 ++++++++++++++++ 9 files changed, 204 insertions(+), 17 deletions(-) create mode 100644 src/mumble/widgets/EventFilters.cpp create mode 100644 src/mumble/widgets/EventFilters.h diff --git a/src/mumble/CMakeLists.txt b/src/mumble/CMakeLists.txt index 1b167e5c243..f09cd697bd1 100644 --- a/src/mumble/CMakeLists.txt +++ b/src/mumble/CMakeLists.txt @@ -289,6 +289,8 @@ set(MUMBLE_SOURCES "widgets/CompletablePage.cpp" "widgets/CompletablePage.h" + "widgets/EventFilters.cpp" + "widgets/EventFilters.h" "widgets/MUComboBox.cpp" "widgets/MUComboBox.h" "widgets/MultiStyleWidgetWrapper.cpp" diff --git a/src/mumble/ListenerVolumeSlider.cpp b/src/mumble/ListenerVolumeSlider.cpp index 3f6239bcbd8..6db5a8fdd3e 100644 --- a/src/mumble/ListenerVolumeSlider.cpp +++ b/src/mumble/ListenerVolumeSlider.cpp @@ -7,10 +7,14 @@ #include "Channel.h" #include "ChannelListenerManager.h" #include "ServerHandler.h" -#include "VolumeAdjustment.h" #include "Global.h" -ListenerVolumeSlider::ListenerVolumeSlider(QWidget *parent) : VolumeSliderWidgetAction(parent) { +ListenerVolumeSlider::ListenerVolumeSlider(QWidget *parent) : VolumeSliderWidgetAction(parent), m_currentSendDelay(0) { + connect(&m_sendTimer, &QTimer::timeout, this, &ListenerVolumeSlider::sendToServer); + connect(&m_resetTimer, &QTimer::timeout, this, [=]() { m_currentSendDelay = 0; }); + + m_sendTimer.setSingleShot(true); + m_resetTimer.setSingleShot(true); } void ListenerVolumeSlider::setListenedChannel(const Channel &channel) { @@ -28,7 +32,7 @@ void ListenerVolumeSlider::on_VolumeSlider_valueChanged(int value) { displayTooltip(value); } -void ListenerVolumeSlider::on_VolumeSlider_sliderReleased() { +void ListenerVolumeSlider::on_VolumeSlider_changeCompleted() { ServerHandlerPtr handler = Global::get().sh; if (!handler || !m_channel || !m_volumeSlider) { @@ -39,18 +43,42 @@ void ListenerVolumeSlider::on_VolumeSlider_sliderReleased() { if (handler->m_version >= Mumble::Protocol::PROTOBUF_INTRODUCTION_VERSION) { // With the new audio protocol, volume adjustments for listeners are handled on the server and thus we want - // to avoid spamming updates to the adjustments, which is why we only update them once the slider is released. - MumbleProto::UserState mpus; - mpus.set_session(Global::get().uiSession); + // to avoid spamming updates to the adjustments, which is why we only update them with a delay. + + if (m_cachedChannelID == m_channel->iId && m_cachedAdjustment == adjustment) { + return; + } - MumbleProto::UserState::VolumeAdjustment *adjustmentMsg = mpus.add_listening_volume_adjustment(); - adjustmentMsg->set_listening_channel(m_channel->iId); - adjustmentMsg->set_volume_adjustment(adjustment.factor); + m_cachedChannelID = m_channel->iId; + m_cachedAdjustment = adjustment; + + // Timer values: 0, 50, 150, 350, 750, 1000 (ms) + m_resetTimer.stop(); + m_sendTimer.start(m_currentSendDelay); + m_currentSendDelay = std::min(1000u, (m_currentSendDelay + 25) * 2); - handler->sendMessage(mpus); } else { // Before the new audio protocol, volume adjustments for listeners are handled locally Global::get().channelListenerManager->setListenerVolumeAdjustment(Global::get().uiSession, m_channel->iId, adjustment); } } + +void ListenerVolumeSlider::sendToServer() { + ServerHandlerPtr handler = Global::get().sh; + + m_resetTimer.start(3000); + + if (!handler) { + return; + } + + MumbleProto::UserState mpus; + mpus.set_session(Global::get().uiSession); + + MumbleProto::UserState::VolumeAdjustment *adjustmentMsg = mpus.add_listening_volume_adjustment(); + adjustmentMsg->set_listening_channel(m_cachedChannelID); + adjustmentMsg->set_volume_adjustment(m_cachedAdjustment.factor); + + handler->sendMessage(mpus); +} diff --git a/src/mumble/ListenerVolumeSlider.h b/src/mumble/ListenerVolumeSlider.h index 62e1532288c..08c651a80c7 100644 --- a/src/mumble/ListenerVolumeSlider.h +++ b/src/mumble/ListenerVolumeSlider.h @@ -6,8 +6,11 @@ #ifndef MUMBLE_MUMBLE_LISTENERLOCALVOLUMESLIDER_H_ #define MUMBLE_MUMBLE_LISTENERLOCALVOLUMESLIDER_H_ +#include "VolumeAdjustment.h" #include "VolumeSliderWidgetAction.h" +#include + class Channel; class ListenerVolumeSlider : public VolumeSliderWidgetAction { @@ -20,12 +23,20 @@ class ListenerVolumeSlider : public VolumeSliderWidgetAction { void setListenedChannel(const Channel &channel); private: - /// The channel of the listener proxy this dialog is operating on + /// The channel of the listener proxy this widget is operating on const Channel *m_channel; + QTimer m_sendTimer; + QTimer m_resetTimer; + unsigned int m_currentSendDelay; + int m_cachedChannelID; + VolumeAdjustment m_cachedAdjustment; + + void sendToServer(); + private slots: void on_VolumeSlider_valueChanged(int value) override; - void on_VolumeSlider_sliderReleased() override; + void on_VolumeSlider_changeCompleted() override; }; #endif diff --git a/src/mumble/UserLocalVolumeSlider.cpp b/src/mumble/UserLocalVolumeSlider.cpp index 59ccfb3f5e7..ea3f23fa4a3 100644 --- a/src/mumble/UserLocalVolumeSlider.cpp +++ b/src/mumble/UserLocalVolumeSlider.cpp @@ -35,7 +35,7 @@ void UserLocalVolumeSlider::on_VolumeSlider_valueChanged(int value) { } } -void UserLocalVolumeSlider::on_VolumeSlider_sliderReleased() { +void UserLocalVolumeSlider::on_VolumeSlider_changeCompleted() { ClientUser *user = ClientUser::get(m_clientSession); if (user) { if (!user->qsHash.isEmpty()) { diff --git a/src/mumble/UserLocalVolumeSlider.h b/src/mumble/UserLocalVolumeSlider.h index 59b632419e0..b79f574710b 100644 --- a/src/mumble/UserLocalVolumeSlider.h +++ b/src/mumble/UserLocalVolumeSlider.h @@ -13,7 +13,7 @@ class ClientUser; class UserLocalVolumeSlider : public VolumeSliderWidgetAction { Q_OBJECT - /// The session ID for the user that the dialog is changing the volume for. + /// The session ID for the user that the widget is changing the volume for. unsigned int m_clientSession; public: @@ -24,7 +24,7 @@ class UserLocalVolumeSlider : public VolumeSliderWidgetAction { private slots: void on_VolumeSlider_valueChanged(int value); - void on_VolumeSlider_sliderReleased(); + void on_VolumeSlider_changeCompleted(); }; #endif diff --git a/src/mumble/VolumeSliderWidgetAction.cpp b/src/mumble/VolumeSliderWidgetAction.cpp index 0d49e258bb4..e2b673bc144 100644 --- a/src/mumble/VolumeSliderWidgetAction.cpp +++ b/src/mumble/VolumeSliderWidgetAction.cpp @@ -5,6 +5,7 @@ #include "VolumeSliderWidgetAction.h" #include "VolumeAdjustment.h" +#include "widgets/EventFilters.h" #include #include @@ -16,10 +17,29 @@ VolumeSliderWidgetAction::VolumeSliderWidgetAction(QObject *parent) m_volumeSlider->setAccessibleName(tr("Slider for volume adjustment")); m_volumeSlider->setSizePolicy(QSizePolicy::MinimumExpanding, QSizePolicy::MinimumExpanding); + KeyEventObserver *keyEventFilter = new KeyEventObserver(this, QEvent::KeyRelease, false, + { Qt::Key_Left, Qt::Key_Right, Qt::Key_Up, Qt::Key_Down }); + m_volumeSlider->installEventFilter(keyEventFilter); + + // The list of wheel events observed seems odd at first. We have to check for multiple + // Qt::ScrollPhase types, because Qt has a weird/non-deterministic behavior regarding + // scroll phases. There seems to be at least 3 different states the application can + // be in at each start-up, which all produce different scroll phases using the same + // inputs. This might very well be a Qt bug that is fixed in future versions and this + // list could possibly be updated to reflect that. According to official Qt docs, observing + // Qt::ScrollUpdate should be enough. But as of Qt 5.15.8 it is not. + MouseWheelEventObserver *wheelEventFilter = + new MouseWheelEventObserver(this, { Qt::ScrollMomentum, Qt::ScrollUpdate, Qt::ScrollEnd }, false); + m_volumeSlider->installEventFilter(wheelEventFilter); + connect(m_volumeSlider.get(), &QSlider::valueChanged, this, &VolumeSliderWidgetAction::on_VolumeSlider_valueChanged); connect(m_volumeSlider.get(), &QSlider::sliderReleased, this, - &VolumeSliderWidgetAction::on_VolumeSlider_sliderReleased); + &VolumeSliderWidgetAction::on_VolumeSlider_changeCompleted); + connect(keyEventFilter, &KeyEventObserver::keyEventObserved, this, + &VolumeSliderWidgetAction::on_VolumeSlider_changeCompleted); + connect(wheelEventFilter, &MouseWheelEventObserver::wheelEventObserved, this, + &VolumeSliderWidgetAction::on_VolumeSlider_changeCompleted); setDefaultWidget(m_volumeSlider.get()); diff --git a/src/mumble/VolumeSliderWidgetAction.h b/src/mumble/VolumeSliderWidgetAction.h index 9d7dfae13cb..4cba54179f4 100644 --- a/src/mumble/VolumeSliderWidgetAction.h +++ b/src/mumble/VolumeSliderWidgetAction.h @@ -27,7 +27,7 @@ class VolumeSliderWidgetAction : public QWidgetAction { protected slots: virtual void on_VolumeSlider_valueChanged(int){}; - virtual void on_VolumeSlider_sliderReleased(){}; + virtual void on_VolumeSlider_changeCompleted(){}; }; #endif diff --git a/src/mumble/widgets/EventFilters.cpp b/src/mumble/widgets/EventFilters.cpp new file mode 100644 index 00000000000..acb6a81fd76 --- /dev/null +++ b/src/mumble/widgets/EventFilters.cpp @@ -0,0 +1,76 @@ +// Copyright 2023 The Mumble Developers. All rights reserved. +// Use of this source code is governed by a BSD-style license +// that can be found in the LICENSE file at the root of the +// Mumble source tree or at . + +#include "EventFilters.h" + +#include + +#include +#include +#include + +#include + +KeyEventObserver::KeyEventObserver(QObject *parent, QEvent::Type eventType, bool consume, std::vector< Qt::Key > keys) + : QObject(parent), m_eventType(eventType), m_consume(consume), m_keys(std::move(keys)) { +} + +bool KeyEventObserver::eventFilter(QObject *obj, QEvent *event) { + QWidget *widget = static_cast< QWidget * >(obj); + + if (!widget || !widget->hasFocus()) { + return false; + } + + QKeyEvent *keyEvent = static_cast< QKeyEvent * >(event); + + if (!keyEvent || keyEvent->type() != m_eventType) { + return false; + } + + Qt::Key key = static_cast< Qt::Key >(keyEvent->key()); + + if (std::find(m_keys.begin(), m_keys.end(), key) == m_keys.end()) { + return false; + } + + emit keyEventObserved(); + + return m_consume; +} + +MouseWheelEventObserver::MouseWheelEventObserver(QObject *parent, std::vector< Qt::ScrollPhase > phases, bool consume) + : QObject(parent), m_phases(std::move(phases)), m_consume(consume) { +} + +bool MouseWheelEventObserver::eventFilter(QObject *obj, QEvent *event) { + QWidget *widget = static_cast< QWidget * >(obj); + + qDebug() << "visible: " << widget->isVisible(); + + if (!widget || !widget->isVisible()) { + return false; + } + + QWheelEvent *wheelEvent = static_cast< QWheelEvent * >(event); + + if (!wheelEvent) { + return false; + } + + qDebug() << "is a wheel event"; + + Qt::ScrollPhase phase = wheelEvent->phase(); + + qDebug() << "phase: " << phase; + + if (std::find(m_phases.begin(), m_phases.end(), phase) == m_phases.end()) { + return false; + } + + emit wheelEventObserved(wheelEvent->pixelDelta()); + + return m_consume; +} diff --git a/src/mumble/widgets/EventFilters.h b/src/mumble/widgets/EventFilters.h new file mode 100644 index 00000000000..ef1aa7076ae --- /dev/null +++ b/src/mumble/widgets/EventFilters.h @@ -0,0 +1,50 @@ +// Copyright 2023 The Mumble Developers. All rights reserved. +// Use of this source code is governed by a BSD-style license +// that can be found in the LICENSE file at the root of the +// Mumble source tree or at . + +#ifndef MUMBLE_MUMBLE_WIDGETS_EVENTFILTERS_H_ +#define MUMBLE_MUMBLE_WIDGETS_EVENTFILTERS_H_ + +#include + +#include +#include +#include + +class KeyEventObserver : public QObject { + Q_OBJECT + +public: + KeyEventObserver(QObject *parent, QEvent::Type eventType, bool consume, std::vector< Qt::Key > keys); + +protected: + bool eventFilter(QObject *obj, QEvent *event) override; + +signals: + void keyEventObserved(); + +private: + QEvent::Type m_eventType; + bool m_consume; + std::vector< Qt::Key > m_keys; +}; + +class MouseWheelEventObserver : public QObject { + Q_OBJECT + +public: + MouseWheelEventObserver(QObject *parent, std::vector< Qt::ScrollPhase > phases, bool consume); + +protected: + bool eventFilter(QObject *obj, QEvent *event) override; + +signals: + void wheelEventObserved(QPoint delta); + +private: + std::vector< Qt::ScrollPhase > m_phases; + bool m_consume; +}; + +#endif