[pulseaudio-discuss] [PATCH] Added loopback module option (gtk3 version)

[NicoHood]

On 05/23/2018 11:01 AM, Tanu Kaskinen wrote:
> On Sun, 2018-05-13 at 10:42 +0200, archlinux at nicohood.de wrote:
>> From: NicoHood <git at 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