Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
@b4n requested changes on this pull request.
This is not an entirely through review, but still not an overpass. Anyway, it seems fairly similar to the previous PR, minus the symbols tree. This said, it makes it a lot smaller, and looking a lot less specific. Admittedly, finding it less specific also comes from me looking a bit deeper in the implementation and understanding more of the choices when they looks awfully specific.
I think there still are a few things that could/should be polished, but it looks fair enough 👍
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.
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.
> +typedef struct {
+ gboolean (*autocomplete_provided)(GeanyDocument *doc);
+ void (*autocomplete_perform)(GeanyDocument *doc, gboolean force);
+
+ gboolean (*calltips_provided)(GeanyDocument *doc);
+ void (*calltips_show)(GeanyDocument *doc, gboolean force);
+
+ gboolean (*goto_provided)(GeanyDocument *doc);
+ void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition);
+
+ gboolean (*doc_symbols_provided)(GeanyDocument *doc);
+ GPtrArray *(*doc_symbols_get)(GeanyDocument *doc);
+
+ gboolean (*symbol_highlight_provided)(GeanyDocument *doc);
+
+ gchar _dummy[1024];
Maybe it'd be safer to have e.g. void (*_dummy[42])(void); here, so we can be reasonably sure how to update the struct when adding new members? Or even just a (long) list of separate dummies, in case alignment of the array is different than alignment of the struct members themselves.
Just that I'm not even sure we could update this properly not to change the size of the structure -- which is probably OK so long as we don't update it past the 1024 bytes mark, but it'll be hard to tell when this happens.
> + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
We might need a way for an extension to know whether it's active or not. In case we might one day support more extensions (or have Geany functions itself go through the extension mechanism), we need the extension to be able to know whether it's gonna be used or not when it performs ancillary things, like handle sci-notify and display calltips or such.
I'm playing with making Geany functions the base extension to see if the API would work for this, and one thing that it lacks is this, so it can't tell whether or not it should do its thing.
It's also probably important to introduce something like this rather early because it'll require the plugins to willfully behave. And it's relevant even if we support a single active extension, so that if two plugins provide an extension, although the last one "wins", they would still partly fight one another if they don't know they are not actually active.
For now I'm playing with something as basic as gboolean plugin_extension_active(PluginExtension *extension), but we might want to provide something more flexible maybe, if we want to be able to mix extensions (e.g. if an extension only provides on of the features, complementing another one). It could be as simple as const PluginExtension *plugin_extension_get_current(void) (assuming we'd update the extension structure with the overrides instead of overriding the whole thing) and then callers could be supposed to check if (plugin_extension_get_current()->autocomplete_perform == myplugin_atocomplete_perform) /* … */ -- or any better idea 🙂
In src/symbols.c:
> + plugin_extension_goto_perform(doc, pos, definition); + return TRUE;
I see two issues here:
name in the API call, which makes sense (the extension might have better knowledge on what word actually is). However, all callers are guarding against the "current word" being non-empty, which is at least odd if not a possible issue.I'm not entirely sure how to reconcile both those points in one single API call making all current callers happy though.
In src/editor.c:
> + if (plugin_extension_calltips_provided(doc)) + return FALSE;
This should rather be in the SCN_AUTOCCANCELLED handling below (where it calls request_reshowing_calltip(nt))
> + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc);
This isn't used in this version of the patchset, is it? I guess it's for the symbols tab, right?
I'm not against having it here, but there should be code to do something with it, or we leave it out and see whether it might be useful later.
> +
+
+typedef struct {
+ gboolean (*autocomplete_provided)(GeanyDocument *doc);
+ void (*autocomplete_perform)(GeanyDocument *doc, gboolean force);
+
+ gboolean (*calltips_provided)(GeanyDocument *doc);
+ void (*calltips_show)(GeanyDocument *doc, gboolean force);
+
+ gboolean (*goto_provided)(GeanyDocument *doc);
+ void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition);
+
+ gboolean (*doc_symbols_provided)(GeanyDocument *doc);
+ GPtrArray *(*doc_symbols_get)(GeanyDocument *doc);
+
+ gboolean (*symbol_highlight_provided)(GeanyDocument *doc);
I guess there isn't any _perform() because there isn't a canonical place where everybody should do this?
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef GEANY_PLUGIN_EXTENSION_H
+#define GEANY_PLUGIN_EXTENSION_H 1
+
+#include "document.h"
+
+/* Temporary define so the plugin can check whether it's compiled against
+ * Geany with plugin extension support or not */
+#define HAVE_GEANY_PLUGIN_EXTENSION 1
+
+G_BEGIN_DECLS
+
+
+typedef struct {
Maybe @elextr would be right to scream no doooocs! 😆
> + void (*calltips_show)(GeanyDocument *doc, gboolean force); + + gboolean (*goto_provided)(GeanyDocument *doc); + void (*goto_perform)(GeanyDocument *doc, gint pos, gboolean definition); + + gboolean (*doc_symbols_provided)(GeanyDocument *doc); + GPtrArray *(*doc_symbols_get)(GeanyDocument *doc); + + gboolean (*symbol_highlight_provided)(GeanyDocument *doc); + + gchar _dummy[1024]; +} PluginExtension; + + +void plugin_extension_register(PluginExtension *extension); +void plugin_extension_unregister(PluginExtension *extension);
Hum… actually as whether a feature is provided or not is dynamic, a static check wouldn't be enough for actually supporting more than one extension.
plugin_extension_autocomplete_provider(doc) == myplugin_extension maybe? (or the function itself even)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.