Re: Actions extension, take 2

[Christian Persch <chpe gnome org>]

Hi,

Le vendredi 21 janvier 2005 à 22:41 +0100, Jean-Yves Lefort a écrit :
> I have modified the Actions extension according to Adam and Christian
> comments, patch attached.
> 
> Christian: I use one .glade file per dialog because there is no need
> to make libglade parse the whole UI when only one dialog is needed.

Here's another review pass:

- it doesn't compile: EphyWindow no longer has a ui_merge variable. You
have
to use ephy_window_get_ui_manager()

- it doesn't compile with -Werror, due to missing prototypes (although
we want
to avoid prototypes whereever possible, you have to have them for glade
callbacks)

+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif

Just include it unconditionally (config.h will always be present).

+#ifndef ngettext
+#define ngettext(Msgid1, Msgid2, N) \
+	((N) == 1 ? (const char *) (Msgid1) : (const char *) (Msgid2))
+#endif

#include <glib/gi18n-lib.h> will provide a i18n-correct 'ngettext'.

+struct _EphyActionsExtensionEditorDialogPrivate

This is getting loooong :) perhaps just "EphyActionsEditor[Private]" ?
(same for the class and instance names; but keep the long name in the
get_type
func).

+static void
+ephy_actions_extension_editor_dialog_init
+	(EphyActionsExtensionEditorDialog *dialog)

Code style is 1st param on the same line.

+static GtkWidget *
+ephy_actions_extension_editor_dialog_append_popup_item
+	(EphyActionsExtensionEditorDialog *dialog,
+	 const char *stock_id,
+	 GCallback callback)
+{
+	GtkWidget *item;
+
+	g_return_val_if_fail (EPHY_IS_ACTIONS_EXTENSION_EDITOR_DIALOG
(dialog), NULL);
+	g_return_val_if_fail (dialog->priv->popup_menu != NULL, NULL);
+	g_return_val_if_fail (stock_id != NULL, NULL);
+	g_return_val_if_fail (callback != NULL, NULL);
+

You're calling this from just 2 places in the same file, so it doesn't
need all those checks.

+	g_return_if_fail (GTK_IS_LIST_STORE (store));
+	g_return_if_fail (iter != NULL);
+	g_return_if_fail (EPHY_IS_NODE (action));
+

Same.

+	g_return_if_fail (GTK_IS_LIST_STORE (store));
+	g_return_if_fail (EPHY_IS_NODE (action));

And here.

+	g_return_val_if_fail (GTK_IS_LIST_STORE (store), FALSE);
+	g_return_val_if_fail (iter != NULL, FALSE);
+	g_return_val_if_fail (EPHY_IS_NODE (action), FALSE);

And here.

Looks like this could use EphyNodeView instead of getting the info from
the node db
and stuffing it in a list store?

+gboolean
+ephy_actions_extension_editor_dialog_view_popup_menu_cb
+	(GtkWidget *widget, EphyActionsExtensionEditorDialog *dialog)
+{
+	gtk_menu_popup (GTK_MENU (dialog->priv->popup_menu), NULL, NULL,
+			NULL, NULL, 0, gtk_get_current_event_time ());
+
+	return TRUE;		/* menu activated */
+}

Needs to select the first menu item (gtk_menu_shell_select_first()) when
popping up the context menu
with the keyboard (but not in the other function where it's done from
mouse button press).
Also needs to use ephy_gui_menu_position_tree_selection menu positioning
function (otherwise
the menu will pop up wherever the mouse is, but this is from a key
event!)
If you use a node view, ephy_node_view_popup does all that for you.

gboolean
+ephy_actions_extension_editor_dialog_view_button_press_event_cb
+	(GtkWidget *widget,
+	 GdkEventButton *event,
+	 EphyActionsExtensionEditorDialog *dialog)
+{
+	if (event->button == 3)
+		gtk_menu_popup (GTK_MENU (dialog->priv->popup_menu), NULL,
+				NULL, NULL, NULL, event->button, event->time);
+
+	return FALSE;		/* propagate event */
+}

The event was consumed, why propagate further?

+static void
+ephy_actions_extension_properties_dialog_link
+	(EphyActionsExtensionPropertiesDialog *dialog, ...)
+{
+	const char *control_id;
+	va_list args;
+
+	g_return_if_fail (EPHY_IS_ACTIONS_EXTENSION_PROPERTIES_DIALOG
(dialog));
+
+	va_start (args, dialog);
+	while ((control_id = va_arg (args, const char *)))
+

Code style: while (() != NULL)

This dialogue could profit from generic EphyDialog support for nodes
instead of gconf... I have
half-finished code somewhere... just something to keep in mind, I don't
expect you to write that
now :)

+	else
+		title = g_strdup (_("Action Properties"));

Code style: { } even around 1-liners.

+void
+ephy_actions_extension_properties_dialog_response_cb
+	(GtkDialog *dialog,
+	 int response,
+	 EphyActionsExtensionPropertiesDialog *pdialog)
+{
+	if (pdialog->priv->add && response == GTK_RESPONSE_OK)
+	{
+		EphyNode *actions;
+	  
+		actions = ephy_actions_extension_get_actions
+			(pdialog->priv->extension);
+
+		ephy_node_ref (pdialog->priv->action);
+		ephy_node_add_child (actions, pdialog->priv->action);
+	}
+
+	g_object_unref (pdialog);
+}
+

Why ref the node here? The dialogue will be finialised immediately
afterwards, and the node
unreffed in finalize().

+typedef struct _EphyActionsExtensionPropertiesDialog
EphyActionsExtensionPropertiesDialog;

Rather long too... maybe "EphyActionProperties[Private|Class]" ?

+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <glib.h>
+
+char *
+ephy_actions_extension_format_name_for_display (const char *name)
+{

#include "ephy-actions-extension-util.h" too.

+#include <string.h>
+#include <stdarg.h>
+#include <glib/gi18n.h>

No. glib/gi18n-lib.h instead.

+static void
+ephy_actions_extension_save_actions (EphyActionsExtension *extension)
+{
+	g_return_if_fail (EPHY_IS_ACTIONS_EXTENSION (extension));
+	g_return_if_fail (extension->priv->dirty == TRUE);
+
+	if (ephy_node_db_write_to_xml_safe (extension->priv->db,
+					    extension->priv->xml_file,
+					    ACTIONS_XML_ROOT, ACTIONS_XML_VERSION,
+					    NULL, extension->priv->actions, 0,
+					    NULL) == 0)
+		extension->priv->dirty = FALSE;
+	else
+		g_warning (_("unable to save actions"));
+}

Code style {\n ... \n } (also in many places below and above).

No need to translate warnings on console (g_warning).

+static void
+ephy_actions_extension_dequeue_save_actions (EphyActionsExtension
*extension)
+{
+	g_return_if_fail (EPHY_IS_ACTIONS_EXTENSION (extension));

Unnecessary check.

+		dialog = gtk_message_dialog_new
+			(GTK_WINDOW (window), GTK_DIALOG_DESTROY_WITH_PARENT,
+			 GTK_MESSAGE_ERROR, GTK_BUTTONS_OK,
+			 _("Could not run command: %s"),
+			 err->message);
+		g_error_free (err);
+
+		g_signal_connect (dialog, "response",
+				  G_CALLBACK (gtk_widget_destroy), NULL);
+		gtk_dialog_set_has_separator (GTK_DIALOG (dialog), FALSE);
+		gtk_widget_show (dialog);

Could use better wording (command and err details in secondary not
primary text). No need
to unset has_sep (gtk+ 2.6 does that for message dialogues). 

+	gtk_ui_manager_remove_ui (GTK_UI_MANAGER(window->ui_merge),
+				  data->user_ui_id);

manager = GTK_UI_MANAGE R(ephy_window_get_ui_manager (window));
gtk_ui_manager_remove_ui (manager, data->user_ui_id).

etc.

+	const char *popups[] = {
+		"/EphyDocumentPopup",
+		"/EphyFramedDocumentPopup",
+		"/EphyLinkPopup",
+		"/EphyImageLinkPopup",
+		"/EphyImagePopup"
+	};

There's also now EphyFullscreen[Framed]DocumentPopup.

+#include <gmodule.h>
+#include "ephy-actions-extension.h"
+#include "ephy-actions-extension-editor-dialog.h"
+#include "ephy-actions-extension-properties-dialog.h"
+
#include <glib/gi18n-lib.h> too.

+G_MODULE_EXPORT GType
+register_module (GTypeModule *module)
+{

needs a prototype.




Regards,
	Christian