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
