The patch exceeds the limit for the size of patches (patch is 1.3Mb however the limit is 1Mb)....

Wow, that's quite a patch. Can you upload it somewhere please. Thanks.

In the process of tidying up the code and commenting. Should have it submitted in two parts by weekend.

Sorry for the delay, fixing a few bugs that have cropped up and doing some refactoring.

Conor, did you get any chance of getting this patch ready?

Created attachment 215932 [details] [review]
First in a series of patches to upstream ubuntu's 12.04 work.

This patch adds the new class needed for this new approach. GvcMixerUIDevice is used to represent an input or output on the UI. Essentially it is abstraction of a Pulse card port, network sink or bluetooth device.

Created attachment 215937 [details] [review]
Add the new files to represent the abstraction of the device on the input and output dialog

Comment on attachment 215937 [details] [review]
Add the new files to represent the abstraction of the device on the input and output dialog

This patch adds the new class needed for this new approach. GvcMixerUIDevice is
used to represent an input or output on the UI. Essentially it is abstraction
of a Pulse card port, network sink or bluetooth device.

Created attachment 215944 [details] [review]
Part 2

Tweaked the Makefile to include the files from the previous patch.
This patch largely contains the for the gvc-mixer-card c/h to enable to use Pulse's new card ports.

Created attachment 215968 [details] [review]
Part 3

Comment on attachment 215968 [details] [review]
Part 3

This patch to the control class represents the bulk of  the work. Essentially we have decoupled the UI from the sink and sources and instead used UIDevices to represent what is available to the user. The work mainly involved detecting card ports and creating ui devices from the ports, matching sinks and sources with existing ui devices, controlling the UI as it should as the controller within the architecture.

Created attachment 215977 [details] [review]
PART 4

This is the UI work to go with the existing backend patches. I have not included the speaker test on purpose as I think this is something David could do once he takes over this task, he did that work so would be more familiar with the tweaks. 
The GVC-mixer-dialog has been radically changed to accommodate the new pattern. The balance bar and combobox were altered to suit the new UI design. Maybe some of the port might regress some of the newer work on these two classes. They are only small diffs so these can be modified on file by file basis.

Doesn't build here at all, unfortunately.

Will have a go later, sorry only got to prepping the patches on Friday.
At least we can start the dialogue around the code changes  ?

Review of attachment 215937 [details] [review]:

"instropsection"?

::: panels/sound/gvc-mixer-ui-device.c
@@ +37,3 @@
+	GList*       		supported_profiles;
+	UiDeviceDirection 	type;
+	GHashTable* 		profiles;

Could you tell a hashtable of what this is? key= value=?

@@ +67,3 @@
+        serial = output_serial++;
+
+        if ((gint32)output_serial < 0) {

no need for the braces for one-liners.

@@ +82,3 @@
+
+	  switch (property_id)
+		{

curly brace on the line above.

@@ +122,3 @@
+		case PROP_DESC_LINE_1:
+			g_free (self->priv->first_line_desc);
+			self->priv->first_line_desc = g_value_dup_string (value);

This would be much cleaner to do in separate set() functions.

@@ +174,3 @@
+        self = GVC_MIXER_UI_DEVICE (object);
+        self->priv->id = get_next_output_serial ();
+        self->priv->profiles = g_hash_table_new_full (g_str_hash,

No need for linefeeds here.

@@ +182,3 @@
+	self->priv->port_name = NULL;
+	self->priv->disable_profile_swapping = FALSE;
+	self->priv->user_preferred_profile = NULL;

the priv struct is already zero'ed. Remove everything that's not relevant to understanding the code better.

@@ +195,3 @@
+gvc_mixer_ui_device_dispose (GObject *object)
+{
+        g_return_if_fail (object != NULL);

Don't add guards in private API.

@@ +198,3 @@
+        g_return_if_fail (GVC_MIXER_UI_DEVICE (object));
+
+	GvcMixerUIDevice *device;

Declarations at the beginning of the block.

@@ +221,3 @@
+		device->priv->user_preferred_profile = NULL;
+	}
+    G_OBJECT_CLASS (gvc_mixer_ui_device_parent_class)->dispose (object);	

Space/tabs mismatch.

@@ +227,3 @@
+gvc_mixer_ui_device_finalize (GObject *object)
+{
+	G_OBJECT_CLASS (gvc_mixer_ui_device_parent_class)->finalize (object);

If you're only chaining up, remove that.

@@ +317,3 @@
+
+/*
+ gvc_mixer_ui_device_set_profiles (GvcMixerUIDevice *device, const GList *in_profiles)

Comment lines usually have lined-up "*" unless there's a very good reason not to do so.

@@ +340,3 @@
+     -- It will choose the profile with the highest priority. (the pattern assumes a device will always have a profile in it's profile list)
+Note: 
+ I think this algorithm is inherently flawed.

Do we really want that then?

@@ +358,3 @@
+ */
+static gchar *
+get_profile_canonical_name(const gchar *profile_name, const gchar *skip_prefix)

Isn't there a better way to do this? Any reason why PulseAudio's libs can't do that for us?

@@ +400,3 @@
+	gchar *skip_prefix = device->priv->type == UiDeviceInput ? "output:" : "input:";
+	gchar *target_cname = get_profile_canonical_name(profile, skip_prefix);
+	GList *t, *values = g_hash_table_get_values(device->priv->profiles);

Use "*l" for temporary list pointers.

@@ +405,3 @@
+	for (t = values; t != NULL; t = t->next) {
+		GvcMixerCardProfile* p = t->data;
+		gchar *canonical_name = get_profile_canonical_name(p->profile, skip_prefix);

Don't assign in the declaration when it allocates new memory (the = t->data is fine).

@@ +406,3 @@
+		GvcMixerCardProfile* p = t->data;
+		gchar *canonical_name = get_profile_canonical_name(p->profile, skip_prefix);
+        g_debug("analysing %s", p->profile);

mismatches of spaces/tabs.

@@ +581,3 @@
+gvc_mixer_ui_device_get_id (GvcMixerUIDevice *op)
+{
+        g_return_val_if_fail (GVC_IS_MIXER_UI_DEVICE (op), 0);

You need to do that in all the public API calls.

@@ +590,3 @@
+	gint sink_id;
+	g_object_get (G_OBJECT (op),
+		      "stream-id", &sink_id, NULL);

Return op->priv->stream_id, surely?

@@ +595,3 @@
+
+void					
+gvc_mixer_ui_device_invalidate_stream (GvcMixerUIDevice *self)

Why not set_stream (GVC_MIXER_UI_DEVICE_INVALID)?

@@ +624,3 @@
+	GList *last;
+	last = g_list_last (dev->priv->supported_profiles);
+	GvcMixerCardProfile *profile;

Declarations at the beginning of the block.

@@ +632,3 @@
+gvc_mixer_ui_device_set_user_preferred_profile (GvcMixerUIDevice *device, const gchar* profile)
+{
+	if (device->priv->user_preferred_profile != NULL){

Space after the bracket.

@@ +654,3 @@
+gvc_mixer_ui_device_is_bluetooth (GvcMixerUIDevice *dev)
+{
+	return dev->priv->port_name == NULL && dev->priv->card_id != GVC_MIXER_UI_DEVICE_INVALID;	

Is that really a good heuristic to detect Bluetooth devices?

@@ +660,3 @@
+gvc_mixer_ui_device_is_output (GvcMixerUIDevice *dev)
+{
+	return dev->priv->type == UiDeviceOutput;	

brackets around comparison.

::: panels/sound/gvc-mixer-ui-device.h
@@ +50,3 @@
+	UiDeviceInput,
+	UiDeviceOutput,	
+}UiDeviceDirection;

UI, not Ui, and please prefix definitions.

@@ +59,3 @@
+const gchar*			gvc_mixer_ui_device_get_origin				 		(GvcMixerUIDevice *dev);
+const gchar*			gvc_mixer_ui_device_get_port				 		(GvcMixerUIDevice *dev);
+const gchar* 			gvc_mixer_ui_device_get_best_profile				(GvcMixerUIDevice *dev, const gchar *selected, const gchar *current);

linefeed after each argument

Review of attachment 215944 [details] [review]:

s/it's/its/

You should integrate the files into the build system in the previous patch.
The subject/commit message will also need splitting.

::: panels/sound/gvc-mixer-card.c
@@ +313,3 @@
+ */
+GIcon *
+gvc_mixer_card_get_gicon (GvcMixerCard *card)

That looks like it should be a separate patch.

Review of attachment 215968 [details] [review]:

::: panels/sound/gvc-mixer-control.c
@@ +86,3 @@
+        gboolean          new_default_is_source; /* true if currently setting the default source, false if setting the default sink */
+
+        /*

See previous comments about the formatting of comments.

@@ +164,3 @@
+        /* Don't change inputs to outputs and vice versa */
+        prefix = control->priv->new_default_is_source ? "source-output-by" : "sink-input-by";
+        if (!g_str_has_prefix(info->name, prefix))

Better split the check in another function.

@@ +214,3 @@
+        ports = gvc_mixer_stream_get_ports (stream);
+
+        // really this should be called 'is_network_or_bluetooth_stream'

No C++ style comments please.

@@ +227,3 @@
+                if (is_network_stream) {
+                        if (stream_id == gvc_mixer_stream_get_id (stream)) {
+                                g_debug ("\n lookup device from stream - %s - it is a network_stream \n",

Why add "\n"?

@@ +2723,3 @@
+                }
+                // otherwise check to make sure to invalidate other devices which may have that stream
+                else{

Coding style, that's gross.

@@ +3294,3 @@
+                              g_cclosure_marshal_VOID__UINT,
+                              G_TYPE_NONE, 1, G_TYPE_UINT);
+        signals [INPUT_REMOVED] =

Can't we use a "stream-removed" or something like that, and add an argument as to whether it's an output or input (or if that can be gathered from the arguments already passed)

Review of attachment 215977 [details] [review]:

All the changes in gvc-mixer-dialog need to be separate from the rest of this patch.

Same comments about the coding style as previous patches.

::: panels/sound/gvc-balance-bar.c
@@ +34,3 @@
 #include "gvc-channel-map-private.h"
 
+#define SCALE_SIZE 220

What's the reasoning behind that change?

@@ +543,3 @@
+{
+        g_object_set (G_OBJECT (self),
+                      "channel-map", channel_map, NULL);

Move the actual setting here, and make set_property() call here instead.

::: panels/sound/gvc-combo-box.c
@@ +343,3 @@
          * but that we can still read the full length of it */
         g_object_set (G_OBJECT (renderer), "ellipsize", PANGO_ELLIPSIZE_END, NULL);
+        gtk_cell_renderer_set_fixed_size (renderer, 5, -1);        

Split this in another patch?

(In reply to comment #17)
> Review of attachment 215968 [details] [review]:
> 
> ::: panels/sound/gvc-mixer-control.c
> @@ +86,3 @@
> +        gboolean          new_default_is_source; /* true if currently setting
> the default source, false if setting the default sink */
> +
> +        /*
> 
> See previous comments about the formatting of comments.
> 
> @@ +164,3 @@
> +        /* Don't change inputs to outputs and vice versa */
> +        prefix = control->priv->new_default_is_source ? "source-output-by" :
> "sink-input-by";
> +        if (!g_str_has_prefix(info->name, prefix))
> 
> Better split the check in another function.
> 
> @@ +214,3 @@
> +        ports = gvc_mixer_stream_get_ports (stream);
> +
> +        // really this should be called 'is_network_or_bluetooth_stream'
> 
> No C++ style comments please.
> 
> @@ +227,3 @@
> +                if (is_network_stream) {
> +                        if (stream_id == gvc_mixer_stream_get_id (stream)) {
> +                                g_debug ("\n lookup device from stream - %s -
> it is a network_stream \n",
> 
> Why add "\n"?
> 
> @@ +2723,3 @@
> +                }
> +                // otherwise check to make sure to invalidate other devices
> which may have that stream
> +                else{
> 
> Coding style, that's gross.

I'm not so sure why this is so 'gross'. Can you elaborate ? I have tidied up the comments if that is what you mean. 

There must be a way to invalidate streams on devices as they come and go. 
Off the top of my head there is a use case whereby one stream maybe assigned to multiple ports (take for instance the HDA intel h/ph and speaker port - it's the same stream or sink). Therefore we should double check when a sink or source (just in case) is removed that there are no devices which still have that stream assigned.

> 
> @@ +3294,3 @@
> +                              g_cclosure_marshal_VOID__UINT,
> +                              G_TYPE_NONE, 1, G_TYPE_UINT);
> +        signals [INPUT_REMOVED] =
> 
> Can't we use a "stream-removed" or something like that, and add an argument as
> to whether it's an output or input (or if that can be gathered from the
> arguments already passed)

I used a whole new set of signals just so it is clear as to when they should be used. I don't really see why this needs to be refactored. Obviously most of the time when a stream is removed it will not result in the need for an input/output device to be removed. For clarity, I think it's best to keep it separate.

Just a heads up, David and I are preparing a git branch for the merge. We are currently working through Bastian's comments below and amending where appropriate.

Created attachment 217476 [details] [review]
Patch 1/5

Created attachment 217477 [details] [review]
Patch 2/5

Created attachment 217478 [details] [review]
Patch 3/5 (gvc-mixer-control)

Created attachment 217479 [details] [review]
Patch 4/5 (gvc-speaker-test)

Created attachment 217481 [details] [review]
Patch 5/5 (gvc-mixer-dialog etc)

Okay, finally a new set of patches, sorry for the delay.
 
Differences:

 * It actually builds. At least it does here (and runs too ;-) ), so you can get a feeling for what it looks like. 

 * I fixed up most of the comments to patch 1 and Conor did the same for patch 3.

 * I've added a patch that makes speaker-test work for sinks without cards.

 * Rebased on yesterday's master tree (including yesterday's ext-stream-restore patch).

Remaining things (off the top of my head):

 * I noticed that the output volume/mute was not working in this version. There might be more stuff that shows up when we test it more thoroughly.

 * Add build dependency on PulseAudio 2.0 headers.

 * I don't think all coding style and other fixes have been fixed yet.

(In reply to comment #15)
> @@ +122,3 @@
> +        case PROP_DESC_LINE_1:
> +            g_free (self->priv->first_line_desc);
> +            self->priv->first_line_desc = g_value_dup_string (value);
> 
> This would be much cleaner to do in separate set() functions.

Note to self: Not yet fixed

> @@ +227,3 @@
> +gvc_mixer_ui_device_finalize (GObject *object)
> +{
> +    G_OBJECT_CLASS (gvc_mixer_ui_device_parent_class)->finalize (object);
> 
> If you're only chaining up, remove that.

Do you mean like this (in gvc_mixer_ui_device_class_init)?

object_class->finalize = G_OBJECT_CLASS (gvc_mixer_ui_device_parent_class)->finalize (object);

Or are there more elegant ways to do it?

> @@ +340,3 @@
> +     -- It will choose the profile with the highest priority. (the pattern
> assumes a device will always have a profile in it's profile list)
> +Note: 
> + I think this algorithm is inherently flawed.
> 
> Do we really want that then?

The comment was obsolete, I rewrote the algorithm to make it not flawed.

> @@ +358,3 @@
> + */
> +static gchar *
> +get_profile_canonical_name(const gchar *profile_name, const gchar
> *skip_prefix)
> 
> Isn't there a better way to do this? Any reason why PulseAudio's libs can't do
> that for us?

Good point, but unfortunately, no. This is part of how we turn PulseAudio's objects around to provide something more user friendly.

We might consider adding it in a future PulseAudio version.

> +void                    
> +gvc_mixer_ui_device_invalidate_stream (GvcMixerUIDevice *self)
> 
> Why not set_stream (GVC_MIXER_UI_DEVICE_INVALID)?

Note to self: not yet fixed

> @@ +654,3 @@
> +gvc_mixer_ui_device_is_bluetooth (GvcMixerUIDevice *dev)
> +{
> +    return dev->priv->port_name == NULL && dev->priv->card_id !=
> GVC_MIXER_UI_DEVICE_INVALID;    
> 
> Is that really a good heuristic to detect Bluetooth devices?

I've changed the name to better reflect what the function detects.

One observation: these patches use pa 2.0 api. The configure check for pulseaudio should be adjusted. And we need to bump pa in jhbuild to 2.0

Another observation: when I plug in a set of usb speakers it shows up in the list. When I switch the output device, the panel crashes.
I'll try to capture a stacktrace next time

Created attachment 217558 [details]
how it looks

Created attachment 217559 [details]
how it looks

Created attachment 217560 [details]
how it looks

Created attachment 217561 [details]
how it looks

I've bumped the pulseaudio version in our modulesets to 2.0 now.

(In reply to comment #26)
<snip>
> Remaining things (off the top of my head):
> 
>  * I noticed that the output volume/mute was not working in this version. There
> might be more stuff that shows up when we test it more thoroughly.
> 
>  * Add build dependency on PulseAudio 2.0 headers.
> 
>  * I don't think all coding style and other fixes have been fixed yet.

There are pretty severe UI problems as well:
- Output volume needs to be at the top of the dialogue, it's the most important thing, so goes up top.
- volume bars that are central need to align, for example, in the sound effects tab, the left and right edges of the volume bars should align.
- The input and output tabs have the label in the wrong place, which means that the sliders are seriously too short for anything useful:
https://github.com/gnome-design-team/gnome-mockups/raw/master/system-settings/sound/png/output.png

With this layout, we also lose all the alignment that we used to have (all the sliders lined up with the top/always visible slider). This is something that Alan can comment on.

I'll try and go through the patches and commit the backend code.

Review of attachment 217476 [details] [review]:

A large number of the problems mentioned here were already mentioned in the first review. Please make sure the coding style is up-to-scratch, it makes reviewing the accuracies of the code easier.

I'll also add "space before brackets" to the list of general problems.

::: panels/sound/gvc-mixer-ui-device.c
@@ +20,3 @@
+
+#include <string.h>
+#include "gvc-mixer-ui-device.h"

Separate the system headers from the local ones.

@@ +29,3 @@
+struct GvcMixerUIDevicePrivate
+{
+	gchar*	     		first_line_desc;

asterisk on the right side.

@@ +38,3 @@
+	GList*       		supported_profiles;
+	UIDeviceDirection 	type;
+	GHashTable* 		profiles; /* profiles to be added to combobox - key is

I don't understand why we need this to be a hashtable. It looks like a list would have done. You don't seem to be checking for existing profiles before adding new ones, and you loop over the list every time.

@@ +195,3 @@
+	g_return_if_fail (GVC_MIXER_UI_DEVICE (object));
+
+	GvcMixerUIDevice *device;

declaration at the beginning of the block.

@@ +199,3 @@
+	
+	if (device->priv->port_name != NULL) {
+		g_free (device->priv->port_name);

g_free() can handle NULL parameters just fine.

@@ +238,3 @@
+	object_class->get_property = gvc_mixer_ui_device_get_property;
+
+	GParamSpec *pspec;

declaration at the beginning of the block.

@@ +243,3 @@
+				"Description construct prop",
+				"Set first line description",
+				"no-name-set",

What's the point of this name? Does it need to be translated? Or does it always need to be set?
If it always needs to be set, make it a construct property.

@@ +262,3 @@
+				  "Set/Get card id",
+				  -1,
+				  1000,

Why 1000?

@@ +283,3 @@
+				  "Set/Get stream id",
+				  -1,
+				   10000, 

Why 10000? This doesn't match the code in get_next_output_serial().

@@ +290,3 @@
+					 pspec);
+
+	pspec = g_param_spec_uint ("type",

I'm guessing this is supposed to be an enum instead.

@@ +293,3 @@
+				   "ui-device type",
+				   "determine whether its an input and output",
+     				0,

indentation.

@@ +301,3 @@
+					 pspec);
+
+	pspec = g_param_spec_boolean ( "port-available",

no need for a space after the bracket.

@@ +322,3 @@
+get_profile_canonical_name(const gchar *profile_name, const gchar *skip_prefix)
+{
+    gchar *result = g_strdup(profile_name);

No assignment in declarations when there's memory allocation.

@@ +328,3 @@
+        /* Find '\0' or past next '+' */
+        gchar *d = c;
+        while (*d) {

Various levels of indentation problems.

@@ +350,3 @@
+    }
+
+    /* Cut a final '+' off */

I don't understand what that function does, but I'm pretty certain there's a cleaner way to do this.

@@ +369,3 @@
+		GvcMixerCardProfile* p = l->data;
+		canonical_name = get_profile_canonical_name(p->profile, skip_prefix);
+		if (!strcmp(canonical_name, target_cname))

use == 0 for strcmp(). Using "!" makes it look like failure.

@@ +383,3 @@
+   @param in_profiles a list of GvcMixerCardProfile
+   assigns value to
+    * device->priv->profiles (profiles to be added to combobox)

The asterisk usage is completely broken here.

@@ +418,3 @@
+
+	/* Run two iterations: First, add profiles which are canonical themselves,
+	   Second, add profiles for which the canonical name is not added already.

Missing asterisk at the beginning of the line.

@@ +420,3 @@
+	   Second, add profiles for which the canonical name is not added already.
+	*/
+	for (only_canonical = 1; only_canonical >= 0; only_canonical--) {

This is really ugly.
Make this whole loop into a function, and call it twice.

::: panels/sound/gvc-mixer-ui-device.h
@@ +31,3 @@
+#define GVC_IS_MIXER_UI_DEVICE_CLASS(klass)  (G_TYPE_CHECK_CLASS_TYPE ((klass), GVC_TYPE_MIXER_UI_DEVICE))
+#define GVC_MIXER_UI_DEVICE_GET_CLASS(obj)   (G_TYPE_INSTANCE_GET_CLASS ((obj), GVC_TYPE_MIXER_UI_DEVICE, GvcMixerUIDeviceClass))
+#define GVC_MIXER_UI_DEVICE_INVALID          -1

Separate this macro from the boilerplate code.

@@ +50,3 @@
+	UIDeviceInput,
+	UIDeviceOutput,
+}UIDeviceDirection;

namespace it please.

Review of attachment 217477 [details] [review]:

The set_ports() and get_gicon() API addition should be separate.

Review of attachment 217478 [details] [review]:

Please remove the leading linefeeds from the debug messages and make sure that those messages are unique enough that we can pinpoint where they were called from.

::: panels/sound/gvc-mixer-control.c
@@ +231,3 @@
+        }
+
+        gboolean is_network_stream;

declarations at the beginning of the block.

@@ +239,3 @@
+	for (d = devices; d != NULL; d = d->next) {
+		device = d->data;
+		gint stream_id = -2;

I don't like magic numbers. Please add a #define for it.

@@ +252,3 @@
+                        }       
+                }
+                else {

} else {
on the same line.

@@ +349,3 @@
         pa_operation_unref (o);
 
+        // source change successfull => update the UI.

No C++ style comments.

@@ +350,3 @@
 
+        // source change successfull => update the UI.
+        GvcMixerUIDevice* input;

Declarations at the beginning of the block.

@@ +2731,3 @@
+
+        if (device != NULL){
+#if 0                

Remove debug that you don't want in the final version.

@@ +2741,3 @@
+                                       gvc_mixer_ui_device_get_id (device));                            
+                }
+                else{

Indentation.

Review of attachment 217479 [details] [review]:

No particular comments here.

Review of attachment 217481 [details] [review]:

Please split the balance bar, combo box and other changes.

::: panels/sound/gvc-balance-bar.c
@@ +34,3 @@
 #include "gvc-channel-map-private.h"
 
+#define SCALE_SIZE 220

This is an unrelated change, please split and document.

@@ +558,3 @@
 
+void
+gvc_balance_bar_set_map (GvcBalanceBar* self,

Usually, it's the opposite. You do the work in the set() function, and call it from the set_property code.

::: panels/sound/gvc-combo-box.c
@@ +210,3 @@
 
+        const GList *l;
+        const GList *keys;

declarations, beginning of block.

@@ +343,3 @@
          * but that we can still read the full length of it */
         g_object_set (G_OBJECT (renderer), "ellipsize", PANGO_ELLIPSIZE_END, NULL);
+        gtk_cell_renderer_set_fixed_size (renderer, 5, -1);        

Unrelated UI change, please split and document.

::: panels/sound/gvc-mixer-dialog.c
@@ +551,3 @@
+// Do we need this ?
+// UI devices now pull description material mainly for the card ports.
+// Therefore the need for stream description dynamic changes more than ever seems unneccessary. 

Isn't this used for applications too?

@@ +1651,3 @@
+        }
+                
+        do {

Use a for loop instead.

@@ +1672,3 @@
+        }        
+
+        GvcMixerUIDevice *output;

declarations!

@@ +2029,3 @@
+                            alignment,
+                            FALSE, FALSE, 0);
+        // Output volume

C++ comment.

@@ +2170,2 @@
         PAGE_EVENTS,
+        PAGE_HARDWARE,

This obviously needs to go.

@@ +2194,3 @@
                 num = PAGE_APPLICATIONS;
+        else
+                num = 0;

Why use "0" as the default?

I noticed something as I read through the first patch (while fixing up some of the stuff from Bastien's latest review) - GvcMixerUIDevice contains pointers to GvcMixerCardProfiles, which are owned by the GvcMixerCard. Therefore, the GvcMixerUIDevice should have a weak reference to the GvcMixerCard and remove the lists of GvcMixerCardProfiles if the GvcMixerCard is freed.

As a result, we could skip the card-id property (or at least make it read only), and use a direct GvcMixerCard pointer instead.

Correct?

GvcMixerUIDevice should probably not keep a hold of the mixer card profiles, but simply get them from the underlying GvcMixerCard, if that's an acceptable shortcut in the code.

Created attachment 218506 [details] [review]
Patch 1 (the ui device object)

Created attachment 218507 [details] [review]
Patch 2 (card can read profiles/ports)

Created attachment 218508 [details] [review]
Patch 3 (adds card icon)

Created attachment 218509 [details] [review]
Patch 4 (gvc-mixer-control)

Created attachment 218510 [details] [review]
Patch 6 (dialog - work in progress)

According to Bastien's request on IRC yesterday, try to minimise layout changes for now. This is how far I've come on that patch. Most things seem to work but support for input profiles (which is quite unusual) and more testing remains.

If patch 4 fails to apply, you might need to apply this one first, from bug 674295:
http://bugzilla-attachments.gnome.org/attachment.cgi?id=218525

Review of attachment 218506 [details] [review]:

All fixed and committed.

::: panels/sound/gvc-mixer-ui-device.c
@@ +34,3 @@
+
+	gint						card_id;
+	gchar						*port_name;

Alignment:
letters line up with letters, eg.
gint    card_id
GList  *profiles
(hopefully appears correctly in bugzilla)

@@ +199,3 @@
+	device = GVC_MIXER_UI_DEVICE (object);
+	
+	g_free (device->priv->port_name);

Use g_clear_pointer() instead.

@@ +312,3 @@
+
+/**
+ * get_profile_canonical_name - removes the part of the string that starts with

Either's it's internal only documentation, in which case don't start with "/**" (which marks the beginning of gtk-doc API docs), or it's gtk-doc documentation, in which case you need to follow its syntax.

@@ +374,3 @@
+
+
+static void add_canonical_names_of_profiles(GvcMixerUIDevice *device,

Missing linefeed, missing space before bracket.

@@ +375,3 @@
+
+static void add_canonical_names_of_profiles(GvcMixerUIDevice *device,
+											const GList *in_profiles,

indentation

@@ +406,3 @@
+}
+
+/** gvc_mixer_ui_device_set_profiles

This is not gtk-doc syntax at all.

@@ +453,3 @@
+}
+
+/** gvc_mixer_ui_device_get_best_profile

Ditto.

@@ +555,3 @@
+
+guint
+gvc_mixer_ui_device_get_id (GvcMixerUIDevice *op)

Why call it "op" here, and "device" everywhere else?

@@ +608,3 @@
+}
+
+void 

Trailing whitespace.
Add:
[color]
        ui = auto
to your ~/.gitconfig to see the trailing whitespaces in red

::: panels/sound/gvc-mixer-ui-device.h
@@ +8,3 @@
+ * (at your option) any later version.
+ * 
+ * gvc-mixer-output.c is distributed in the hope that it will be useful, but

Check for renamed sources.

@@ +39,3 @@
+{
+	GObjectClass parent_class;
+}GvcMixerUIDeviceClass;

Missing space after brace.

@@ +45,3 @@
+	GObject parent_instance;
+	GvcMixerUIDevicePrivate *priv;
+}GvcMixerUIDevice;

Ditto.

@@ +51,3 @@
+	UIDeviceInput,
+	UIDeviceOutput,
+}GvcMixerUIDeviceDirection;

Ditto.

@@ +55,3 @@
+GType gvc_mixer_ui_device_get_type (void) G_GNUC_CONST;
+
+guint					gvc_mixer_ui_device_get_id						(GvcMixerUIDevice *dev);

You use "dev" here, but "device" in the .c file.

@@ +60,3 @@
+const gchar*			gvc_mixer_ui_device_get_origin					(GvcMixerUIDevice *dev); /* Unused */
+const gchar*			gvc_mixer_ui_device_get_port					(GvcMixerUIDevice *dev);
+const gchar* 			gvc_mixer_ui_device_get_best_profile			(GvcMixerUIDevice *dev, const gchar *selected, const gchar *current);

Mixing spaces and tabs.

Review of attachment 218507 [details] [review]:

::: panels/sound/gvc-mixer-card.c
@@ +490,3 @@
 }
 
+

Useless linefeed.

::: panels/sound/gvc-mixer-card.h
@@ +3,3 @@
  * Copyright (C) 2008-2009 Red Hat, Inc.
+ * Copyright (C) Conor Curran 2011 <conor.curran@canonical.com>
+ 

Empty line?

@@ +25,2 @@
 #include <glib-object.h>
+#include <gio/gio.h>

It's not used in the header, so there's no need to expose it here.
In fact, it's not used in the .c file changes either. Removed!

@@ +66,3 @@
+        gint  available;
+        gint  direction;
+        GList *profiles;

Alignment.

Review of attachment 218508 [details] [review]:

::: panels/sound/gvc-mixer-card.h
@@ +80,3 @@
 gboolean              gvc_mixer_card_change_profile    (GvcMixerCard *card,
                                                         const char *profile);
+GIcon *               gvc_mixer_card_get_gicon         (GvcMixerCard *card);

Now we need our gio #include.

Review of attachment 218509 [details] [review]:

Every use of g_hash_table_get_values() is leaking.

::: panels/sound/gvc-mixer-card.h
@@ +83,3 @@
 
+int                   gvc_mixer_card_profile_compare   (GvcMixerCardProfile *a,
+                                                        GvcMixerCardProfile *b);

gvc-mixer-card.[ch] changes merged separately.

::: panels/sound/gvc-mixer-control.c
@@ +87,3 @@
 
+        GHashTable       *ui_outputs; /* Ui visible outputs */
+        GHashTable       *ui_inputs; /* Ui visible inputs */

UI, not Ui

@@ +93,3 @@
+        it will jump back to the default sink set by the server to prevent the audio setup from being 'outputless'.
+        All well and good but then when we get the new stream created for the new profile how do we know 
+        that this is the intended default or selected device the user wishes to use.

Coding style.

@@ +112,3 @@
+        INPUT_ADDED,
+        OUTPUT_REMOVED,
+        INPUT_REMOVED,

I sorted those properly, they're all jumbled up.

@@ +120,3 @@
+static void                     gvc_mixer_control_class_init (GvcMixerControlClass *klass);
+static void                     gvc_mixer_control_init       (GvcMixerControl      *mixer_control);
+static void                     gvc_mixer_control_finalize   (GObject              *object);

Unnecessary whitespace changes.

@@ +215,3 @@
+ * @control:
+ * @stream:
+ * Returns: GvcUIDevice (transfer none) or NULL:

Incorrect syntax.
Returns: (transfer none): a #GvcUIDevice or %NULL

@@ +221,3 @@
+                                             GvcMixerStream *stream)
+{
+        GList                   *devices;

You're leaking the list container.

@@ +223,3 @@
+        GList                   *devices;
+        GList                   *d;
+        GvcMixerUIDevice        *device;

device is only used inside the loop, so move to the loop block.

@@ +229,3 @@
+        if (GVC_IS_MIXER_SOURCE (stream))
+               devices = g_hash_table_get_values (control->priv->ui_inputs);
+        else{

braces usage?

@@ +269,3 @@
+                }
+        }
+        g_debug ("gvc_mixer_control_lookup_device_from_stream - Could not find a device ?  %s",gvc_mixer_stream_get_description (stream));		 

Trailing whitespace.

@@ +349,3 @@
         pa_operation_unref (o);
 
+        /* source change successful => update the UI. */

Replace your "=>" by words. Does it mean "then", "so", "and"?

@@ +485,3 @@
+ * Returns: (transfer container) (element-type gchar*):
+   The profile full name stored in that device
+   TODO: This belongs within the GvcMixerUIDevice.

Moved to GvcMixerUIDevice

@@ +509,3 @@
+ * gvc_mixer_control_get_stream_from_device:
+ * @control:
+ * @dev

Missing colon.

@@ +534,3 @@
+ * @control:
+ * @device:
+ * @param profile Can be null if any profile present on this port is okay

Not in gtk-doc format.

@@ +535,3 @@
+ * @device:
+ * @param profile Can be null if any profile present on this port is okay
+ * Returns: (transfer container) (element-type gboolean):

A gboolean isn't a container.

@@ +536,3 @@
+ * @param profile Can be null if any profile present on this port is okay
+ * Returns: (transfer container) (element-type gboolean):
+   This method will attempt to swap the profile on the card of the device with given profile name

Missing "*"

@@ +1246,3 @@
+        stream_card_id =  gvc_mixer_stream_get_card_index (stream);
+
+        devices  = g_hash_table_get_values (GVC_IS_MIXER_SOURCE (stream) ? control->priv->ui_inputs : control->priv->ui_outputs);

leaking devices again.

@@ +1301,3 @@
+ * Using static card port introspection implies that we know beforehand what outputs and inputs are available to the user.
+ * But that does not mean that all of these inputs and outputs are available to be used. 
+ * For instance we might be able to see that there is a HDMI port available but if we are on the default analog stereo output profile there is no valid sink for that HDMI device. We first need to change profile and when update_sink is called only then can we match the new hdmi sink with it's corresponding device.

indentation

@@ +1322,3 @@
+        stream_ports = gvc_mixer_stream_get_ports (stream);
+
+        if (g_list_length ((GList *) stream_ports) == 0) {

stream_ports == NULL?

@@ +2096,3 @@
+        port_count = g_list_length ((GList *) card_ports);
+
+        if (port_count == 0 && is_new) {

if card_ports == NULL && is_new

@@ +2101,3 @@
+                GvcMixerUIDevice *out;
+                const GList      *profiles = NULL;
+                /* TODO : move to its own method.*/

Done.

@@ +2110,3 @@
+
+                object = g_object_new (GVC_TYPE_MIXER_UI_DEVICE,
+                                       "type", 0,

enum

@@ +2124,3 @@
+                                     g_object_ref (in));
+                object = g_object_new (GVC_TYPE_MIXER_UI_DEVICE,
+                                       "type", 1,

enum

@@ +3249,3 @@
                               g_cclosure_marshal_VOID__UINT,
                               G_TYPE_NONE, 1, G_TYPE_UINT);
+        signals [OUTPUT_ADDED] =

Reordered those.

::: panels/sound/gvc-mixer-control.h
@@ +70,3 @@
         void (*default_source_changed) (GvcMixerControl *control,
                                         guint            id);
+        void (*output_added)           (GvcMixerControl *control,

Reordered.

::: panels/sound/gvc-mixer-stream.h
@@ +65,1 @@
 } GvcMixerStreamPort;

Moved to a separate commit.

Review of attachment 217479 [details] [review]:

Don't forget the necessary GvcMixerDialog changes for this to work correctly.

Review of attachment 218510 [details] [review]:

Don't forget to remove the unused update_default_input() and update_default_output().

::: panels/sound/gvc-mixer-dialog.c
@@ +321,3 @@
                 }
 
+                active_profile = gvc_mixer_control_get_active_profile_from_ui_device (dialog->priv->mixer_control,

Use gvc_mixer_ui_device_get_active_profile() instead.

Once all of this is done, we also need to remove all the public but unused functions.

Created attachment 218645 [details] [review]
Patch Dialog 1

Uploading today's work.

Rebased on git master.
This makes the dialog use the new UI devices and turns the port combobox into a profile combobox.

Will continue tomorrow with moving the speaker-test button from the hardware tab to the output tab, and more testing. 

Things seem to work fine with my laptop internal card and my USB headset, but some stuff such as bluetooth and network sources/sinks can trigger unusual code paths and those things remain to be tested.

Created attachment 218701 [details] [review]
Patch Dialog 1

More fixups compared to the previous patch.

Created attachment 218706 [details] [review]
Patch Dialog 2 (gvc-speaker-test)

(No changes to this one, actually)

Created attachment 218707 [details] [review]
Patch Dialog 3 (move speaker test to output tab)

Created attachment 218714 [details] [review]
Patch Dialog 4 (hide hardware tab)

I almost forgot... :-)

Created attachment 218719 [details] [review]
Patch Dialog 1

Fixed a small bluetooth related bug in this version.

All committed with a few styling fixes applied, the code for the hardware tab really removed, and a couple of bug fixes, including some warnings generated when switching outputs.