After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 629455 - Audio volume setting needs PulseAudio
Audio volume setting needs PulseAudio
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks: 630736
 
 
Reported: 2010-09-12 20:09 UTC by Giovanni Campagna
Modified: 2010-10-20 15:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Introduce ShellAudio PulseAudio wrapper (15.01 KB, patch)
2010-09-12 20:10 UTC, Giovanni Campagna
none Details | Review
Introduce ShellAudio PulseAudio wrapper (22.13 KB, patch)
2010-09-15 17:07 UTC, Giovanni Campagna
needs-work Details | Review
Introduce ShellAudio (fixed) (24.68 KB, patch)
2010-09-15 20:12 UTC, Giovanni Campagna
none Details | Review
Status area: implement audio volume indicator (6.09 KB, patch)
2010-09-15 20:38 UTC, Giovanni Campagna
none Details | Review
Introduce ShellAudio (24.79 KB, patch)
2010-09-26 15:09 UTC, Giovanni Campagna
none Details | Review
Add private gnome-volume-control library (209.26 KB, patch)
2010-09-27 14:29 UTC, Bastien Nocera
none Details | Review
Add private gnome-volume-control library (212.00 KB, patch)
2010-10-06 20:48 UTC, Giovanni Campagna
reviewed Details | Review
Add volume indicator (9.45 KB, patch)
2010-10-07 14:42 UTC, Giovanni Campagna
needs-work Details | Review
current volume UI (24.71 KB, image/png)
2010-10-08 15:30 UTC, Dan Winship
  Details
Add private gnome-volume-control library (210.91 KB, patch)
2010-10-08 16:15 UTC, Giovanni Campagna
reviewed Details | Review
Add volume indicator (9.38 KB, patch)
2010-10-08 17:42 UTC, Giovanni Campagna
reviewed Details | Review
Solve spacing issue in system status area (866 bytes, patch)
2010-10-08 17:45 UTC, Giovanni Campagna
committed Details | Review
Cleanups from gnome-media (4.04 KB, patch)
2010-10-18 17:59 UTC, Bastien Nocera
none Details | Review
Add private gnome-volume-control library (210.94 KB, patch)
2010-10-18 18:00 UTC, Bastien Nocera
none Details | Review
Add volume indicator (12.55 KB, patch)
2010-10-18 19:46 UTC, Giovanni Campagna
reviewed Details | Review
Add private gnome-volume-control library (212.46 KB, patch)
2010-10-18 20:04 UTC, Bastien Nocera
reviewed Details | Review
Add private gnome-volume-control library (213.57 KB, patch)
2010-10-19 14:22 UTC, Bastien Nocera
committed Details | Review
Add volume indicator (15.26 KB, patch)
2010-10-19 16:18 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2010-09-12 20:09:56 UTC
... but PulseAudio is not introspected (and generally too complex to be used from JS).
We need a small ShellAudio in C, exposing as little as needed to the system audio indicator (to be written).

(This assumes that the current decision about libgnomevolumecontrol won't change)
Comment 1 Giovanni Campagna 2010-09-12 20:10:24 UTC
Created attachment 170094 [details] [review]
Introduce ShellAudio PulseAudio wrapper

Add a ShellAudio class which is introspectable and has muted/volume
properties hooked to the default sink volume, for use by the system
audio indicator.
Comment 2 Giovanni Campagna 2010-09-15 17:07:11 UTC
Created attachment 170358 [details] [review]
Introduce ShellAudio PulseAudio wrapper

Add a ShellAudio class which is introspectable and has muted/volume
properties hooked to the default sink and source volumes, as well as
some hopefully localized names, for use by the system audio indicator.
Comment 3 Dan Winship 2010-09-15 17:53:14 UTC
Comment on attachment 170358 [details] [review]
Introduce ShellAudio PulseAudio wrapper

I don't know libpulse at all, so I can't comment much on the guts of the patch...


>+                                 libpulse
>+                                 libpulse-mainloop-glib)

We will need changes to tools/build/gnome-shell-build-setup.sh to depend on those as well. (Figure out the packages for whatever distro you use at least.)

>+struct _ShellAudioClass {
>+  GObjectClass parent_class;
>+};
>+
>+struct _ShellAudio {

These should be in the .h file, with most of the current contents of ShellAudio moved to a ShellAudioPrivate.

>+  gchar* sink_name;

"gchar *sink_name" (likewise elsewhere. "*"s should always be attached to variable names, not type names).

>+static void
>+generic_completion_callback (pa_context *context, int success, void* userdata) {

"{" should be on the next line

>+  g_return_if_fail(SHELL_IS_AUDIO (userdata));

space after "fail"

>+  if (!success) {

"{" should be on the next line. etc, etc

>+  case PROP_SINK_VOLUME:
>+    g_value_set_double (value, shell_audio_get_sink_volume (SHELL_AUDIO (gobject)));
>+    return;
>+  case PROP_SINK_MUTED:
>+    g_value_set_boolean (value, shell_audio_is_sink_muted (SHELL_AUDIO (gobject)));

would be cleaner if you just did "ShellAudio *audio = SHELL_AUDIO (gobject);" at the start.

>+    pa_operation_unref (pa_context_set_sink_mute_by_index (self->context, self->default_sink, 1, generic_completion_callback, self));

this would be clearer if it was split into two statements. (likewise everywhere else you do "pa_operation_unref (thing_that_you_actually_care_about ())"

>+shell_audio_get_source_name (ShellAudio *self) {
>+  return self->sink_name;

s/sink/source/

>+  self->sink_name = g_strdup(_("no sink available"));
>+  self->source_name = g_strdup(_("no source available"));

Not sure it makes sense to specify these strings here... it would be better to have it just return NULL, and the caller can represent that however it wants.

>+  pa_proplist_sets (proplist, "application.process.binary", "mutter");

might be better to use g_get_prgname() there; if the binary name ever changed, people wouldn't remember that it needed to be updated here.

>+  properties[PROP_SINK_NAME] = g_param_spec_string ("sink-name",
>+      "Default sink name",

second line should line up with the '"' of the first

>+      "", G_PARAM_READABLE | G_PARAM_STATIC_NICK | G_PARAM_STATIC_NAME | G_PARAM_STATIC_BLURB);

flags are generally put on a separate line from the default value

>+void shell_audio_set_sink_muted (ShellAudio*, gboolean);
>+gboolean shell_audio_is_sink_muted (ShellAudio*);

some lining-up-declarations would be good here

>+  global->audio = SHELL_AUDIO (g_object_new (SHELL_TYPE_AUDIO, NULL));

you don't need to cast the return value of g_object_new()
Comment 4 Giovanni Campagna 2010-09-15 20:12:59 UTC
Created attachment 170370 [details] [review]
Introduce ShellAudio (fixed)

Fixed style issues, as well as bugs (I've also completed the JS indicator part, but it is not ready yet)
Comment 5 Giovanni Campagna 2010-09-15 20:38:06 UTC
Created attachment 170372 [details] [review]
Status area: implement audio volume indicator

Replace current status icon with a JS indicator leveraging the new
ShellAudio and offering the usual consistent behaviour.
Comment 6 Bastien Nocera 2010-09-20 13:52:52 UTC
Why on earth would you want to use PulseAudio directly, when the only thing you need is to have 2 or 3 data structures from PA made introspectable?

gvc_mixer_control_get_pa_context() which uses a pa_context * doesn't need to be exported for the audio volume monitor, and pa_volume_t is an opaque datatype, but typedef to guint32.

So Pulseaudio itself probably doesn't even need to be made introspectable.
Comment 7 Owen Taylor 2010-09-20 15:30:01 UTC
(In reply to comment #6)
> Why on earth would you want to use PulseAudio directly, when the only thing you
> need is to have 2 or 3 data structures from PA made introspectable?
> 
> gvc_mixer_control_get_pa_context() which uses a pa_context * doesn't need to be
> exported for the audio volume monitor, and pa_volume_t is an opaque datatype,
> but typedef to guint32.
> 
> So Pulseaudio itself probably doesn't even need to be made introspectable.

Bastien - can you be a little more specific about how you think this should be approached? It sounds like there is some GNOME library that you think should be used instead of PulseAudio directly?

If only a small amount of PulseAudio needs to be introspected to use this library (perhaps just pa_volume_t) - how do you see that being handled:

 - Building a mini-PulseAudio introspection gir as part of GNOME?
 - Doing a partial job adding introspection to PulsaAudio?
 - Using C glue code in gnome-shell?
Comment 8 Bastien Nocera 2010-09-20 15:40:20 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Why on earth would you want to use PulseAudio directly, when the only thing you
> > need is to have 2 or 3 data structures from PA made introspectable?
> > 
> > gvc_mixer_control_get_pa_context() which uses a pa_context * doesn't need to be
> > exported for the audio volume monitor, and pa_volume_t is an opaque datatype,
> > but typedef to guint32.
> > 
> > So Pulseaudio itself probably doesn't even need to be made introspectable.
> 
> Bastien - can you be a little more specific about how you think this should be
> approached?

As I mentioned to Giovanni, the C code should be cut'n'pasted from gnome-media, into gnome-shell, to build a private introspected library. Giovanni mentioned that he didn't manage to make the library introspectable because it would have needed PulseAudio to be introspectable.

This isn't the fact, as very few PA implementation details leak into the headers that should be exported.

> It sounds like there is some GNOME library that you think should be
> used instead of PulseAudio directly?

Not really a library, but the code in gnome-media/gnome-volume-control/src is mostly stand-alone (apart from a few defines that PA brings in).

> If only a small amount of PulseAudio needs to be introspected to use this
> library (perhaps just pa_volume_t) - how do you see that being handled:
> 
>  - Building a mini-PulseAudio introspection gir as part of GNOME?

Not necessary.

>  - Doing a partial job adding introspection to PulsaAudio?
>  - Using C glue code in gnome-shell?

Add some C glue with defines to gnome-media's original code would probably be more than enough.

In pseudo code, something like:
#if GETTEXT_PACKAGE == "gnome-shell"
typedef guint32 pa_volume_t;
#else
#include <pulseaudio.h>
#endif

and ifdef the functions that need pa_context_t the same way. We could also move them to a separate include file which you'd replace for gnome-shell (gvc-pa-compat.h or something).
Comment 9 Giovanni Campagna 2010-09-25 20:18:42 UTC
First of all, I should have referenced bug 624951 and bug 625029.

Second, libgnomevolumecontrol is not just a layer over PulseAudio, is a lot of code like Gtk sliders, status icons, applets and stuff which should not be copied over to gnome-shell, and dividing them is not as easy as it stands.

Thirdly, the choice of copying code between gnome-settings-daemon and gnome-media has worked so far because Bastien maintains both modules, which is not true for gnome-shell (IIRC), so the gnome-shell code would end being separately maintained.

The sum of this and having integrated working code which is the minimum needed means that using libgnomevolumecontrol would just change the namespace in JS files.
Comment 10 Giovanni Campagna 2010-09-26 15:09:59 UTC
Created attachment 171130 [details] [review]
Introduce ShellAudio

Fixed a build problem with gobject-introspection and parameter names
Comment 11 Bastien Nocera 2010-09-27 12:40:24 UTC
(In reply to comment #9)
> First of all, I should have referenced bug 624951 and bug 625029.
> 
> Second, libgnomevolumecontrol is not just a layer over PulseAudio, is a lot of
> code like Gtk sliders, status icons, applets and stuff which should not be
> copied over to gnome-shell, and dividing them is not as easy as it stands.

libgnomevolumecontrol is a private library, you're comparing apple and oranges. I'm asking you to copy/paste the very few files you'll actually need:
gvc-channel-map.c
gvc-mixer-card.c
gvc-mixer-control.c
gvc-mixer-event-role.c
gvc-mixer-sink.c
gvc-mixer-sink-input.c
gvc-mixer-source.c
gvc-mixer-source-output.c
gvc-mixer-stream.c
(with the corresponding .h files)

> Thirdly, the choice of copying code between gnome-settings-daemon and
> gnome-media has worked so far because Bastien maintains both modules, which is
> not true for gnome-shell (IIRC), so the gnome-shell code would end being
> separately maintained.

Those files actually change very seldom, so I certainly don't think it would be that much work to monitor gnome-media for required updates. And I'm certainly willing to update the gvc cut'n'paste code in gnome-shell when needed.

> The sum of this and having integrated working code which is the minimum needed
> means that using libgnomevolumecontrol would just change the namespace in JS
> files.

Why not do that in the first place instead of calling into pulseaudio then?
Comment 12 Bastien Nocera 2010-09-27 14:29:27 UTC
Created attachment 171208 [details] [review]
Add private gnome-volume-control library

The library is introspected, and should not require using
Pulseaudio directly.
Comment 13 Bastien Nocera 2010-09-27 14:32:32 UTC
Gio, any chance you could hook up and test the introspected library this patch adds?

The library is installed in a private directory. A number of the functions aren't introspected, but they shouldn't actually be introspected, or made public.

The only thing you might need to define yourself is PA_VOLUME_NORM, for use in the status icon itself.
Comment 14 Giovanni Campagna 2010-09-29 16:07:14 UTC
Introspection fails on Gvc, as all Gvc objects are constructed with a pa_context*, which is an unresolved type (and anyway one that JS cannot provide)

Also, you need to set --identifier-prefix and --symbol-prefix if you change the namespace, and many functions are missing (transfer) annotations.
Comment 15 Bastien Nocera 2010-09-29 17:11:21 UTC
(In reply to comment #14)
> Introspection fails on Gvc, as all Gvc objects are constructed with a
> pa_context*, which is an unresolved type (and anyway one that JS cannot
> provide)

I was certainly getting introspection working (the gir file wasn't empty) when compiling with g-i 0.9.3. I don't see why it would need pa_context when no new objects should be instantiated by the code in gnome-shell.

> Also, you need to set --identifier-prefix and --symbol-prefix if you change the
> namespace, and many functions are missing (transfer) annotations.

Right, I know that annotations are missing. The code was never meant to be used outside of gnome-volume-control, and the need didn't come up. I guess I could merge in your code from the old bugs to get that working.
Comment 16 Giovanni Campagna 2010-09-29 19:10:05 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > Introspection fails on Gvc, as all Gvc objects are constructed with a
> > pa_context*, which is an unresolved type (and anyway one that JS cannot
> > provide)
> 
> I was certainly getting introspection working (the gir file wasn't empty) when
> compiling with g-i 0.9.3. I don't see why it would need pa_context when no new
> objects should be instantiated by the code in gnome-shell.

You need at least to instantiate a GvcMixerSink and a GvcMixerSource, actually, as well as a GvcMixerControl.

> 
> > Also, you need to set --identifier-prefix and --symbol-prefix if you change the
> > namespace, and many functions are missing (transfer) annotations.
> 
> Right, I know that annotations are missing. The code was never meant to be used
> outside of gnome-volume-control, and the need didn't come up. I guess I could
> merge in your code from the old bugs to get that working.

Either that, or make warning not fatal when building.
Comment 17 Giovanni Campagna 2010-10-06 20:48:27 UTC
Created attachment 171850 [details] [review]
Add private gnome-volume-control library

Fixed introspection warnings and bugs.
Comment 18 Giovanni Campagna 2010-10-07 14:42:44 UTC
Created attachment 171899 [details] [review]
Add volume indicator

Add volume control indicator which uses API from gnome-volume-control
to interact with PulseAudio and shows both input and output volumes.
Comment 19 Dan Winship 2010-10-07 18:14:36 UTC
Please mark old patches obsolete (click on the "details" link of the patch, then edit it there) if they no longer need to be reviewed due to new patches. (If you're using git-bz, use the -e flag when attaching the new patches and you're given the option to obsolete old patches).
Comment 20 Dan Winship 2010-10-08 15:12:02 UTC
Comment on attachment 171850 [details] [review]
Add private gnome-volume-control library

>+# FIXME make optional
>+PKG_CHECK_MODULES(SHELL_AUDIO, libpulse libpulse-mainloop-glib gobject-2.0)

why optional?

>+  src/cut-n-paste/Makefile

The build stuff here is inconsistent with the rest of gnome-shell. The code should go into src/gvc, and there should be a src/Makefile-gvc.am to build it. (A la gdmuser.)

>+libshell_audio_la_SOURCES =		\
>+	gvc-mixer-stream.h		\
>+	gvc-mixer-stream.c		\
>+	gvc-mixer-stream-private.h	\
>+	gvc-channel-map.h		\
>+	gvc-channel-map.c		\
>+	gvc-channel-map-private.h	\
>+	gvc-mixer-card.c		\
>+	gvc-mixer-card.h		\
>+	gvc-mixer-card-private.h	\
>+	gvc-mixer-sink.h		\
>+	gvc-mixer-sink.c		\
>+	gvc-mixer-source.h		\
>+	gvc-mixer-source.c		\
>+	gvc-mixer-sink-input.h		\
>+	gvc-mixer-sink-input.c		\
>+	gvc-mixer-source-output.h	\
>+	gvc-mixer-source-output.c	\
>+	gvc-mixer-event-role.h		\
>+	gvc-mixer-event-role.c		\
>+	gvc-mixer-control.h		\
>+	gvc-mixer-control.c		\
>+	gvc-mixer-control-private.h

This is an absolutely crazy amount of code to import to get a simple volume control... but I suppose we can revisit this decision later.

>+		--namespace=ShellAudio			\

I'd just keep the existing "gvc" prefix (and likewise for the library name). (This matches what we do for gdmuser, and makes it clearer that this is somebody else's code.)

>+MAINTAINERCLEANFILES =                  \
>+        *~                              \
>+        Makefile.in

These are both wrong. *~ shouldn't be there because it's not generated by anything in the build process, and Makefile.in shouldn't be there because maintainer-clean isn't supposed to delete anything that would be shipped in the tarball.

>diff --git a/src/cut-n-paste/gvc-channel-map-private.h b/src/cut-n-paste/gvc-channel-map-private.h

I'm not going to look at any of the gvc code... I'll assume it's all fine.
Comment 21 Giovanni Campagna 2010-10-08 15:24:29 UTC
(In reply to comment #20)
> >+libshell_audio_la_SOURCES =		\
> >+	gvc-mixer-stream.h		\
> >+	gvc-mixer-stream.c		\
> >+	gvc-mixer-stream-private.h	\
> >+	gvc-channel-map.h		\
> >+	gvc-channel-map.c		\
> >+	gvc-channel-map-private.h	\
> >+	gvc-mixer-card.c		\
> >+	gvc-mixer-card.h		\
> >+	gvc-mixer-card-private.h	\
> >+	gvc-mixer-sink.h		\
> >+	gvc-mixer-sink.c		\
> >+	gvc-mixer-source.h		\
> >+	gvc-mixer-source.c		\
> >+	gvc-mixer-sink-input.h		\
> >+	gvc-mixer-sink-input.c		\
> >+	gvc-mixer-source-output.h	\
> >+	gvc-mixer-source-output.c	\
> >+	gvc-mixer-event-role.h		\
> >+	gvc-mixer-event-role.c		\
> >+	gvc-mixer-control.h		\
> >+	gvc-mixer-control.c		\
> >+	gvc-mixer-control-private.h
> 
> This is an absolutely crazy amount of code to import to get a simple volume
> control... but I suppose we can revisit this decision later.

This is because GVC is a very generic GObject binding for PulseAudio, including things like GvcChannelMap or GvcSinkInput which are not used, unlike the original ShellAudio class, which was only one file (plus one header) and only served a single purpose (setting the default sink and source volumes).
I suppose the decision on this depends on how much you want to grow the sound indicator. With GVC, for example, only JS is missing to make it set volumes for all applications registering and reproducing audio, like Ubuntu's Sound Menu.
Comment 22 Dan Winship 2010-10-08 15:28:52 UTC
Comment on attachment 171899 [details] [review]
Add volume indicator

the spacing in the status area is wrong now: the gap between a11y and volume is too large.

>Add volume control indicator which uses API from gnome-volume-control
>to interact with PulseAudio and shows both input and output volumes.

So, we need some way to distinguish input and output volumes if we're going to have both, and there's no current design for that on the wiki. But I don't think the designers are going to like how you did it (especially the explicit % indication). I'm going to attach a screenshot and mark this ui-review.

>+        PanelMenu.SystemStatusButton.prototype._init.call(this, 'audio-volume-muted-symbolic', null);

You don't need to specify -symbolic here or in volumeToIcon. It should use the symbolic icon automatically.

>+        this._control.open();

is is possible this may cause signals to be emitted before returning? If so it should be moved after the variable initializations...

>+                if (!id || (id == 'org.gnome.VolumeControl' || id == 'org.PulseAudio.pavucontrol')) {
>+                    showSource = true;

Having one magic string would be plausible... having two magic strings suggests maybe we might need more in some cases? What do those two strings refer to / Why just those ones?
Comment 23 Dan Winship 2010-10-08 15:30:40 UTC
Created attachment 171953 [details]
current volume UI

(Note that the input section is only visible if something is using the microphone.)
Comment 24 Bastien Nocera 2010-10-08 15:54:37 UTC
(In reply to comment #22)
> >+                if (!id || (id == 'org.gnome.VolumeControl' || id == 'org.PulseAudio.pavucontrol')) {
> >+                    showSource = true;
> 
> Having one magic string would be plausible... having two magic strings suggests
> maybe we might need more in some cases? What do those two strings refer to /
> Why just those ones?

That will ignore pavucontrol and the sound panel's input sinks (iirc), that are used to show volume levels.
Comment 25 Bastien Nocera 2010-10-08 15:55:12 UTC
Note that the gvc cut'n'paste might need some cleaning up as well, to integrate the comments from bug 630736.
Comment 26 Giovanni Campagna 2010-10-08 16:15:27 UTC
Created attachment 171956 [details] [review]
Add private gnome-volume-control library

The library is introspected, and should not require using
Pulseaudio directly.
Comment 27 Giovanni Campagna 2010-10-08 17:39:24 UTC
(In reply to comment #25)
> Note that the gvc cut'n'paste might need some cleaning up as well, to integrate
> the comments from bug 630736.

You don't need that, as long as you provide the correct annotations (you can override the GIR type in an annotation).

(In reply to comment #22)
> (From update of attachment 171899 [details] [review])
> the spacing in the status area is wrong now: the gap between a11y and volume is
> too large.

I knew that, back when the new status tray was introduced, parallel to the legacy one (bug 621705), but you never answered. Anyway, patch coming.

> >+        this._control.open();
> 
> is is possible this may cause signals to be emitted before returning? If so it
> should be moved after the variable initializations...

All of PulseAudio API is asynchronous, so I suppose no, but fixed anyway.

Patch coming!
Comment 28 Giovanni Campagna 2010-10-08 17:42:51 UTC
Created attachment 171963 [details] [review]
Add volume indicator

Add volume control indicator which uses API from gnome-volume-control
to interact with PulseAudio and shows both input and output volumes in
a PopupMenu.
Comment 29 Giovanni Campagna 2010-10-08 17:45:37 UTC
Created attachment 171964 [details] [review]
Solve spacing issue in system status area

Shell implemented indicators got padding from default button style,
and also spacing from their container, so too much space was left
between them. This only became apparent when more than one was
implemented, though.
Comment 30 William Jon McCann 2010-10-09 20:57:22 UTC
Trying this out.  I'm still not convinced that we want this horizontal layout instead of the much simpler one found:
http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/AudioVolume

But I do have to admit you did a very nice job with it so far.  So from testing it the way it is now:

 * The indicator icon should update live as you move the slider (instant apply)
 * Preferences should be capitalized
 * The width of the menu changes when I change from 0% to 10% to 100% presumably due to the width of the value label.  It should stay the same size
 * It should make a pop sound when finished dragging to a new value.  This provides important feedback - an auditory rather than visual indicator of level.
 * I think I would prefer if the "Output: 30%" string was in a slightly grayer color so help indicate that it isn't an active option.  Similar to what you see in (See top menu item in):
http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/Bluetooth

Beyond that it seems to work great.  Nice work so far.
Comment 31 William Jon McCann 2010-10-09 21:14:34 UTC
One more thing.  It might be nice if the slider was already focused when the menu is dropped down so it can be used without navigating to it.
Comment 32 Dan Winship 2010-10-12 17:03:02 UTC
(In reply to comment #30)
> Trying this out.  I'm still not convinced that we want this horizontal layout
> instead of the much simpler one found:
> http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/AudioVolume

There was a discussion, at some point, with someone, in which it was agreed that the horizontal layout made more sense... Alas, I have no more specific memory of it than that :-}

Of course, the fact that we're doing the UI in the shell rather than having external applets means we can easy swap the slider to be vertical later if we want.

>  * I think I would prefer if the "Output: 30%" string was in a slightly grayer
> color so help indicate that it isn't an active option.

It *is* active though; it doubles as the "mute" button.
Comment 33 Dan Winship 2010-10-12 17:22:57 UTC
Comment on attachment 171956 [details] [review]
Add private gnome-volume-control library

This looks good. The only thing I'd change is, for consistency with gdmuser, move the Gvc-1.0.gir rule into Makefile.am, not Makefile-gvc.am.
Comment 34 Dan Winship 2010-10-15 14:03:40 UTC
(In reply to comment #24)
> (In reply to comment #22)
> > What do those two strings refer to /
> > Why just those ones?
> 
> That will ignore pavucontrol and the sound panel's input sinks (iirc), that are
> used to show volume levels.

So what does that mean? Is it expected that gvc users should explicitly check for and ignore exactly those two strings?
Comment 35 Bastien Nocera 2010-10-16 23:46:19 UTC
(In reply to comment #34)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > What do those two strings refer to /
> > > Why just those ones?
> > 
> > That will ignore pavucontrol and the sound panel's input sinks (iirc), that are
> > used to show volume levels.
> 
> So what does that mean? Is it expected that gvc users should explicitly check
> for and ignore exactly those two strings?

It means that if you don't block those 2 IDs, it will look like there's a microphone recording when pavucontrol or gnome-volume-control is running, because they create fake input devices that are really snooping on the data coming in from the microphone.
Comment 36 Dan Winship 2010-10-18 14:19:31 UTC
(In reply to comment #35)
> It means that if you don't block those 2 IDs, it will look like there's a
> microphone recording when pavucontrol or gnome-volume-control is running,
> because they create fake input devices that are really snooping on the data
> coming in from the microphone.

Ah, but if the user opens the Skype audio preferences dialog, which does the same thing, then it *will* show up in the volume menu, because we're not special-casing Skype's name. It seems like there should be some way for the app to indicate to pulseaudio that it's just doing the "showing the input level" thing, so other volume controls can use that information directly rather than needing a list of special cases. (Or maybe have an API to open the microphone device but get back just the input level rather than the actual sound data, which would then not show up as a source to other apps? Or something...)
Comment 37 Bastien Nocera 2010-10-18 14:29:47 UTC
(In reply to comment #36)
> (In reply to comment #35)
> > It means that if you don't block those 2 IDs, it will look like there's a
> > microphone recording when pavucontrol or gnome-volume-control is running,
> > because they create fake input devices that are really snooping on the data
> > coming in from the microphone.
> 
> Ah, but if the user opens the Skype audio preferences dialog, which does the
> same thing, then it *will* show up in the volume menu, because we're not
> special-casing Skype's name.

Most likely not, unless they use native PulseAudio ways of doing this themselves. The API I'm talking about is only for volume control applications, and the likes. If you're already capturing the audio, as Skype is likely doing, there's no point in using this API, and thus no point in working around it in volume controls.

> It seems like there should be some way for the app
> to indicate to pulseaudio that it's just doing the "showing the input level"
> thing, so other volume controls can use that information directly rather than
> needing a list of special cases. (Or maybe have an API to open the microphone
> device but get back just the input level rather than the actual sound data,
> which would then not show up as a source to other apps? Or something...)

Probably, but that's beyond the scope of this discussion.
Comment 38 Dan Winship 2010-10-18 14:29:47 UTC
Comment on attachment 171963 [details] [review]
Add volume indicator

>+const VOLUME_MAX = 65536.0; /* PA_VOLUME_NORM */

Not a bug in your patch, but it seems like gvc ought to be defining that?

>+        this._sink = null;

"source"/"sink" is pretty technical, and I have to stop and think to figure out which is which. Can you rename them to "input"/"output" or "speaker"/"microphone" or something? (All of the sink*/source* variables, not just _sink and _source.)

>+                let id = outputStream.get_application_id();
>+                if (!id || (id == 'org.gnome.VolumeControl' || id == 'org.PulseAudio.pavucontrol')) {
>+                    showSource = true;

add a comment explaining this. eg, "Don't show the input volume if the microphone is only being read to show an input level display in the sound capplet."

Also, isn't the test backward? You want to show the source if the id *isn't* one of those two, right?


Also, the stuff Jon said in comment 30.
Comment 39 Giovanni Campagna 2010-10-18 14:38:28 UTC
(In reply to comment #38)
> Also, isn't the test backward? You want to show the source if the id *isn't*
> one of those two, right?

Yes, it was a bug, now I fixed it, but I cannot test it since gnome-shell fails to run at the moment.

I will post an updated patch as soon as I have it checked.
Comment 40 Dan Winship 2010-10-18 14:49:54 UTC
(In reply to comment #37)
> Most likely not, unless they use native PulseAudio ways of doing this
> themselves. The API I'm talking about is only for volume control applications,
> and the likes. If you're already capturing the audio, as Skype is likely doing,
> there's no point in using this API, and thus no point in working around it in
> volume controls.

No, I meant when you're not actually talking. You can go into the audio preferences, and it shows a little input level meter so you can verify that your microphone is working, just like the sound capplet does.

> Probably, but that's beyond the scope of this discussion.

Yes
Comment 41 Bastien Nocera 2010-10-18 17:59:48 UTC
Created attachment 172633 [details] [review]
Cleanups from gnome-media

On top of https://bugzilla.gnome.org/attachment.cgi?id=171956
Comment 42 Bastien Nocera 2010-10-18 18:00:40 UTC
Created attachment 172634 [details] [review]
Add private gnome-volume-control library

The library is introspected, and should not require using
Pulseaudio directly.
Comment 43 Bastien Nocera 2010-10-18 18:01:20 UTC
(In reply to comment #41)
> Created an attachment (id=172633) [details] [review]
> Cleanups from gnome-media
> 
> On top of https://bugzilla.gnome.org/attachment.cgi?id=171956

For reference only.
Comment 44 Giovanni Campagna 2010-10-18 19:41:51 UTC
Review of attachment 172634 [details] [review]:

::: src/Makefile-gvc.am
@@ +1,3 @@
+CLEANFILES =
+
+noinst_LTLIBRARIES = libgvc.la

CLEANFILES is not needed (set by toplevel Makefile.am), while noinst_LTLIBRARIES needs +=, else libst and libgdmuser are not built.

::: src/gvc/gvc-mixer-stream.h
@@ +23,3 @@
+
+#include <glib-object.h>
+#include "gvc-pulseaudio-fake.h"

This file is missing in your patch
Comment 45 Giovanni Campagna 2010-10-18 19:46:19 UTC
Created attachment 172639 [details] [review]
Add volume indicator

All suggestions included, bugs fixed. Tested with previous libgvc patch (which needs the same Makefile.am fix as the new one, if you want to test it yourselves).
Comment 46 Bastien Nocera 2010-10-18 20:04:57 UTC
Created attachment 172641 [details] [review]
Add private gnome-volume-control library

The library is introspected, and should not require using
Pulseaudio directly.

With help from Giovanni Campagna <scampa.giovanni@gmail.com>
(introspection annotations, build fixes)
Comment 47 Bastien Nocera 2010-10-18 20:06:23 UTC
Any chance you could test your new patch against this one? My system is 3D drivers challenged right now.
Comment 48 Giovanni Campagna 2010-10-18 20:24:19 UTC
Patch tested with the JS indicator, everything works cleanly.
Comment 49 Dan Winship 2010-10-19 14:01:15 UTC
Comment on attachment 172641 [details] [review]
Add private gnome-volume-control library

Need to kill off the g_debug()s. Look at how the gdmuser code does it; add -DG_LOG_DOMAIN=\"Gvc\" to Makefile-gvc.am, and then call g_set_log_handler() like in gdm_user_manager_ref_default() to ignore debug level messages in that domain. (You can put the g_set_log_handler() call in the shell sources if you don't want to modify the gvc sources. Or alternatively, add it upstream, but wrapped in an #ifdef.)

OK to commit with that fixed.
Comment 50 Bastien Nocera 2010-10-19 14:22:01 UTC
Created attachment 172725 [details] [review]
Add private gnome-volume-control library

The library is introspected, and should not require using
Pulseaudio directly.

With help from Giovanni Campagna <scampa.giovanni@gmail.com>
(introspection annotations, build fixes)
Comment 51 Bastien Nocera 2010-10-19 14:57:33 UTC
Comment on attachment 172725 [details] [review]
Add private gnome-volume-control library

Attachment 172725 [details] pushed as 8064c6c - Add private gnome-volume-control library
Comment 52 Dan Winship 2010-10-19 15:35:36 UTC
Comment on attachment 172639 [details] [review]
Add volume indicator

>+    _notifyVolumeChange: function() {
>+        let p = new Shell.Process({ args: [ 'canberra-gtk-play', '-i', 'audio-volume-change' ] });

That's ugly, and potentially a little bit slow... It would be better to add a shell_global_play_theme_sound() method or something. Mutter is already linked to libcanberra, so that won't add any new dependencies to the build.
Comment 53 Giovanni Campagna 2010-10-19 15:46:06 UTC
(In reply to comment #52)
> (From update of attachment 172639 [details] [review])
> >+    _notifyVolumeChange: function() {
> >+        let p = new Shell.Process({ args: [ 'canberra-gtk-play', '-i', 'audio-volume-change' ] });
> 
> That's ugly, and potentially a little bit slow... It would be better to add a
> shell_global_play_theme_sound() method or something. Mutter is already linked
> to libcanberra, so that won't add any new dependencies to the build.

Better yet it would be for libcanberra to be introspected, but in the mean time I'll add shell_global_play_theme_sound()
Comment 54 Giovanni Campagna 2010-10-19 16:18:42 UTC
Created attachment 172736 [details] [review]
Add volume indicator

Add volume control indicator which uses API from gnome-volume-control
to interact with PulseAudio and shows both input and output volumes.
Also adds a small wrapper around libcanberra in ShellGlobal, used by the
volume indicator to provide auditive feedback.
Comment 55 Dan Winship 2010-10-20 14:52:01 UTC
Comment on attachment 172736 [details] [review]
Add volume indicator

looks good