Re: New extension: Actions

On Wed, 2005-01-19 at 02:33 +0100, Jean-Yves Lefort wrote:
> Please consider committing the patch.

Here's a quick review before Christian gets to it:

+static void ephy_action_properties_dialog_set_property (GObject
+							unsigned int prop_id,
+							const GValue *value,
+							GParamSpec *pspec);
+static void ephy_action_properties_dialog_get_property (GObject
+							unsigned int prop_id,
+							GValue *value,
+							GParamSpec *pspec);
+static void ephy_action_properties_dialog_update_title
(EphyActionPropertiesDialog *dialog);

Try to avoid declaring functions at the top of the file. Notice how
little it's done elsewhere in epiphany(-extensions)....

+ephy_action_properties_dialog_register_type (GTypeModule *module)
+  static const GTypeInfo info = {
+    sizeof(EphyActionPropertiesDialogClass),
+    NULL,

Indent with 8-space tabs, not spaces. (See the HACKING file....)

+  if (dialog->priv->id)
+    apply_type = PT_AUTOAPPLY;
+  else
+    {

See the HACKING file....

+      dialog->priv->id = g_strdup_printf("action_%lu_%lu", now.tv_sec,

Is there no better way?

+  dialog->priv->popup_remove_item =
ephy_actions_editor_dialog_append_popup_item(dialog, GTK_STOCK_REMOVE,

As a general guideline, we like to stay below 80 characters wide, too.
(I think that's in the HACKING file.)

+      markup = g_markup_printf_escaped("<span weight=\"bold\">%
s</span>\n%s", formatted, description);

Can't you just use <b></b>?

+  ephy_actions_editor_dialog_update_list(user_data);

I'm not 100% sure, but I think extensions are always invoked from within
the same thread. Christian would know for sure.

 # List of source files containing translatable strings.
 # Please keep this file sorted alphabetically.

I haven't checked them all, but extensions/actions/extension.c doesn't
have translatable strings and I imagine there are others which don't, as

That's all I can see. The code itself looks great as far as I can tell,
but I'm sure Christian will spot other problems....

Adam Hooper <adamh densi com>

Attachment: signature.asc
Description: This is a digitally signed message part

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]