On Sat, 22 Jan 2005 20:58:11 +0100
Christian Persch <chpe gnome org> wrote:
> Here's another review pass:
I've attached an updated patch addressing your review.
However:
> +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).
That's not consistent with the naming scheme, so no.
> +static void
> +ephy_actions_extension_editor_dialog_init
> + (EphyActionsExtensionEditorDialog *dialog)
>
> Code style is 1st param on the same line.
That'd make too many lines go beyond 80 chars.
> +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.
My policy is to exhaustively check function arguments, unless the
function is a callback. I'm not making any exception.
> 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 */
> +}
I don't think I can use markup with an EphyNodeView. I could use one
column for the name and one column for the description, but it would
not look as good.
> 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?
Because I want the row to be selected even if the context menu was
popped up.
> +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 *)))
> +
>
> 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 :)
I was thinking about that while writing the code. :)
> +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().
Because ephy_node_add_child() does not ref the child.
> +typedef struct _EphyActionsExtensionPropertiesDialog
> EphyActionsExtensionPropertiesDialog;
>
> Rather long too... maybe "EphyActionProperties[Private|Class]" ?
I'm not compromising naming scheme consistency just to type a few
characters less.
> +static void
> +ephy_actions_extension_dequeue_save_actions (EphyActionsExtension
> *extension)
> +{
> + g_return_if_fail (EPHY_IS_ACTIONS_EXTENSION (extension));
>
> Unnecessary check.
See above.
> + 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).
Primary/secondary text support has been introduced in GTK+ 2.6, I
guess you don't want to depend on it yet.
Greetings,
Jean-Yves Lefort
--
Jean-Yves Lefort
jylefort brutele be
http://lefort.be.eu.org/
Attachment:
epiphany-extensions-1.5.3-actions.diff.gz
Description: Binary data
Attachment:
pgpomSUqkccbY.pgp
Description: PGP signature