Added loopback module option (gtk3 version)

Submitted by archlinux@nicohood.de on May 13, 2018, 8:42 a.m.

Details

Message ID 20180513084256.23222-1-archlinux@nicohood.de
State New
Series "Added loopback module option (gtk3 version)"
Headers show

Commit Message

archlinux@nicohood.de May 13, 2018, 8:42 a.m.
From: NicoHood <git@nicohood.de>

---
 src/paprefs.cc    | 32 ++++++++++++++++++++++++++++++++
 src/paprefs.glade | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/paprefs.cc b/src/paprefs.cc
index 123fea5..8bd9884 100644
--- a/src/paprefs.cc
+++ b/src/paprefs.cc
@@ -61,6 +61,7 @@  public:
         *rtpLoopbackCheckButton,
         *rtpPortCheckButton,
         *combineCheckButton,
+        *loopbackCheckButton,
         *upnpMediaServerCheckButton,
         *upnpNullSinkCheckButton;
 
@@ -70,6 +71,7 @@  public:
         *rtpNullSinkRadioButton;
 
     Glib::RefPtr<Gio::Settings> combineSettings;
+    Glib::RefPtr<Gio::Settings> loopbackSettings;
     Glib::RefPtr<Gio::Settings> remoteAccessSettings;
     Glib::RefPtr<Gio::Settings> zeroconfSettings;
     Glib::RefPtr<Gio::Settings> raopSettings;
@@ -89,6 +91,7 @@  public:
     void onChangeRtpReceive();
     void onChangeRtpSend();
     void onChangeCombine();
+    void onChangeLoopback();
     void onChangeUpnp();
 
     void onZeroconfDiscoverInstallButtonClicked();
@@ -110,6 +113,7 @@  public:
     void writeToGSettingsRtpReceive();
     void writeToGSettingsRtpSend();
     void writeToGSettingsCombine();
+    void writeToGSettingsLoopback();
     void writeToGSettingsUPnP();
 
     void onGSettingsChange(const Glib::ustring& key);
@@ -156,6 +160,7 @@  MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
     x->get_widget("rtpLoopbackCheckButton", rtpLoopbackCheckButton);
     x->get_widget("rtpPortCheckButton", rtpPortCheckButton);
     x->get_widget("combineCheckButton", combineCheckButton);
+    x->get_widget("loopbackCheckButton", loopbackCheckButton);
     x->get_widget("upnpMediaServerCheckButton", upnpMediaServerCheckButton);
     x->get_widget("upnpNullSinkCheckButton", upnpNullSinkCheckButton);
 
@@ -169,6 +174,9 @@  MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
     combineSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
                                             MODULE_GROUPS_PATH "/combine/");
 
+    loopbackSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
+                                             MODULE_GROUPS_PATH "/loopback/");
+
     remoteAccessSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
                                                  MODULE_GROUPS_PATH "/remote-access/");
 
@@ -188,6 +196,7 @@  MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
                                          MODULE_GROUPS_PATH "/upnp-media-server/");
 
     combineSettings->signal_changed().connect(sigc::mem_fun(*this, &MainWindow::onGSettingsChange));
+    loopbackSettings->signal_changed().connect(sigc::mem_fun(*this, &MainWindow::onGSettingsChange));
     remoteAccessSettings->signal_changed().connect(sigc::mem_fun(*this, &MainWindow::onGSettingsChange));
     zeroconfSettings->signal_changed().connect(sigc::mem_fun(*this, &MainWindow::onGSettingsChange));
     raopSettings->signal_changed().connect(sigc::mem_fun(*this, &MainWindow::onGSettingsChange));
@@ -215,6 +224,7 @@  MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
     rtpNullSinkRadioButton->signal_clicked().connect(sigc::mem_fun(*this, &MainWindow::onChangeRtpSend));
 
     combineCheckButton->signal_toggled().connect(sigc::mem_fun(*this, &MainWindow::onChangeCombine));
+    loopbackCheckButton->signal_toggled().connect(sigc::mem_fun(*this, &MainWindow::onChangeLoopback));
 
     upnpMediaServerCheckButton->signal_toggled().connect(sigc::mem_fun(*this, &MainWindow::onChangeUpnp));
     upnpNullSinkCheckButton->signal_toggled().connect(sigc::mem_fun(*this, &MainWindow::onChangeUpnp));
@@ -322,6 +332,13 @@  void MainWindow::onChangeCombine() {
     writeToGSettingsCombine();
 }
 
+void MainWindow::onChangeLoopback() {
+    if (ignoreChanges)
+        return;
+
+    writeToGSettingsLoopback();
+}
+
 void MainWindow::onChangeUpnp() {
 
     if (ignoreChanges)
@@ -430,6 +447,20 @@  void MainWindow::writeToGSettingsCombine() {
     combineSettings->apply();
 }
 
+void MainWindow::writeToGSettingsLoopback() {
+    loopbackSettings->delay();
+
+    if (loopbackCheckButton->get_active()) {
+        loopbackSettings->set_string("name0", Glib::ustring("module-loopback"));
+        loopbackSettings->set_string("args0", Glib::ustring("--latency-msec=5"));
+
+        loopbackSettings->set_boolean("enabled", true);
+    } else
+        loopbackSettings->set_boolean("enabled", false);
+
+    loopbackSettings->apply();
+}
+
 void MainWindow::writeToGSettingsRemoteAccess() {
     bool zeroconfEnabled, anonymousEnabled;
 
@@ -663,6 +694,7 @@  void MainWindow::readFromGSettings() {
         rtpNullSinkRadioButton->set_active(TRUE);
 
     combineCheckButton->set_active(combineSettings->get_boolean("enabled"));
+    loopbackCheckButton->set_active(loopbackSettings->get_boolean("enabled"));
 
     upnpMediaServerCheckButton->set_active(upnpSettings->get_boolean("enabled"));
 
diff --git a/src/paprefs.glade b/src/paprefs.glade
index 401e9c1..0f250cc 100644
--- a/src/paprefs.glade
+++ b/src/paprefs.glade
@@ -1,12 +1,15 @@ 
-<?xml version="1.0"?>
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Generated with glade 3.22.1 -->
 <interface>
-  <!-- interface-requires gtk+ 2.16 -->
-  <!-- interface-naming-policy toplevel-contextual -->
+  <requires lib="gtk+" version="3.10"/>
   <object class="GtkWindow" id="mainWindow">
     <property name="can_focus">False</property>
     <property name="title" translatable="yes">PulseAudio Preferences</property>
     <property name="resizable">False</property>
     <property name="icon_name">preferences-desktop</property>
+    <child>
+      <placeholder/>
+    </child>
     <child>
       <object class="GtkVBox" id="vbox20">
         <property name="visible">True</property>
@@ -337,6 +340,9 @@ 
                   </packing>
                 </child>
               </object>
+              <packing>
+                <property name="position">1</property>
+              </packing>
             </child>
             <child type="tab">
               <object class="GtkLabel" id="label3">
@@ -537,6 +543,9 @@ 
                   </packing>
                 </child>
               </object>
+              <packing>
+                <property name="position">2</property>
+              </packing>
             </child>
             <child type="tab">
               <object class="GtkLabel" id="label2">
@@ -572,13 +581,31 @@ 
                     <property name="position">0</property>
                   </packing>
                 </child>
+                <child>
+                  <object class="GtkCheckButton" id="loopbackCheckButton">
+                    <property name="label" translatable="yes">Add _loopback output device for routing audio from a source to a sink</property>
+                    <property name="visible">True</property>
+                    <property name="can_focus">True</property>
+                    <property name="receives_default">False</property>
+                    <property name="use_underline">True</property>
+                    <property name="draw_indicator">True</property>
+                  </object>
+                  <packing>
+                    <property name="expand">False</property>
+                    <property name="fill">False</property>
+                    <property name="position">1</property>
+                  </packing>
+                </child>
               </object>
+              <packing>
+                <property name="position">3</property>
+              </packing>
             </child>
             <child type="tab">
               <object class="GtkLabel" id="label4">
                 <property name="visible">True</property>
                 <property name="can_focus">False</property>
-                <property name="label" translatable="yes">Simultaneous _Output</property>
+                <property name="label" translatable="yes">_Miscellaneous</property>
                 <property name="use_underline">True</property>
               </object>
               <packing>
@@ -625,4 +652,3 @@ 
     </child>
   </object>
 </interface>
-

Comments

Tanu Kaskinen May 23, 2018, 9:01 a.m.
On Sun, 2018-05-13 at 10:42 +0200, archlinux@nicohood.de wrote:
> From: NicoHood <git@nicohood.de>
> 
> ---
>  src/paprefs.cc    | 32 ++++++++++++++++++++++++++++++++
>  src/paprefs.glade | 36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 63 insertions(+), 5 deletions(-)

You need to provide a patch also for
org.freedesktop.pulseaudio.gschema.xml (which is in pulseaudio, not
paprefs).

> @@ -169,6 +174,9 @@ MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
>      combineSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
>                                              MODULE_GROUPS_PATH "/combine/");
>  
> +    loopbackSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
> +                                             MODULE_GROUPS_PATH "/loopback/");

I don't think the loopback settings object will be added to the schema
before PulseAudio 12.0 is released, so you'll have to deal with the
situation where the object doesn't exist. I'm not sure what's the best
way to do that. The create() function is not documented, so I don't
know what it does if the requested path doesn't exist. If it just
returns null and doesn't print any warnings, then you can simply check
if loopbackSettings is null in every place that references it (and grey
out the option in the UI).

If create() doesn't behave nicely with non-existing paths, then you
need to check if the path exists before you call create(). You can use
Gio::Settings::list_children() for that.

In order to test this, you'll need a new PulseAudio version. 11.99.1
contains the necessary stuff, but if Arch doesn't provide that version
and you don't want to install PulseAudio from source, you'll have to
wait for 12.0. You'll still have to modify the schema that is provided
by PulseAudio, though... It should be possible to avoid building
PulseAudio from source, if you just modify the installed schema xml and
then use glib-compile-schemas to apply the changes.

>                      <property name="position">0</property>
>                    </packing>
>                  </child>
> +                <child>
> +                  <object class="GtkCheckButton" id="loopbackCheckButton">
> +                    <property name="label" translatable="yes">Add _loopback output device for routing audio from a source to a sink</property>

The terminology isn't quite right here. A loopback is not an "output
device". I suggest the following wording:

Add a _loopback connection from the default source to the default sink
NicoHood July 1, 2018, 5:25 p.m.
On 05/23/2018 11:01 AM, Tanu Kaskinen wrote:
> On Sun, 2018-05-13 at 10:42 +0200, archlinux@nicohood.de wrote:
>> From: NicoHood <git@nicohood.de>
>>
>> ---
>>  src/paprefs.cc    | 32 ++++++++++++++++++++++++++++++++
>>  src/paprefs.glade | 36 +++++++++++++++++++++++++++++++-----
>>  2 files changed, 63 insertions(+), 5 deletions(-)
> 
> You need to provide a patch also for
> org.freedesktop.pulseaudio.gschema.xml (which is in pulseaudio, not
> paprefs).
> 
>> @@ -169,6 +174,9 @@ MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
>>      combineSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
>>                                              MODULE_GROUPS_PATH "/combine/");
>>  
>> +    loopbackSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
>> +                                             MODULE_GROUPS_PATH "/loopback/");
> 
> I don't think the loopback settings object will be added to the schema
> before PulseAudio 12.0 is released, so you'll have to deal with the
> situation where the object doesn't exist. I'm not sure what's the best
> way to do that. The create() function is not documented, so I don't
> know what it does if the requested path doesn't exist. If it just
> returns null and doesn't print any warnings, then you can simply check
> if loopbackSettings is null in every place that references it (and grey
> out the option in the UI).
> 
> If create() doesn't behave nicely with non-existing paths, then you
> need to check if the path exists before you call create(). You can use
> Gio::Settings::list_children() for that.
> 
> In order to test this, you'll need a new PulseAudio version. 11.99.1
> contains the necessary stuff, but if Arch doesn't provide that version
> and you don't want to install PulseAudio from source, you'll have to
> wait for 12.0. You'll still have to modify the schema that is provided
> by PulseAudio, though... It should be possible to avoid building
> PulseAudio from source, if you just modify the installed schema xml and
> then use glib-compile-schemas to apply the changes.
> 
>>                      <property name="position">0</property>
>>                    </packing>
>>                  </child>
>> +                <child>
>> +                  <object class="GtkCheckButton" id="loopbackCheckButton">
>> +                    <property name="label" translatable="yes">Add _loopback output device for routing audio from a source to a sink</property>
> 
> The terminology isn't quite right here. A loopback is not an "output
> device". I suggest the following wording:
> 
> Add a _loopback connection from the default source to the default sink
> 

Hi Tanu,
thanks for the review. Pulseaudio 12 is now available for Arch and I was
able to retest my changes.

1. You are right that a pulseaudio schema is missing for the loopback
module.
2. The parameter should get corrected:
loopbackSettings->set_string("args0", Glib::ustring("latency_msec=5"));
3. The label name change suggestion from you is better.

The create function seems to create a new path, why should it fail if it
does not yet exist? That is what create is meant for: create something
new, that does not yet exist. I had no problems so far. But I might miss
the point. Now that pulse 12 is out, do we even need to care about that?

Tanu, are you able to do the few minor changes yourself? I think you
have direct git access and can do the few edits real quick.

Thanks
Nico
Tanu Kaskinen July 3, 2018, 6:50 p.m.
On Sun, 2018-07-01 at 19:25 +0200, NicoHood wrote:
> On 05/23/2018 11:01 AM, Tanu Kaskinen wrote:
> > On Sun, 2018-05-13 at 10:42 +0200, archlinux@nicohood.de wrote:
> > > From: NicoHood <git@nicohood.de>
> > > 
> > > ---
> > >  src/paprefs.cc    | 32 ++++++++++++++++++++++++++++++++
> > >  src/paprefs.glade | 36 +++++++++++++++++++++++++++++++-----
> > >  2 files changed, 63 insertions(+), 5 deletions(-)
> > 
> > You need to provide a patch also for
> > org.freedesktop.pulseaudio.gschema.xml (which is in pulseaudio, not
> > paprefs).
> > 
> > > @@ -169,6 +174,9 @@ MainWindow::MainWindow(BaseObjectType* cobject, const Glib::RefPtr<Gtk::Builder>
> > >      combineSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
> > >                                              MODULE_GROUPS_PATH "/combine/");
> > >  
> > > +    loopbackSettings = Gio::Settings::create(MODULE_GROUP_SCHEMA,
> > > +                                             MODULE_GROUPS_PATH "/loopback/");
> > 
> > I don't think the loopback settings object will be added to the schema
> > before PulseAudio 12.0 is released, so you'll have to deal with the
> > situation where the object doesn't exist. I'm not sure what's the best
> > way to do that. The create() function is not documented, so I don't
> > know what it does if the requested path doesn't exist. If it just
> > returns null and doesn't print any warnings, then you can simply check
> > if loopbackSettings is null in every place that references it (and grey
> > out the option in the UI).
> > 
> > If create() doesn't behave nicely with non-existing paths, then you
> > need to check if the path exists before you call create(). You can use
> > Gio::Settings::list_children() for that.
> > 
> > In order to test this, you'll need a new PulseAudio version. 11.99.1
> > contains the necessary stuff, but if Arch doesn't provide that version
> > and you don't want to install PulseAudio from source, you'll have to
> > wait for 12.0. You'll still have to modify the schema that is provided
> > by PulseAudio, though... It should be possible to avoid building
> > PulseAudio from source, if you just modify the installed schema xml and
> > then use glib-compile-schemas to apply the changes.
> > 
> > >                      <property name="position">0</property>
> > >                    </packing>
> > >                  </child>
> > > +                <child>
> > > +                  <object class="GtkCheckButton" id="loopbackCheckButton">
> > > +                    <property name="label" translatable="yes">Add _loopback output device for routing audio from a source to a sink</property>
> > 
> > The terminology isn't quite right here. A loopback is not an "output
> > device". I suggest the following wording:
> > 
> > Add a _loopback connection from the default source to the default sink
> > 
> 
> Hi Tanu,
> thanks for the review. Pulseaudio 12 is now available for Arch and I was
> able to retest my changes.
> 
> 1. You are right that a pulseaudio schema is missing for the loopback
> module.
> 2. The parameter should get corrected:
> loopbackSettings->set_string("args0", Glib::ustring("latency_msec=5"));
> 3. The label name change suggestion from you is better.
> 
> The create function seems to create a new path, why should it fail if it
> does not yet exist? That is what create is meant for: create something
> new, that does not yet exist. I had no problems so far. But I might miss
> the point. Now that pulse 12 is out, do we even need to care about that?

Yes, we need to care. PulseAudio 12 doesn't know about the loopback
settings object, and if a new paprefs version with the loopback feature
is used in combination with PulseAudio 12, paprefs needs to deal with
that.

From your description it sounds like paprefs is capable of creating the
settings object, and PulseAudio is able to use it without an updated
schema. I'm sure that there's still a problem: when using the new
paprefs version for the first time, the loopback option doesn't have
any effect before PulseAudio is restarted, because PulseAudio doesn't
get any notification about the new settings object (this is a
limitation of GSettings). Can you confirm this? You need to remove the
settings object in order to test this, and I don't really know how to
do that... The dconf command line tool doesn't seem to have a delete
command.

Because of this problem, I still think that paprefs shouldn't create
the settings object if it doesn't already exist.