Nothing more to say. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/703
-- Commit Summary --
* commander: use new plugin API.
-- File Changes --
M commander/src/commander-plugin.c (60)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/703.patch https://github.com/geany/geany-plugins/pull/703.diff
@LarsGit223 just one question: why?
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@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@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@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)
And there's at least one unrelated change (help location), which should at least be a separate commit.
Ok, I did not realize that it is opening the local file. Will remove it from this PR.
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).
Hmmm....well it is not a change for the user in terms of functionality of course. But IMHO it is not just a change for the sake of change but a change for consistency and cleanup. Also, there was some reason to design the new API or not? The new API seems to be more easily extendable. This does not matter right now but maybe in the future if a API change/extension is required. I thought it would be nice to have all plugins using the new API and throw away the code which is required to support the old one.
And maybe I should not say "new API" but current API as it was introduced in Geany 1.26 (November 15, 2015).
Anyway, I should maybe have opened a discussion before starting this one. So I say sorry for my "bold" PR flood. This is your plugin, if you rather like to keep it like it is then I accept it of course.
But IMHO it is not just a change for the sake of change but a change for consistency and cleanup.
I sympathize with the idea of using current APIs in general. However, as said we made quite some efforts to make sure the older API is still fully functional -- and if you check Geany's code, it's basically a compatibility module which does not add complexity to the actual loader :) So yeah it's likely some new features could be exclusive to the the API, but it's unlikely we'd *have* to drop support for the old one. And while it could indeed be nice for cleaning up some of Geany's code, we like avoiding unnecessary burden on plugin developers -- the reason why we went to great length not to drop the old API at first. Remember that there are some plugins not on this repository. So… I'm feeling kind of conservative here, but I'd prefer to see this kind of changes as part of a development effort on the plugin, suggesting interest and testing in that very plugin -- because yeah, it seems like a fairly innocent change without too much risk, but…
Also, there was some reason to design the new API or not?
Yes, mostly to make it easier/possible to write "proxy" plugins that can load other plugins (like https://github.com/kugel-/peasy).
Anyway, I should maybe have opened a discussion before starting this one. So I say sorry for my "bold" PR flood.
Well, don't get me wrong :) I like the initiative of proposing changes and writing the code. Yes, sometimes discussing it ahead can save a little time (mostly on the person actually writing the code), but so long as you can accept "no" (or "please change this that and that") as an answer it's all good.
And despite what I'm saying that I don't really like changing working code for something that isn't tremendously simpler or adding new features or fixing bugs, if @frlan or other want it to happen I'll be fine :)
@LarsGit223 pushed 2 commits.
c834160 commander: corrected style. 9611450 commander: reverted change of help URL.
LarsGit223 commented on this pull request.
@@ -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@herbesfolles.org" -) +static GeanyPlugin *geany_plugin = NULL; +static GeanyData *geany_data = NULL;
How? Move them into ```struct plugin_data``` or try to pass them as user pointers, e.g. from ```plugin_commander_init()``` to ```on_plugin_idle_init (-->create_panel...)```.
github-comments@lists.geany.org