GNOME Bugzilla – Bug 304407
Allow renaming of mixer channels
Last modified: 2015-01-19 11:49:38 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'."
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.
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.
I like this, we should apply this (assuming it doesn't crash & all the obvious).
> 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.
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)
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).
Created attachment 90024 [details] [review] Next iteration of "Allow renaming of mixer channels" I've changed it according to your suggestions.
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.
Looks good, thanks!
Andrea, do you have a gnome svn account? If not, "someone" should apply. Ronald? ;)
No I haven't :) so I can't commit the patch.
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).
Just returned from vacation, so this is a ping for Ronald or Marc Andre
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).... :(
Actually, we could also ask for help :) Vincent, do you think this patch could be applied now?
Bastien, since you are doing reviewing, are you interested in helping here?
It can be applied, I already reviewed it. It won't be for 2.20, but that's ok.
(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.
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.
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) :(
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.
removing TM
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.