GNOME Bugzilla – Bug 634917
Add a 'Media and Autorun' panel
Last modified: 2010-11-23 14:26:31 UTC
Hi, together with Bastien and Jon, we've been discussing about splitting the "media" tab of nautilus' preferences to a control-center panel. This will allow removing nautilus' old capplet altogether, and helps nautilus' goal of being just an application in the 3.0 world and not managing system settings (the actual autorun code is being moved to gnome-settings-daemon, see https://bugzilla.gnome.org/show_bug.cgi?id=585545).
This is implemented in the 'media-panel' [1] branch now. The code is just a bit more than a straight port of what we had in Nautilus, so it should be well-tested.
Created attachment 174518 [details] [review] squashed patch
Note that there are plans for moving NautilusOpenWithDialog to GTK+ proper, but this should be completely orthogonal to this branch.
Review of attachment 174518 [details] [review]: A few niggles. Not that I didn't test the code, and would do the UI review once those are fixed. ::: panels/media/Makefile.am @@ +23,3 @@ + +desktopdir = $(datadir)/applications +Desktop_in_files = gnome-media-panel.desktop.in Add to EXTRA_DIST? ::: panels/media/cc-media-panel.c @@ +119,3 @@ + +static void +remove_elem_from_str_array (char **v, const char *s) See below, use GPtrArray instead. @@ +139,3 @@ + +static char ** +add_elem_to_str_array (char **v, const char *s) I would really prefer that you used GPtrArray here, that code is looking pretty gross. @@ +213,3 @@ + char *x_content_type; + + x_content_type = g_strdup (data->x_content_type); What's the point of copying it to free it a second later? prepare_combo_box already makes a copy internally. @@ +362,3 @@ + +static int +eel_g_strv_find (char **strv, Do we need the "eel_" prefix? media_panel_ would probably be better. @@ +365,3 @@ + const char *find_me) +{ + int index; guint instead. @@ +621,3 @@ + + if (num_apps == 0) { + gtk_combo_box_set_active (GTK_COMBO_BOX (combo_box), 0); define an enum, and use the symbolic name here. @@ +626,3 @@ + gtk_widget_set_sensitive (combo_box, TRUE); + if (pref_ask) { + gtk_combo_box_set_active (GTK_COMBO_BOX (combo_box), 0); And here. @@ +628,3 @@ + gtk_combo_box_set_active (GTK_COMBO_BOX (combo_box), 0); + } else if (pref_ignore) { + gtk_combo_box_set_active (GTK_COMBO_BOX (combo_box), 1); etc. @@ +676,3 @@ + GtkTreeIter iter; + GtkBuilder *builder = self->priv->builder; + const char *s[] = {"media_audio_cdda_combobox", "x-content/audio-cdda", I'd prefer splitting this, it would make it easier to read, and you could use G_N_ELEMENTS then. struct { const char *widget; const char *mimetype; } defs[] = { { "media_audio_cdda_combobox", "x-content/audio-cdda" }, etc. @@ +772,3 @@ + res = gtk_builder_add_from_file (priv->builder, GNOMECC_UI_DIR "/gnome-media-properties.ui", NULL); + } else { + res = gtk_builder_add_from_file (priv->builder, "./gnome-media-properties.ui", NULL); Given that you need to install the GIO extension point, you can remove this hack to run uninstalled. ::: panels/media/nautilus-open-with-dialog.c @@ +34,3 @@ +#include <gio/gio.h> + +#define sure_string(s) ((const char *)((s)!=NULL?(s):"")) spacing, etc. #define sure_string(s) ((const char*) s ? s : "") looks better
Bastien, thanks for the review. I updated my branch according to your comments; I will attach a squashed diff too. As for the UI, it's pretty much what we have in nautilus' "Media" preferences tab right now. (In reply to comment #4) > ::: panels/media/Makefile.am > @@ +23,3 @@ > + > +desktopdir = $(datadir)/applications > +Desktop_in_files = gnome-media-panel.desktop.in > > Add to EXTRA_DIST? I just checked all other Makefile.am files in panels/ and it seems none of them add their desktop.in files to EXTRA_DIST, so this is probably not necessary? Otherwise we should add it to all the other panels too. > ::: panels/media/cc-media-panel.c > @@ +119,3 @@ > + > +static void > +remove_elem_from_str_array (char **v, const char *s) > > See below, use GPtrArray instead. Done. > @@ +213,3 @@ > + char *x_content_type; > + > + x_content_type = g_strdup (data->x_content_type); > > What's the point of copying it to free it a second later? prepare_combo_box > already makes a copy internally. That actually needs to stay that way, because: - autorun_rebuild_combo_box() is called with |data|, which is set with g_object_set_data() on the combobox - prepare_combo_box() is called with the combobox and the content type, taken from data - prepare_combo_box() does this /* See if we have an old data around */ data = g_object_get_data (G_OBJECT (combo_box), "autorun_combobox_data"); if (data) { new_data = FALSE; g_free (data->x_content_type); } else { data = g_new0 (AutorunComboBoxData, 1); } and then wants to copy x_content_type again into the new data. If we don't strdup() it before the call, it could access x_content_type() after it has already been g_free()-d. I realize this is not the prettiest code in the world, but it's well-tested from some years in nautilus, and I will probably write a cleaner implementation of a MIME combo box in GTK+ soon, so I don't really feel like refactoring it all now :) > @@ +362,3 @@ > + > +static int > +eel_g_strv_find (char **strv, > > Do we need the "eel_" prefix? media_panel_ would probably be better. Done. > @@ +365,3 @@ > + const char *find_me) > +{ > + int index; > > guint instead. Done. > @@ +621,3 @@ > + > + if (num_apps == 0) { > + gtk_combo_box_set_active (GTK_COMBO_BOX (combo_box), 0); > > define an enum, and use the symbolic name here. Done. > @@ +676,3 @@ > + GtkTreeIter iter; > + GtkBuilder *builder = self->priv->builder; > + const char *s[] = {"media_audio_cdda_combobox", "x-content/audio-cdda", > > I'd prefer splitting this, it would make it easier to read, and you could use > G_N_ELEMENTS then. Done. > > @@ +772,3 @@ > + res = gtk_builder_add_from_file (priv->builder, GNOMECC_UI_DIR > "/gnome-media-properties.ui", NULL); > + } else { > + res = gtk_builder_add_from_file (priv->builder, > "./gnome-media-properties.ui", NULL); > > Given that you need to install the GIO extension point, you can remove this > hack to run uninstalled. Good point. > ::: panels/media/nautilus-open-with-dialog.c > @@ +34,3 @@ > +#include <gio/gio.h> > + > +#define sure_string(s) ((const char *)((s)!=NULL?(s):"")) > > spacing, etc. > #define sure_string(s) ((const char*) s ? s : "") > looks better Done.
Created attachment 175100 [details] [review] squashed patch v2
Merged to master after Bastien's ACK on IRC.