On Mon, 30 Oct 2017 16:15:52 +0200, Tanu Kaskinen <tanuk at iki.fi> wrote:
Hi Tanu,
Here's an updated patch and more precise replies to your comments.
> The division of responsibilities isn't always very clean, but I don't
> have ideas for simple fixes for that. I think PavuApplication should
> create the UI and an object for managing the libpulse stuff, and the
> libpulse stuff shouldn't directly depend on the UI parts. Currently
> the libpulse stuff is mostly handled in pavucontrol.cc, but that is
> very much dependent on the UI stuff, so fixing this would be a big
> project.
I agree on this, and didn't change anything in this regard.
> This is a weird function. The name implies that it will select the
> best tab on some heuristics, but it only does that if default_tab is
> 0. If default_tab is -1, then the function does absolutely nothing.
> The "default_tab = -1" assignment at the end doesn't make any sense.
>
> I think it would be better if this function was only called when the
> caller really wants to apply the heuristics.
The new patch separates "select best tab" and "select what you're told
to" in two functions for clarity.
> Why does the PavuApplication need to be wrapped in a RefPtr?
To build :)
This is apparently inherent to overloading Gtk::Application as I get
compilation errors without it. (I find cpp errors very hard to read but
this is rather clear that it's a template thing).
> > + add_window(*pavucontrol_window);
> > + [...]
> > + auto windows = get_windows();
> > +
> > + if (windows.size() > 0) {
> > + pavucontrol_window =
> > dynamic_cast<Gtk::Window*>(windows[0]);
> > + }
>
> This seems a bit ugly. Couldn't the main window be a member variable
> of PavuApplication?
I've made it a member.
I still have to call Gtk::Application->add_window() (or the GtkApp
doesn't correctly initialize - GtkApps need at least one window
registered). I don't use get_windows() anymore though.
Sidenote: I still have to make an ugly (MainWindow*) cast, because I
can't use MainWindow in pavucontrol.h as I would have to include
mainwindow.h which already includes pavucontrol.h. (Which proves
there's a responsabilities separation issue).
> This is probably part of the "switch tab if already running" logic,
> but I don't understand how that works in practice. If there's already
> one instance running, how does the tab index get passed from the new
> process to the old one? I'm sure I could figure it out from the GLib
> documentation, but there's apparently a lot of magic happening behind
> the scenes, and it would be good to have a comment that explains how
> the magic works.
I've added comments on every Gtk::Application aspect of the thing. I
hope they make things more clear.
Thanks,
--
Colin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-single-launch-with-Gtk-Application.patch
Type: text/x-patch
Size: 15221 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20171031/4269da4e/attachment-0001.bin>
