This is my second attempt on the LSP API as originally proposed by #3571.
I changed several things: 1. I renamed the struct `Lsp` to `PluginExtension` - there's nothing LSP-specific in this interface and it could be used by other plugins too (or possibly extended in the future if some plugins wanted to provide another functionality, we could extend this API). 2. I reduced the number of functions in the API to the absolute minimum and tried to do as much as possible in the plugin itself. 3. For this PR I removed the API related to the sidebar symbol tree - this one requires most changes on the Geany side and possibly more discussion and I don't want to block this PR by it. One possible alternative is also just keep using TM for the symbol tree, ignoring the symbols provided by LSP (the symbol tree is the least problematic part of TM IMO and could stay the way it is). I'll post the extra patches to allow sidebar symbols in a separate PR.
I also updated the combined Geany+LSP plugin at https://github.com/techee/geany-lsp to use the new API and also to work even when compiled against unmodified Geany. This is quite clumsy, however - to avoid conflicts between Geany's TM implementation and the plugin, it requires disabling TM by adding ``` [settings] tag_parser= ``` to the filetype config file of the affected filetypes, basically disabling all TM features. It also requires separate keybindings for e.g. tag goto or autocompletion instead of re-using the Geany ones. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3849
-- Commit Summary --
* Add interface used by plugins to replace some Geany functionality with their own implementation * Add Geany code delegating autocompletion to plugins * Add Geany code delegating calltips to plugins * Add Geany code delegating symbol goto to plugins * Add Geany code delegating keyword highlighting to plugins
-- File Changes --
M meson.build (3) M plugins/geanyplugin.h (1) M src/Makefile.am (2) M src/document.c (4) M src/editor.c (34) M src/keybindings.c (13) A src/pluginextension.c (141) A src/pluginextension.h (76) M src/symbols.c (11) M src/symbols.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/3849.patch https://github.com/geany/geany/pull/3849.diff
@techee pushed 1 commit.
1931fcc2f4d5de46871c85a782903d8c4d41e699 Export some types/functions so plugins have access to Geany symbol icons
@b4n Ping 2 :-)
Basically this is the API I currently propose. I believe it's quite nice also that it doesn't serve for LSP only but it could be used by other plugins too. I for instance noticed https://github.com/geany/geany/issues/3858 and if somebody wanted they could use this API for implementing a Tree-sitter backend.
What the API does is that it just simply disables the Geany functionality and makes the plugin do the work instead. This also reduces the amount of changes needed in Geany as Geany doesn't have to care what exactly the plugin does.
While in some cases it might be possible to reduce the number of checks whether the plugin implements certain feature, I didn't include it in this PR - such a refactoring can come later. Also, there are no docstrings, API bump etc. yet as the API may change based on the discussion here.
Now the symbol tree. I came to the conclusion that LSP symbols, which we know nothing about and which are only meant to be blindly displayed in the symbol tree without any processing, and TM symbols where we really know what they contain, are fundamentally incompatible and any form of unification will lead to this kind of ugly code everywhere: ```C if (tag->is_lsp) //LSP symbol - do one thing else //TM symbol - do other thing ``` I tried this in the extra patches of https://github.com/geany/geany/pull/3850 and I don't like the result at all.
So for the symbol tree I propose this: 1. For now, keep using TM for it and wait for users feedback. The thing is that TM works very well for it and I kind of suspect that nobody will notice anything. 2. If there are complaints that users actually really want LSP-based symbol tree, I'd just make a separate "LSP Symbols" tab in the sidebar that would be implemented completely in the plugin (by stealing most of Geany's symbol tree code). Even though this means some code duplication, it isn't as bad as making Geany code ugly and hard to maintain because of the incompatible symbol representations.
Thoughts?
First a general comment, the idea of a general idiom that allows items of Geany functionality to be substituted by plugins is a good one. It has the potential to be expanded for many uses. For example you may remember a "discussion" some time ago between me and @b4n about auto indentation and how a general solution was never going to be right and how the built-in indentation needed to be replaceable somehow for some languages. This is an example of a non-LSP use that something general like this could be expanded to cover by an "Add Geany code for delegating indentation to plugins" commit (in the future, not this PR).
Leaving the symbols tree TM seems ok for now, its only a navigation tool when LSP is running and its adequate for that.
@techee pushed 1 commit.
e2f9dfb54bd0bd8d432022813118319af7ca427d Add "force" parameter to autocomplete_perform() which may be useful for plugins
@b4n requested changes on this pull request.
This is not an entirely through review, but still not an overpass. Anyway, it seems fairly similar to the previous PR, minus the symbols tree. This said, it makes it a lot smaller, and looking a lot less specific. Admittedly, finding it less specific also comes from me looking a bit deeper in the implementation and understanding more of the choices when they looks awfully specific.
I think there still are a few things that could/should be polished, but it looks fair enough :+1:
To actually understand it better, I tried porting the Geany features it replaces to use the API -- so to see if it was enough for our internals. The answer seems to be "almost", which is a fair start :) Here is the current diff I have for this (not polished). Beware that it is *not* ready for working with another PluginExtension. <details><summary>Big diff ahead!</summary>
```diff diff --git a/src/document.c b/src/document.c index 3e5e1972d..adf5b8cc6 100644 --- a/src/document.c +++ b/src/document.c @@ -2720,7 +2720,7 @@ void document_highlight_tags(GeanyDocument *doc) GString *keywords_str; gint keyword_idx;
- if (plugin_extension_symbol_highlight_provided(doc)) + if (! plugin_extension_active(plugin_extension_geany)) return;
/* some filetypes support type keywords (such as struct names), but not diff --git a/src/editor.c b/src/editor.c index 336afadf2..7a1d2fdc1 100644 --- a/src/editor.c +++ b/src/editor.c @@ -672,12 +672,8 @@ static gboolean reshow_calltip(gpointer data)
g_return_val_if_fail(calltip.sci != NULL, FALSE);
- doc = document_get_current(); - - if (plugin_extension_calltips_provided(doc)) - return FALSE; - SSM(calltip.sci, SCI_CALLTIPCANCEL, 0, 0); + doc = document_get_current();
if (doc && doc->editor->sci == calltip.sci) { @@ -816,8 +812,6 @@ static void on_char_added(GeanyEditor *editor, SCNotification *nt) break; } case '>': - editor_start_auto_complete(editor, pos, FALSE); /* C/C++ ptr-> scope completion */ - /* fall through */ case '/': { /* close xml-tags */ handle_xml(editor, pos, nt->ch); @@ -826,23 +820,6 @@ static void on_char_added(GeanyEditor *editor, SCNotification *nt) case '(': { auto_close_chars(sci, pos, nt->ch); - if (!plugin_extension_calltips_provided(editor->document)) - /* show calltips */ - editor_show_calltip(editor, --pos); - break; - } - case ')': - { /* hide calltips */ - if (SSM(sci, SCI_CALLTIPACTIVE, 0, 0) && - !plugin_extension_calltips_provided(editor->document)) - { - SSM(sci, SCI_CALLTIPCANCEL, 0, 0); - } - g_free(calltip.text); - calltip.text = NULL; - calltip.pos = 0; - calltip.sci = NULL; - calltip.set = FALSE; break; } case '{': @@ -859,12 +836,6 @@ static void on_char_added(GeanyEditor *editor, SCNotification *nt) close_block(editor, pos - 1); break; } - /* scope autocompletion */ - case '.': - case ':': /* C/C++ class:: syntax */ - /* tag autocompletion */ - default: - editor_start_auto_complete(editor, pos, FALSE); }
if (plugin_extension_calltips_provided(editor->document)) @@ -1166,7 +1137,8 @@ static gboolean on_editor_notify(G_GNUC_UNUSED GObject *object, GeanyEditor *edi /* now that autocomplete is finishing or was cancelled, reshow calltips * if they were showing */ autocomplete_scope_shown = FALSE; - request_reshowing_calltip(nt); + if (plugin_extension_active(plugin_extension_geany)) + request_reshowing_calltip(nt); break; case SCN_NEEDSHOWN: ensure_range_visible(sci, nt->position, nt->position + nt->length, FALSE); @@ -1180,7 +1152,7 @@ static gboolean on_editor_notify(G_GNUC_UNUSED GObject *object, GeanyEditor *edi break;
case SCN_CALLTIPCLICK: - if (!plugin_extension_calltips_provided(doc) && nt->position > 0) + if (plugin_extension_active(plugin_extension_geany) && nt->position > 0) { switch (nt->position) { @@ -2231,9 +2203,6 @@ gboolean editor_start_auto_complete(GeanyEditor *editor, gint pos, gboolean forc
g_return_val_if_fail(editor != NULL, FALSE);
- if (plugin_extension_autocomplete_provided(editor->document)) - return FALSE; - if (! editor_prefs.auto_complete_symbols && ! force) return FALSE;
@@ -5326,6 +5295,83 @@ void editor_indent(GeanyEditor *editor, gboolean increase) }
+void editor_extension_autocomplete_perform(GeanyDocument *doc, gboolean force) +{ + gint pos = sci_get_current_position(doc->editor->sci); + + /* FIXME: maybe we could call editor_start_auto_complete() unconditionally? */ + + if (force) + editor_start_auto_complete(doc->editor, pos, force); + else + { + /* we're called after typing something */ + gchar ch = sci_get_char_at(doc->editor->sci, pos - 1); + + switch (ch) + { + case '\r': + case '\n': + case '/': + case '(': + case ')': + case '{': + case '}': + case '[': + case '"': + case ''': + /* not used to trigger autoc */ + break; + + /* scope autocompletion */ + case '>': + case '.': + case ':': /* C/C++ class:: syntax */ + /* tag autocompletion */ + default: + editor_start_auto_complete(doc->editor, pos, force); + } + } +} + + +void editor_extension_calltips_show(GeanyDocument *doc, gboolean force) +{ + gint pos = sci_get_current_position(doc->editor->sci); + + if (force) + editor_show_calltip(doc->editor, pos - 1); + else + { + /* we're called after typing something */ + gchar ch = sci_get_char_at(doc->editor->sci, pos - 1); + + switch (ch) + { + case '(': + { + /* show calltips */ + editor_show_calltip(doc->editor, pos - 1); + break; + } + case ')': + { /* hide calltips */ + if (SSM(doc->editor->sci, SCI_CALLTIPACTIVE, 0, 0)) + { + SSM(doc->editor->sci, SCI_CALLTIPCANCEL, 0, 0); + } + g_free(calltip.text); + calltip.text = NULL; + calltip.pos = 0; + calltip.sci = NULL; + calltip.set = FALSE; + break; + } + } + } +} + + /** Gets snippet by name. * * If @a editor is passed, returns a snippet specific to the document filetype. diff --git a/src/editor.h b/src/editor.h index 4dffe4562..95c36a3e3 100644 --- a/src/editor.h +++ b/src/editor.h @@ -279,6 +279,9 @@ void editor_snippets_free(void);
const GeanyEditorPrefs *editor_get_prefs(GeanyEditor *editor);
+void editor_extension_autocomplete_perform(struct GeanyDocument *doc, gboolean force); +void editor_extension_calltips_show(struct GeanyDocument *doc, gboolean force); +
/* General editing functions */
diff --git a/src/keybindings.c b/src/keybindings.c index 9c8be3796..9eba0ef30 100644 --- a/src/keybindings.c +++ b/src/keybindings.c @@ -2154,14 +2154,10 @@ static gboolean cb_func_editor_action(guint key_id) case GEANY_KEYS_EDITOR_AUTOCOMPLETE: if (plugin_extension_autocomplete_provided(doc)) plugin_extension_autocomplete_perform(doc, TRUE); - else - editor_start_auto_complete(doc->editor, sci_get_current_position(doc->editor->sci), TRUE); break; case GEANY_KEYS_EDITOR_CALLTIP: if (plugin_extension_calltips_provided(doc)) plugin_extension_calltips_show(doc, TRUE); - else - editor_show_calltip(doc->editor, -1); break; case GEANY_KEYS_EDITOR_CONTEXTACTION: if (check_current_word(doc, FALSE)) diff --git a/src/pluginextension.c b/src/pluginextension.c index 8e78eb44e..0460a7c5a 100644 --- a/src/pluginextension.c +++ b/src/pluginextension.c @@ -20,46 +20,38 @@
#include "pluginextension.h"
+#include "editor.h" +#include "symbols.h"
-static gboolean func_return_false(GeanyDocument *doc) + +static gboolean func_return_true(GeanyDocument *doc) { - return FALSE; + return TRUE; }
- static GPtrArray *func_return_ptrarr(GeanyDocument *doc) { return NULL; }
+static PluginExtension plugin_extension_geany_intenral = { + .autocomplete_provided = func_return_true, + .autocomplete_perform = editor_extension_autocomplete_perform,
-static void func_args_doc_bool(GeanyDocument *doc, gboolean dummy1) -{ -} - - -static void func_args_doc_int_bool(GeanyDocument *doc, gint dummy1, gboolean dummy2) -{ -} - - -static PluginExtension dummy_extension = { - .autocomplete_provided = func_return_false, - .autocomplete_perform = func_args_doc_bool, + .calltips_provided = func_return_true, + .calltips_show = editor_extension_calltips_show,
- .calltips_provided = func_return_false, - .calltips_show = func_args_doc_bool, + .goto_provided = func_return_true, + .goto_perform = symbols_goto_perform,
- .goto_provided = func_return_false, - .goto_perform = func_args_doc_int_bool, - - .doc_symbols_provided = func_return_false, + .doc_symbols_provided = func_return_true, .doc_symbols_get = func_return_ptrarr,
- .symbol_highlight_provided = func_return_false + .symbol_highlight_provided = func_return_true };
-static PluginExtension *current_extension = &dummy_extension; +static PluginExtension *current_extension = &plugin_extension_geany_intenral; +PluginExtension *plugin_extension_geany = &plugin_extension_geany_intenral;
GEANY_API_SYMBOL @@ -74,13 +66,20 @@ void plugin_extension_register(PluginExtension *extension) GEANY_API_SYMBOL void plugin_extension_unregister(PluginExtension *extension) { - current_extension = &dummy_extension; + current_extension = &plugin_extension_geany_intenral; +} + + +GEANY_API_SYMBOL +gboolean plugin_extension_active(PluginExtension *extension) +{ + return current_extension == extension; }
/* allow plugins not to implement all the functions and fall back to the dummy * implementation */ -#define CALL_IF_EXISTS(f) (current_extension->f ? current_extension->f : dummy_extension.f) +#define CALL_IF_EXISTS(f) (current_extension->f ? current_extension->f : plugin_extension_geany_intenral.f)
gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc) { diff --git a/src/pluginextension.h b/src/pluginextension.h index bc4506421..48866a4a6 100644 --- a/src/pluginextension.h +++ b/src/pluginextension.h @@ -51,10 +51,13 @@ typedef struct {
void plugin_extension_register(PluginExtension *extension); void plugin_extension_unregister(PluginExtension *extension); +gboolean plugin_extension_active(PluginExtension *extension);
#ifdef GEANY_PRIVATE
+extern PluginExtension *plugin_extension_geany; + gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc); void plugin_extension_autocomplete_perform(GeanyDocument *doc, gboolean force);
diff --git a/src/symbols.c b/src/symbols.c index d579a9a0f..e1585ece1 100644 --- a/src/symbols.c +++ b/src/symbols.c @@ -1710,12 +1710,33 @@ gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition)
if (plugin_extension_goto_provided(doc)) { + /* FIXME: this should return TRUE on success so click handling in + * editor.c can let the even pass through if there was nothing to do here + * -- and we can't do that in _provided() above as we don't have all the + * info (position and whether it's a definition or not) */ plugin_extension_goto_perform(doc, pos, definition); return TRUE; }
+ return FALSE; +} + + +void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition) +{ + const gchar *name; + + /* This is actually the same as what is not by both symbols_goto_tag() callers */ + editor_find_current_word(doc->editor, pos, + editor_info.current_word, GEANY_MAX_WORD_LENGTH, NULL); + + name = *editor_info.current_word ? editor_info.current_word : NULL; + + if (! name) + return; + if (goto_tag(name, definition)) - return TRUE; + return;
/* if we are here, there was no match and we are beeping ;-) */ utils_beep(); @@ -1724,7 +1745,6 @@ gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition) ui_set_statusbar(FALSE, _("Forward declaration "%s" not found."), name); else ui_set_statusbar(FALSE, _("Definition of "%s" not found."), name); - return FALSE; }
diff --git a/src/symbols.h b/src/symbols.h index d4dd7fbfd..00face7f8 100644 --- a/src/symbols.h +++ b/src/symbols.h @@ -60,6 +60,8 @@ void symbols_show_load_tags_dialog(void);
gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition);
+void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition); + gint symbols_get_current_function(GeanyDocument *doc, const gchar **tagname);
gint symbols_get_current_scope(GeanyDocument *doc, const gchar **tagname); ``` </details>
This plus the inline comments should cover most of what I found, and start the discussion on this :) But basically I think this PR could probably land in a "timely" manner. I know Thomas won't like the absence of the symbols tree here, but IIUC pros/cons are not so clear with the reality of things. And we can always add this later if wanted.
+typedef struct {
+ gboolean (*autocomplete_provided)(GeanyDocument *doc); + void (*autocomplete_perform)(GeanyDocument *doc, gboolean force); + + gboolean (*calltips_provided)(GeanyDocument *doc); + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024];
Maybe it'd be safer to have e.g. `void (*_dummy[42])(void);` here, so we can be reasonably sure how to update the struct when adding new members? Or even just a (long) list of separate dummies, in case alignment of the array is different than alignment of the struct members themselves.
Just that I'm not even sure we *could* update this properly not to change the size of the structure -- which is probably OK so long as we don't update it past the 1024 bytes mark, but it'll be hard to tell when this happens.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
We might need a way for an extension to know whether it's active or not. In case we might one day support more extensions (or have Geany functions itself go through the extension mechanism), we need the extension to be able to know whether it's gonna be used or not when it performs ancillary things, like handle sci-notify and display calltips or such.
I'm playing with making Geany functions the base extension to see if the API would work for this, and one thing that it lacks is this, so it can't tell whether or not it should do its thing.
It's also probably important to introduce something like this rather early because it'll require the plugins to willfully behave. And it's relevant even if we support a single active extension, so that if two plugins provide an extension, although the last one "wins", they would still partly fight one another if they don't know they are not actually active.
For now I'm playing with something as basic as `gboolean plugin_extension_active(PluginExtension *extension)`, but we might want to provide something more flexible maybe, if we want to be able to mix extensions (e.g. if an extension only provides on of the features, complementing another one). It could be as simple as `const PluginExtension *plugin_extension_get_current(void)` (assuming we'd update the extension structure with the overrides instead of overriding the whole thing) and then callers could be supposed to check `if (plugin_extension_get_current()->autocomplete_perform == myplugin_atocomplete_perform) /* … */` -- or any better idea :slightly_smiling_face:
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
I see two issues here:
1. editor.c's call expects this to return TRUE *if it actually went to the definition*. I'm not sure how important this actually is, but the current API doesn't allow this. 2. we don't use the `name` in the API call, which makes sense (the extension might have better knowledge on what word actually is). However, all callers are guarding against the "current word" being non-empty, which is at least odd if not a possible issue.
I'm not entirely sure how to reconcile both those points in one single API call making all current callers happy though.
- if (plugin_extension_calltips_provided(doc))
+ return FALSE;
This should rather be in the `SCN_AUTOCCANCELLED` handling below (where it calls `request_reshowing_calltip(nt)`)
- gboolean (*doc_symbols_provided)(GeanyDocument *doc);
+ GPtrArray *(*doc_symbols_get)(GeanyDocument *doc);
This isn't used in this version of the patchset, is it? I guess it's for the symbols tab, right? I'm not against having it here, but there should be code to do something with it, or we leave it out and see whether it might be useful later.
+ +typedef struct { + gboolean (*autocomplete_provided)(GeanyDocument *doc); + void (*autocomplete_perform)(GeanyDocument *doc, gboolean force); + + gboolean (*calltips_provided)(GeanyDocument *doc); + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc);
I guess there isn't any `_perform()` because there isn't a canonical place where everybody should do this?
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */ + +#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + +/* Temporary define so the plugin can check whether it's compiled against + * Geany with plugin extension support or not */ +#define HAVE_GEANY_PLUGIN_EXTENSION 1 + +G_BEGIN_DECLS + + +typedef struct {
Maybe @elextr would be right to scream *no doooocs!* :laughing:
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
Hum… actually as whether a feature is provided or not is dynamic, a static check wouldn't be enough for actually supporting more than one extension.
`plugin_extension_autocomplete_provider(doc) == myplugin_extension` maybe? (or the function itself even)
@b4n commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
Ah also: do we want to add user data to those functions? That could be handy to pass in the general plugin data, especially as the "new" (which isn't so new anymore) plugin API allows this.
@b4n commented on this pull request.
+ .doc_symbols_provided = func_return_false, + .doc_symbols_get = func_return_ptrarr, + + .symbol_highlight_provided = func_return_false +}; + +static PluginExtension *current_extension = &dummy_extension; + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension) +{ + /* possibly, in the future if there's a need for multiple extensions, + * have a list of extensions and add/remove to/from the list */ + current_extension = extension;
Maybe add a warning if the active extension isn't the default one, so 2 conflicting plugins don't go entirely unnoticed?
@techee commented on this pull request.
- gboolean (*doc_symbols_provided)(GeanyDocument *doc);
+ GPtrArray *(*doc_symbols_get)(GeanyDocument *doc);
Just a few easy post-midnight answers, the rest tomorrow ;-).
Yeah, this one should be dropped.
@techee commented on this pull request.
+ +typedef struct { + gboolean (*autocomplete_provided)(GeanyDocument *doc); + void (*autocomplete_perform)(GeanyDocument *doc, gboolean force); + + gboolean (*calltips_provided)(GeanyDocument *doc); + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc);
Basically there's no need for any `perform()` here. In the other API calls it is necessary because the plugin needs to know when e.g. a keybinding invoking goto definition/declaration was pressed to perform the corresponding action. Here, instead, the plugin can perform the highlighting by itself e.g. when the current document becomes visible or the user types something and the only thing necessary is that geany knows highlighting is provided by the plugin so it doesn't do anything.
@techee commented on this pull request.
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */ + +#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + +/* Temporary define so the plugin can check whether it's compiled against + * Geany with plugin extension support or not */ +#define HAVE_GEANY_PLUGIN_EXTENSION 1 + +G_BEGIN_DECLS + + +typedef struct {
No docs because we didn't agree on the final API yet ;-)
@techee commented on this pull request.
+ .doc_symbols_provided = func_return_false, + .doc_symbols_get = func_return_ptrarr, + + .symbol_highlight_provided = func_return_false +}; + +static PluginExtension *current_extension = &dummy_extension; + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension) +{ + /* possibly, in the future if there's a need for multiple extensions, + * have a list of extensions and add/remove to/from the list */ + current_extension = extension;
Or really just add the list here. What I think in principle should happen is something like this (demonstrated on goto definition/declaration): ```C gboolean plugin_extension_goto_provided(GeanyDocument *doc) { PluginExtension *extension; gint i;
foreach_ptr_array(extension, i, all_extensions) { if (extension->goto_provided && extension->goto_provided(doc)) return TRUE; } return FALSE; }
void plugin_extension_goto_perform(GeanyDocument *doc, gint pos, gboolean definition) { PluginExtension *extension; gint i;
foreach_ptr_array(extension, i, all_extensions) { if (extension->goto_provided && extension->goto_provided(doc)) { extension->goto_perform(doc, pos, definition); return; } } } ```
Basically the first item in the list providing the functionality for the document filetype would "win". The `perform()` of other plugins won't be called. This might require some cooperation among plugins so they allow disabling certain functionality and allow the other plugins do what they want. This is possible with the LSP plugin already where one can set `goto_enable=false` so `plugin_extension_goto_provided()` would return `FALSE` which would allow the other plugin in the list provide the goto feature.
@techee commented on this pull request.
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
1. I think it's only used in `on_editor_button_press_event()` to return TRUE/FALSE whether the event was performed or whether it should be propagated further. I don't see an easy solution with the LSP plugin because the call to the server is asynchronous and we don't know the result at the moment when `plugin_extension_goto_perform()` finishes. But I'd say returning TRUE that the event was processed is fine here. 2. I wanted to raise this separately but for word detection I currently use a modified version of https://github.com/geany/geany/blob/96e6fb7d3d924354a1bc9226f389124d29506010... which uses hard-coded `GEANY_WORDCHARS` and not those defined in filetype configuration. This is bad both for Geany itself and the plugin because CSS for instance uses `-` inside identifiers or LaTeX uses `:` inside labels and this then breaks the identifiers. I think there should be an API version of `read_current_word()` that takes configured identifier characters into account which is used both by Geany and plugins. As for the parameter `name` itself, it could be passed to the API but it's kind of unnecessary as the plugin can get it by itself.
@techee commented on this pull request.
- if (plugin_extension_calltips_provided(doc))
+ return FALSE;
For this patch yes.
I think the placement was some left-over of my attempt to rewrite the reshowing code (because the current one doesn't work very well), but it should be done separately.
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
OK, I may not understand this completely, but the idea is that the decision on whether the extension performs the particular feature is directed by the extension itself (so it decides whether it's "active" or not). This is basically what I described in https://github.com/geany/geany/pull/3849#discussion_r1625127493: the return value of the `plugin_extension_goto_provided(doc)` function decides whether the extension wants to perform the feature and the `plugin_extension_goto_perform(doc)` function performs it. The LSP plugin for instance takes a look at the filetype of `doc` and when there is a server configured for it and in addition, this particular server supports goto definition/declaration (this is obtained from the initial handshake with the server), `plugin_extension_goto_provided(doc)` returns TRUE, otherwise it returns FALSE. In the TRUE case, `plugin_extension_goto_perform(doc)` can be called.
@b4n commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
(just a few quick replies, I'll go deeper later)
My point is that currently there is no way for a extension to know whether it "won" or not, so it can't know whether it should do ancillary actions (e.g. things in editor-notify, see e.g. my diff) related to the feature or not (assuming not everything does in `_perform()`). What I am trying to suggest is that there should be a means for the extension to know that, instead of having a setting the user need to know about and tweak (also knowing which plugin effectively "won"). *Maybe* it's fine and the no two extension can return `TRUE` for the same `provided()` call, but it seems a bit brittle and hard to get to work long-term.
A "simple" (but not very elegant I'd say) solution could be *always* calling `_provided()` at least once per document, and the extensions *must* track whether they got called for a document (and so it didn't got "caught" by another extension) before doing anything else.
Basically my point is that if we wanted to have the Geany features use the extension mechanism, how would it know whether it's active or not? It's a fallback for *all* calls (basically returning `TRUE` for all the `_provided()` calls), and so the question is "was it overridden"?
This sort of support could be useful for other things if we expand it further, e.g. a fairly generic indenter based on a fairly generic algorithm, and a specialized one (e.g. clang-format) that both handle some of the same documents, but with different levels of "quality". Currently I don't see a solution for those 2 to work together, unless each has an option to select what it supports or not, and it's properly configured (which could be a nightmare).
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
OK, what about modifying every `_provided(GeanyDocument *doc)` to ``` _provided(GeanyDocument *doc, PluginExtension *ext) ``` which: * if `ext != NULL`, returns TRUE only if the passed `ext` is the one that won, FALSE otherwise, * if `ext == NULL` it behaves like now?
(And, in addition, maybe `_active()` is a better naming than `_provided()`)
@techee commented on this pull request.
+ .doc_symbols_provided = func_return_false, + .doc_symbols_get = func_return_ptrarr, + + .symbol_highlight_provided = func_return_false +}; + +static PluginExtension *current_extension = &dummy_extension; + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension) +{ + /* possibly, in the future if there's a need for multiple extensions, + * have a list of extensions and add/remove to/from the list */ + current_extension = extension;
Just a note that if we want to use this interface for Geany itself, Geany's PluginExtension could be the last one in the chain and serve as a fallback.
Just a note that if we want to use this interface for Geany itself, Geany's PluginExtension could be the last one in the chain and serve as a fallback.
That sounds like [these](https://docs.gtk.org/gobject/signals.html).
Of course the real question is "who decides the order in the chain"?
That sounds like [these](https://docs.gtk.org/gobject/signals.html).
Kind of, but here it has more the semantics of normal calls and not event handlers.
Of course the real question is "who decides the order in the chain"?
Nobody but I don't think it will be such a problem. If someone for instance creates a new plugin using this API for Python autocompletion and someone enables both this plugin and the LSP plugin (and will have python server installed and enabled), the person will probably expect there are two plugins doing the same and will for instance disable autocompletion in LSP so the other plugin gets used. I think plugins should cooperate in this and allow their features to be disabled (except maybe single-purpose plugins like this Python autocompletion plugin where the plugin can be disabled as a whole).
@techee commented on this pull request.
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
Ad (1): `plugin_extension_goto_perform()` could be modified to return gboolean so Geany could use it if it uses the PluginExtensions interface. The LSP plugin though will always have to return TRUE here.
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
Ah also: do we want to add user data to those functions? That could be handy to pass in the general plugin data, especially as the "new" (which isn't so new anymore) plugin API allows this.
Where would this `user_data` get passed? When you call `_perform(..., user_data)`, the plugin won't know what to do with the data as it comes from Geany and it doesn't have any callback where it would pass it. The `_provided()` functions are synchronous and also don't know what to do with such data.
@techee commented on this pull request.
+typedef struct {
+ gboolean (*autocomplete_provided)(GeanyDocument *doc); + void (*autocomplete_perform)(GeanyDocument *doc, gboolean force); + + gboolean (*calltips_provided)(GeanyDocument *doc); + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024];
Maybe it'd be safer to have e.g. void (*_dummy[42])(void);
Yeah, it's better. Basically the idea was to pass "infinity" here from the practical point of view as I don't expect this will grow very fast.
On the other had if it grows slowly, say 5 calls per release and we don't update the padding when adding new functions and keep the struct growing, one would have to still use the 7-8-release-old plugin binary (with that 42 padding) to run into problems.
This plus the inline comments should cover most of what I found, and start the discussion on this :) But basically I think this PR could probably land in a "timely" manner.
How should I proceed? I'd suggest modifying the current PR to possibly use the list of PluginExtensions, modifying the `_available()` functions plus the other minor suggestions you had. But I'd leave the Geany refactoring (which I think is a good idea) to a separate PR that comes afterwards. Even if it means we run into something that requires the API modification - in any case, in it's documentation I'd write that the API isn't stable in case we run into something in the next Geany releases.
Also, I'd like to finally get rid of the `geany-lsp` combined repo and have one official version that has everything necessary in Geany. I'd then ask for some early feedback in https://github.com/geany/geany/issues/2184 before the actual release.
I know Thomas won't like the absence of the symbols tree here, but IIUC pros/cons are not so clear with the reality of things. And we can always add this later if wanted.
I don't want to make the impression that I want to monopolize the LSP stuff for this plugin only. Of course there could be something like ```C void lsp_get_document_symbols(GeanyDocument *doc, GCallback callback, gpointer user_data); void lsp_get_workspace_symbols(GeanyDocument *doc, const gchar *filter, GCallback callback, gpointer user_data); ``` where the callback returns an array of the symbols but I strongly suggest not to use `TMTag` for the symbols as their meaning is different in LSP and there's no clear 1:1 match. I think such an API should mirror the LSP interface.
Such an attempt is in the final patches of https://github.com/geany/geany/pull/3850 and the result is one has to check all the time whether the TMTag comes from LSP or TM. If someone has some better ideas, I'm totally open to them.
Also, it would be good to know what such a plugin that wants to use this interface would like to do - the only thing I can imagine is some other variant of the "Goto panel" I added both in the LSP plugin and ProjectOrganizer or some other form of the symbol tree - for anything else one just cannot be sure what the `name` or `detail` of the symbol contains (one can just blindly display those).
@b4n commented on this pull request.
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
1. I think it's probably fine to always return TRUE and pretend it's handled. I'd have to check, but I doubt it'd change anything. 2. I'm not really thinking it'd be better to pass the word, as said it probably makes sense for the extension to be trusted to know what a word is for a language it handles (although you're basically saying Geany is doing better than what your LSP plugin is currently doing, but that could change). My concern is that we are checking whether there's a word *outside* the call, meaning the extension still depends on what Geany things a word is. If we're happy with ignoring point 1 above, we could have this API return TRUE if the extension handled the request (which could just be "there's something that looks like a legit target, I'll try to go there", not necessarily meaning it did perform the action synchronously).
For point 2, it could lead to something like this: ```c if (! plugin_extension_goto_perform(…)) keybindings_send_command(GEANY_KEY_GROUP_GOTO, GEANY_KEYS_GOTO_MATCHINGBRACE); ``` or ```c if (! plugin_extension_goto_perform(…)) utils_beep(); ```
@b4n commented on this pull request.
- if (plugin_extension_calltips_provided(doc))
+ return FALSE;
Looking forward for the additional fixes in another PR then :)
@b4n commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
OK, what about modifying every `_provided(GeanyDocument *doc)` to […]
Yeah that's a good idea, and I came to it as well partly separately, so it might be a good, or at least obvious, answer :)
Where would this `user_data` get passed? When you call `_perform(..., user_data)`, the plugin won't know what to do with the data as it comes from Geany and it doesn't have any callback where it would pass it. The `_provided()` functions are synchronous and also don't know what to do with such data.
I guess we didn't understand each other here. My point is that at `_register()` time the extension gives some data, and when calling the vfuncs it gets passed in, allowing to remove all global state in the extension's implementation. E.g. for a C++ wrapper it could do (pardon my rusty C++):
```c++ void foobar_provided(GeanyDocument *doc, gpointer data) { static_cast<Foo*>(data)->foobar_provided_impl(doc); }
Foo::Foo() { plugin_extension_register(vtable, this); } ```
@b4n commented on this pull request.
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
Ad (1): `plugin_extension_goto_perform()` could be modified to return gboolean so Geany could use it if it uses the PluginExtensions interface. The LSP plugin though will always have to return TRUE here.
Well, the LSP plugin would look for the word and return FALSE if it didn't find any.
As a diff is sometimes clearer than words, here's an additional one addressing some discussions above (it's on top of my previous patch, but I don't mean to suggest they should be applied as-is). It includes `_provided()` that can tell the caller whether it was the winner, `user_data` for the vfuncs, and a fairly thorough multi-extension support. <details><summary>Another not-actually-so-big diff</summary>
```diff diff --git a/src/document.c b/src/document.c index adf5b8cc6..9bcbc7996 100644 --- a/src/document.c +++ b/src/document.c @@ -2720,7 +2720,7 @@ void document_highlight_tags(GeanyDocument *doc) GString *keywords_str; gint keyword_idx;
- if (! plugin_extension_active(plugin_extension_geany)) + if (! plugin_extension_symbol_highlight_provided(doc, plugin_extension_geany)) return;
/* some filetypes support type keywords (such as struct names), but not diff --git a/src/editor.c b/src/editor.c index 7a1d2fdc1..0d0d5ff23 100644 --- a/src/editor.c +++ b/src/editor.c @@ -838,11 +838,8 @@ static void on_char_added(GeanyEditor *editor, SCNotification *nt) } }
- if (plugin_extension_calltips_provided(editor->document)) - plugin_extension_calltips_show(editor->document, FALSE); - - if (plugin_extension_autocomplete_provided(editor->document)) - plugin_extension_autocomplete_perform(editor->document, FALSE); + plugin_extension_calltips_show(editor->document, FALSE); + plugin_extension_autocomplete_perform(editor->document, FALSE);
check_line_breaking(editor, pos); } @@ -1137,7 +1134,7 @@ static gboolean on_editor_notify(G_GNUC_UNUSED GObject *object, GeanyEditor *edi /* now that autocomplete is finishing or was cancelled, reshow calltips * if they were showing */ autocomplete_scope_shown = FALSE; - if (plugin_extension_active(plugin_extension_geany)) + if (plugin_extension_calltips_provided(doc, plugin_extension_geany)) request_reshowing_calltip(nt); break; case SCN_NEEDSHOWN: @@ -1152,7 +1149,7 @@ static gboolean on_editor_notify(G_GNUC_UNUSED GObject *object, GeanyEditor *edi break;
case SCN_CALLTIPCLICK: - if (plugin_extension_active(plugin_extension_geany) && nt->position > 0) + if (plugin_extension_calltips_provided(doc, plugin_extension_geany) && nt->position > 0) { switch (nt->position) { @@ -5295,7 +5292,7 @@ void editor_indent(GeanyEditor *editor, gboolean increase) }
-void editor_extension_autocomplete_perform(GeanyDocument *doc, gboolean force) +void editor_extension_autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data G_GNUC_UNUSED) { gint pos = sci_get_current_position(doc->editor->sci);
@@ -5335,7 +5332,7 @@ void editor_extension_autocomplete_perform(GeanyDocument *doc, gboolean force) }
-void editor_extension_calltips_show(GeanyDocument *doc, gboolean force) +void editor_extension_calltips_show(GeanyDocument *doc, gboolean force, gpointer data G_GNUC_UNUSED) { gint pos = sci_get_current_position(doc->editor->sci);
diff --git a/src/editor.h b/src/editor.h index 95c36a3e3..cee71df5e 100644 --- a/src/editor.h +++ b/src/editor.h @@ -279,8 +279,8 @@ void editor_snippets_free(void);
const GeanyEditorPrefs *editor_get_prefs(GeanyEditor *editor);
-void editor_extension_autocomplete_perform(struct GeanyDocument *doc, gboolean force); -void editor_extension_calltips_show(struct GeanyDocument *doc, gboolean force); +void editor_extension_autocomplete_perform(struct GeanyDocument *doc, gboolean force, gpointer data); +void editor_extension_calltips_show(struct GeanyDocument *doc, gboolean force, gpointer data);
/* General editing functions */ diff --git a/src/keybindings.c b/src/keybindings.c index 9eba0ef30..b5e08172f 100644 --- a/src/keybindings.c +++ b/src/keybindings.c @@ -2152,12 +2152,10 @@ static gboolean cb_func_editor_action(guint key_id) sci_send_command(doc->editor->sci, SCI_LINETRANSPOSE); break; case GEANY_KEYS_EDITOR_AUTOCOMPLETE: - if (plugin_extension_autocomplete_provided(doc)) - plugin_extension_autocomplete_perform(doc, TRUE); + plugin_extension_autocomplete_perform(doc, TRUE); break; case GEANY_KEYS_EDITOR_CALLTIP: - if (plugin_extension_calltips_provided(doc)) - plugin_extension_calltips_show(doc, TRUE); + plugin_extension_calltips_show(doc, TRUE); break; case GEANY_KEYS_EDITOR_CONTEXTACTION: if (check_current_word(doc, FALSE)) diff --git a/src/libmain.c b/src/libmain.c index d849bfa45..a54831d5a 100644 --- a/src/libmain.c +++ b/src/libmain.c @@ -46,6 +46,7 @@ #include "navqueue.h" #include "notebook.h" #include "plugins.h" +#include "pluginextension.h" #include "projectprivate.h" #include "prefs.h" #include "printing.h" @@ -1033,6 +1034,8 @@ void main_init_headless(void) memset(&template_prefs, 0, sizeof(GeanyTemplatePrefs)); memset(&ui_prefs, 0, sizeof(UIPrefs)); memset(&ui_widgets, 0, sizeof(UIWidgets)); + + plugin_extension_register(plugin_extension_geany, 0, NULL); }
diff --git a/src/pluginextension.c b/src/pluginextension.c index 0460a7c5a..5d0c8d546 100644 --- a/src/pluginextension.c +++ b/src/pluginextension.c @@ -24,12 +24,12 @@ #include "symbols.h"
-static gboolean func_return_true(GeanyDocument *doc) +static gboolean func_return_true(GeanyDocument *doc, gpointer data) { return TRUE; }
-static GPtrArray *func_return_ptrarr(GeanyDocument *doc) +static GPtrArray *func_return_ptrarr(GeanyDocument *doc, gpointer data) { return NULL; } @@ -50,86 +50,161 @@ static PluginExtension plugin_extension_geany_intenral = { .symbol_highlight_provided = func_return_true };
-static PluginExtension *current_extension = &plugin_extension_geany_intenral; +typedef struct +{ + PluginExtension *extension; + gpointer data; + gint priority; +} PluginExtensionEntry; + +static GList *all_extensions = NULL; + PluginExtension *plugin_extension_geany = &plugin_extension_geany_intenral;
-GEANY_API_SYMBOL -void plugin_extension_register(PluginExtension *extension) +/* sort higher priorities first */ +static gint sort_extension_entries(gconstpointer a, gconstpointer b) { - /* possibly, in the future if there's a need for multiple extensions, - * have a list of extensions and add/remove to/from the list */ - current_extension = extension; + const PluginExtensionEntry *entry_a = a; + const PluginExtensionEntry *entry_b = b; + + return entry_b->priority - entry_a->priority; }
GEANY_API_SYMBOL -void plugin_extension_unregister(PluginExtension *extension) +void plugin_extension_register(PluginExtension *extension, gint priority, gpointer data) { - current_extension = &plugin_extension_geany_intenral; + PluginExtensionEntry *entry = g_malloc(sizeof *entry); + + entry->extension = extension; + entry->data = data; + entry->priority = priority; + + all_extensions = g_list_insert_sorted(all_extensions, entry, sort_extension_entries); }
GEANY_API_SYMBOL -gboolean plugin_extension_active(PluginExtension *extension) +void plugin_extension_unregister(PluginExtension *extension) { - return current_extension == extension; + for (GList *node = all_extensions; node; node = node->next) + { + PluginExtensionEntry *entry = node->data; + + if (entry->extension == extension) + { + g_free(entry); + all_extensions = g_list_delete_link(all_extensions, node); + break; + } + } }
-/* allow plugins not to implement all the functions and fall back to the dummy - * implementation */ -#define CALL_IF_EXISTS(f) (current_extension->f ? current_extension->f : plugin_extension_geany_intenral.f) +/* + * @brief Checks whether a feature is provided + * @param f The virtual function name + * @param doc The document to check the feature on + * @param ext A @c PluginExtension, or @c NULL + * @returns @c TRUE if the feature is provided, @c FALSE otherwise. If @p ext + * is @c NULL, it check whether any extension provides the feature; + * if it is an extension, it check whether it's this extension that + * provides the feature (taking into account possible overrides). + */ +#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + } \ + return FALSE; \ + } G_STMT_END + +/* + * @brief Calls the extension implementation for f_provided/f_perform + * @param f_provided The name of the virtual function checking if the feature is provided + * @param doc The document to check the feature on + * @param f_perform The name of the virtual function implementing the feature + * @param args Arguments for @p f_perform. This should include @c entry->data as the last argument + * @param defret Return value if the feature is not implemented + * @returns The return value of @p f_perform or @p defret + */ +#define CALL_PERFORM(f_provided, doc, f_perform, args, defret) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f_provided && \ + entry->extension->f_provided(doc, entry->data)) \ + { \ + if (entry->extension->f_perform) \ + return entry->extension->f_perform args; \ + break; \ + } \ + } \ + return defret; \ + } G_STMT_END +
-gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc) +GEANY_API_SYMBOL +gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc, PluginExtension *ext) { - return CALL_IF_EXISTS(autocomplete_provided)(doc); + CALL_PROVIDED(autocomplete_provided, doc, ext); }
void plugin_extension_autocomplete_perform(GeanyDocument *doc, gboolean force) { - CALL_IF_EXISTS(autocomplete_perform)(doc, force); + CALL_PERFORM(autocomplete_provided, doc, autocomplete_perform, (doc, force, entry->data), /* void */); }
-gboolean plugin_extension_calltips_provided(GeanyDocument *doc) +GEANY_API_SYMBOL +gboolean plugin_extension_calltips_provided(GeanyDocument *doc, PluginExtension *ext) { - return CALL_IF_EXISTS(calltips_provided)(doc); + CALL_PROVIDED(calltips_provided, doc, ext); }
void plugin_extension_calltips_show(GeanyDocument *doc, gboolean force) { - CALL_IF_EXISTS(calltips_show)(doc, force); + CALL_PERFORM(calltips_provided, doc, calltips_show, (doc, force, entry->data), /* void */); }
-gboolean plugin_extension_goto_provided(GeanyDocument *doc) +GEANY_API_SYMBOL +gboolean plugin_extension_goto_provided(GeanyDocument *doc, PluginExtension *ext) { - return CALL_IF_EXISTS(goto_provided)(doc); + CALL_PROVIDED(goto_provided, doc, ext); }
void plugin_extension_goto_perform(GeanyDocument *doc, gint pos, gboolean definition) { - CALL_IF_EXISTS(goto_perform)(doc, pos, definition); + CALL_PERFORM(goto_provided, doc, goto_perform, (doc, pos, definition, entry->data), /* void */); }
-gboolean plugin_extension_doc_symbols_provided(GeanyDocument *doc) +GEANY_API_SYMBOL +gboolean plugin_extension_doc_symbols_provided(GeanyDocument *doc, PluginExtension *ext) { - return CALL_IF_EXISTS(doc_symbols_provided)(doc); + CALL_PROVIDED(doc_symbols_provided, doc, ext); }
GPtrArray *plugin_extension_doc_symbols_get(GeanyDocument *doc) { - return CALL_IF_EXISTS(doc_symbols_get)(doc); + CALL_PERFORM(doc_symbols_provided, doc, doc_symbols_get, (doc, entry->data), NULL); }
-gboolean plugin_extension_symbol_highlight_provided(GeanyDocument *doc) +GEANY_API_SYMBOL +gboolean plugin_extension_symbol_highlight_provided(GeanyDocument *doc, PluginExtension *ext) { - return CALL_IF_EXISTS(symbol_highlight_provided)(doc); + CALL_PROVIDED(symbol_highlight_provided, doc, ext); } diff --git a/src/pluginextension.h b/src/pluginextension.h index 48866a4a6..3f018efd8 100644 --- a/src/pluginextension.h +++ b/src/pluginextension.h @@ -31,47 +31,42 @@ G_BEGIN_DECLS
typedef struct { - gboolean (*autocomplete_provided)(GeanyDocument *doc); - void (*autocomplete_perform)(GeanyDocument *doc, gboolean force); + gboolean (*autocomplete_provided)(GeanyDocument *doc, gpointer data); + void (*autocomplete_perform)(GeanyDocument *doc, gboolean force, gpointer data);
- gboolean (*calltips_provided)(GeanyDocument *doc); - void (*calltips_show)(GeanyDocument *doc, gboolean force); + gboolean (*calltips_provided)(GeanyDocument *doc, gpointer data); + void (*calltips_show)(GeanyDocument *doc, gboolean force, gpointer data);
- gboolean (*goto_provided)(GeanyDocument *doc); - void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + gboolean (*goto_provided)(GeanyDocument *doc, gpointer data); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);
- gboolean (*doc_symbols_provided)(GeanyDocument *doc); - GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + gboolean (*doc_symbols_provided)(GeanyDocument *doc, gpointer data); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc, gpointer data);
- gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);
gchar _dummy[1024]; } PluginExtension;
-void plugin_extension_register(PluginExtension *extension); +void plugin_extension_register(PluginExtension *extension, gint priority, gpointer data); void plugin_extension_unregister(PluginExtension *extension); -gboolean plugin_extension_active(PluginExtension *extension);
+gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc, PluginExtension *ext); +gboolean plugin_extension_calltips_provided(GeanyDocument *doc, PluginExtension *ext); +gboolean plugin_extension_goto_provided(GeanyDocument *doc, PluginExtension *ext); +gboolean plugin_extension_doc_symbols_provided(GeanyDocument *doc, PluginExtension *ext); +gboolean plugin_extension_symbol_highlight_provided(GeanyDocument *doc, PluginExtension *ext);
#ifdef GEANY_PRIVATE
extern PluginExtension *plugin_extension_geany;
-gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc); void plugin_extension_autocomplete_perform(GeanyDocument *doc, gboolean force); - -gboolean plugin_extension_calltips_provided(GeanyDocument *doc); void plugin_extension_calltips_show(GeanyDocument *doc, gboolean force); - -gboolean plugin_extension_goto_provided(GeanyDocument *doc); void plugin_extension_goto_perform(GeanyDocument *doc, gint pos, gboolean definition); - -gboolean plugin_extension_doc_symbols_provided(GeanyDocument *doc); GPtrArray *plugin_extension_doc_symbols_get(GeanyDocument *doc);
-gboolean plugin_extension_symbol_highlight_provided(GeanyDocument *doc); - #endif /* GEANY_PRIVATE */
G_END_DECLS diff --git a/src/symbols.c b/src/symbols.c index e1585ece1..366db783a 100644 --- a/src/symbols.c +++ b/src/symbols.c @@ -1708,7 +1708,7 @@ gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition) { GeanyDocument *doc = document_get_current();
- if (plugin_extension_goto_provided(doc)) + if (plugin_extension_goto_provided(doc, NULL)) { /* FIXME: this should return TRUE on success so click handling in * editor.c can let the even pass through if there was nothing to do here @@ -1722,7 +1722,7 @@ gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition) }
-void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition) +void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition, gpointer data G_GNUC_UNUSED) { const gchar *name;
diff --git a/src/symbols.h b/src/symbols.h index 00face7f8..070f4ed42 100644 --- a/src/symbols.h +++ b/src/symbols.h @@ -60,7 +60,7 @@ void symbols_show_load_tags_dialog(void);
gboolean symbols_goto_tag(const gchar *name, gint pos, gboolean definition);
-void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition); +void symbols_goto_perform(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);
gint symbols_get_current_function(GeanyDocument *doc, const gchar **tagname);
``` </details>
But I'd leave the Geany refactoring (which I think is a good idea) to a separate PR that comes afterwards.
Agreed.
Even if it means we run into something that requires the API modification - in any case, in it's documentation I'd write that the API isn't stable in case we run into something in the next Geany releases.
Again, me attempting to implement the Geany features using it was driven by the impression that it would give me more hands-on reviews, and show me whether the API was "good enough" to support this use case as well as what you need in the LSP plugin. I'm not saying we *must* get it perfect here, but it's a good opportunity to review it :)
I know Thomas won't like the absence of the symbols tree here, but IIUC pros/cons are not so clear with the reality of things. And we can always add this later if wanted.
I don't want to make the impression that I want to monopolize the LSP stuff for this plugin only. Of course there could be something like […]
Again, we can add something for this at a later point if somebody actually has a use case (s)he likes. I understand both POVs: in a perfect world it sounds (to me) like a LSP server should be able to give a "better" TOC-style symbols tree; but I get that in reality it's not what we get, and what we get is not better than TM -- if not undoubtedly worse.
But if at some point there's actually a useful alternative people want, we could add the extension point that works best (possibly adding more abstraction if needed). But I don't think it has to be *now*.
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
My point is that at _register() time...
Alright, it was the `_register()` function and I get the example you gave. But I don't understand what global state you mean in the PluginExtension case. When I convert all the `_provided()` and `_perform()` functions to go through the list of registered extensions, we still need to store this list "globally" in the file. And there won't be any other global variable, the "dummy" LSP will disappear.
@techee commented on this pull request.
plugin_extension_goto_perform(doc, pos, definition);
+ return TRUE;
OK, sounds good.
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
OK, I just managed to scroll to your patch and the data is meant to be used by Geany (or whatever wants to implement the plugin) and not the plugin or the API which is what I mistunderstood.
As a diff is sometimes clearer than words, here's an additional one addressing some discussions above (it's on top of my previous patch, but I don't mean to suggest they should be applied as-is). It includes _provided() that can tell the caller whether it was the winner, user_data for the vfuncs, and a fairly thorough multi-extension support.
Nice, you pretty much did it for me, thanks! I'll just apply it, call it mine code and everyone will be happy :-).
I think I'd just drop the `priority` thing. Geany can call the `_register()` function before any plugins so it will be guaranteed to be last and all other plugins would just be prepended in the list. I think it would be hard for plugin developers to decide what priority their plugin is and whether it should have higher priority than some other plugin.
@techee commented on this pull request.
- void (*calltips_show)(GeanyDocument *doc, gboolean force);
+ + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
OK, I just managed to scroll to your patch and the data is meant to be used by Geany (or whatever wants to implement the plugin) and not the plugin or the API which is what I mistunderstood.
Midnight again, I stop making sense. Anyway, I got it :-).
in a perfect world it sounds (to me) like a LSP server should be able to give a "better" TOC-style symbols tree; but I get that in reality it's not what we get, and what we get is not better than TM -- if not undoubtedly worse.
It's not that - in general at least the better LSPs have better symbol information. But what you get from the LSP API is "symbol as it should be displayed in the symbol tree" and not "symbol information you can further process". So for instance what you get for Go symbols is this:
https://github.com/geany/geany/pull/3571#issuecomment-1820900404
where symbol `name` is for instance `(*Mutex).Lock` - it's completely alright for the symbol tree (and actually nice) but this isn't the symbol name. The TM-style symbol name is just `Lock` and `Mutex` is the scope (with Go it's a little more complicated as Lock isn't syntactically nested under Mutex, it just operates on the Mutex pointer type, so I'm not sure what ctags would return here). But the point is that you cannot process the result from the server in any way as you don't know in what format it is - you can just display it as it is and that's all. Also results can vary depending on the version of the language server and how its developers decide to present the symbols.
I think I'd just drop the `priority` thing. Geany can call the `_register()` function before any plugins so it will be guaranteed to be last and all other plugins would just be prepended in the list. I think it would be hard for plugin developers to decide what priority their plugin is and whether it should have higher priority than some other plugin.
I think if authors are reasonable and we give a guideline it could work nicely. Say Geany is 0, a plugin providing somewhat generic support would be say 100 (I would put multi language LSP here), and a highly specialized one say 1000 or whatever. Basically the more specialized the higher the priority (my reasoning being that it's both more likely to be good at what it does, and less likely to be enabled by somebody that doesn't actually want to use it).
IMO although not necessarily perfect I think it still gives a better chance for extensions to cooperate, mostly because it gives *some* ability to get cooperation without explicit user selection of a per-plugin option. Without something like this it's a game of luck to whether an extension is active or not (I don't think we enforce a particular loading order, do we?)
OK, let's keep the `priority` then (for the single plugin that will use this API for a long time :-P)
I also noticed one tiny problem in ```C +#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + } \ + return FALSE; \ + } G_STMT_END ```
I would also add ```C if ((ext) && entry->extension == (ext)) return FALSE; ``` at the end of the loop because without it the loop will continue unnecessarily. This doesn't seem like a big problem but the call `entry->extension->f(doc, entry->data)` has the potential to start the LSP server process which would be started unnecessarily if some extension earlier in the chain were the target.
Again future-proofing our single-plugin use case ;-).
I'll try to prepare the patch tomorrow.
If there are gonna be "priorities" (in reality its "order") they should be user controllable, someone might like @b4n indentation for C but language specific ones for C++. Anyway thats something we can work on in the future.
@techee pushed 6 commits.
02a898d8672a74d9ad4970e54f7f87446e02b74b Add changes from Colomban f7a36f7f4b619f98f7456d3af935ff197d271727 Remove some unneeded stuff fb03a756def1aabb55a7a30bf150790cdc49bd82 Avoid possible unnecessary list traversal a13a9875fae40ed06f7b6febebb525f6002f536f Make goto_perform() return gboolean 6ff32869e4afe1c52bc76ce300ae14162056a2be Convert padding to an array of pointers to functions 503dc181c13f9b9c5a32fc8887b03230a0aade4c Move plugin_extension_calltips_provided() into request_reshowing_calltip()
@techee pushed 1 commit.
947ea7b377fa76a9118972ccf7ac74f09e9cfecb Add back _provided() functions to the plugin API
@b4n I applied both of your patches, dropped the other file modifications from your patches and added the extra NULL parameter to the `_provided()` functions. This is the "Add changes from Colomban" commit.
Then I removed some stuff unneeded for this PR from pluginextension.h/c and made various changes we discussed here (please let me know if I forgot about something).
I originally didn't get why ``` +GEANY_API_SYMBOL +gboolean plugin_extension_autocomplete_provided(GeanyDocument *doc, PluginExtension *ext) ``` functions were made part of the plugin API so I removed them but then I remembered it's because of the explicitly provided `ext` parameter so I re-added these to the API in the last commit.
Documentation is still missing but if these changes look good to you, I'll start working on it.
Still thinking a bit about the symbol tree - it would be nice not to have a separate tab that users have to switch to. What about: - allowing the plugin to access the GtkTreeView of the symbol tree so it can put whatever it wants there - have some "filter-modified" event so the plugin knows when the filter changed - plus the corresponding `_provided()` function to disable Geany's normal symbol tree update
In theory this should be all it's needed but something else may appear during implementation. How does it sound?
@techee I do not understand what the difference with having a separate LSP symbol tree sidebar is.
If the user has switched away from the symbols sidebar to any other sidebar that one remains selected, even over a shutdown/restart, until the user switches back to the symbols sidebar. So other than the first time LSP is started the user should not have to select the LSP symbols sidebar.
Switch to LSP symbols keybindings are/should be provided by the LSP plugin.
Have I missed something?
@techee I do not understand what the difference with having a separate LSP symbol tree sidebar is.
It's a problem when working on different filetypes, one for which LSP is enabled, one for which there's no LSP. It then means constant switching between the panels if you need the symbol tree.
Ok, good point
@b4n commented on this pull request.
- if (plugin_extension_calltips_provided(doc, NULL))
+ return; +
I don't really care, but I'd have put the test at the single call site rather than adding a parameter only for returning early
If there are gonna be "priorities" (in reality its "order") they should be user controllable, someone might like @b4n indentation for C but language specific ones for C++. Anyway thats something we can work on in the future.
Hum, if we want this to be user-controlled outside the plugins (e.g. not have each plugin allow selecting its "priority"), we'll need a way to identify the extensions, like a name. We can add this to `register()` if need be (more artificial parameters for hypothetical cases :smile:), but better add it now that change the API if we know it gotta change.
@techee commented on this pull request.
- if (plugin_extension_calltips_provided(doc, NULL))
+ return; +
Just to be sure, you mean inside `case SCN_AUTOCCANCELLED:` below, right?
Hum, if we want this to be user-controlled outside the plugins (e.g. not have each plugin allow selecting its "priority"), we'll need a way to identify the extensions, like a name. We can add this to register() if need be (more artificial parameters for hypothetical cases 😄), but better add it now that change the API if we know it gotta change.
But are we ever going to add a GUI (or config file) for the configuration?
Having human readable names for extensions is useful in many ways, even just having something to debug print, so @b4n is right, adding a name now is a good idea and we can then use it lots of ways.
@techee pushed 1 commit.
80fbed1671c5901059c34acce9e93245333933dd Add plugin name argument to plugin_extension_register()
Having human readable names for extensions is useful in many ways, even just having something to debug print, so @b4n is right, adding a name now is a good idea and we can then use it lots of ways.
OK, done.
Now thinking, instead of `name`, shouldn't rather `GeanyPlugin *` be passed as an argument of `register()`? The function can read from it whatever it wants.
@b4n commented on this pull request.
- if (plugin_extension_calltips_provided(doc, NULL))
+ return; +
Yes
@b4n commented on this pull request.
- if (plugin_extension_calltips_provided(editor->document, NULL))
+ plugin_extension_calltips_show(editor->document, FALSE); + + if (plugin_extension_autocomplete_provided(editor->document, NULL)) + plugin_extension_autocomplete_perform(editor->document, FALSE);
There's no need to manually call `_provided()`, at least not anymore, is there? ```suggestion plugin_extension_calltips_show(editor->document, FALSE); plugin_extension_autocomplete_perform(editor->document, FALSE); ```
@b4n commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
there's still the question of whether this should really be guarded behind Geany thinking there's a word. IMO it shouldn't, and callers should stop being guarded behind it, and the `perform()` call should return `FALSE` if there's no word to act on. This would alleviate the artificial dependency on Geany dictating there's a word -- but not actually providing it.
So the callers would look like ```c static void goto_tag(GeanyDocument *doc, gboolean definition) { if (! symbols_goto_tag(sci_get_current_position(doc->editor->sci), definition)) utils_beep(); } ``` and ```c if (! symbols_goto_tag(editor_info.click_pos, TRUE)) keybindings_send_command(GEANY_KEY_GROUP_GOTO, GEANY_KEYS_GOTO_MATCHINGBRACE); return TRUE; ```
And this here just removes the `name` argument and replaces it with ```c const gchar *name;
/* … */
editor_find_current_word(doc->editor, pos, editor_info.current_word, GEANY_MAX_WORD_LENGTH, NULL); name = *editor_info.current_word ? editor_info.current_word : NULL;
if (! name) return FALSE; ``` (and possibly return `TRUE` below when there was a match but it wasn't handled -- or keep it returning `FALSE`, the only thing that happens then is calling *goto mathcing brace*, which might be fine).
--- This might or might not be a detail, it depends on whether extensions depend on the fact there's a word, or if they are written carefully enough not to actually depend on it.
Now thinking, instead of `name`, shouldn't rather `GeanyPlugin *` be passed as an argument of `register()`? The function can read from it whatever it wants.
I thought of it, but I don't know… on one hand it indeed allows accessing a lot more stuff if need be, and we could even make sure the extension got unregistered on plugin unload (so a plugin forgetting to unregister wouldn't crash, although I wouldn't love authors to disregard deregistration), but it also means that it's not possible to discriminate several extensions registered by a single plugin, which might (or might not) be an issue for e.g. Peasy-based plugins (not that I can tell whether this API is Peasy-friendly to begin with).
Still thinking a bit about the symbol tree - it would be nice not to have a separate tab that users have to switch to. What about:
- allowing the plugin to access the GtkTreeView of the symbol tree so it can put whatever it wants there
Could the extension be called once (per document, and probably when changing filetype) to create the TreeView? This could allow the extension answer `GtkWidget *create_tag_view(GeanyDocument, …)`, returning a `GtkTreeView` with whatever renderers and model it wants.
- have some "filter-modified" event so the plugin knows when the filter changed
If we reimplement the feature using `GtkTreeModelFilter` instead of the custom code from d3ded11ad2c8caeb0dd4aef2fcff517c5672b2e2, and require the model in the view to be a filter, we could probably just call [`refilter()`](https://docs.gtk.org/gtk3/method.TreeModelFilter.refilter.html) and give access to the entry data somehow.
Or require the plugin to manually listen to `changed` itself like we do.
In any case, the plugin need to have access to the filter value, so either have an event like you said, or a mean to access it at will.
[…] In theory this should be all it's needed but something else may appear during implementation. How does it sound?
Good if it's not too hard to implement the details like activating a row, navigating them an whatnot, but I guess we don't have much custom stuff apart knowing where to go, which is exactly what the extension *would* know how to do.
@techee commented on this pull request.
- if (plugin_extension_calltips_provided(editor->document, NULL))
+ plugin_extension_calltips_show(editor->document, FALSE); + + if (plugin_extension_autocomplete_provided(editor->document, NULL)) + plugin_extension_autocomplete_perform(editor->document, FALSE);
There's no need to manually call _provided(), at least not anymore, is there?
No, there isn't, it's a left-over from the previous implementation.
I thought of it, but I don't know… on one hand it indeed allows accessing a lot more stuff if need be, and we could even make sure the extension got unregistered on plugin unload (so a plugin forgetting to unregister wouldn't crash, although I wouldn't love authors to disregard deregistration), but it also means that it's not possible to discriminate several extensions registered by a single plugin, which might (or might not) be an issue for e.g. Peasy-based plugins (not that I can tell whether this API is Peasy-friendly to begin with).
Oh sure, multiple extensions provided by a single plugin - we must have that! :-)
In theory this should be all it's needed but something else may appear during implementation. How does it sound?
Good if it's not too hard to implement the details like activating a row, navigating them an whatnot, but I guess we don't have much custom stuff apart knowing where to go, which is exactly what the extension would know how to do.
I'd really just copy/paste what's in Geany and just replace TMTag with some other struct so the code creating/updating the tree would mostly be the same apart from some details like: - no support of root groups - no copying icons from the parents in the symbol tree - no construction of displayed name or tooltip (these we get directly from the server)
I'd even artificially construct `scope` from the tree-like information that is obtained from the server because really scared to touch anything in the symbol tree code.
In any case, I think the good starting point would be to create a separate Symbol tab with all the functionality and then try experimenting with connecting the tree to the existing Geany code. Not stuff for the upcoming release (unless it's postponed before Christmas :-).
@techee commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
there's still the question of whether this should really be guarded behind Geany thinking there's a word. IMO it shouldn't, and callers should stop being guarded behind it, and the perform() call should return FALSE if there's no word to act on. This would alleviate the artificial dependency on Geany dictating there's a word -- but not actually providing it.
Completely agree - it should be the plugin's decision regardless of what Geany itself considers to be the necessary condition for the goto to be performed. I'll modify the code accordingly.
@techee pushed 1 commit.
5f18bbfd7f755918dcfa0cd15ee6f9cbe2a4bcb3 Remove unnecessary _provided() calls
@techee commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
I'd just point out that this eliminates `get_current_word_or_sel()` from the original `goto_tag()` so `editor_find_current_word()` is used both in the ctrl+click and keybinding case. Is the goto selection something worth preserving? I've never used it myself and didn't even know it existed.
@techee pushed 2 commits.
0f1bf7cedd3b13cb964539e78c196020b26ed0ad Move plugin_extension_calltips_provided() check inside on_editor_notify() d34a0a36e761cb2844ae50ffaceaee18fee0d348 Make plugin_extension_goto_provided() free to decide whether to perform goto
@b4n I believe I addressed all your comments - if I forgot about something, please let me know.
Can I start working on the documentation or is there still a risk the API will turn into something completely different? :-)
@b4n commented on this pull request.
if (plugin_extension_goto_provided(doc, NULL)) return plugin_extension_goto_perform(doc, pos, definition);
+ editor_find_current_word(doc->editor, pos, + editor_info.current_word, GEANY_MAX_WORD_LENGTH, NULL); + name = *editor_info.current_word ? editor_info.current_word : NULL; +
Any reason not to include the `if (! name) return FALSE;`? Apparently `goto_tag()` seems NULL-safe, but we'll still end up with updating the statusbar with a message including a NULL representation.
@b4n commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
Ah, right, it calls `get_current_word_or_sel()`, and is indeed different if there's a selection. I really don't see why we handle selection in this instance, but maybe there's a use case? I'll try and think about it, but it seems odd (unless it's useful for some specific languages where we might get wordchars wrong?).
@b4n commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
…or just use `get_current_word_or_sel()`? Not tested, but the click handler (that doesn't handle the selection) should actually be *clearing* the selection by setting the caret+anchor position, so the selection will always be empty in that code path, won't it?
@techee commented on this pull request.
if (plugin_extension_goto_provided(doc, NULL)) return plugin_extension_goto_perform(doc, pos, definition);
+ editor_find_current_word(doc->editor, pos, + editor_info.current_word, GEANY_MAX_WORD_LENGTH, NULL); + name = *editor_info.current_word ? editor_info.current_word : NULL; +
Yeah, midnight, my enemy :-).
@techee pushed 1 commit.
b7c6b84ee0d77af826994bce8c59073d1f363dd8 Add missing check
@techee yeah, the only thing is whether to pass `GeanyPlugin*` to the register function, the rest looks good to me.
@b4n commented on this pull request.
if (plugin_extension_goto_provided(doc, NULL)) return plugin_extension_goto_perform(doc, pos, definition);
+ editor_find_current_word(doc->editor, pos, + editor_info.current_word, GEANY_MAX_WORD_LENGTH, NULL); + name = *editor_info.current_word ? editor_info.current_word : NULL; +
Hehe :) I'll turn in myself, hopefully not letting you hanging too much, but I'll stop being of any use pretty soon :slightly_smiling_face:
@techee yeah, the only thing is whether to pass GeanyPlugin* to the register function, the rest looks good to me.
Since we'll be flooded by plugins wanting to implement this API, some of them using Peasy, some implementing several extensions just for fun, we'll need to distinguish all these extensions so let's leave the string.
@techee commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
Just tested and yes, I get identical numbers from ```C int start = sci_get_selection_start(doc->editor->sci); int end = sci_get_selection_end(doc->editor->sci); ``` placed inside `symbols_goto_tag()` and after control-clicking and having something selected.
@elextr commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
Just a note, nothing should depend on Geany thinking there is a "word" since its always wrong for languages where non-ASCII is allowed for identifiers.
@techee commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
or just use get_current_word_or_sel()?
But it's static inside `keybindings.c` and we'd need it in `symbols.c`. And with this epochal discovery, I'm going to bed.
Keep the string, its for _human_ consumption and use, and a pointer isn't.
@elextr commented on this pull request.
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */ + +#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + +/* Temporary define so the plugin can check whether it's compiled against + * Geany with plugin extension support or not */ +#define HAVE_GEANY_PLUGIN_EXTENSION 1 + +G_BEGIN_DECLS + + +typedef struct {
@elextr is reserving his scream for later :smiling_imp:
Keep the string, its for _human_ consumption and use, and a pointer isn't.
Is it? It might be if it's for showing messages or having a list of priorities in a config file, but as @techee said we could do more with a `GeanyPlugin` instance. I'm still not sure if it's needed, but the idea to e.g. be able to cleanup extensions on plugin unload is starting to grow on me -- not that we need to or will do that, but we *could* :grin:
But there's probably no need to worry so much about this though.
the idea to e.g. be able to cleanup extensions on plugin unload is starting to grow on me
I wouldn't do that, extensions should just behave correctly. And if the plugin is unloaded and the extension is still registered, there will probably be a huge crash that the extension developer notices (not sure what happens with resident plugins in this case which the LSP plugin is).
@techee commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
So what should I do here? The options I see are: 1. Make the function non-static in keybindings.c and export it (but it has nothing to do with keybindings so not good API-wise) 2. Move it to say editor.c and make keybindings.c and symbols.c use it from there (makes more sense API-wise) 3. Make another copy of it in symbols.c 4. Keep the code as it is and not to use the selection mode
@b4n pushed 1 commit.
09295cea1d13e849f8269da666a96db519e08d6c Honor selection again in symbols_goto_tag()
@b4n commented on this pull request.
{
+ GeanyDocument *doc = document_get_current(); + + if (plugin_extension_goto_provided(doc, NULL)) + return plugin_extension_goto_perform(doc, pos, definition);
I'd say 2 or 3 (definitely not 1). I just pushed a version of 3, given how small the logic is.
I wouldn't do that, extensions should just behave correctly.
OK. Indeed I don't want this to be *necessary*, but if it's there for safety people will start relying on it (and rightfully so, otherwise it's just dead code, right?)
And if the plugin is unloaded and the extension is still registered, there will probably be a huge crash that the extension developer notices
Most extension developers don't actually manually unload their plugin at runtime; and it's unlikely to actually cause an issue when shutting down. It's the kind of things that get noticed by random users that disable a plugin.
(not sure what happens with resident plugins in this case which the LSP plugin is).
That'll depend on the plugin, if it clears required data and doesn't guard against that, it'll probably crash. Resident modules just don't unload the code and such, but they are still getting asked to cleanup.
@b4n commented on this pull request.
- plugin_extension_calltips_show(editor->document, FALSE);
+ plugin_extension_autocomplete_perform(editor->document, FALSE);
Hum… do we actually want the calls here? I'm fine with them, don't get me wrong, but does this actually help the extensions, or do they have to listen to `editor-notify` anyway and this becomes kind of artificial?
@b4n commented on this pull request.
- const PluginExtensionEntry *entry_b = b;
+ + return entry_b->priority - entry_a->priority; +} + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension, const gchar *plugin_name, + gint priority, gpointer data) +{ + PluginExtensionEntry *entry = g_malloc(sizeof *entry); + + entry->extension = extension; + entry->data = data; + entry->priority = priority; + entry->plugin_name = g_strdup(plugin_name ? plugin_name : "unnamed plugin");
any reason to allow a `NULL` name? (and if so, not to leave it `NULL` internally, at least for now?)
@b4n pushed 1 commit.
052eb37425a3ec32535de35b0712f01bf9488317 fixup! Move plugin_extension_calltips_provided() check inside on_editor_notify()
Anyway, apart from those two fairly little things, I think I'm happy with how this looks :slightly_smiling_face:
@techee commented on this pull request.
- const PluginExtensionEntry *entry_b = b;
+ + return entry_b->priority - entry_a->priority; +} + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension, const gchar *plugin_name, + gint priority, gpointer data) +{ + PluginExtensionEntry *entry = g_malloc(sizeof *entry); + + entry->extension = extension; + entry->data = data; + entry->priority = priority; + entry->plugin_name = g_strdup(plugin_name ? plugin_name : "unnamed plugin");
Well, I thought it was for the user interface (we don't have). But maybe for now I could even remove it from `PluginExtensionEntry` completely as it is unused and we can decide later what we want to do with the parameter. What do you think?
@techee commented on this pull request.
- plugin_extension_calltips_show(editor->document, FALSE);
+ plugin_extension_autocomplete_perform(editor->document, FALSE);
Technically they could be obtained from `editor-notify` and plugins could call `plugin_extension_autocomplete_provided()` and `plugin_extension_calltips_provided()` to check it is them who executes them. But then you have the calls where the `force` parameter is `TRUE` and these are still needed. So you'd end up with something like ``` plugin_extension_calltips_show_force(GeanyDocument *doc); plugin_extension_autocomplete_perform_force(GeanyDocument *doc); ``` inside the API and the non-force variants would have to be handled in a separate way outside the API which is quite strange IMO. So I'd leave them as they are.
Anyway, apart from those two fairly little things, I think I'm happy with how this looks 🙂
I'm mostly done with the documentation, I just need to read the garbage I have written after myself with a fresh head tomorrow :-)
@b4n commented on this pull request.
- plugin_extension_calltips_show(editor->document, FALSE);
+ plugin_extension_autocomplete_perform(editor->document, FALSE);
OK, if you think extensions implementing those will basically never have to re do this separately because they have some other logic to when it should happen let's keep it that way. It's pretty reasonable to check this upon character insertion I guess, and easy enough for an extension to ignore the force=FALSE cases if it really doesn't want to do it that way I guess.
@b4n commented on this pull request.
- const PluginExtensionEntry *entry_b = b;
+ + return entry_b->priority - entry_a->priority; +} + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension, const gchar *plugin_name, + gint priority, gpointer data) +{ + PluginExtensionEntry *entry = g_malloc(sizeof *entry); + + entry->extension = extension; + entry->data = data; + entry->priority = priority; + entry->plugin_name = g_strdup(plugin_name ? plugin_name : "unnamed plugin");
Indeed, good idea. I'd just add a `g_return_if_fail(name != NULL)` so callers give something that can later be used (it's easier to make the API less strict later on if wanted than the other way around)
@techee commented on this pull request.
- plugin_extension_calltips_show(editor->document, FALSE);
+ plugin_extension_autocomplete_perform(editor->document, FALSE);
Well, I have just 2 real-world examples to base this on - the LSP plugin and Geany itself. For Geany, both calltips and and autocomplete originate in `on_char_added()` (except the `force` variants triggered from elsewhere). For calltips, LSP uses "trigger characters" which, when typed, trigger the calltip display (it's typically `(` and `,`). Autocompletion also happens as you type and what we call "scope autocompletion" is also based on trigger characters.
One could think of some kind of autocompletion also happen when e.g. deleting characters but I think it would be annoying and don't think anyone does that.
So I think it is reasonably safe to assume `on_char_added()` is sufficient.
@techee commented on this pull request.
- const PluginExtensionEntry *entry_b = b;
+ + return entry_b->priority - entry_a->priority; +} + + +GEANY_API_SYMBOL +void plugin_extension_register(PluginExtension *extension, const gchar *plugin_name, + gint priority, gpointer data) +{ + PluginExtensionEntry *entry = g_malloc(sizeof *entry); + + entry->extension = extension; + entry->data = data; + entry->priority = priority; + entry->plugin_name = g_strdup(plugin_name ? plugin_name : "unnamed plugin");
Will do that.
@techee pushed 2 commits.
37194e920cd671d660a483d6b813e5ab88dd7e00 Add PluginExtension documentation 4a40ff0e3a0bb90818c7a31fbe778bf4efcb2039 Remove currently unused plugin_name from PluginExtensionEntry
@b4n @elextr Alright, documentation pushed, please let me know if I missed something, something should be reworded or fixed.
I didn't write much about the "priority" parameter of `_register()` because I don't know yet if Geany itself will use this interface too or not or what priority it will have. If Geany starts using priorities and has priority 0, do we want to allow plugins to have negative priorities too and serve as a fallback if geany doesn't implement something? I can imagine something like Geany's `autocomplete_doc_word()` to be placed after anything else. Anyway, this is not very important at the moment.
Also, even though we don't have settings in Geany yet, I think plugins could provide the priority settings by themselves e.g. in their config file and if the value is changed, just re-register their extension. But again, we have just one plugin now.
@techee pushed 1 commit.
3785bc92bf24e2898ffc2b40abbc516e3a9ba04d Rename plugin_name parameter of plugin_extension_register() to ext_name
Had a first look at the documentation, will comment in detail soon where needed, but one point that has become apparent is that it would be nice to have some overview to introduce the concept of extensions, what they are (plugins) what is special that makes them "extensions" (replace Geany functionality, not add functionality, hmmm, the name "extension" really overlaps "add", but c'est la vie). Otherwise jumping into details is likely to raise questions about what? why? where? who? when? etc.
@elextr Agree, I was thinking about it myself too - the documentation looks the whole thing is much more complicated than it really is. I think I'll create some `PluginExtension` howto similarly to e.g. `Proxy Plugin HowTo`
https://www.geany.org/manual/reference/proxy.html
with some minimal plugin example.
@techee pushed 2 commits.
e5287871054759c45d3f19871daa5d4db2bec769 Fix parameter name 2b0b55df399eb3685c88e254a290f89fb0fb3efd Add simple PluginExtension demo
@techee pushed 2 commits.
64a2f5a6b5a72f238cfc303c6a2395ee25c0047b Improve demopluginext.c a little b111bc160a2176e158fe83c4d476b8a018d65b59 Use "modern" Geany API when talking about init() and cleanup() functions
@techee pushed 1 commit.
f99af6648bbfd4b2b16b20e6bcfca5e1b6486e00 Add PluginExtension HowTo to API documentation
I've added the howto and a simple example under `plugins/demopluginext.c`.
@elextr requested changes on this pull request.
@techee a (mostly) documentation review, lots of minor tweaks, but a significant point that I have commented on several times and might be best if I summarise here:
It is confusing the way function pointers that are members of struct PluginExtension are referred to as eg "`_perform()` function". They are not functions, they are function pointers with specific member names that end in `_perform` and `_provided`, the functions they point to are in the plugin and can have any name. You know what you mean and after some confusion I think I now know what you mean, so it would be best to fix it to use proper terminology so future readers don't go through the same confusion.
IIUC when it says "`_provided()` function" it should be "the function pointed to by the `XXXX_provided` member". Yes its longer but one global replace should get most of them.
@@ -1116,4 +1118,130 @@ static void proxy_cleanup(GeanyPlugin *plugin, gpointer pdata)
@endcode
+@page plugin_extension Plugin Extension HowTo + +@section plugin_extension_intro Introduction + +Since Geany 2.1 the PluginExtension API allows plugins to take over some of
Originally the Geany plugin API only allowed plugins to add to Geany functionality, plugins could not modify Geany built-in functionality, but since Geany 2.1 ...
@@ -1116,4 +1118,130 @@ static void proxy_cleanup(GeanyPlugin *plugin, gpointer pdata)
@endcode
+@page plugin_extension Plugin Extension HowTo + +@section plugin_extension_intro Introduction + +Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc.
instead of hand wavey "such as" would be better to have a proper list of all the functions available for replacement that could then be kept up to date.
@@ -1116,4 +1118,130 @@ static void proxy_cleanup(GeanyPlugin *plugin, gpointer pdata)
@endcode
+@page plugin_extension Plugin Extension HowTo + +@section plugin_extension_intro Introduction + +Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup +
Not sure if doxygen will allow an actual link, but otherwise a reference to the plugin docs "Extension plugins are just normal plugins and behave as described in [link to plugin docs]." That provides the docs about what an `init()` function of a plugin is and then leads into extensions also having to register.
+Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to
whats a plugin extension instance?
+the core Geany functionality, such as autocopletion, calltip display, symbol goto,
+typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to +the functions it wishes to implement. Typically, these functions come in pairs: + - @c _provided() functions are used to query the plugin whether it implements
... used by Geany to query ...
Hmmm, its not until I got to the example below that I realised that `_provided()` is prefixed with the function name, maybe something like "`XXXX_provided()` where `XXXX` is the name of the function being replaced"
(cultural aside, XXXX is a local brand of beer, some say because the locals can't spell beer :grinning: )
+@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to +the functions it wishes to implement. Typically, these functions come in pairs: + - @c _provided() functions are used to query the plugin whether it implements + the particular feature for the passed document + - @c _perform() passes control to the plugin to perform the feature
... perform() used by Geany to pass control ...
...perform the feature instead of performing the Geany built-in functionality.
+@c PluginExtension instance using @c plugin_extension_register(). This typically
+happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to +the functions it wishes to implement. Typically, these functions come in pairs: + - @c _provided() functions are used to query the plugin whether it implements + the particular feature for the passed document + - @c _perform() passes control to the plugin to perform the feature + +When the plugin returns @c TRUE from the @c _provided() function, it indicates +it wants to take control of the particular feature and disable Geany's +implementation. However, returing @c TRUE does not automatically guarantee that
returning
+@section plugin_extension_impl Implementing extensions
+ +Inside the @c PluginExtension instance, the plugin fills-in the pointers to +the functions it wishes to implement. Typically, these functions come in pairs: + - @c _provided() functions are used to query the plugin whether it implements + the particular feature for the passed document + - @c _perform() passes control to the plugin to perform the feature + +When the plugin returns @c TRUE from the @c _provided() function, it indicates +it wants to take control of the particular feature and disable Geany's +implementation. However, returing @c TRUE does not automatically guarantee that +the plugin's implementation is executed - if there are multiple plugins competing +for implementing a feature, the extension with the highest priority +passed into the @c plugin_extension_register() function gets executed. + +Plugins can perform a check whether it is them who gets executed for the
A plugin can perform a check if it gets executed ...
A plugin isn't a people (at least until the AI plugin) so its "it" not "them"
- @c _provided() functions are used to query the plugin whether it implements
+ the particular feature for the passed document + - @c _perform() passes control to the plugin to perform the feature + +When the plugin returns @c TRUE from the @c _provided() function, it indicates +it wants to take control of the particular feature and disable Geany's +implementation. However, returing @c TRUE does not automatically guarantee that +the plugin's implementation is executed - if there are multiple plugins competing +for implementing a feature, the extension with the highest priority +passed into the @c plugin_extension_register() function gets executed. + +Plugins can perform a check whether it is them who gets executed for the +particular feature; e.g. for autocompletion the plugin can use +@c plugin_extension_autocomplete_provided() which returns @c TRUE if the +passed extension is executed, taking into account all registered extension +priorities and their interest in providing the particular feature.
I'd end the sentence at "priorities". Not sure what their "interest" is and again a plugin isn't a people :grin:
+static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data)
+{ + /* Check whether the plugin provides the feature for the passed document */ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + /* The autocompletion logic comes here, including the autocompletion UI + * display (either using some custom widget or using Scintilla's + * SCI_AUTOCSHOW) */ +} + + +/* The PluginExtension instance - we only implement autocompletion here. */
...here so only fill in the pointers for that function.
So in fact it doesn't matter what the functions are called, its what entry in the struct its passed to that matters:
``` .autocomplete_provided = foo_bar, .autocomplete_perform = foo_bletch ```
Would be just as legal, thats not really clear pre-example, but maybe when you explain what an "instance" is (see comment above) that can be made clear.
the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +/** + * Demo plugin extension - example of a plugin using the PluginExtension API + * to provide simple autocompletion of Python keywords.
"This is the plugin example used in the documentation."
+ +/* sort higher priorities first */ +static gint sort_extension_entries(gconstpointer a, gconstpointer b) +{ + const PluginExtensionEntry *entry_a = a; + const PluginExtensionEntry *entry_b = b; + + return entry_b->priority - entry_a->priority; +} + + +/** + * Registers the provided extension in Geany. There can be multiple extensions + * registered in Geany - these are stored in a list sorted by the priporty + * parameter. When executing @c _perform() functions, Geany goes through
See comments above on the overview docs.
+/** + * Registers the provided extension in Geany. There can be multiple extensions + * registered in Geany - these are stored in a list sorted by the priporty + * parameter. When executing @c _perform() functions, Geany goes through + * the list and executes the @c _perform() function of the first extension in + * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin.
Would be better to describe what the actual parameter does, we know its provided by the plugin, its calling this function. :grin:
"The @c PluginExtension structure initialised with the function pointers for the functionality the plugin wishes to replace."
- the list and executes the @c _perform() function of the first extension in
+ * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin. + * @param ext_name Human-readable name of the extension that can appear in + * the user interface. The string should be reasonably unique so extensions can + * be distinguished from each other. + * @param priority The higher number, the earlier in the extension list the + * plugin is and the higher chance it becomes called when multiple extensions
don't think we need the bit about the list, unless I am missing something thats implementation and is irrelevant to the user, so "The higher the number the higher the chance ...."
And what if two have the same priority? Do we want to define it, or my preference is to state its implementation defined which wins and that may change.
- Plugins wishing to re-register themselves, e.g. with a different priority,
+ * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin. + * @param ext_name Human-readable name of the extension that can appear in + * the user interface. The string should be reasonably unique so extensions can + * be distinguished from each other. + * @param priority The higher number, the earlier in the extension list the + * plugin is and the higher chance it becomes called when multiple extensions + * compete for the same feature. + * @param data User data passed to the functions from the @c PluginExtension + * struct. + * @warning Plugins are responsible for calling @c plugin_extension_unregister() + * when they no longer provide the extension.
... and when the plugin is unloaded.
- g_return_if_fail(ext_name != NULL);
+ + entry = g_malloc(sizeof *entry); + entry->extension = extension; + entry->data = data; + entry->priority = priority; + + all_extensions = g_list_insert_sorted(all_extensions, entry, sort_extension_entries); +} + + +/** + * Plugins are responsible for calling this function when they no longer + * provide the extension, at the latest in the plugin @c cleanup() function. + * + * @param extension The @c PluginExtension structure that is being unregistered.
Question, does this need to be the actual same chunk of memory, or just one with the same contents, and what if its not?
return entry->extension->f_perform args; \
+ break; \ + } \ + } \ + return defret; \ + } G_STMT_END + + +/* + * Geany itself can also pass NULL as the extension parameter to determine + * whether there is any extension implementing autocompletion. Plugins should + * not use this and rely on Geany's extension list and the priorities so + * this feature is not documented in the plugin API. + */ +/** + * Plugins can call this function to check whether, based on the extension list
"extension list" S/B "extensions registered" as above its implementation if its a list
- */
+#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + \ + if ((ext) && entry->extension == (ext)) \ + return FALSE; \ + } \ + return FALSE; \ + } G_STMT_END +
I hate to reopen the implementation in what is mostly a documentation review, but will :smiling_imp:. Instead of one list of extensions could there not be a separate list per extension point of extensions offering that functionality sorted by priority as they are registered, so each _PROVIDED and _PERFORM just need to access the first on the list not search every time the extension is accessed?
- Geany itself can also pass NULL as the extension parameter to determine
+ * whether there is any extension implementing autocompletion. Plugins should + * not use this and rely on Geany's extension list and the priorities so + * this feature is not documented in the plugin API. + */ +/** + * Plugins can call this function to check whether, based on the extension list + * and the provided extension priorities, the extension passed in the @c ext + * parameter is used for autocompletion. + * + * Plugins will typically call this function with their own @c PluginExtension + * to check, if it is them who gets (or got) executed for autocompletion of the + * provided document. This is useful for various auxiliary functions such as + * cleanups after the @c _perform() function is completed so plugins know it was + * them who executed the @c _perform() function and do not have to store this + * information by some other means.
reminder all the "them"s S/B "it"
+ * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +/** + * @file pluginextension.h + * This file defines an interface allowing plugins to take over some of the + * core functionality provided by Geany, such as autocompletion, calltips, + * symbol goto, types highlighting inside document, etc.
again proper complete list, not just "such as" which implies its not a complete list
- This file defines an interface allowing plugins to take over some of the
+ * core functionality provided by Geany, such as autocompletion, calltips, + * symbol goto, types highlighting inside document, etc. + **/ + +#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing
... to inform ...
+#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always
statically on the heap?
what is the required lifetime of the object? when can it be freed?
In the G* terminology who "owns" it if the pointer is also cached in the callee, and is that handled for non-C plugins?
+#include "document.h"
+ + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always + * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface.
assign pointers to the functions implementing this interface to the appropriate members of the structure.
Actually its pairs isn't it? What if only one of the pair is assigned, UB?
+G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always + * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface. + * Not all of the functions have to be implemented by the plugin - unimplemented + * functions can be left to contain the NULL pointer.
member pointers for unimplemented ...
+/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always + * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface. + * Not all of the functions have to be implemented by the plugin - unimplemented + * functions can be left to contain the NULL pointer. + * + * Typically, functions from this interface come in pairs. Functins ending with
Ok, so question above about what if only one S/B answered here
As noted above its not the function names that end in `_provided()` or `_perform()` its the structure members that have those names.
typo "Functions"
- or dynamically. When allocated dynamically, plugins should always
+ * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface. + * Not all of the functions have to be implemented by the plugin - unimplemented + * functions can be left to contain the NULL pointer. + * + * Typically, functions from this interface come in pairs. Functins ending with + * @c _provided() inform Geany whether the plugin implements the given feature + * for the passed document. Functions ending with @c _perform() are called by + * Geany at appropriate moments to inform the plugin when to perform the given + * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function.
This is just a data structure passed to the register function to provide a bunch of pointers, it shouldn't be elevated to the status of "Instances of the ... structures are registered in Geany" as if its the extension, the plugin is the extension.
- functions can be left to contain the NULL pointer.
+ * + * Typically, functions from this interface come in pairs. Functins ending with + * @c _provided() inform Geany whether the plugin implements the given feature + * for the passed document. Functions ending with @c _perform() are called by + * Geany at appropriate moments to inform the plugin when to perform the given + * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function. + * + * @warning The API provided by this file is subject to change and should not be + * considered stable at this point. That said, it is highly probable that if + * a change of this API is required in the future, it will not be of a major + * nature and should not require major modifications of the affected plugins + * (e.g. added/modified parameter of a function and similar changes).
is it going to be stabilised in the future? Probably @b4n question.
- Geany at appropriate moments to inform the plugin when to perform the given
+ * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function. + * + * @warning The API provided by this file is subject to change and should not be + * considered stable at this point. That said, it is highly probable that if + * a change of this API is required in the future, it will not be of a major + * nature and should not require major modifications of the affected plugins + * (e.g. added/modified parameter of a function and similar changes). + **/ +typedef struct +{ + /** + * Called by Geany to check whether the plugin implements autocompletion
Ok, its understandable, but technically it should be "Pointer to function called by Geany ..."
Same for all below.
+static GList *all_extensions = NULL;
+ + +/* sort higher priorities first */ +static gint sort_extension_entries(gconstpointer a, gconstpointer b) +{ + const PluginExtensionEntry *entry_a = a; + const PluginExtensionEntry *entry_b = b; + + return entry_b->priority - entry_a->priority; +} + + +/** + * Registers the provided extension in Geany. There can be multiple extensions + * registered in Geany - these are stored in a list sorted by the priporty
priporty
- Instances of the @c PluginExtension structures are registered in Geany using
+ * the @c plugin_extension_register() function. + * + * @warning The API provided by this file is subject to change and should not be + * considered stable at this point. That said, it is highly probable that if + * a change of this API is required in the future, it will not be of a major + * nature and should not require major modifications of the affected plugins + * (e.g. added/modified parameter of a function and similar changes). + **/ +typedef struct +{ + /** + * Called by Geany to check whether the plugin implements autocompletion + * for the provided document. + * + * @param doc The document for which Geany checks whether the plugin
"checks" S/B is "is querying"
Maybe a note that this allows plugins to restrict their replacement of the function to documents of specific filetypes or other characteristics.
@@ -258,6 +259,23 @@ const gchar *symbols_get_context_separator(gint ft_id)
}
+/** Gets the icon data corresponding to the provided TMIcon. + * @param icon TMIcon. + * + * Returns the GdkPixbuf corresponding to the provided TMIcon. + * + * @since 2.1 + */ +GEANY_API_SYMBOL
Is this really part of the extensions patches? Is it another PR that needs merging first?
@b4n commented on this pull request.
- g_return_if_fail(ext_name != NULL);
+ + entry = g_malloc(sizeof *entry); + entry->extension = extension; + entry->data = data; + entry->priority = priority; + + all_extensions = g_list_insert_sorted(all_extensions, entry, sort_extension_entries); +} + + +/** + * Plugins are responsible for calling this function when they no longer + * provide the extension, at the latest in the plugin @c cleanup() function. + * + * @param extension The @c PluginExtension structure that is being unregistered.
Currently, it has to be the same pointer that was registered, and if it's not one of the registered pointers it's not removing anything.
```suggestion * @param extension The @c PluginExtension structure pointer to unregister, as previously registered with @c plugin_extension_register() ```
@elextr commented on this pull request.
- g_return_if_fail(ext_name != NULL);
+ + entry = g_malloc(sizeof *entry); + entry->extension = extension; + entry->data = data; + entry->priority = priority; + + all_extensions = g_list_insert_sorted(all_extensions, entry, sort_extension_entries); +} + + +/** + * Plugins are responsible for calling this function when they no longer + * provide the extension, at the latest in the plugin @c cleanup() function. + * + * @param extension The @c PluginExtension structure that is being unregistered.
Ok, so the lifetime of the `PluginExtension` struct has to be at least from register to unregister, and the plugin has to keep the pointer.
@b4n commented on this pull request.
- the list and executes the @c _perform() function of the first extension in
+ * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin. + * @param ext_name Human-readable name of the extension that can appear in + * the user interface. The string should be reasonably unique so extensions can + * be distinguished from each other. + * @param priority The higher number, the earlier in the extension list the + * plugin is and the higher chance it becomes called when multiple extensions
We probably don't want to be too specific here, at least not for now.
Also, I feel like the wording here is a little too much of an incentive for extension authors to just put a ridiculously high value -- because their extension is awesome, right? I'd like to have some rough guidelines on values here as mentioned earlier somewhere. I'd suggest something like this (actual numbers are pretty arbitrary though):
* if the extension is the only thing the plugin provides, and if it targets a single filetype, use `400 <= priority < 500`. * if the extension is the only thing the plugin provides, but it targets several filetypes, use ` 300 <= priority < 400`. * if the plugin provides other features than the extension, but it only targets a single filetype, use ` 200 <= priority < 300`. * if the plugin provides other features than the extension, and targets several filetypes, use `100 <= priority < 200`. * a priority of 0 or less is reserved, and using it has unspecified behavior.
My idea being that the more specific the plugin is, the less likely the user enabled it without actually wanting the extension feature, while if it's one of the many things the plugin does, disabling the plugin might not be a good option to let another plugin handle one of the extension calls.
This might even be a good reason for a plugin to register separate extensions, so if one of the calls is very specific but some other very generic it can use different priorities for each.
@b4n commented on this pull request.
- Plugins wishing to re-register themselves, e.g. with a different priority,
+ * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin. + * @param ext_name Human-readable name of the extension that can appear in + * the user interface. The string should be reasonably unique so extensions can + * be distinguished from each other. + * @param priority The higher number, the earlier in the extension list the + * plugin is and the higher chance it becomes called when multiple extensions + * compete for the same feature. + * @param data User data passed to the functions from the @c PluginExtension + * struct. + * @warning Plugins are responsible for calling @c plugin_extension_unregister() + * when they no longer provide the extension.
probably worth making explicit indeed
@elextr commented on this pull request.
- the list and executes the @c _perform() function of the first extension in
+ * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin. + * @param ext_name Human-readable name of the extension that can appear in + * the user interface. The string should be reasonably unique so extensions can + * be distinguished from each other. + * @param priority The higher number, the earlier in the extension list the + * plugin is and the higher chance it becomes called when multiple extensions
and @elextr plugins priority == INT_MAX [end humility] :stuck_out_tongue_winking_eye:
For now lets see how well that advice works (or not).
@b4n commented on this pull request.
+static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data)
+{ + /* Check whether the plugin provides the feature for the passed document */ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + /* The autocompletion logic comes here, including the autocompletion UI + * display (either using some custom widget or using Scintilla's + * SCI_AUTOCSHOW) */ +} + + +/* The PluginExtension instance - we only implement autocompletion here. */
Maybe there's a clarification needed, but I feel that it's pretty self-explanatory that if you pass a structure full of function pointers (and to non-exported functions even), it's the function pointer that matters, not its name. But well, if there's a way to make that explicit, sure.
@elextr commented on this pull request.
+static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data)
+{ + /* Check whether the plugin provides the feature for the passed document */ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + /* The autocompletion logic comes here, including the autocompletion UI + * display (either using some custom widget or using Scintilla's + * SCI_AUTOCSHOW) */ +} + + +/* The PluginExtension instance - we only implement autocompletion here. */
See the overall comment on the review.
@b4n commented on this pull request.
+/** + * Registers the provided extension in Geany. There can be multiple extensions + * registered in Geany - these are stored in a list sorted by the priporty + * parameter. When executing @c _perform() functions, Geany goes through + * the list and executes the @c _perform() function of the first extension in + * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin.
"A pointer to the @c `PluginExtension` structure to register. This pointer and the @c `PluginExtension` it points to have to remain valid until the extension is unregistered. All fields of the @c `PluginExtension` structure have to be fully initialized either to a @c `NULL` pointer or to a proper implementation. Usually, this structure is statically allocated which automatically zero-initializes uninitialized members as appropriate."
Or specify this some place else (maybe I missed it?)
@elextr commented on this pull request.
+/** + * Registers the provided extension in Geany. There can be multiple extensions + * registered in Geany - these are stored in a list sorted by the priporty + * parameter. When executing @c _perform() functions, Geany goes through + * the list and executes the @c _perform() function of the first extension in + * the list whose @c _provided() function returns @c TRUE. + * + * This function is typically called in the plugin @c init() function. + * + * Plugins wishing to re-register themselves, e.g. with a different priority, + * should first unregister themselves using @c plugin_extension_unregister() + * and call @c plugin_extension_register() afterwards. + * + * @param extension The @c PluginExtension structure instance provided by the + * plugin.
LGTM
Or specify this some place else (maybe I missed it?)
Not sure, here looks ok for now anyway.
@techee commented on this pull request.
@@ -1116,4 +1118,130 @@ static void proxy_cleanup(GeanyPlugin *plugin, gpointer pdata)
@endcode
+@page plugin_extension Plugin Extension HowTo + +@section plugin_extension_intro Introduction + +Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc.
The list is complete currently but I was really preparing for the future when someone forgets to update this - and in fact, I don't think the list has to be complete here.
@techee commented on this pull request.
+Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to
It's the variable of the `PluginExtension` type (that is, an object of the `PluginExtension` type) - but probably too OO style wording. Should probably be `PluginExtension variable`.
@techee commented on this pull request.
- */
+#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + \ + if ((ext) && entry->extension == (ext)) \ + return FALSE; \ + } \ + return FALSE; \ + } G_STMT_END +
That's not the whole story - in addition to the static priority, there's also a dynamic aspect of all this.
Consider for instance `autocomplete_provided()` and `autocomplete_perform()` and the LSP plugin and let's say that for the queried document there's a LSP server configured. 1. Geany calls `autocomplete_provided()` of the LSP plugin - since the server isn't started yet, the plugin returns `FALSE` but launches the LSP server in parallel. 2. Until the server is started and the handshake with it completed, other registered extensions may perform the work even if they are behind in the chain. 3. Some time passes and `autocomplete_provided()` is called again by Geany. This time, the server is started. But even though it's started, it doesn't automatically mean it supports autocompletion - so depending on the server capabilities, the plugin returns either `TRUE` and starts performing autocompletion, or `FALSE` and keep others in the chain performing it. 4. If the server is re-configured or e.g. crashes or doesn't run for some other reason, others may perform autocompletion again. 5. And of course, if you disable autocompletion in the LSP config, it will always return `FALSE` without any attempt to start the LSP server.
Anyway, every `_provided()` function should be fast and return quickly without much processing. And I really don't expect thousands of plugins wanting to implement this interface.
@techee commented on this pull request.
@@ -258,6 +259,23 @@ const gchar *symbols_get_context_separator(gint ft_id)
}
+/** Gets the icon data corresponding to the provided TMIcon. + * @param icon TMIcon. + * + * Returns the GdkPixbuf corresponding to the provided TMIcon. + * + * @since 2.1 + */ +GEANY_API_SYMBOL
It's part of the LSP patches but not really one of the extension patches. I could make it a separate PR if desired.
@elextr commented on this pull request.
@@ -1116,4 +1118,130 @@ static void proxy_cleanup(GeanyPlugin *plugin, gpointer pdata)
@endcode
+@page plugin_extension Plugin Extension HowTo + +@section plugin_extension_intro Introduction + +Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc.
If the list is complete then make it a proper complete list, its got more chance of being updated if it is, remember "such as" only means "for example" so there is no need for anyone to update it as it is now. It is important for a complete list of the extension points to be somewhere visible without reading through all the details, and here in the summary is a good place where readers will see it.
@elextr commented on this pull request.
+Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to
Geany is C, not C++ or Java :grin: so use the C term, "struct"
@elextr commented on this pull request.
- */
+#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + \ + if ((ext) && entry->extension == (ext)) \ + return FALSE; \ + } \ + return FALSE; \ + } G_STMT_END +
True, I omitted that Geany will still walk the per extension list until it finds an extension that accepts its request, but at least its only walking a list of the extensions that offer that feature, I would expect most extensions (other than LSP) will only offer one or two features.
But as you say, for now its not critical, but what would be a Geany discussion without a little premature optimisation :grin:?
@techee commented on this pull request.
+#include "document.h"
+ + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always + * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface.
Nothing terrible like crash will happen, but the particular feature just won't work - I think anyone who makes this mistake will realize it soon.
@elextr commented on this pull request.
@@ -258,6 +259,23 @@ const gchar *symbols_get_context_separator(gint ft_id)
}
+/** Gets the icon data corresponding to the provided TMIcon. + * @param icon TMIcon. + * + * Returns the GdkPixbuf corresponding to the provided TMIcon. + * + * @since 2.1 + */ +GEANY_API_SYMBOL
Probably best practice to keep unrelated stuff separate.
@techee commented on this pull request.
- or dynamically. When allocated dynamically, plugins should always
+ * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface. + * Not all of the functions have to be implemented by the plugin - unimplemented + * functions can be left to contain the NULL pointer. + * + * Typically, functions from this interface come in pairs. Functins ending with + * @c _provided() inform Geany whether the plugin implements the given feature + * for the passed document. Functions ending with @c _perform() are called by + * Geany at appropriate moments to inform the plugin when to perform the given + * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function.
But it kind of is the extension - when you fill in the function pointers, the functions define the extension itself. And there could be multiple extensions per plugin so `plugin == extension` isn't completely right either.
@elextr commented on this pull request.
+#include "document.h"
+ + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always + * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface.
ok, so long as its safe we can probably not mention anything about it, so just my first line change.
@elextr commented on this pull request.
- or dynamically. When allocated dynamically, plugins should always
+ * zero-fill the structure before use. + * + * Depending on the functionality they provide, plugins should assign the + * pointers from the structure to the functions implementing this interface. + * Not all of the functions have to be implemented by the plugin - unimplemented + * functions can be left to contain the NULL pointer. + * + * Typically, functions from this interface come in pairs. Functins ending with + * @c _provided() inform Geany whether the plugin implements the given feature + * for the passed document. Functions ending with @c _perform() are called by + * Geany at appropriate moments to inform the plugin when to perform the given + * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function.
Ok, maybe "The extension is defined by the pointers in the PluginExtension structure and is registered in Geany using ..."
It is confusing the way function pointers that are members of struct PluginExtension are referred to as eg "_perform() function". They are not functions, they are function pointers with specific member names that end in _perform and _provided, the functions they point to are in the plugin and can have any name. You know what you mean and after some confusion I think I now know what you mean, so it would be best to fix it to use proper terminology so future readers don't go through the same confusion.
That's technically right but it depends on what level of abstraction you use when describing this interface. If we used e.g. Java, `PluginExtension` would be an interface and the extensions themselves classes (or objects of these classes) that implement this interface. The fact that we are using C requires certain implementation details but I'm not sure if we have to describe all that. I think everyone wanting to implement this interface will understand it and I don't think that instead of "function" saying something like "pointer inside the structure which is assigned a function" everywhere will contribute to better understanding of the interface.
IIUC when it says "_provided() function" it should be "the function pointed to by the XXXX_provided member". Yes its longer but one global replace should get most of them.
I'll have a look at the places this is mentioned, if there's not too many of them, I'll just add "ending with" before them.
@techee commented on this pull request.
+Since Geany 2.1 the PluginExtension API allows plugins to take over some of +the core Geany functionality, such as autocopletion, calltip display, symbol goto, +typename highlighting inside document, etc. + +@section plugin_extension_init Initialization and cleanup + +First, any plugin interested in using this interface has to register its +@c PluginExtension instance using @c plugin_extension_register(). This typically +happens in the @c init() function of the plugin. Registered extensions have to be +unregistered before the plugin is unloaded using @c plugin_extension_unregister(), +typically inside the @c cleanup() function of the plugin. + +@section plugin_extension_impl Implementing extensions + +Inside the @c PluginExtension instance, the plugin fills-in the pointers to
Well, "PluginExtension struct" would be incorrect here, it should be something like "variable of the PluginExtension type".
I think everyone wanting to implement this interface will understand it
But you already do understand it, so you are a bad judge of that, there is a name for this [curse of knowledge](https://en.wikipedia.org/wiki/Curse_of_knowledge). Better that you believe the advice from someone who was initially confused (me) that it is not as obvious as you think it is.
I'll just add "ending with" before them.
Fine.
But you already do understand it, so you are a bad judge of that, there is a name for this [curse of knowledge](https://en.wikipedia.org/wiki/Curse_of_knowledge). Better that you believe the advice from someone who was initially confused (me) that it is not as obvious as you think it is.
It's not that I don't believe that you or anybody else didn't initially understand. I'm just worried it will add too much unnecessary clutter and that this clutter will make the documentation actually worse. Anyway, I'll try to do that (in a separate commit that's easy to revert if the result is too horrible ;-)
@cwendling commented on this pull request.
- */
+#define CALL_PROVIDED(f, doc, ext) \ + G_STMT_START { \ + for (GList *node = all_extensions; node; node = node->next) \ + { \ + PluginExtensionEntry *entry = node->data; \ + \ + if (entry->extension->f && entry->extension->f(doc, entry->data)) \ + return (ext) ? entry->extension == (ext) : TRUE; \ + \ + if ((ext) && entry->extension == (ext)) \ + return FALSE; \ + } \ + return FALSE; \ + } G_STMT_END +
But as you say, for now its not critical, but what would be a Geany discussion without a little premature optimisation 😁?
Hehe :grin: But really here I think it's best to keep it a bit simpler. If this *really* becomes an issue, I'm sure you'll come up with an optimized implementation :smile: It shouldn't have to change the interface as well, so it can wait.
@techee pushed 1 commit.
4e1adce4ef6ee7cacd4d069fe824246b59a71cd2 Revert "Export some types/functions so plugins have access to Geany symbol icons"
@techee pushed 1 commit.
935a24f93aa3c053b24c2b53cdb4a2173fc330d7 Update documentation based on review comments
@techee commented on this pull request.
+#ifndef GEANY_PLUGIN_EXTENSION_H +#define GEANY_PLUGIN_EXTENSION_H 1 + +#include "document.h" + + +G_BEGIN_DECLS + +/** + * Structure serving as an interface between plugins and Geany allowing + * plugins inform Geany about what features they provide and allowing + * Geany to delegate its functionality to the plugins. + * + * Plugins should allocate this structure on the heap - either statically, + * or dynamically. When allocated dynamically, plugins should always
I dropped the part here and used the wording Colomban suggested in the `_register()` function.
@techee commented on this pull request.
- functions can be left to contain the NULL pointer.
+ * + * Typically, functions from this interface come in pairs. Functins ending with + * @c _provided() inform Geany whether the plugin implements the given feature + * for the passed document. Functions ending with @c _perform() are called by + * Geany at appropriate moments to inform the plugin when to perform the given + * feature. + * + * Instances of the @c PluginExtension structures are registered in Geany using + * the @c plugin_extension_register() function. + * + * @warning The API provided by this file is subject to change and should not be + * considered stable at this point. That said, it is highly probable that if + * a change of this API is required in the future, it will not be of a major + * nature and should not require major modifications of the affected plugins + * (e.g. added/modified parameter of a function and similar changes).
Probably question for `@future` itself ;-)
@techee commented on this pull request.
@@ -258,6 +259,23 @@ const gchar *symbols_get_context_separator(gint ft_id)
}
+/** Gets the icon data corresponding to the provided TMIcon. + * @param icon TMIcon. + * + * Returns the GdkPixbuf corresponding to the provided TMIcon. + * + * @since 2.1 + */ +GEANY_API_SYMBOL
Dropped here, created as https://github.com/geany/geany/pull/3916
Alright, I hope I addressed all the review comments and didn't forget about anything. I tried to replace most of the `_provided()` functions and friends by something like "functions assigned to the PluginExtension members ending with _provided". I think there were a few exceptions where I referred to it as `_provided()` functions but it was typically in the same sentence where the long version was already present and I didn't want to make it too crazy.
(This night I'm going to dream about functions assigned to PluginExtension members ending with `_provided`...)
@techee pushed 1 commit.
f5da5508c2c4fe5cf6557f4769e93e590b9397b5 Update the priority of the example extension based on the guidelines
github-comments@lists.geany.org