Re: New extension: Actions

[Christian Persch <chpe gnome org>]

Hi,

I'm attaching my initial review comments since evo crashes when I try to
paste it in the mail window :(

Half-way through the review I realised I don't like the way you store
the actions and all in gconf; it's not made for that. You should use a
different way, probably a EphyNodeDb.

Regards,
	Christian

+libactionsextension_la_CPPFLAGS = \
+        -I$(top_srcdir)/include					\
+	-DSHARE_DIR=\"$(pkgdatadir)\"				\
+	-DEPHY_EXTENSIONS_LOCALEDIR=\"$(datadir)/locale\"	\
+	$(AM_CPPFLAGS)

Use tabs instead of spaces instead on the 2nd line.

+++ epiphany-extensions-1.5.3/extensions/actions/action-properties.glade	Wed Jan 19 01:59:15 2005
+++ epiphany-extensions-1.5.3/extensions/actions/actions-editor.glade	Wed Jan 19 01:59:15 2005

Why not just pack those in one glade file?

Adam already commented on the code style issues...

+  ephy_action_properties_dialog_type
+    = g_type_module_register_type(module,
+				  EPHY_TYPE_DIALOG,
+				  "EphyActionPropertiesDialog",

"EphyActionExtPropertiesDialog", so that we'll never conflict with anything in ephy itself
(I know we're not consistent with that everywhere in e-e).

+  dialog->priv = G_TYPE_INSTANCE_GET_PRIVATE(dialog, EPHY_TYPE_ACTION_PROPERTIES_DIALOG, EphyActionPropertiesDialogPrivate);
Usually we #define a .._GET_PRIVATE for that somewhere.

+  object = parent_class->constructor(type, n_construct_properties, construct_params);
Space before (

+  dialog->priv->dialog = ephy_dialog_get_control(EPHY_DIALOG(dialog), "action_properties");
+  dialog->priv->name_entry = ephy_dialog_get_control(EPHY_DIALOG(dialog), "name_entry");

Better to use the same as with all other EphyDialog:s in ephy, define an anon enum and reference
the string props by properties[PROP_NAME].

+  if (name && *name)
name != NULL && name[0] != '\0'

+void
+ephy_action_properties_dialog_response_h (GtkDialog *dialog,
+					  int response,
+					  EphyActionPropertiesDialog *pdialog)
[...]
+
+  gtk_widget_destroy(GTK_WIDGET(dialog));
+}

g_object_unref (pdialog) instead.

+      else			/* cancelled, window closed, etc */
+	{
+	  EphyDialog *edialog = EPHY_DIALOG(pdialog);
+
+	  ephy_dialog_set_pref(edialog, "name_entry", NULL);
+	  ephy_dialog_set_pref(edialog, "description_entry", NULL);
+	  ephy_dialog_set_pref(edialog, "command_entry", NULL);
+	  ephy_dialog_set_pref(edialog, "applies_to_pages_check", NULL);
+	  ephy_dialog_set_pref(edialog, "applies_to_images_check", NULL);

Why are you setting the prefs to NULL on cancel, and afterwards destroy the dialogue?

Back above:
+      { "name_entry",			keys->name, apply_type, 0 },
+      { "description_entry",		keys->description, apply_type, 0 },
+      { "command_entry",		keys->command, apply_type, 0 },
+      { "applies_to_pages_check",	keys->applies_to_pages, apply_type, 0 },
+      { "applies_to_images_check",	keys->applies_to_images, apply_type, 0 },

I think the actions don't belong in gconf; they should be in a different internal store (maybe 
a node db) instead.

+struct _EphyActionPropertiesDialog
+{
+  EphyDialog				parent;
+
+  /*< private >*/
+  EphyActionPropertiesDialogPrivate	*priv;

No need to align these.

+  dialog->priv->dialog = ephy_dialog_get_control(EPHY_DIALOG(dialog), "actions_editor");
+  dialog->priv->view = ephy_dialog_get_control(EPHY_DIALOG(dialog), "view");
+  dialog->priv->selection_count_label = ephy_dialog_get_control(EPHY_DIALOG(dialog), "selection_count_label");
+  dialog->priv->remove_button = ephy_dialog_get_control(EPHY_DIALOG(dialog), "remove_button");
+  dialog->priv->properties_button = ephy_dialog_get_control(EPHY_DIALOG(dialog), "properties_button");

Same as above, use an enum to access the properties array.

+typedef struct
+{
+  EphyActionsExtension		*extension;
+
+  GtkActionGroup		*action_group;
+  unsigned int			ui_id;
+
+  GtkActionGroup		*user_action_group;
+  unsigned int			user_ui_id;
+
+  unsigned int			notification_id;
+} WindowData;

Use glib types here; guint.

+  for (i = 0; i < G_N_ELEMENTS(popups); i++)
+    gtk_ui_manager_add_ui(GTK_UI_MANAGER(window->ui_merge),
+			  data->user_ui_id,
+			  popups[i],
+			  "EphyActionsExtensionSeparator",

Separators don't need a name.


----

Commenting on Adam's notes:

>+      markup = g_markup_printf_escaped("<span weight=\"bold\">%
>s</span>\n%s", formatted, description);
>
>Can't you just use <b></b>?

Actually I think <span> is better :)

>+  GDK_THREADS_ENTER();
>+  ephy_actions_editor_dialog_update_list(user_data);
>+  GDK_THREADS_LEAVE();
>
>I'm not 100% sure, but I think extensions are always invoked from within
>the same thread. Christian would know for sure.

I don't know either, but it's certainly not wrong to use THREADS_ENTER/LEAVE.