Created attachment 357408 [details] [review]
Add Vim Words as completion provider

Created attachment 357409 [details] [review]
Add VimWordsProposal

Created attachment 357411 [details] [review]
Makefile plumbing

Review of attachment 357408 [details] [review]:

I think this is still missing the primary focus we want, which is to have distance from cursor taken into account?

It looks like you have the hashtable for quick access to what has been found and you may need a second data structure to keep things sorted by their offset from the cursor.

::: gtksourceview/completion-providers/vim-words/gtksourcecompletionvimwords.c
@@ +54,3 @@
+  GHashTable *all_proposals;
+  gchar *word;
+  guint cancel_id;

Signal connections are gulong, not guint.

@@ +77,3 @@
+  gunichar ch = 0;
+
+  g_return_val_if_fail (GTK_SOURCE_IS_COMPLETION_CONTEXT (context), NULL);

g_assert() for static functions.

@@ +93,3 @@
+
+  if (ch && !g_unichar_isalnum (ch) && (ch != '_'))
+    gtk_text_iter_forward_char (&begin);

This is an infinite loop if you reach the end of the buffer. You need to break if forward_char() returns non-TRUE

@@ +115,3 @@
+                                                &match_end,
+                                                &has_wrapped_around,
+                                                &error))

Nothing cleans up &error, so it is leaked. If you don't need the error, just pass NULL instead.

@@ +125,3 @@
+          proposal = gtk_source_completion_vim_words_proposal_new (text);
+
+          g_hash_table_insert (self->priv->all_proposals, g_strdup (text), g_object_ref (proposal));

Proposal is leaked here because you already own the new reference and the increment it to add it to the hash table.

@@ +136,3 @@
+    }
+
+  list = g_hash_table_get_values (self->priv->all_proposals);

If we are just taking the list of values from the hash table, how are we preserving the distance from the insertion cursor?

@@ +159,3 @@
+static void
+population_finished (GtkSourceCompletionVimWords *self)
+{

Add precondition check and include all the function parameters from the signal (and ensure they are correct too).

@@ +182,3 @@
+  GtkTextIter iter;
+
+  // NOTE: Return if the completion is not USER_REQUESTED

Since you specify the activation, there is no need for a USER_REQUESTED check. the base match() should do that for you.

@@ +194,3 @@
+  g_hash_table_remove_all (self->priv->all_proposals);
+
+  self->priv->search_settings = gtk_source_search_settings_new ();

Add some assertions that these are NULL before-hand so that we know that the population_finished callback has cleaned things up first.

@@ +207,3 @@
+  gtk_source_search_settings_set_search_text (self->priv->search_settings, self->priv->word);
+
+  self->priv->cancel_id = g_signal_connect_swapped (context, "cancelled", G_CALLBACK (population_finished), self);

Is cancelled guaranteed to be called before the next populate() call is made? Instead of calling population_finished(), add a callback function with the proper signature and precondition checks which then calls population_finished().

@@ +323,3 @@
+  g_clear_object (&self->priv->icon);
+  g_clear_object (&self->priv->search_context);
+  g_clear_pointer (&self->priv->all_proposals, (GDestroyNotify) g_hash_table_unref);

I don't think the cast is necessary here?

@@ +410,3 @@
+     * The type of activation.
+     *
+     * Since: 3.10

Remove the gtk-doc or update it.

@@ +432,3 @@
+
+GtkSourceCompletionVimWords *
+gtk_source_completion_vim_words_new (const gchar *name, GdkPixbuf   *icon)

Use a GIcon instead of a GdkPixbuf and then implement ->get_gicon().

::: gtksourceview/completion-providers/vim-words/gtksourcecompletionvimwords.h
@@ +57,3 @@
+
+GTK_SOURCE_AVAILABLE_IN_ALL
+GtkSourceCompletionVimWords *gtk_source_completion_vim_words_new (const gchar *name, GdkPixbuf *icon);

A GdkPixbuf is a rendered framebuffer of an icon. We probably don't want to pass those around as they would become invalid if you move the window between a HiDPI and non-HiDPI monitor. Probably better to use a GIcon here (and re-use them as much as possible to avoid lots of object creations).

Review of attachment 357409 [details] [review]:

::: gtksourceview/completion-providers/vim-words/gtksourcecompletionvimwordsproposal.h
@@ +58,3 @@
+GTK_SOURCE_INTERNAL
+GtkSourceCompletionVimWordsProposal *
+                 gtk_source_completion_vim_words_proposal_new       (const gchar *word);

Proposals can have a GIcon, so pass that on from the provider.

@@ +63,3 @@
+const gchar     *gtk_source_completion_vim_words_proposal_get_word  (GtkSourceCompletionVimWordsProposal *proposal);
+
+GTK_SOURCE_INTERNAL

I don't think you are using these signals anywhere, so they can be removed.

Review of attachment 357411 [details] [review]:

Cool, although only necessary if upstream is amenable to having new code in gtksourceview 3.x.

> @@ +207,3 @@
> +  gtk_source_search_settings_set_search_text (self->priv->search_settings,
> self->priv->word);
> +
> +  self->priv->cancel_id = g_signal_connect_swapped (context, "cancelled",
> G_CALLBACK (population_finished), self);
> 
> Is cancelled guaranteed to be called before the next populate() call is
> made? Instead of calling population_finished(), add a callback function with
> the proper signature and precondition checks which then calls
> population_finished().
> 

Can you tell me what preconditions checks are required for e.g. , to call population_finished() subsequently? 

And I think the call is guaranteed before next populate() as in population_finished() we g_clear_object (context), no?

(In reply to Umang Jain from comment #7)
> Can you tell me what preconditions checks are required for e.g. , to call
> population_finished() subsequently? 
> 
> And I think the call is guaranteed before next populate() as in
> population_finished() we g_clear_object (context), no?

I think we've cleared this up via IRC.

Created attachment 358035 [details] [review]
Add Vim Words as completion provider

Created attachment 358036 [details] [review]
Add VimWordsProposal

Review of attachment 358036 [details] [review]:

::: gtksourceview/completion-providers/vim-words/gtksourcecompletionvimwordsproposal.c
@@ +35,3 @@
+enum
+{
+  UNUSED,

If you're not using "unused" signal, get rid of it, the functions, signals[], etc.

@@ +109,3 @@
+
+GtkSourceCompletionVimWordsProposal *
+gtk_source_completion_vim_words_proposal_new (const gchar *word, gint offset, GIcon *icon)

One parameter per line and align them.

@@ +122,3 @@
+
+void
+gtk_source_completion_vim_words_proposal_use (GtkSourceCompletionVimWordsProposal *proposal)

Remove this, you aren't using it.

@@ +130,3 @@
+
+void
+gtk_source_completion_vim_words_proposal_unuse (GtkSourceCompletionVimWordsProposal *proposal)

And this.

Review of attachment 358035 [details] [review]:

It would be nice to see some tests for this so we know it is working as you expect.

::: gtksourceview/completion-providers/vim-words/gtksourcecompletionvimwords.c
@@ +144,3 @@
+        {
+          g_warning ("Unable to get completion proposals: %s", error->message);
+          g_error_free (error);

Don't leave invalid pointers around when possible. g_clear_error (&error);

@@ +157,3 @@
+
+              gtk_text_buffer_get_end_iter (gtk_text_iter_get_buffer (&self->priv->insert_iter), &end_iter);
+              offset = gtk_text_iter_get_offset (&end_iter) - gtk_text_iter_get_offset (&self->priv->insert_iter) +

This would be easier to read if each function call was on it's own line.

@@ +165,3 @@
+
+	  proposal = gtk_source_completion_vim_words_proposal_new (text, offset, NULL);
+	  g_hash_table_insert (self->priv->all_proposals, g_strdup (text), g_object_ref (proposal));

"text" is leaked, so maybe g_steal_pointer (&text) here.

@@ +175,3 @@
+                                               self);
+      return;
+

Missing g_free (text);

@@ +180,3 @@
+finish:
+  list = g_hash_table_get_values (self->priv->all_proposals);
+  list = g_list_sort (list, sort_by_offset);

This is rather expensive to do on every keypress, but once this is using IdeCompletionResults and IdeCompletionItem (as the base proposal class) that won't be much of an issue because we can just replay the previous results and sorting adjustments.

@@ +216,3 @@
+        }
+
+        g_clear_object (&self->priv->context);

Alignment is 2-spaces off

@@ +217,3 @@
+
+        g_clear_object (&self->priv->context);
+    }

Extra newline after braces

@@ +303,3 @@
+      if (!g_unichar_isalnum (ch) && ch != '_')
+        return FALSE;
+    }

Maybe just put the return TRUE inside the if block and return FALSE outside the block without the else.

Created attachment 358444 [details] [review]
Add IdeWordCompletionProvider

Created attachment 358445 [details] [review]
Add IdeWordCompletionResults

Created attachment 358446 [details] [review]
Add IdeWordCompletionItem

Created attachment 358447 [details] [review]
Add IdeWordCompletionResults

Review of attachment 358446 [details] [review]:

Couple quick fixes

::: libide/sourceview/ide-word-completion-item.c
@@ +19,3 @@
+#define G_LOG_DOMAIN "ide-word-completion-item"
+
+#ifdef HAVE_CONFIG_H

I don't think anything needs config.h here, so remove this

@@ +92,3 @@
+
+IdeWordCompletionItem *
+ide_word_completion_item_new (const gchar *word, gint offset, GIcon *icon)

One parameter per line

@@ +102,3 @@
+  proposal->priv->word = g_strdup (word);
+  proposal->priv->offset = offset;
+  proposal->priv->icon = icon;

This is not safe, you need to own your own reference.

::: libide/sourceview/ide-word-completion-item.h
@@ +26,3 @@
+G_BEGIN_DECLS
+
+#define IDE_TYPE_WORD_COMPLETION_ITEM                   (ide_word_completion_item_get_type ())

For Builder, we should be using the G_DECLARE_FINAL_TYPE() macro instead of the old style declarations.

Review of attachment 358447 [details] [review]:

Cool. If you are short on time, no need to do the G_DECLARE stuff for your final project submission, but we should do it soon to keep the code-base consistent.

::: libide/sourceview/ide-word-completion-results.h
@@ +26,3 @@
+G_BEGIN_DECLS
+
+#define IDE_TYPE_WORD_COMPLETION_RESULTS            (ide_word_completion_results_get_type())

G_DECLARE_FINAL_TYPE() for this as well. See the other files for examples, but it should come out like:

#define IDE_TYPE_WORD_COMPLETION_RESULTS (ide_word_completion_results_get_type())

G_DECLARE_FINAL_TYPE (IdeWordCompletionResults, ide_word_completion_results, IDE, WORD_COMPLETION_RESULTS, IdeCompletionResults)

and then you'll have to add to your .c file:

struct _IdeWordCompletionResults
{
  IdeCompletionResults parent;
  /* Put stuff here instead of via ->priv */
};

Review of attachment 358444 [details] [review]:

Few things to fix up, nice work!

::: libide/sourceview/ide-word-completion-provider.c
@@ +19,3 @@
+#define G_LOG_DOMAIN "ide-word-completion-provider"
+
+#ifdef HAVE_CONFIG_H

Drop the HAVE_CONFIG_H stuff unless you're really using config.h (i dont think you are)

@@ +29,3 @@
+#include "sourceview/ide-word-completion-results.h"
+#include "sourceview/ide-completion-provider.h"
+#include "ide-debug.h"

To be like the rest of the code base, toplevel includes like ide-debug.h, ide-object.h, etc go first, and then the others like:

#include "ide-debug.h"

#include "sourceview/ide-word-compltion-provider.h
# ...
# ...

@@ +41,3 @@
+  N_PROPERTIES
+};
+static GParamSpec *properties[N_PROPERTIES];

Extra new line before globals

@@ +73,3 @@
+refresh_iters (IdeWordCompletionProvider *self,
+               GtkTextIter *match_start,
+               GtkTextIter *match_end)

Align parameters like so:

73	refresh_iters (IdeWordCompletionProvider *self,
74	               GtkTextIter               *match_start,
75	               GtkTextIter               *match_end)

@@ +82,3 @@
+  g_assert (IDE_IS_COMPLETION_PROVIDER (self));
+
+  insert_mark = GTK_TEXT_MARK (g_object_get_data (G_OBJECT (self), "insert-mark"));

I don't like using GObject data for stuff like this. You already have the GtkSourceCompletionContext in self->priv, so you can get an iter from that and then the buffer.

if (priv->context == NULL ||
    !gtk_source_completion_context_get_iter (priv->context, &iter))
  return FALSE;

buffer = gtk_text_iter_get_buffer (&iter);

@@ +85,3 @@
+  buffer = gtk_text_mark_get_buffer (insert_mark);
+
+  start_mark = gtk_text_buffer_get_mark (buffer, "start-mark");

Don't use named marks. This will break if you have multiple views who try to do completion and then stomp over each others marks.

Instead, create a textmark with a NULL name, and store it in Private. Then use that pointer later on. If you do this, you can even simplify the code above to just get the buffer from the stashed marks.

@@ +132,3 @@
+      g_assert (buffer != NULL);
+
+      start_mark = gtk_text_buffer_create_mark (buffer, "start-mark", &match_start, FALSE);

Create unnamed marks (use NULL) and save them in Private so that you don't clobber another views marks on the same buffer.

@@ +191,3 @@
+                                               (GAsyncReadyCallback) forward_search_finished,
+                                               self);
+      gtk_text_mark_get_deleted (start_mark);

This function doesn't do anything, perhaps you meant gtk_text_buffer_delete_mark()?

@@ +200,3 @@
+  ide_completion_results_present (IDE_COMPLETION_RESULTS (self->priv->results),
+                                  GTK_SOURCE_COMPLETION_PROVIDER (self), self->priv->context);
+  gtk_text_mark_get_deleted (insert_mark);

Same here

@@ +201,3 @@
+                                  GTK_SOURCE_COMPLETION_PROVIDER (self), self->priv->context);
+  gtk_text_mark_get_deleted (insert_mark);
+  g_hash_table_destroy (self->priv->all_proposals);

always clear destroyed pointers to NULL in the Builder code-base. The extra cycle is worth it to simplify debugging dangling pointers.

@@ +223,3 @@
+  if (self->priv->context != NULL)
+    {
+      if (self->priv->cancel_id)

This condition/block can be replaced with:

ide_clear_signal_handler (self->priv->context, &self->priv->cancel_id);

from "ide-macros.h"

@@ +238,3 @@
+
+static void
+completion_cancelled_cb (IdeWordCompletionProvider *self)

Include the second parameter for this signal handler (GtkSourceCompletionContext *context) and add precondition check for it.

@@ +266,3 @@
+  g_assert (self->priv->search_settings == NULL);
+  g_assert (self->priv->search_context == NULL);
+  g_assert (buffer != NULL);

assert that self->priv->cancel_id == 0) too

@@ +300,3 @@
+  self->priv->results = ide_word_completion_results_new (self->priv->current_word);
+
+  insert_mark = gtk_text_buffer_get_insert (GTK_TEXT_BUFFER (buffer));

Drop this, marks are only useful with buffers and if you have the buffer you can get the insert mark (and there are other ways to get the buffer).

@@ +309,3 @@
+                                           NULL,
+                                           (GAsyncReadyCallback)forward_search_finished,
+                                           self);

You haven't provided a way to guarantee that this async operation completes before your object get's finalized. So instead of self, increment the reference count with

 g_object_ref (self)

and then be sure to drop the reference in all cases from forward_search_finished(). The easiest way is probably to use "gpointer user_data" for the 3rd parameter and then:

 g_autoptr(IdeWordCopmletionProvider) self = user_data;

::: libide/sourceview/ide-word-completion-provider.h
@@ +24,3 @@
+G_BEGIN_DECLS
+
+#define IDE_TYPE_WORD_COMPLETION_PROVIDER            (ide_word_completion_provider_get_type ())

Convert to G_DECLARE_ stuff when you get a chance

@@ +47,3 @@
+
+GType                      ide_word_completion_provider_get_type (void) G_GNUC_CONST;
+IdeWordCompletionProvider *ide_word_completion_provider_new      (const gchar *name, GIcon *icon);

One parameter per line

Created attachment 358804 [details] [review]
Add word completion provider

Classes introduced:

IdeWordCompletionProvider
IdeWordCompletionItem
IdeWordCompletionResults

Created attachment 358805 [details] [review]
sourceview: Add begin-word-completion signal

Created attachment 358806 [details] [review]
Improve on how to replay results

Created attachment 358807 [details] [review]
word-completion-provider: Remove offset computation logic

Created attachment 358822 [details] [review]
Improve on how to replay results

Created attachment 358823 [details] [review]
word-completion-provider: Remove offset computation logic

Remove offset computation logic and order new buffer
scan if we detect completion is made on new lines.

Congrats, it's in!

We should still work on polishing this up, but it's in good shape for merging.

In particular, we could *possibly* share the IdeWordCompletionProvider between
views (and just share it on the IdeBuffer).

Also, I'd like to see if we can tweak the sorting and placement of selected
item to more closely match vim. (I'm not sure if this will be possible yet, but
since we only have one provider visible, it might be). Basically, I want to see
the current word selected, with things before the word being above the
selection, and things after the word after the selection.

Attachment 358805 [details] pushed as 1805ed7 - sourceview: Add begin-word-completion signal
Attachment 358822 [details] pushed as b3f43fc - Improve on how to replay results
Attachment 358823 [details] pushed as 27a2d61 - word-completion-provider: Remove offset computation logic