Created attachment 165090 [details] [review]
Added PeasUIHelpable and a Help button to the plugin manager

To be honest I don't like the idea to add an extension interface for that at all.

I would prefer to have a new plugin info property ("Help-URI" ?) which would contain the URI for the help file. The host program could do whatever it wants to do with it, and libpeasui would just use it to add a "help" button in the configuration dialog for the plugin and/or to the about box.

Using an URI also has the advantage that you can specify either a gnome help page through ghelp:, a website page through http: or even a local file through file:, just by using gtk_show_uri or equivalent.

The issue I can see is about portability and win32/osx support. win32 help files could be a simple URL as well, but I have no idea how help works in osX.
Also it might be good if we allowed to specify the help file url for all the supported systems at once, so plugins can be copied from gnome to win32 painlessly.

Created attachment 165147 [details] [review]
Added Help to the plugin info and a Help button to the plugin manager

Review of attachment 165147 [details] [review]:

::: libpeas/peas-plugin-info-priv.h
@@ +45,3 @@
   gchar *website;
   gchar *version;
+  gchar *help;

Please help something more explicit, like "help_uri".

::: libpeasui/peas-ui-plugin-info.h
@@ +29,2 @@
 const gchar  *peas_ui_plugin_info_get_icon_name            (PeasPluginInfo *info);
+const gchar  *peas_ui_plugin_info_get_help                 (const PeasPluginInfo *info);

Idem, use help_uri.

I guess it could go as peas_plugin_info_get_help_uri in peas-plugin-info.h: I secretely dream about killing peas-ui-plugin-info.h

::: peas-demo/plugins/helloworld/helloworld.peasdemo-plugin
@@ +9,3 @@
+[Linux]
+
+Help=http://git.gnome.org/browse/libpeas/

Could you please *not* use an INI section?

The reason is that I'd like to keep them for other uses (like plugins for several apps)

I'd propose Help, Help-Gnome, Help-Windows, Help-MacOS-X

Created attachment 165435 [details] [review]
Added a Help uro to the plugin info and a Help button to the plugin manager v2

Review of attachment 165435 [details] [review]:

::: libpeas/peas-plugin-info.c
@@ +302,3 @@
 
+  /* Get Help */
+  tmp = g_strdup_printf ("Help-%s", os_name);

Why don't you do something like this:

  str = g_key_file_get_string (plugin_file, section_header, OS_SPECIFIC_HELP_KEY, NULL);

and use a #define instead of g_strdup_printf ?

::: libpeasui/peas-ui-plugin-manager.c
@@ +988,3 @@
+  pm->priv->help_button = gtk_button_new_from_stock (GTK_STOCK_HELP);
+  gtk_container_add (GTK_CONTAINER (hbuttonbox), pm->priv->help_button);
+  gtk_button_box_set_child_secondary (GTK_BUTTON_BOX (hbuttonbox), pm->priv->help_button, TRUE);

I still think this help button should be located on the configure dialog rather than on the bottom left of the plugin manager: putting the button on the plugin manager sort of implies the help is about the plugin manager dialog.

Created attachment 165453 [details] [review]
Added a Help URI to the plugin info and a Help button to the plugin manager v3

Review of attachment 165453 [details] [review]:

This patch looks mostly good. It will go soon I'm sure. After the manager split patch?


BTW the patch title exceeds 72 chars. You can probably mention the help button only in the commit message since it's a consequency of the support for the help uri...

the pythonhello.py part should probably go as a separate plugin, and not extend the existing class but another one. A configurable python plugin is really missing right now.

::: libpeas/peas-plugin-info.c
@@ +135,3 @@
+          g_str_equal (keys[i], "Version") ||
+          g_str_equal (keys[i], "Help") ||
+          g_str_equal (keys[i], OS_HELP_KEY))

I liked the strprefix solution more. If you do what you do now, you will add Help-MacOS-X in the hash table if you are using gedit on GNOME

::: libpeasui/peas-ui-plugin-info.h
@@ +29,2 @@
 const gchar  *peas_ui_plugin_info_get_icon_name            (PeasPluginInfo *info);
+const gchar  *peas_ui_plugin_info_get_help_uri             (const PeasPluginInfo *info);

I think it could go in libpeas/peas-plugin-info.h as you can imagine writing the URI string or launching a browser to it even if you're not using Gtk+ (Clutter comes to mind)

::: libpeasui/peas-ui-plugin-manager.c
@@ +214,3 @@
+  GtkWindow *toplevel;
+  GtkWidget *error_dlg;
+  GtkWindowGroup *wg;

I know it's harder, but those vars should be defined at the top of the function to avoid having some compilers complain (I can hear the solaris guys screaming)

@@ +267,3 @@
   GtkWindow *toplevel;
   GtkWidget *conf_widget = NULL;
+  gchar *title;

What is this new variable for?

::: peas-demo/plugins/pythonhello/pythonhello.py
@@ +9,3 @@
 LABEL_STRING="Python Says Hello!"
 
+class PythonHelloPlugin(gobject.GObject, Peas.Activatable, PeasUI.Configurable):

You should create a new class instead of extending the existing one in order to show "best practices".

Created attachment 166233 [details] [review]
Added a Help button to the plugin manager v4

Review of attachment 166233 [details] [review]:

Looks great to me. Feel free to push, with an explanation of the way you implemented things (using Help: in the plugin info file) in the commit message.

Thanks.

The patch 166233 has been committed. The fix will be available in the next release.

Thanks!