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 304407 - Allow renaming of mixer channels
Allow renaming of mixer channels
Status: RESOLVED OBSOLETE
Product: gnome-media
Classification: Deprecated
Component: gst-mixer
2.10.x
Other Linux
: Normal enhancement
: ---
Assigned To: gnome media maintainers
gnome media maintainers
Depends on:
Blocks:
 
 
Reported: 2005-05-16 19:00 UTC by Sebastien Bacher
Modified: 2015-01-19 11:49 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Allow channels rename throught Edit->Preference (9.34 KB, patch)
2007-06-13 00:04 UTC, Andrea Del Signore
none Details | Review
"display-label-changed" implementation (22.50 KB, patch)
2007-06-15 10:42 UTC, Andrea Del Signore
needs-work Details | Review
Next iteration of "Allow renaming of mixer channels" (21.01 KB, patch)
2007-06-15 17:50 UTC, Andrea Del Signore
accepted-commit_now Details | Review
This one will apply to head (21.31 KB, patch)
2007-07-21 18:50 UTC, Andrea Del Signore
needs-work Details | Review

Description Sebastien Bacher 2005-05-16 19:00:12 UTC
This bug has been opened here: https://bugzilla.ubuntu.com/9852

"Hi,
  Just a small wishlist item; it would be nice if the mixer channels could have
their names changed to reflect reality on your particular machine; for example
my CD drive is connected to what the Alsa mixer thinks is 'Aux' and the OSS
mixer thinks is Line-1, with my DVD drive being connected to the one marked 'CD'."
Comment 1 Andrea Del Signore 2007-06-13 00:04:07 UTC
Created attachment 89855 [details] [review]
Allow channels rename throught Edit->Preference

This is a first rought patch that implement the rename functionality.

I don't know if I miss something, but I think that there are some memory leaks in preference.c (see GConfValues and the use of the "key" variabile for example).

This patch also fix a bug when you unset a gconf key manually and the mixer is open.
Comment 2 Andrea Del Signore 2007-06-13 12:41:04 UTC
Some more comment about the patch (yesterday was a little late when I submitted it).

First in preferences.c I implemented a new gconf key per track with the exact name of the toggle one plus the suffix '_label' where I store the new label.

Then I've modified track.c to read the gconf keys and to fallback on default track name if the key is not found. Here I'm not sure if the use of the function
gconf_client_get_default() in track_add_title is appropriate.

Finally in element.c I've modified the gconf callback function to update the UI on track name change.
Comment 3 Ronald Bultje 2007-06-14 12:31:32 UTC
I like this, we should apply this (assuming it doesn't crash & all the obvious).
Comment 4 Ronald Bultje 2007-06-14 12:43:17 UTC
>   render = gtk_cell_renderer_text_new ();
>+  g_object_set(render,"editable",TRUE,NULL);
>+  g_signal_connect(render,"edited",
>+        G_CALLBACK (cb_edited), prefs);

mind your indenting here, everything needs to be aligned along the (.

>     while (valid == TRUE) {
>       gtk_tree_model_get (model, &iter,
>-			  COL_TRACK, &track,
>-			  -1);
>-      if (strcmp (track->label, gconf_entry_get_key (entry) + strlen (keybase))) {
>-	gtk_list_store_set( GTK_LIST_STORE(model), &iter, COL_ACTIVE, active, -1);
>-	break ;
>+        COL_TRACK, &track,
>+        -1);

Indenting again.

>+static void
>+cb_edited(GtkCellRendererToggle *cell,
>+	   gchar                 *path_str,
>+	   gchar                 *new_text,
>+	   gpointer               data)
>+{
>+  GnomeVolumeControlPreferences *prefs = data;
>+  g_debug("track label changed %s, path %s\n", new_text, path_str);
>+
>+  GtkTreeModel *model = gtk_tree_view_get_model (GTK_TREE_VIEW (prefs->treeview));

This won't compile on <C99, please remove the g_debug().

>@@ -241,12 +242,30 @@
>     }
>   }
> 
>+  gchar *key = get_gconf_key (mixer, track);
>+  gchar *key_track_label = g_strconcat(key, "_label", NULL);
>+  const gchar *label = track->label;
>+  GConfValue *value;
>+  GConfClient *client = gconf_client_get_default();

Move all those declarations to the top of the function, C99ism again. As for the client, provide it to the function, I don't like it if functions try to get a new reference. 

I think more generally, this introduces a difficulty with this patch, in that it leaves track->label defaulted. I would recommend to add a track->display_label property and emit a display-label-changed signal when it changes (gconf callback), so that we can update everywhere as appropriate with only small amounts of code. Right now, it's a "ask gconf what the real value is everywhere" sort of thing, I don't think I like that way of implementing the feature.
Comment 5 Andrea Del Signore 2007-06-15 10:42:08 UTC
Created attachment 90003 [details] [review]
"display-label-changed" implementation

I'tryed the "display-label" property and "display-label-changed" signal as you have suggested, I like it better, but I've some concerns about it.

First I don't like the property changed signal vs set_label "dance" between track.c and element.c and I think that we should modify the label directly in the set_label function and drop the code in element.c.

Second I don't know if one gconf client notification per track is the best approach performance wise, so some input is needed.

If this method is correct we can go even futher and modify the code of the active flag and use the same logic.

Finally I need some help to set my vim with your indentation style, no matter what I try I've to indent every line manually with spaces :( (I know, I'm a newbie)
Comment 6 Ronald Bultje 2007-06-15 12:43:47 UTC
I wouldn't worry about performance, we're in no way in a critical code path here. This code will run once every time a user changes a label, which is either never or, if the user is extremely active, once every few seconds. Critical code is code that runs on a video pixel level (768x567x25 frames per second, to give you an idea). So don't worry about performance too much just yet. :-).

We can use gconf notifications for active as well, but let's do one thing at a time. Here's my comments on the patch, I like it better but it's not totally correct:

>   if (prefs->mixer) {
>+    /* disconnect from tracks signals */
>+    for (item = gst_mixer_list_tracks (prefs->mixer);
>+         item != NULL; item = item->next) {
>+      GstMixerTrack *track = item->data;
>+      GnomeVolumeControlTrack *trkw;
>+      gpointer display_label_changed_id;
>+
>+      trkw = g_object_get_data (G_OBJECT (track),
>+                                "gnome-volume-control-trkw");
>+      display_label_changed_id = g_object_get_data (G_OBJECT (track),
>+                                                    "preferences-display-label-changed-signal-id");
>+      if (display_label_changed_id != NULL)
>+      {
>+        g_signal_handler_disconnect (trkw, (guint) display_label_changed_id);
>+        g_object_set_data (G_OBJECT (track),
>+                           "preferences-display-label-changed-signal-id",
>+	  		   NULL);
>+
>+      }
>+    }
>     gst_object_unref (GST_OBJECT (prefs->mixer));
>     prefs->mixer = NULL;
>   }

Use g_signal_handler_disconnect_by_func (which is based on func+user_data), seems easier to me and you will not need the signal_id property.

>     while (valid == TRUE) {
>       gtk_tree_model_get (model, &iter,
> 			  COL_TRACK, &track,
> 			  -1);
>-      if (strcmp (track->label, gconf_entry_get_key (entry) + strlen (keybase))) {
>+      gchar *key=get_gconf_key(prefs->mixer, track);
>+      if (!strncmp (key + strlen(keybase), 
>+	            gconf_entry_get_key (entry) + strlen (keybase),
>+		    strlen(gconf_entry_get_key (entry) + strlen (keybase)))) {
>+        active = FALSE;
>+	if (value != NULL && value->type == GCONF_VALUE_BOOL) 
>+	  active = gconf_value_get_bool (value);
>+

The gchar *key should be before the gtk_tree_model_get() (C99ism).

(in track.c)
>+static void
>+cb_gconf (GConfClient *client,
>+          guint        connection_id,
>+          GConfEntry  *entry,
>+          gpointer     data)
>+{
>+	
>+  GnomeVolumeControlTrack *track_widget = (GnomeVolumeControlTrack *) data;
>+  gchar *key_base = get_gconf_key (track_widget->mixer, track_widget->track);
>+  gchar *key = g_strconcat (key_base, "_label", NULL);
>+  GConfValue *value;
>+
>+  if (!strncmp (gconf_entry_get_key (entry),
>+                key, strlen (key))) {
>+    value = gconf_entry_get_value (entry);
>+
>+    if (value != NULL && value->type == GCONF_VALUE_STRING)
>+    {
>+      const gchar *label = gconf_value_get_string (value);
>+      gnome_volume_control_track_set_display_label (track_widget, label);
>+    }
>+    else
>+      gnome_volume_control_track_set_display_label (track_widget, track_widget->track->label);
>+  }
>+
>+  g_free (key_base);
>+  g_free (key);
>+}

So _this_ is not a good idea. :-). Don't forget that you'll have +/- 50 tracks for some sound cards. I would add this code to the gconf callback above, which runs only once, and emit the signal (or call g_v_c_track_set_display_label()) from there. Then, the code only runs once per change instead of 50 times per change. This would basically look similar to how it finds which track is active. You can even rearrange that a bit so that it only compares strings once for both the active bool and the label string before checking the gconf value key type or something along those lines. (you don't have to do this, I'd accept it without also, just giving you something to play with if you feel like it :-) ).

>+void    
>+gnome_volume_control_track_set_display_label (GnomeVolumeControlTrack *track,
>+                                              const gchar *display_label)
>+{
>+  if (strncmp (track->display_label,
>+               display_label, strlen (track->display_label))) {
>+    if (track->display_label)
>+      g_free (track->display_label);
>+
>+    track->display_label = g_strdup (display_label);
>+    g_signal_emit (track,GNOME_VOLUME_CONTROL_TRACK_GET_CLASS (track)->display_label_changed_id,
>+		   0, NULL);
>+  }
>+}

I think gconf does this already (the strcmp()), so you can probably omit that.

>+static void
>+gnome_volume_control_track_class_init (GnomeVolumeControlTrackClass *klass)
>+{
>+   klass->display_label_changed_id =
>+     g_signal_newv ("display-label-changed", 
>+ 		    G_TYPE_FROM_CLASS (klass), 
>+		    G_SIGNAL_RUN_LAST | G_SIGNAL_NO_RECURSE | G_SIGNAL_NO_HOOKS, 
>+		    NULL, 
>+		    NULL,
>+		    NULL,
>+		    g_cclosure_marshal_VOID__VOID, 
>+		    G_TYPE_NONE,
>+		    0,
>+		    NULL);
>+}
>+
[..]
>+typedef struct _GnomeVolumeControlTrackClass
>+{
>+  GObjectClass parent;
>+
>+  /* signal id */
>+  guint display_label_changed_id;
>+
>+} GnomeVolumeControlTrackClass;

So, people tend to put the signal signature in the klass struct, and then have a static signals[NUM_SIGNALS] variable in the .c file that uses the signal, could you follow that? Also, the g_signal_newv() should probably use the signal signature in the klass struct (see glib doc or gtk source code for how to do this).
Comment 7 Andrea Del Signore 2007-06-15 17:50:29 UTC
Created attachment 90024 [details] [review]
Next iteration of "Allow renaming of mixer channels"

I've changed it according to your suggestions.
Comment 8 Ronald Bultje 2007-06-15 18:30:03 UTC
I think this looks pretty good, please allow me to closely review it tonight just to make sure about the details, but I think you've got it all here.
Comment 9 Ronald Bultje 2007-06-16 20:52:18 UTC
Looks good, thanks!
Comment 10 Marc-Andre Lureau 2007-07-21 15:13:49 UTC
Andrea, do you have a gnome svn account? If not, "someone" should apply. Ronald? ;)
Comment 11 Andrea Del Signore 2007-07-21 17:05:09 UTC
No I haven't :) so I can't commit the patch.
Comment 12 Andrea Del Signore 2007-07-21 18:50:36 UTC
Created attachment 92133 [details] [review]
This one will apply to head

The prevous patch doesn't apply anymore to svn head :(
This one will and I've fixed a bug in preferences.c (cb_gconf).
Comment 13 Andrea Del Signore 2007-08-22 17:05:02 UTC
Just returned from vacation, so this is a ping for Ronald or Marc Andre
Comment 14 Marc-Andre Lureau 2007-08-22 20:47:07 UTC
I did not review it myself yet.

Lets wait for Ronald. Unfortunately, it won't make it for 2.20 I guess, except if Ronald decide it. (2.20 feature freeze since more than 1 month, and UI freeze for the last week, if I understand releases correctly).... :(
Comment 15 Marc-Andre Lureau 2007-08-22 20:50:32 UTC
Actually, we could also ask for help :)

Vincent, do you think this patch could be applied now?
Comment 16 Marc-Andre Lureau 2007-08-22 20:52:57 UTC
Bastien, since you are doing reviewing, are you interested in helping here?
Comment 17 Ronald Bultje 2007-08-22 20:53:19 UTC
It can be applied, I already reviewed it. It won't be for 2.20, but that's ok.
Comment 18 Vincent Untz 2007-08-23 08:33:19 UTC
(In reply to comment #15)
> Actually, we could also ask for help :)
> 
> Vincent, do you think this patch could be applied now?

Better to wait for 2.22.
Comment 19 Bastien Nocera 2007-09-06 16:12:01 UTC
Sorry about the lag. I wouldn't accept this patch, on the grounds that:
- it adds unnecessary complexity to the code
- it only renames the tracks in gnome-volume-control, not in the volume control applet, or any other apps that uses the same sound system (eg. ALSA apps on recent Linux systems)
- the ALSA (which is the main problem in this case) maintainer(s) (Takashi who talked to Lennart about this) will simplify volume controls, so that things like that are unnecessary.
Comment 20 Andrea Del Signore 2007-09-06 18:16:58 UTC
No problem for me, but if you think that patching the volume control applet too will help I can try it.

Unfortunately I can't due a lot for the other issues (complexity & the alsa mixer module) :(


Comment 21 Marc-Andre Lureau 2008-03-17 23:16:15 UTC
Following Bastien comments, marking the patch as "needs-work". 

The capability to rename tracks should be solved in a lower level. 

GstMixer, HAL or libalsa directly? this is not a simple question though. Better wait for some better idea.

Comment 22 Marc-Andre Lureau 2008-04-20 00:42:38 UTC
removing TM
Comment 23 Bastien Nocera 2015-01-19 11:49:38 UTC
gnome-media has been obsolete since the release of GNOME 3, nearly 4 years ago.
Furthermore, the gnome-volume-control program in gnome-media has been replaced
by the Sound panel in gnome-control-center.

The new Settings panel should not be affected by the bugs you filed, however, please
make sure to file new bugs against the gnome-control-center product.