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 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?
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.
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
