[Github-comments] [geany/geany-plugins] commander: use new plugin API. (#703)

[Colomban Wendling]

b4n requested changes on this pull request.

I fail to see what value these changes give, given that the "old" API is fully functional (we spent some effort to make sure it'd still work flawlessly), and the new one, while nicer, doesn't provide any actual new feature this plugin would need.  This looks like a change for the sake of change, which I don't really fancy (one reason being a bug likeliness to improvement ration *very* low).  Also, *if* this gets done, it should fully embrace the new API philosophy and try and get rid of the global variables (unless actually problematic).

And there's at least one unrelated change (help location), which should at least be a separate commit.

> @@ -32,18 +32,8 @@
 /*#define DISPLAY_SCORE 1*/
 
 
-GeanyPlugin      *geany_plugin;
-GeanyData        *geany_data;
-
-PLUGIN_VERSION_CHECK(226)
-
-PLUGIN_SET_TRANSLATABLE_INFO (
-  LOCALEDIR, GETTEXT_PACKAGE,
-  _("Commander"),
-  _("Provides a command panel for quick access to actions, files and more"),
-  VERSION,
-  "Colomban Wendling <ban at herbesfolles.org>"
-)
+static GeanyPlugin      *geany_plugin = NULL;
+static GeanyData        *geany_data = NULL;

These globals should be removed then, unless it's *highly* impractical

> @@ -762,11 +752,15 @@ on_plugin_idle_init (gpointer dummy)
   return FALSE;
 }
 
-void
-plugin_init (GeanyData *data)
+
+static gboolean
+plugin_commander_init (GeanyPlugin *plugin, G_GNUC_UNUSED gpointer pdata)

style: each argument on its own line

> @@ -783,10 +777,12 @@ plugin_init (GeanyData *data)
   /* delay for other plugins to have a chance to load before, so we will
    * include their items */
   plugin_idle_add (geany_plugin, on_plugin_idle_init, NULL);
+  return TRUE;

style: blank line above return

>  }
 
-void
-plugin_cleanup (void)
+
+static void
+plugin_commander_cleanup (G_GNUC_UNUSED GeanyPlugin *plugin, G_GNUC_UNUSED gpointer pdata)

style: each argument on its own line

> @@ -796,8 +792,32 @@ plugin_cleanup (void)
   }
 }
 
-void
-plugin_help (void)
+
+static void
+plugin_commander_help (G_GNUC_UNUSED GeanyPlugin *plugin, G_GNUC_UNUSED gpointer pdat)

style: each argument on its own line

> @@ -796,8 +792,32 @@ plugin_cleanup (void)
   }
 }
 
-void
-plugin_help (void)
+
+static void
+plugin_commander_help (G_GNUC_UNUSED GeanyPlugin *plugin, G_GNUC_UNUSED gpointer pdat)
+{
+  utils_open_browser("https://plugins.geany.org/commander.html");

style: space before the open parenthesis

> @@ -796,8 +792,32 @@ plugin_cleanup (void)
   }
 }
 
-void
-plugin_help (void)
+
+static void
+plugin_commander_help (G_GNUC_UNUSED GeanyPlugin *plugin, G_GNUC_UNUSED gpointer pdat)
+{
+  utils_open_browser("https://plugins.geany.org/commander.html");

Why changing the target URI?  Why open a website when the same documentation is available locally?

> @@ -762,11 +752,15 @@ on_plugin_idle_init (gpointer dummy)
   return FALSE;
 }
 
-void
-plugin_init (GeanyData *data)
+

style: only one blank line

>  {
-  utils_open_browser (DOCDIR "/" PLUGIN "/README");
+	/* Setup translation */

style: indentation should be 2 spaces

>  {
-  utils_open_browser (DOCDIR "/" PLUGIN "/README");
+	/* Setup translation */
+	main_locale_init(LOCALEDIR, GETTEXT_PACKAGE);

style: space before open parenthesis

> -  utils_open_browser (DOCDIR "/" PLUGIN "/README");
+	/* Setup translation */
+	main_locale_init(LOCALEDIR, GETTEXT_PACKAGE);
+
+	/* Set metadata */
+	plugin->info->name = _("Commander");
+	plugin->info->description = _("Provides a command panel for quick access to actions, files and more");
+	plugin->info->version = VERSION;
+	plugin->info->author = "Colomban Wendling <ban at herbesfolles.org>";
+
+	/* Set functions */
+	plugin->funcs->init = plugin_commander_init;
+	plugin->funcs->cleanup = plugin_commander_cleanup;
+	plugin->funcs->help = plugin_commander_help;
+
+	/* Register! */

this comment should be removed, as it adds no value and is not very formal

> +	/* Setup translation */
+	main_locale_init(LOCALEDIR, GETTEXT_PACKAGE);
+
+	/* Set metadata */
+	plugin->info->name = _("Commander");
+	plugin->info->description = _("Provides a command panel for quick access to actions, files and more");
+	plugin->info->version = VERSION;
+	plugin->info->author = "Colomban Wendling <ban at herbesfolles.org>";
+
+	/* Set functions */
+	plugin->funcs->init = plugin_commander_init;
+	plugin->funcs->cleanup = plugin_commander_cleanup;
+	plugin->funcs->help = plugin_commander_help;
+
+	/* Register! */
+	GEANY_PLUGIN_REGISTER(plugin, 226);

style: space before open parenthesis (OK, it was wrong for `PLUGIN_VERSOIN_CHECK()` before, but well)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/703#pullrequestreview-99132369
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20180224/8667cc52/attachment.html>