Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin extensions (aka LSP API) #3849

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Plugin extensions (aka LSP API) #3849

wants to merge 8 commits into from

Conversation

techee
Copy link
Member

@techee techee commented Apr 21, 2024

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.

@techee
Copy link
Member Author

techee commented May 26, 2024

@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 #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:

if (tag->is_lsp)
    //LSP symbol - do one thing
else
    //TM symbol - do other thing

I tried this in the extra patches of #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?

@elextr
Copy link
Member

elextr commented May 26, 2024

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.

@b4n b4n self-requested a review June 2, 2024 13:32
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

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.

Big diff ahead!
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);

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.

src/pluginextension.h Outdated Show resolved Hide resolved
src/pluginextension.h Show resolved Hide resolved
src/symbols.c Outdated Show resolved Hide resolved
src/editor.c Outdated Show resolved Hide resolved
src/pluginextension.h Outdated Show resolved Hide resolved
src/pluginextension.h Outdated Show resolved Hide resolved
G_BEGIN_DECLS


typedef struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @elextr would be right to scream no doooocs! 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No docs because we didn't agree on the final API yet ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elextr is reserving his scream for later 😈

src/pluginextension.c Outdated Show resolved Hide resolved
@elextr
Copy link
Member

elextr commented Jun 4, 2024

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.

Of course the real question is "who decides the order in the chain"?

But if that question can be decided there is significant benefit to the separation of "what to do" decided by Geany and the "doers" be that plugins, or Geany fallback capabilities.

Unfortunately the "who decides" question is one we havn't solved even for simple things like plugins fighting over icons :-(

@techee
Copy link
Member Author

techee commented Jun 4, 2024

That sounds like these.

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
Copy link
Member Author

techee commented Jun 4, 2024

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 #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

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 #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
Copy link
Member

b4n commented Jun 5, 2024

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.

Another not-actually-so-big 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);
 

@b4n
Copy link
Member

b4n commented Jun 5, 2024

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
Copy link
Member Author

techee commented Jun 5, 2024

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 :-).

@techee
Copy link
Member Author

techee commented Jun 5, 2024

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
Copy link
Member Author

techee commented Jun 5, 2024

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:

#3571 (comment)

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.

@b4n
Copy link
Member

b4n commented Jun 6, 2024

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?)

@techee
Copy link
Member Author

techee commented Jun 6, 2024

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

+#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

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.

@elextr
Copy link
Member

elextr commented Jun 6, 2024

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
Copy link
Member Author

techee commented Jun 7, 2024

@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.

@techee techee mentioned this pull request Jun 8, 2024
@techee
Copy link
Member Author

techee commented Jun 8, 2024

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?

@elextr
Copy link
Member

elextr commented Jun 9, 2024

@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
Copy link
Member Author

techee commented Jun 9, 2024

@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.

@elextr
Copy link
Member

elextr commented Jun 9, 2024

Ok, good point

src/editor.c Outdated Show resolved Hide resolved
@b4n
Copy link
Member

b4n commented Jun 10, 2024

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 😄), but better add it now that change the API if we know it gotta change.

@techee
Copy link
Member Author

techee commented Jun 10, 2024

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?

@elextr
Copy link
Member

elextr commented Jun 17, 2024

Had a first look at the documentation, will comment in detail soon where needed, but one point what 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.

@techee
Copy link
Member Author

techee commented Jun 17, 2024

@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
Copy link
Member Author

techee commented Jun 21, 2024

I've added the howto and a simple example under plugins/demopluginext.c.

Copy link
Member

@elextr elextr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

doc/plugins.dox Outdated

@section plugin_extension_intro Introduction

Since Geany 2.1 the PluginExtension API allows plugins to take over some of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

doc/plugins.dox Outdated

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

typename highlighting inside document, etc.

@section plugin_extension_init Initialization and cleanup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

doc/plugins.dox Outdated

@section plugin_extension_impl Implementing extensions

Inside the @c PluginExtension instance, the plugin fills-in the pointers to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats a plugin extension instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Geany is C, not C++ or Java 😁 so use the C term, "struct"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, "PluginExtension struct" would be incorrect here, it should be something like "variable of the PluginExtension type".

doc/plugins.dox Outdated

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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 😀 )

src/pluginextension.h Show resolved Hide resolved
typedef struct
{
/**
* Called by Geany to check whether the plugin implements autocompletion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, its understandable, but technically it should be "Pointer to function called by Geany ..."

Same for all below.


/**
* Registers the provided extension in Geany. There can be multiple extensions
* registered in Geany - these are stored in a list sorted by the priporty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priporty

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

src/symbols.c Outdated Show resolved Hide resolved
@techee
Copy link
Member Author

techee commented Jun 24, 2024

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.

@elextr
Copy link
Member

elextr commented Jun 24, 2024

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. 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.

@techee
Copy link
Member Author

techee commented Jun 24, 2024

But you already do understand it, so you are a bad judge of that, there is a name for this 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 ;-)

@techee
Copy link
Member Author

techee commented Jun 25, 2024

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...)

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks, but looks pretty good now I'd say 👍

doc/plugins.dox Show resolved Hide resolved
plugins/demopluginext.c Show resolved Hide resolved
plugins/demopluginext.c Show resolved Hide resolved
src/pluginextension.c Outdated Show resolved Hide resolved
src/pluginextension.c Outdated Show resolved Hide resolved
src/pluginextension.c Outdated Show resolved Hide resolved
src/pluginextension.h Show resolved Hide resolved
@techee
Copy link
Member Author

techee commented Jun 29, 2024

Minor nitpicks, but looks pretty good now I'd say 👍

@b4n If it's kind of alright, what should I do now - squash into a single commit or try to preserve the history and squash just the various fixes?

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I didn't re-test it extensively, but not much has changed in the code anyway. A little squashing after @elextr's last proofread if he wants, and we should be golden

@b4n
Copy link
Member

b4n commented Jun 29, 2024

@b4n If it's kind of alright, what should I do now - squash into a single commit or try to preserve the history and squash just the various fixes?

I'd like d34a0a3 (and its fix(es?)) be separate; for the rest it's your call. I'd keep the same separation as you initially did and squash the changes, but if it becomes too complicated you can also just squash the whole thing together but the commit above.

@techee techee force-pushed the lsp2 branch 2 times, most recently from b895f82 to f70b5d5 Compare June 30, 2024 09:34
techee and others added 5 commits June 30, 2024 11:45
Most of the changes just make it possible for the extension freely
decide whether to perform goto based on its internal logic.
This moves obtaining the current word and related logic for Geany's
TM-based goto into symbols_goto_tag().

Co-authored-by: Colomban Wendling <[email protected]>
Co-authored-by: Colomban Wendling <[email protected]>
Co-authored-by: Lex Trotman <[email protected]>
@techee
Copy link
Member Author

techee commented Jun 30, 2024

I'd like d34a0a3 (and its fix(es?)) be separate; for the rest it's your call. I'd keep the same separation as you initially did and squash the changes, but if it becomes too complicated you can also just squash the whole thing together but the commit above.

This is what I did. At the end I just added all the goto changes into the commit that makes Geany use the new interface.

For the non-trivial review comments and smaller commits of others I also added Co-Authored-By - I hope it's fine this way.

Finally I bumped the Geany API version so this API can now be officially used by plugins.

Does it look OK?

@kugel-
Copy link
Member

kugel- commented Jun 30, 2024

FWIW, I would like to ensure peasy plugins can use this APIs. I'm going to try

*
* @since 2.1
**/
gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is a recipe for getting it wrong. Why doesn't Geany define "appropriate moments based on Scintilla and Geany events" and calls a symbol_highlight_perform() then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the issue is that it basically depends on when the symbols get ready, which is not something Geany can control for e.g. LSP. Sure, it could be added to various places, but for the case of the LSP plugin it would never be useful. For other synchronous applications it could potentially be useful I guess, so possibly calling it where Geany already does might be a reasonable thing, and an async plugin would just do nothing there… dunno.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is a recipe for getting it wrong. Why doesn't Geany define "appropriate moments based on Scintilla and Geany events" and calls a symbol_highlight_perform() then?

@kugel- I did it this way originally but there were situations where it didn't work. Consider for instance LSP server start - once it is started, it should colorize the current document. But for Geany "nothing happened" in the meantime - there was no Geany event that would trigger the colorization as Geany knows nothing about LSP processes and their start. So in this case you wouldn't get any colorization and documents would only get colorized once you trigger some Geany event like when you e.g. type something or switch tabs.

There will be a similar problem with the symbol tree update in #3910 where I think it would be best to have some symbol_tree_reload() function the plugin could call to force Geany reload the symbol tree as Geany doesn't know about all the situations in which it may be necessary.

To be precise, all of the other calls from this API suffer from the same problem - it's just that this isn't critical for something like autocompletion, calltips, or symbol goto which are originally processed by Geany before the server is started and only then switch to the server implementation.

typedef struct
{
/**
* Pointer to function called by Geany to check whether the plugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got that @elextr requested this kind of wording, but to me it sounds absolutely weird. I would expect simpler language like "callback called by Geany to …" or even just "called by Geany to…"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The struct member is not the function, its a pointer to the function, it can point to any function, its not a fixed function. The original description seemed to suggest that the functions had fixed names, whereas its the member pointers that have the fixed names, and it was very confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps confusing for you, but to me it's clear: This is a table of callbacks implemented by the plugin. It's obvious to me that the plugin can chose any name internally.

For reference, see GeanyProxyFuncs that we already have documented


/** Hooks that need to be implemented by every proxy
 *
 * @see geany_plugin_register_proxy() for a full description of the proxy mechanism.
 *
 * @since 1.26 (API 226)
 **/
struct GeanyProxyFuncs
{
	/** Called to determine whether the proxy is truly responsible for the requested plugin.
	 * A NULL pointer assumes the probe() function would always return @ref GEANY_PROXY_MATCH */
	gint		(*probe)     (GeanyPlugin *proxy, const gchar *filename, gpointer pdata);
	/** Called after probe(), to perform the actual job of loading the plugin */
	gpointer	(*load)      (GeanyPlugin *proxy, GeanyPlugin *subplugin, const gchar *filename, gpointer pdata);
	/** Called when the user initiates unloading of a plugin, e.g. on Geany exit */
	void		(*unload)    (GeanyPlugin *proxy, GeanyPlugin *subplugin, gpointer load_data, gpointer pdata);
};

Anyway I don't request a change here, just that it's worded strangely to my programmer eyes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment to @techee, because somebody understands it they are the wrong person to judge how understandable it is to others, its a normal human cognitive bias that we all have and really hard to avoid.

Perhaps my confusion was because of the use of the member name as if it actually was a function name in the original documentation, I havn't had a chance to re-read it after @techee updated it, maybe it will be clearer, but of course now I also understand it, sigh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp Related to LSP API and plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants