GNOME Bugzilla – Bug 624309
Split the PeasUIPluginManager
Last modified: 2010-07-19 19:15:24 UTC
Created attachment 165851 [details] [review] Split the PeasUIPluginManager into a manager, view and store The PeasUIPluginManager should be split into three files the manager the view and the store. Note: the attached patch requires the patches that add a plugin-list property to PeasEngine and add support for builtin plugins.
Review of attachment 165851 [details] [review]: The patch looks mostly good. A few style issues, an API issue and one or two mistakes to fix and it can go :-) I don't know if it can be done quickly but it would be nice if the show-builtin property was added as a separate patch if you have time. If not, then please mention it in your commit message. ::: libpeasui/Makefile.am @@ +26,3 @@ +NOINST_H_FILES = \ + peas-ui-plugin-manager-view.h \ + peas-ui-plugin-manager-store.h I think we might want to make the view public as it was the point behind making the split. ::: libpeasui/peas-ui-plugin-manager-store.c @@ +3,3 @@ + * This file is part of libpeas + * + * Copyright (C) 2010 - Garrett Regier I think you should keep the copyright notices of the old peas-ui-plugin-manager.c code in the new files (and add your own, of course). You should do the same for all the new files that reuse a significant amount of the previous code. @@ +140,3 @@ +{ + G_OBJECT_CLASS (peas_ui_plugin_manager_store_parent_class)->finalize (object); +} No need for an empty handler here. Just drop it. @@ +231,3 @@ + +static void +peas_ui_plugin_manager_store_init (PeasUIPluginManagerStore *store) A question of order: I prefer it when the _init() handler is before _dispose() and _finalize(), and when _class_init() comes after all the virtuals. Could you please reorder? @@ +354,3 @@ + if (peas_ui_plugin_manager_store_can_enable (store, &iter)) + peas_ui_plugin_manager_store_set_enabled (store, &iter, enabled); + } while (gtk_tree_model_iter_next (model, &iter)); Indentation is weird. Please make } go at the same level as { @@ +370,3 @@ + enabled = !enabled; + + peas_ui_plugin_manager_store_set_enabled (store, iter, enabled); Just use !enabled in this last line. @@ +424,3 @@ + { + current_info = peas_ui_plugin_manager_store_get_plugin (store, + iter); No need for multiple lines for such a tiny argument ;-) ::: libpeasui/peas-ui-plugin-manager-store.h @@ +56,3 @@ +struct _PeasUIPluginManagerStore +{ + GtkListStore parent_instance; Usually we use "parent". Also, the { should not go on a new line. @@ +90,3 @@ +gboolean peas_ui_plugin_manager_store_get_iter (PeasUIPluginManagerStore *store, + GtkTreeIter *iter, + const PeasPluginInfo *info); I don't like get_iter: it's going to be problematic for bindings because there is also a get_iter() method for GtkTreeModel. What about find_plugin() or get_iter_for_plugin() ? ::: libpeasui/peas-ui-plugin-manager-view.c @@ +84,3 @@ + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } PROP_SHOW_BUILTIN is not handled here. @@ +103,3 @@ + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } Same here @@ +156,3 @@ + + if (view->priv->popup_menu != NULL) + gtk_widget_destroy (view->priv->popup_menu); You could put this one in dispose() as well, and remove the _finalize handler altogether. @@ +188,3 @@ + "show-builtin", + "If builtin plugins should be shown", + FALSE, I'm divided here. Actually I think it makes sense to show builtin plugins by default, as builtin doesn't mean "invisible" and they could have a preference dialog as well. @@ +258,3 @@ + g_assert (PEAS_UI_IS_PLUGIN_MANAGER_STORE (model)); + else + g_assert (GTK_IS_TREE_MODEL_FILTER (model)); I think you should rather test that the store is either our model or a filter which contains our model. The reason is that someone might want to use its own filter. ::: libpeasui/peas-ui-plugin-manager-view.h @@ +44,3 @@ +struct _PeasUIPluginManagerView +{ + GtkTreeView parent_instance; Idem, use "parent". Also the { should not go on a new line. ::: libpeasui/peas-ui-plugin-manager.c @@ +56,3 @@ PeasEngine *engine; + GtkWidget *viewport; Please use "sw" instead of "viewport" as it is not a GtkViewport... @@ +205,3 @@ +{ + G_OBJECT_CLASS (peas_ui_plugin_manager_parent_class)->finalize (object); +} Idem, drop this handler as it does nothing. @@ +244,3 @@ + PeasPluginInfo *info) +{ + gboolean configurable = FALSE; is_configurable @@ +386,3 @@ GtkWindowGroup *wg; + view = PEAS_UI_PLUGIN_MANAGER_VIEW (pm->priv->view); Usually when there is no doubt about the type (ie when I don't use g_return_if_fail) I do the cast on the same line as the definition. @@ +413,3 @@ + conf_widget, + TRUE, TRUE, + 0); You can leave this as a single line, it's short @@ +468,3 @@ + pm->priv->viewport, + TRUE, TRUE, + 0); Idem, I think it would be ok as one line. @@ +476,3 @@ + hbuttonbox, + FALSE, FALSE, + 0); Idem
Garrett, any news on this one?
Created attachment 166173 [details] [review] Split the PeasUIPluginManager into a manager, view and store v2
Review of attachment 166173 [details] [review]: This patch looks good. I will test it a bit and if it works ok then it'll be time to commit :-) ::: libpeasui/peas-ui-plugin-manager.h @@ +36,3 @@ +#define PEAS_UI_IS_PLUGIN_MANAGER(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), PEAS_UI_TYPE_PLUGIN_MANAGER)) +#define PEAS_UI_IS_PLUGIN_MANAGER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), PEAS_UI_TYPE_PLUGIN_MANAGER)) +#define PEAS_UI_PLUGIN_MANAGER_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj), PEAS_UI_TYPE_PLUGIN_MANAGER, PeasUIPluginManagerClass)) Why did you change the indentation here? @@ +55,3 @@ }; +GType peas_ui_plugin_manager_get_type (void) G_GNUC_CONST; Same here
Ok, please fix the indentation issue and push. Thanks :-)
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.