@techee pushed 1 commit.

1931fcc2f4d5de46871c85a782903d8c4d41e699 Export some types/functions so plugins have access to Geany symbol icons

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

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

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

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

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

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

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.

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

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.

• if (plugin_extension_calltips_provided(doc))

+ return FALSE;

Looking forward for the additional fixes in another PR then :)

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

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.

• plugin_extension_goto_perform(doc, pos, definition);
  

+ return TRUE;

OK, sounds good.

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

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

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

947ea7b377fa76a9118972ccf7ac74f09e9cfecb Add back _provided() functions to the plugin API

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.

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.

@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

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

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.

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.

@b4n commented on this pull request.

• if (plugin_extension_calltips_provided(doc, NULL))

+ return; +

Yes

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

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.

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

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

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

{

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

@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 yeah, the only thing is whether to pass `GeanyPlugin*` to the register function, the rest looks good to me.

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

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

Keep the string, its for _human_ consumption and use, and a pointer isn't.

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.

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

@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 pushed 1 commit.

052eb37425a3ec32535de35b0712f01bf9488317 fixup! Move plugin_extension_calltips_provided() check inside on_editor_notify()

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

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.

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

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

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

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.

@techee pushed 2 commits.

e5287871054759c45d3f19871daa5d4db2bec769 Fix parameter name 2b0b55df399eb3685c88e254a290f89fb0fb3efd Add simple PluginExtension demo

@techee pushed 1 commit.

f99af6648bbfd4b2b16b20e6bcfca5e1b6486e00 Add PluginExtension HowTo to API documentation

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

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

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

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

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

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

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

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.

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.

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

935a24f93aa3c053b24c2b53cdb4a2173fc330d7 Update documentation based on review comments

@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 ;-)

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

@b4n commented on this pull request.

Minor nitpicks, but looks pretty good now I'd say :+1:

• Geany built-in functionality.

+ +When the plugin returns @c TRUE from the function assigned to the @c _provided +member of @c PluginExtension, it indicates +it wants to take control of the particular feature and disable Geany's +implementation. However, returning @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. + +A plugin can perform a check if it 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 the return values of all functions assigned to +@c autocomplete_provided members of the registered extensions.

Maybe give a short example on when this is useful, just like in `plugin_extension_autocomplete_provided()`'s documentation?

*"This can be used if the plugin needs to perform auxiliary actions outside the `_perform()` function, to verify it is actually active for this feature."*

• • cd plugins

+ * make demopluginext.so

Just maybe, just makes it a single command which could make it easier to use ```suggestion * make -C plugins demopluginext.so ``` This said, it has to run in the build directory anyway… but people using OOT builds probably know how to deal with that.

+ * Then copy or symlink the plugins/demopluginext.so file to ~/.config/geany/plugins + * - it will be loaded at next startup. + */ + +#include <geanyplugin.h> + +static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data) +{ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + const gchar *kwd_str = "False None True and as assert async await break case class continue def del elif else except finally for from global if import in is lambda match nonlocal not or pass raise return try while with yield";

It really doesn't matter, but why not having an array directly?

+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 sorted by the priority + * parameter. When executing functions assigned to the @c PluginExtension + * members ending with @c _perform, Geany goes + * through the registered extensions and executes the @c _perform() function of + * the first extension for whih the function assigned to the corresponding

```suggestion * the first extension for which the function assigned to the corresponding ```

• 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 sorted by the priority + * parameter. When executing functions assigned to the @c PluginExtension + * members ending with @c _perform, Geany goes + * through the registered extensions and executes the @c _perform() function of + * the first extension for whih the function assigned to the corresponding + * @c _provided member returns @c TRUE. + * + * This function is typically called in the plugin @c init() function.

```suggestion * This function is typically called in the plugin's @c init() function. ```

• PluginExtensionEntry *entry;

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

```suggestion * provide the extension, at the latest in the plugin's @c cleanup() function. ```

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

@elextr I expect it would eventually, but it would be nice to see if it fits more real use cases first.

@techee pushed 1 commit.

ec9365127e980d81656f1c5184388ae80ddd7945 Update src/pluginextension.c

@techee pushed 1 commit.

036ef8e8ead976f52b842c760fb81bbcdb8c51b9 Update src/pluginextension.c

@b4n commented on this pull request.

+ * Then copy or symlink the plugins/demopluginext.so file to ~/.config/geany/plugins + * - it will be loaded at next startup. + */ + +#include <geanyplugin.h> + +static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data) +{ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + const gchar *kwd_str = "False None True and as assert async await break case class continue def del elif else except finally for from global if import in is lambda match nonlocal not or pass raise return try while with yield";

Ah OK makes sense :) And yeah it doesn't matter.

Though… what about allowing extensions to *complement* Geany's completion to provide keyword completion if no other completion is done? :)))))))) OK I stop (though, we could potentially introduce this one day by having a priority below Geany's)

@techee commented on this pull request.

• Geany built-in functionality.

+ +When the plugin returns @c TRUE from the function assigned to the @c _provided +member of @c PluginExtension, it indicates +it wants to take control of the particular feature and disable Geany's +implementation. However, returning @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. + +A plugin can perform a check if it 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 the return values of all functions assigned to +@c autocomplete_provided members of the registered extensions.

Done. I've just modified it not to use `_perform()` and not to call it a function to avoid @elextr 's complaints ;-)

@techee commented on this pull request.

+ * Then copy or symlink the plugins/demopluginext.so file to ~/.config/geany/plugins + * - it will be loaded at next startup. + */ + +#include <geanyplugin.h> + +static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data) +{ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + const gchar *kwd_str = "False None True and as assert async await break case class continue def del elif else except finally for from global if import in is lambda match nonlocal not or pass raise return try while with yield";

Though… what about allowing extensions to complement Geany's completion to provide keyword completion if no other completion is done? :))))))))

This is what crossed my mind when I mentioned negative priorities here:

https://github.com/geany/geany/pull/3849#issuecomment-2165686224

But let's leave some fun for the future :-)

@b4n approved this pull request.

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

@elextr commented on this pull request.

+ * Then copy or symlink the plugins/demopluginext.so file to ~/.config/geany/plugins + * - it will be loaded at next startup. + */ + +#include <geanyplugin.h> + +static gboolean autocomplete_provided(GeanyDocument *doc, gpointer data) +{ + return doc->file_type->id == GEANY_FILETYPES_PYTHON; +} + + +static void autocomplete_perform(GeanyDocument *doc, gboolean force, gpointer data) +{ + const gchar *kwd_str = "False None True and as assert async await break case class continue def del elif else except finally for from global if import in is lambda match nonlocal not or pass raise return try while with yield";

AI Pair Programming Plugin anybody? :grinning:

I'd like https://github.com/geany/geany/commit/d34a0a36e761cb2844ae50ffaceaee18fee0d3... (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- commented on this pull request.

• gboolean (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);

+ + /** + * Pointer to function called by Geany to check whether the plugin implements + * additional symbol (e.g. type) highlighting in Scintilla. + * + * @see @c autocomplete_provided() for more details. + * @note There is no function in the @c PluginExtension structure informing + * plugins to perform symbol highlighting. Plugins + * implementing symbol highlighting should perform it at the appropriate + * moments based on Scintilla and Geany events such as when the document + * becomes visible or when the document is modified. + * + * @since 2.1 + **/ + gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);

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?

@elextr commented on this pull request.

• • to the members ending with @c _perform are called by Geany at appropriate

+ * moments to inform the plugin when to perform the given feature. + * + * The extension is defined by the pointers in the PluginExtension structure and + * is 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 +{ + /** + * Pointer to function called by Geany to check whether the plugin

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.

@elextr commented on this pull request.

• • to the members ending with @c _perform are called by Geany at appropriate

+ * moments to inform the plugin when to perform the given feature. + * + * The extension is defined by the pointers in the PluginExtension structure and + * is 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 +{ + /** + * Pointer to function called by Geany to check whether the plugin

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.

@techee commented on this pull request.

• gboolean (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);

+ + /** + * Pointer to function called by Geany to check whether the plugin implements + * additional symbol (e.g. type) highlighting in Scintilla. + * + * @see @c autocomplete_provided() for more details. + * @note There is no function in the @c PluginExtension structure informing + * plugins to perform symbol highlighting. Plugins + * implementing symbol highlighting should perform it at the appropriate + * moments based on Scintilla and Geany events such as when the document + * becomes visible or when the document is modified. + * + * @since 2.1 + **/ + gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);

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 https://github.com/geany/geany/pull/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.

@techee commented on this pull request.

• gboolean (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);

+ + /** + * Pointer to function called by Geany to check whether the plugin implements + * additional symbol (e.g. type) highlighting in Scintilla. + * + * @see @c autocomplete_provided() for more details. + * @note There is no function in the @c PluginExtension structure informing + * plugins to perform symbol highlighting. Plugins + * implementing symbol highlighting should perform it at the appropriate + * moments based on Scintilla and Geany events such as when the document + * becomes visible or when the document is modified. + * + * @since 2.1 + **/ + gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);

I find it highly problematic that it's up to the LSP plugins to get the places, from where highlighting is triggered, right. At best the author looks where Geany calls document_highlight_tags() and draw conclusions from there.

Nothing like that is necessary - the plugin just invokes it based on whether it determines the highlighting is necessary which is in 2 situations which are quite obvious from the plugin writer's perspective: - contents of the document changes, i.e. in `SC_MOD_INSERTTEXT | SC_MOD_DELETETEXT` scintilla notification - a tab is switched - i.e. `document-activate` Geany signal

That's it (and I actually don't know when exactly Geany triggers it and don't really care). I think the logic when to do it is pretty clear and much simpler than having to forward it to Geany.

There's another related problem - Geany uses Scintilla's lexer keyword index

https://github.com/geany/geany/blob/9bf5769f582cbf3d82a1faca035e7f63e8908afe...

and highlights those by setting the keywords using `sci_set_keywords()`. I pretty much copied this part and it's one type of highlighting. The trouble with this approach is: 1. It's limited to only those lexers that support it 2. It highlights all the occurrences of `foo` (if `foo` is defined as a type somewhere) no matter whether `foo` is actually used as a type in the particular file

This is why the colorization using `INDIC_TEXTFORE` was added in https://github.com/techee/geany-lsp/issues/18 based on @elextr 's suggestion. This allows colorization of only specific positions in the document so when you have `foo` as a type and `foo` as a variable in the same document, the first `foo` gets colorized and the second one doesn't. (One limitation of this approach is that the text cannot be made bold, see the linked issue above for more discussion.)

This second mode wouldn't be possible with current Geany's colorization.

@elextr commented on this pull request.

• gboolean (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition, gpointer data);

+ + /** + * Pointer to function called by Geany to check whether the plugin implements + * additional symbol (e.g. type) highlighting in Scintilla. + * + * @see @c autocomplete_provided() for more details. + * @note There is no function in the @c PluginExtension structure informing + * plugins to perform symbol highlighting. Plugins + * implementing symbol highlighting should perform it at the appropriate + * moments based on Scintilla and Geany events such as when the document + * becomes visible or when the document is modified. + * + * @since 2.1 + **/ + gboolean (*symbol_highlight_provided)(GeanyDocument *doc, gpointer data);

Its not really a Lexilla issue, lexers do not have any idea about types so they cannot distinguish which instances of a name are types and which are variables and which are something else.