Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-11-02 Thread Colin Leroy 
On Wed, 01 Nov 2017 13:50:02 +0200, Tanu Kaskinen  wrote:

> Thanks! I applied your patch now.

Hi Tanu,

Thanks ! and thanks for your patches on top of it :)

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-11-01 Thread Tanu Kaskinen 
On Tue, 2017-10-31 at 16:20 +0100, Colin Leroy wrote:
> On Mon, 30 Oct 2017 16:15:52 +0200, Tanu Kaskinen  wrote:
> > 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).

I think your problem was that the PavuApplication constructor was
declared as protected, so calling it from main() was not allowed. I
made a patch that removes the RefPtr wrapping.

> > > +add_window(*pavucontrol_window);
> > > +[...]
> > > +auto windows = get_windows();
> > > +
> > > +if (windows.size() > 0) {
> > > +pavucontrol_window =
> > > dynamic_cast(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).

I made a patch for the cast.

> > 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! I applied your patch now.

-- 
Tanu

https://www.patreon.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-10-31 Thread Colin Leroy 
On Mon, 30 Oct 2017 16:15:52 +0200, Tanu Kaskinen  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(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
>From 6326135813c0ec95739db94f0d81a0cd696dd106 Mon Sep 17 00:00:00 2001
From: Colin Leroy 
Date: Thu, 26 Oct 2017 22:20:36 +0200
Subject: [PATCH] Implement single-launch with Gtk::Application

This introduces a new file for clarity. Options
handling changes so that --tab changes the tab
if the window is already opened. Other options
are only used at start time.
---
 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/mainwindow.cc  |  16 +
 src/mainwindow.h   |   3 +
 src/pavuapplication.cc | 181 +
 src/pavuapplication.h  |  55 +++
 src/pavucontrol.cc | 102 
 src/pavucontrol.h  |   2 +
 8 files changed, 274 insertions(+), 87 deletions(-)
 create mode 100644 src/pavuapplication.cc
 create mode 100644 src/pavuapplication.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c952a83..e9d0f55 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -12,3 +12,4 @@ src/sinkwidget.cc
 src/sourceoutputwidget.cc
 src/sourcewidget.cc
 src/streamwidget.cc
+src/pavuapplication.cc
diff --git a/src/Makefile.am b/src/Makefile.am
index 7257260..b5c0314 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
   rolewidget.h rolewidget.cc \
   mainwindow.h mainwindow.cc \
   pavucontrol.h pavucontrol.cc \
+  pavuapplication.cc pavuapplication.h \
   i18n.h
 
 pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
diff --git a/src/mainwindow.cc b/src/mainwindow.cc
index 31d5695..da598fd 100644
--- a/src/mainwindow.cc
+++ b/src/mainwindow.cc
@@ -340,6 +340,22 @@ static void updatePorts(DeviceWidget *w, std::map 
 }
 }
 
+void MainWindow::selectBestTab() {
+if (sinkInputWidgets.size() > 0)
+notebook->set_current_page(0);
+else if (sourceOutputWidgets.size() > 0)
+notebook->set_current_page(1);
+else if (sourceWidgets.size() > 0 && sinkWidgets.size() == 0)
+notebook->set_current_page(3);
+else
+notebook->set_current_page(2);
+}
+
+void MainWindow::selectTab(int tab_number) {
+if (tab_number > 0 && tab_number <= notebook->get_n_pages())
+

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-10-30 Thread Colin Leroy 
On 30 October 2017 at 16h15, Tanu Kaskinen wrote:

Hi, 

> This is a weird function. [...]
> I think it would be better if this function was only called when the
> caller really wants to apply the heuristics.

I'll update the patch to do that.
Below are answers to the rest of your questions:

> > +Glib::RefPtr PavuApplication::create() {
> > +return Glib::RefPtr(new PavuApplication());
> > +
> > +}  
> 
> Why does the PavuApplication need to be wrapped in a RefPtr?

I'm not sure it needs. I used the same general architecture as the
GtkMM Gtk::Application example.

> > +if (windows.size() > 0) {
> > +pavucontrol_window =
> > dynamic_cast(windows[0]);
> > +}  
> 
> This seems a bit ugly. Couldn't the main window be a member variable
> of PavuApplication?

I'll try that. 

> > +
> > +if (pavucontrol_window == NULL) {
> > +pavucontrol_window = create_window();
> > +} else if (tab != -1) {
> > +((MainWindow *)pavucontrol_window)->selectBestTab(tab);
> > +}
> > +
> > +pavucontrol_window->present();
> > +}  
> 
> 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?

We're already in the first existing process here. I think the second
process sends a message to the first one with argc and argv[]
somewhere. It then gets handled by the first process's on_command_line
callback. I'll have to check gtkApplication's doc to know how it's
communicating. I'm guessing Unix socket, at least on Unix systems.

> 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'll add that :)

Thanks!
-- 
Colin


pgpCPDCowD1au.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-10-30 Thread Tanu Kaskinen 
On Thu, 2017-10-26 at 22:28 +0200, Colin Leroy wrote:
> From 2120613fa5bd68ebbdd0e9428357030736fce013 Mon Sep 17 00:00:00 2001
> From: Colin Leroy 
> Date: Thu, 26 Oct 2017 22:20:36 +0200
> Subject: [PATCH] Implement single-launch with Gtk::Application
> 
> This introduces a new file for clarity. Options
> handling changes so that --tab changes the tab
> if the window is already opened. Other options
> are only used at start time.

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.

> ---
>  po/POTFILES.in |   1 +
>  src/Makefile.am|   1 +
>  src/mainwindow.cc  |  18 ++
>  src/mainwindow.h   |   2 +
>  src/pavuapplication.cc | 151 
> +
>  src/pavuapplication.h  |  51 +
>  src/pavucontrol.cc |  98 
>  src/pavucontrol.h  |   2 +
>  8 files changed, 237 insertions(+), 87 deletions(-)
>  create mode 100644 src/pavuapplication.cc
>  create mode 100644 src/pavuapplication.h
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index c952a83..e9d0f55 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -12,3 +12,4 @@ src/sinkwidget.cc
>  src/sourceoutputwidget.cc
>  src/sourcewidget.cc
>  src/streamwidget.cc
> +src/pavuapplication.cc
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7257260..b5c0314 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
>rolewidget.h rolewidget.cc \
>mainwindow.h mainwindow.cc \
>pavucontrol.h pavucontrol.cc \
> +  pavuapplication.cc pavuapplication.h \
>i18n.h
>  
>  pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
> diff --git a/src/mainwindow.cc b/src/mainwindow.cc
> index 31d5695..b73108f 100644
> --- a/src/mainwindow.cc
> +++ b/src/mainwindow.cc
> @@ -340,6 +340,24 @@ static void updatePorts(DeviceWidget *w, 
> std::map 
>  }
>  }
>  
> +void MainWindow::selectBestTab(int default_tab) {
> +if (default_tab != -1) {
> +if (default_tab < 1 || default_tab > notebook->get_n_pages()) {
> +if (sinkInputWidgets.size() > 0)
> +notebook->set_current_page(0);
> +else if (sourceOutputWidgets.size() > 0)
> +notebook->set_current_page(1);
> +else if (sourceWidgets.size() > 0 && sinkWidgets.size() == 0)
> +notebook->set_current_page(3);
> +else
> +notebook->set_current_page(2);
> +} else {
> +notebook->set_current_page(default_tab - 1);
> +}
> +default_tab = -1;
> +}
> +}

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.

> +
>  void MainWindow::updateCard(const pa_card_info ) {
>  CardWidget *w;
>  bool is_new = false;
> diff --git a/src/mainwindow.h b/src/mainwindow.h
> index 30e1ad0..a163069 100644
> --- a/src/mainwindow.h
> +++ b/src/mainwindow.h
> @@ -60,6 +60,8 @@ public:
>  void removeSourceOutput(uint32_t index);
>  void removeClient(uint32_t index);
>  
> +void selectBestTab(int default_tab);
> +
>  void removeAllWidgets();
>  
>  void setConnectingMessage(const char *string = NULL);
> diff --git a/src/pavuapplication.cc b/src/pavuapplication.cc
> new file mode 100644
> index 000..18f8d0b
> --- /dev/null
> +++ b/src/pavuapplication.cc
> @@ -0,0 +1,151 @@
> +/***
> +  This file is part of pavucontrol.
> +
> +  Copyright 2006-2008 Lennart Poettering
> +  Copyright 2008 Sjoerd Simons 
> +
> +  pavucontrol is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU General Public License as published by
> +  the Free Software Foundation, either version 2 of the License, or
> +  (at your option) any later version.
> +
> +  pavucontrol is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with pavucontrol. If not, see .
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include 
> +#endif
> +
> +#include "i18n.h"
> +

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

2017-10-26 Thread Colin Leroy 

Aand, I sent the wrong patch. Here's the correct one.

-- 
Colin
From 2120613fa5bd68ebbdd0e9428357030736fce013 Mon Sep 17 00:00:00 2001
From: Colin Leroy 
Date: Thu, 26 Oct 2017 22:20:36 +0200
Subject: [PATCH] Implement single-launch with Gtk::Application

This introduces a new file for clarity. Options
handling changes so that --tab changes the tab
if the window is already opened. Other options
are only used at start time.
---
 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/mainwindow.cc  |  18 ++
 src/mainwindow.h   |   2 +
 src/pavuapplication.cc | 151 +
 src/pavuapplication.h  |  51 +
 src/pavucontrol.cc |  98 
 src/pavucontrol.h  |   2 +
 8 files changed, 237 insertions(+), 87 deletions(-)
 create mode 100644 src/pavuapplication.cc
 create mode 100644 src/pavuapplication.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c952a83..e9d0f55 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -12,3 +12,4 @@ src/sinkwidget.cc
 src/sourceoutputwidget.cc
 src/sourcewidget.cc
 src/streamwidget.cc
+src/pavuapplication.cc
diff --git a/src/Makefile.am b/src/Makefile.am
index 7257260..b5c0314 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
   rolewidget.h rolewidget.cc \
   mainwindow.h mainwindow.cc \
   pavucontrol.h pavucontrol.cc \
+  pavuapplication.cc pavuapplication.h \
   i18n.h
 
 pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
diff --git a/src/mainwindow.cc b/src/mainwindow.cc
index 31d5695..b73108f 100644
--- a/src/mainwindow.cc
+++ b/src/mainwindow.cc
@@ -340,6 +340,24 @@ static void updatePorts(DeviceWidget *w, std::map 
 }
 }
 
+void MainWindow::selectBestTab(int default_tab) {
+if (default_tab != -1) {
+if (default_tab < 1 || default_tab > notebook->get_n_pages()) {
+if (sinkInputWidgets.size() > 0)
+notebook->set_current_page(0);
+else if (sourceOutputWidgets.size() > 0)
+notebook->set_current_page(1);
+else if (sourceWidgets.size() > 0 && sinkWidgets.size() == 0)
+notebook->set_current_page(3);
+else
+notebook->set_current_page(2);
+} else {
+notebook->set_current_page(default_tab - 1);
+}
+default_tab = -1;
+}
+}
+
 void MainWindow::updateCard(const pa_card_info ) {
 CardWidget *w;
 bool is_new = false;
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 30e1ad0..a163069 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -60,6 +60,8 @@ public:
 void removeSourceOutput(uint32_t index);
 void removeClient(uint32_t index);
 
+void selectBestTab(int default_tab);
+
 void removeAllWidgets();
 
 void setConnectingMessage(const char *string = NULL);
diff --git a/src/pavuapplication.cc b/src/pavuapplication.cc
new file mode 100644
index 000..18f8d0b
--- /dev/null
+++ b/src/pavuapplication.cc
@@ -0,0 +1,151 @@
+/***
+  This file is part of pavucontrol.
+
+  Copyright 2006-2008 Lennart Poettering
+  Copyright 2008 Sjoerd Simons 
+
+  pavucontrol is free software; you can redistribute it and/or modify
+  it under the terms of the GNU General Public License as published by
+  the Free Software Foundation, either version 2 of the License, or
+  (at your option) any later version.
+
+  pavucontrol is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  General Public License for more details.
+
+  You should have received a copy of the GNU General Public License
+  along with pavucontrol. If not, see .
+***/
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include "i18n.h"
+
+#include 
+
+#include "pavuapplication.h"
+#include "pavucontrol.h"
+#include "mainwindow.h"
+
+PavuApplication::PavuApplication() : Gtk::Application("org.pulseaudio.pavucontrol", Gio::ApplicationFlags::APPLICATION_HANDLES_COMMAND_LINE) {
+}
+
+Glib::RefPtr PavuApplication::create() {
+return Glib::RefPtr(new PavuApplication());
+
+}
+
+Gtk::Window* PavuApplication::create_window()
+{
+m = pa_glib_mainloop_new(g_main_context_default());
+g_assert(m);
+
+Gtk::Window* pavucontrol_window = pavucontrol_get_window(m, maximize, retry, tab);
+add_window(*pavucontrol_window);
+
+pavucontrol_window->signal_hide().connect(
+ sigc::bind(sigc::mem_fun(*this,
+ ::on_hide_window), pavucontrol_window));
+
+return pavucontrol_window;
+}
+
+void PavuApplication::on_activate()
+{
+Gtk::Window* pavucontrol_window = NULL;
+
+auto windows = get_windows();
+
+if (windows.size() > 0) {
+pavucontrol_window =