[vlc-devel] [PATCH 6/7] qt: add AudioDeviceConfigControl

[Rémi Denis-Courmont]

Le 27 février 2018 15:43:09 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>
>
>On Tue, Feb 27, 2018, at 14:39, Rémi Denis-Courmont wrote:
>> Le 27 février 2018 12:41:10 GMT+02:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>> >Same as StringListConfigControl() but it also calls aout_DeviceSet()
>> >when
>> >applying the configuration.
>> >---
>> >modules/gui/qt/components/preferences_widgets.cpp | 19
>> >+++++++++++++++++++
>> > modules/gui/qt/components/preferences_widgets.hpp | 15
>+++++++++++++--
>> > 2 files changed, 32 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/modules/gui/qt/components/preferences_widgets.cpp
>> >b/modules/gui/qt/components/preferences_widgets.cpp
>> >index 1419bac114..6bbcb91765 100644
>> >--- a/modules/gui/qt/components/preferences_widgets.cpp
>> >+++ b/modules/gui/qt/components/preferences_widgets.cpp
>> >@@ -35,6 +35,7 @@
>> > #include "util/customwidgets.hpp"
>> > #include "util/searchlineedit.hpp"
>> > #include "util/qt_dirs.hpp"
>> >+#include "input_manager.hpp"
>> > #include <vlc_actions.h>
>> > #include <vlc_intf_strings.h>
>> > #include <vlc_modules.h>
>> >@@ -466,6 +467,24 @@ QString StringListConfigControl::getValue()
>const
>> >     return combo->itemData( combo->currentIndex() ).toString();
>> > }
>> > 
>> >+AudioDeviceConfigControl::AudioDeviceConfigControl( intf_thread_t
>> >*_p_intf,
>> >+                module_config_t *_p_item, QLabel *_label, QComboBox
>> >*_combo )
>> >+    : StringListConfigControl( VLC_OBJECT(_p_intf), _p_item,
>_label,
>> >_combo, false )
>> >+    , p_intf(_p_intf)
>> >+{
>> >+}
>> >+
>> >+void AudioDeviceConfigControl::doApply()
>> >+{
>> >+    StringListConfigControl::doApply();
>> >+    audio_output_t *aout = THEMIM->getAout();
>> >+    if (aout != NULL)
>> >+    {
>> >+        aout_DeviceSet(aout, qtu(getValue()));
>> >+        vlc_object_release(aout);
>> >+    }
>> >+}
>> >+
>> >void setfillVLCConfigCombo( const char *configname, intf_thread_t
>> >*p_intf,
>> >                             QComboBox *combo )
>> > {
>> >diff --git a/modules/gui/qt/components/preferences_widgets.hpp
>> >b/modules/gui/qt/components/preferences_widgets.hpp
>> >index 5f516fbd5d..00b466fd13 100644
>> >--- a/modules/gui/qt/components/preferences_widgets.hpp
>> >+++ b/modules/gui/qt/components/preferences_widgets.hpp
>> >@@ -302,7 +302,7 @@ class VStringConfigControl : public
>ConfigControl
>> > public:
>> >     virtual QString getValue() const = 0;
>> >     int getType() const Q_DECL_OVERRIDE;
>> >-    void doApply() Q_DECL_OVERRIDE;
>> >+    virtual void doApply() Q_DECL_OVERRIDE;
>> > protected:
>> >     VStringConfigControl( vlc_object_t *a, module_config_t *b ) :
>> >                 ConfigControl(a,b) {}
>> >@@ -451,13 +451,24 @@ protected:
>> >     void fillGrid( QGridLayout*, int ) Q_DECL_OVERRIDE;
>> >     QComboBox *combo;
>> > private:
>> >-    void finish(module_config_t * );
>> >+    virtual void finish(module_config_t * );
>> >     QLabel *label;
>> >     QList<QPushButton *> buttons;
>> > private slots:
>> >     void comboIndexChanged( int );
>> > };
>> > 
>> >+class AudioDeviceConfigControl : public StringListConfigControl
>> >+{
>> >+    Q_OBJECT
>> >+public:
>> >+    AudioDeviceConfigControl( intf_thread_t *, module_config_t *,
>> >QLabel *,
>> >+                              QComboBox* );
>> >+    virtual void doApply() Q_DECL_OVERRIDE;
>> >+private:
>> >+    intf_thread_t *p_intf;
>> >+};
>> >+
>> >void setfillVLCConfigCombo(const char *configname, intf_thread_t
>> >*p_intf,
>> >                         QComboBox *combo );
>> > 
>> >-- 
>> >2.11.0
>> >
>> >_______________________________________________
>> >vlc-devel mailing list
>> >To unsubscribe or modify your subscription options:
>> >https://mailman.videolan.org/listinfo/vlc-devel
>> 
>> This is a bad idea on multiple levels.
>> 
>> At technical level, there are no obvious or documented reasons why
>audio 
>> volume should be a special snowflake in preferences. There are plenty
>of 
>> settings that could be applied without restart through custom code, 
>> including many in the simple preferences.
>> 
>> On user level, there is a clear and simple story now that VLC should
>be 
>> restarted after changing preferences. This only makes stuff more 
>> complicated. This ia bound to cascade into even more support problems
>as 
>> users will expect everything to work like audio volume.
>
>We are in 2018, I don't think any new users will expect to restart a
>program when they change a preference.
>I'm not saying to change everything now, but we should focus on
>improving it for future VLC versions.
>
>> 
>> Unlike the ancient preferences live update code, this has no glaring 
>> bugs. But it looks like setting a very dangerous precedent / stepping
>on 
>> a slippery slope.
>> -- 
>> Remi Denis-Courmont
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

Opening a big can of worms that _will_ turn into a maintenance nightmare (why do you think this feature was removed, hmm?) is not focusing to improve the software. Especially not for a feature that is ultimately rarely used.

In 2018, the reality is that any software component with more than a few settings needs restart of the affected component (not necessarily whole app) to apply new settings. The complexity+cost is just too high for the meager user benefit.
-- 
Remi Denis-Courmont