Created attachment 356997 [details] [review]
documentation-card: card showing relevant information about a library function

Review of attachment 356997 [details] [review]:

Everything is well put together and I'm very happy with the work you've done so far. In the various comments I've tried to add some notes on how I've been implementing features which might influence future patches. I'm excited to see this land!

I think the overarching design pieces we should work through are:

 - Create a new GObject subclasses for IdeDocumentationInfo/IdeDocumentationInfoCard
 - Possibly rename IdeDocumentationInfoCard to IdeDocumentationProposal
 - Try to reduce the amount of code executed in the motion-notify-event handler

Once we iterate on that, I think this will be ready to go!

::: libide/ide-context.c
@@ +51,3 @@
 #include "search/ide-search-provider.h"
 #include "snippets/ide-source-snippets-manager.h"
+#include "sourceview/ide-documentation.h"

We should probably move this to it's own directory, "documentation/" seems appropriate.

@@ +73,3 @@
   IdeDeviceManager         *device_manager;
   IdeDoap                  *doap;
+  IdeDocumentation            *documentation;

Make sure *documentation is aligned with the other fields.

@@ +232,2 @@
 /**
+ * ide_xontext_get_documentation:

s/xontext/context

::: libide/sourceview/ide-documentation-provider.h
@@ +29,3 @@
+
+#define IDE_TYPE_DOCUMENTATION_PROVIDER             (ide_documentation_provider_get_type())
+#define IDE_DOCUMENTATION_PROVIDER_GET_INTERFACE(o) (G_TYPE_INSTANCE_GET_INTERFACE((o), IDE_TYPE_DOCUMENTATION_PROVIDER, IdeDocumentationProviderInterface))

This GET_INTERFACE() macro should not be necessary. The G_DECLARE_INTERFACE() macro will generate an inline function named IDE_DOCUMENTATION_PROVIDER_GET_IFACE(). I know it's not very well documented that is the case...

@@ +39,3 @@
+  void                    (*get_info)        (IdeDocumentationProvider    *self,
+                                              IdeDocumentationInfo        *info);
+  gchar		               *(*get_name)        (IdeDocumentationProvider    *self);

alignment needs adjusting.

::: libide/sourceview/ide-documentation.c
@@ +50,3 @@
+  IdeDocumentationInfo *info = user_data;
+
+  if (ide_documentation_provider_get_context (provider) != info->context)

If you do == here instead of !=, you can drop the return and newline.

@@ +88,3 @@
+}
+
+IdeDocumentationInfo *

Since this function returns a newly allocated IdeDocumentationInfo, we need to let GObject Introspection know it can take ownership of the resulting structure. We do that with a documentation block that would look something like this:

/**
 * ide_documentation_get_info:
 * @self: An #IdeDocumentation
 * @input: the search keyword
 * @context: the context for the request
 *
 * Requests documentation for the keyword.
 *
 * Returns: (transfer full): An #IdeDocumentationInfo
 */

Changing IdeDocumentationInfo to become a GObject will help here because you won't have to register a custom type for it.

::: libide/sourceview/ide-documentation.h
@@ +31,3 @@
+                      IDE, DOCUMENTATION,
+                      IdeObject)
+typedef enum {

Prefix the enumeration types with an "UPPER_CASE" prefix matching the type. So something similar to:

 IDE_DOCUMENTATION_CONTEXT_NONE,
 IDE_DOCUMENTATION_CONTEXT_CARD,

@@ +36,3 @@
+} IdeDocumentationContext;
+
+typedef struct

I think the IdeDocumentationInfo should be turned into it's own object. Currently, it looks like the get_info() function returns a newly allocated structure, but no standard way to free them. Making them a GObject would solve that.

It will result in a bit more code for things like getters/setters, but it will be easier to maintain with regards to ABI. Otherwise, if we need to change the structure, we break ABI.

@@ +49,3 @@
+  const gchar *book_name;
+  gchar       *uri;
+} IdeDocumentationInfoCard;

Is IdeDocumentationInfoCard meant to be stored in IdeDocumentationInfo.proposals? Maybe naming it IdeDocumentationProposal would be more clear?

::: plugins/devhelp/gbp-devhelp-documentation-provider.c
@@ +29,3 @@
+
+  IdeDocumentation         *documentation;
+  IdeDocumentationContext   context;

One habit I've formed is to keep pointer types and integer types (such as enums) grouped together. Since IdeDocumentationContext is an enum and enums are integers in C, on 64-bit you'll have 4 bytes of padding after context and before card since the compiler wants to "naturally align" pointers.

It's not at all important for space here, which is why I only mention it in terms for habit.

@@ +70,3 @@
+
+static IdeDocumentationInfoCard*
+card_create ()

Technically, "card_create()" and "card_create (void)" are not equivalent declarations in C. We want "(void)" here.

@@ +74,3 @@
+  IdeDocumentationInfoCard *init_card;
+
+  init_card = g_slice_new0 (IdeDocumentationInfoCard);

As mentioned elsewhere, I think IdeDocumentationInfoCard might benefit from being a GObject. It would allow the devhelp plugin to subclass it for things like the "book name" which aren't useful to other providers.

Also, it feels a bit awkward to have structure allocation/free code in the plugin, where as the IdeDocumentationInfoCard structure is defined in libide. By moving to a GObject that becomes more natural because it's just the normal object allocation/free routines.

@@ +98,3 @@
+
+static gchar*
+regex_replace_line (GRegex      *regex,

I like this. Definitely makes the regex replace code nicely readable.

@@ +100,3 @@
+regex_replace_line (GRegex      *regex,
+                    gchar       *line,
+                    gchar       *replace)

"const gchar *replace" because we don't modify replace in this function.

@@ +129,3 @@
+  gboolean text_tag = FALSE;
+
+  file_stream = g_file_read (g_file_new_for_uri (file_name), NULL, &error);

"error" is being leaked because we can return here when error is non-NULL.

@@ +131,3 @@
+  file_stream = g_file_read (g_file_new_for_uri (file_name), NULL, &error);
+  if (file_stream == NULL)
+    return FALSE;

When returning in places like this, we leak some variables like "header" and "text".

One way to avoid this is to use g_autoptr like:

  g_autoptr(GString) header = g_string_new (NULL);
  g_autoptr(GString) text = g_string_new (NULL);

@@ +136,3 @@
+  start_text = g_regex_new (regex_char, 0, 0, NULL);
+
+  data_stream = g_data_input_stream_new (G_INPUT_STREAM (file_stream));

We need to make sure data_stream is freed when no longer in use. When defining the variable, do something like:

  g_autoptr(GDataInputStream) data_stream = NULL;

@@ +140,3 @@
+  while (TRUE)
+    {
+      line = g_data_input_stream_read_line_utf8 (data_stream, NULL, NULL, &error);

The error can be leaked here too.

Since we don't propagate the error to the caller, we can probably just use g_autoptr() like:

  g_autoptr(GError) error = NULL;

@@ +156,3 @@
+          line = regex_replace_line (regexes[NEW_LINE], line, "\n");
+
+          if (g_regex_match (regexes[REMOVE_MULTI_SPACES], line, 0, &match_info))

One thing I very much dislike about the g_regex_match() API is that you MUST free match_info even if g_regex_match() returns FALSE.

So this, and others in this function, need to have g_match_info_free() called. You might find it cleaner to do that with separate match_info pointer for each call and using g_autoptr(GMatchInfo) so that you don't overwrite and leak the previously stored value.

@@ +202,3 @@
+            g_string_append_printf (text, "\n<tt>%s</tt>", line);
+          else
+            g_string_append_printf (text, "%s ", line);

It looks like "line" is being leaked here (or after any "continue").

@@ +206,3 @@
+    }
+
+  self->card->header = g_string_free (header, FALSE);

If you decide to use g_autoptr(GString) to avoid the leak upon failure, we need to steal the pointer here like: g_string_free (g_steal_pointer (&header), FALSE) to avoid a double free.

@@ +207,3 @@
+
+  self->card->header = g_string_free (header, FALSE);
+  self->card->text = g_string_free (text, FALSE);

And here.

@@ +241,3 @@
+{
+  GbpDevhelpDocumentationProvider *self = (GbpDevhelpDocumentationProvider *)provider;
+  gchar **tokens;

Since tokens is kept within this function, we can use the g_auto helper here to avoid having to manually call g_strfreev() before returning.

  g_auto(GStrv) tokens = NULL;

It's not very well known I guess, but GStrv is a typedef to gchar** so it's equivalent here.

@@ +326,3 @@
+  regexes[INFORMAL_EXAMPLE] = g_regex_new ("<div class=\"informalexample\">", 0, 0, NULL);
+  regexes[INFORMAL_EXAMPLE_END] = g_regex_new ("</div>", 0, 0, NULL);
+  regexes[CLEAN_UP] = g_regex_new ("</?[acdehlpsu].*?>|</?td.*?>|</?ta.*?>|</?tb.*?>", 0, 0, NULL);

After creating all of the regexes, it might make sense to assert they all were created successfully. Mostly just for development time sanity in case we need to adjust one later. Something like this would suffice:

#ifdef IDE_ENABLE_TRACE
  for (guint i = 0; i < N_REGEXES; i++)
    {
      if (regexes[i] == NULL)
        g_error ("Failed to create regex %d", i);
    }
#endif

::: plugins/devhelp/gbp-devhelp-documentation-provider.h
@@ +24,3 @@
+G_BEGIN_DECLS
+
+//#define GBP_DEVHELP_DOCUMENTATION_PROVIDER_PRIORITY 200

This can be removed?

::: plugins/devhelp/gbp-devhelp-editor-view-addin.c
@@ +19,3 @@
 #define G_LOG_DOMAIN "gbp-devhelp-editor-view-addin"
 
+#include <devhelp/devhelp.h>

It looks like the changes in this file were incremental and can now be removed.

::: plugins/documentation-card/gbp-documentation-card-view-addin.c
@@ +30,3 @@
+  IdeEditorView        *editor_view;
+  GbpDocumentationCard *popover;
+  gchar                *previous_text;

It looks like previous_text is leaked when the addin is unloaded.

@@ +61,3 @@
+
+  source_view = ide_editor_view_get_view (self->editor_view);
+  if (source_view == NULL || !GTK_SOURCE_IS_VIEW (source_view))

GTK_SOURCE_IS_VIEW() checks for NULL, so you remove the "== NULL" check here.

@@ +83,3 @@
+  display = gdk_window_get_display (window);
+  device = gdk_seat_get_pointer (gdk_display_get_default_seat (display));
+

The motion-notify-event signal can be emitted at high-frequency while moving the mouse. So a lot of this will get executed every 16.7msec (assuming a refresh rate of 60hz, as I think we limit the motion events to match the display frame-rate).

One thing we can do to make the computer do less work, is to simply add a timeout from the motion-notify-event handler. Each time the motion handler is called, if the timeout exists, remove it and add a new timeout. By doing this, we only update the documentation information when the mouse has stabilized. I'd venture a guess and say that we don't need to update more than twice per-second.

@@ +90,3 @@
+
+  while (g_unichar_islower (gtk_text_iter_get_char (&begin)) || g_unichar_isdigit(gtk_text_iter_get_char (&begin)) || gtk_text_iter_get_char (&begin) == '_')
+    gtk_text_iter_backward_char (&begin);

backward_char() returns FALSE if you've reached the beginning of the buffer. We need to break the loop in that case or we can have an infinite loop.

@@ +94,3 @@
+
+  while (g_unichar_islower (gtk_text_iter_get_char (&end)) || g_unichar_isdigit (gtk_text_iter_get_char (&end)) || gtk_text_iter_get_char (&end) == '_')
+    gtk_text_iter_forward_char (&end);

Same here if forward_char() reaches the end of the buffer.

@@ +98,3 @@
+  selected_text = gtk_text_buffer_get_text (GTK_TEXT_BUFFER (buffer), &begin, &end, FALSE);
+
+  if (!g_strcmp0 (selected_text, self->previous_text) == 0)

Stray ! before g_strcmp0().

@@ +108,3 @@
+      gbp_documentation_card_set_text (self->popover, info);
+      g_free (self->previous_text);
+      self->previous_text = selected_text;

selected_text is leaked if we don't reach this line. I suggest doing something like this:

  g_autofree gchar *selected_text = NULL;

in the declaration, and then "steal" the pointer when assigning it here:

  g_free (self->previous_text);
  self->previous_text = g_steal_pointer (&selected_text);

@@ +126,3 @@
+  self->editor_view = view;
+  self->popover = g_object_new (GBP_TYPE_DOCUMENTATION_CARD,
+                                "relative-to", GTK_WIDGET (view),

No need for a cast here, you can just pass "view".

@@ +131,3 @@
+                                 NULL);
+
+  g_signal_connect_object (view,

We want to save the "handler id" for the g_signal_connect_object() here. And then we need an unload implementation that does something like:

  g_signal_handler_disconnect (view, self->motion_handler_id);

Otherwise, if the plugin is unloaded, we could theoretically get a call to the function after it as been unloaded but before it's been freed. An unload func would also be a good place to g_free (self->previous_text);

::: plugins/documentation-card/gbp-documentation-card.c
@@ +55,3 @@
+  gint x, y;
+
+  if (self->timeout_id)

It's not well documented, but we have a helper in ide-macros.h for this common pattern.

ide_clear_source (&self->timeout_id);

@@ +89,3 @@
+static void
+gbp_documentation_card__button_clicked (GbpDocumentationCard *self,
+                                        GtkButton                   *button)

There are a few places where there is extraneous whitespace. Probably from renaming things at some point.

@@ +95,3 @@
+
+  gtk_popover_set_modal (GTK_POPOVER (self), TRUE);
+  gtk_label_set_width_chars (self->text, CARD_WIDTH);

You might be able to set max-width-chars here instead, so that the popover can shrink, but request no width wider than the 80 characters.

@@ +119,3 @@
+  gtk_widget_init_template (GTK_WIDGET (self));
+
+  self->timeout_id = 0;

GObject's are always initialized to zero, so no need to set timeout_id here.

@@ +135,3 @@
+  IdeDocumentationInfoCard *card;
+
+  card = (g_list_first (info->proposals))->data;

Check for NULL before dereferencing the list item.

Do you think it makes sense to have controls to let the user switch between the proposals if there is more than one? Or maybe that is more useful for the auto-completion info window (if we get to that)?

@@ +146,3 @@
+  GdkDisplay *display;
+
+  g_assert (GBP_IS_DOCUMENTATION_CARD (self));

We try to use g_return_if_fail() in API that can be called from other files and g_assert() in local functions (so those starting with "static").

The reasoning behind this is that we try to be tolerant (to the degree we can) of people calling our API incorrectly, but strict about internal consistency. It doesn't always work, but that is at least the thought behind the pattern.

@@ +156,3 @@
+    g_source_remove (self->timeout_id);
+
+  self->timeout_id = gdk_threads_add_timeout (HOVER_TIMEOUT,

This is leaking a reference to self, because nothing is performing the unref to go with it. Where this gets tricky, is when calling g_source_remove(). In that scenario card_popup() will never get called. So we need a way to release the reference both when card_popup() is called and when the timeout is removed without being called.

Thankfully, we have a convenient solution for this. If we use gdk_threads_add_timeout_full(), we can pass a "cleanup" function to unref our data pointer. So something like this:

  self->timeout_id = gdk_threads_add_timeout_full (G_PRIORITY_LOW,
                                                   HOVER_TIMEOUT,
                                                   card_popup,
                                                   g_object_ref (self),
                                                   g_object_unref);

When the timeout is removed (or after successfully completing), g_object_unref() will be called and passed our data pointer (self) and leave us in a consistent state.

@@ +163,3 @@
+void
+gbp_documentation_card_popdown (GbpDocumentationCard *self)
+{

g_return_if_fail (GBP_IS_DOCUMENTATION_CARD (self)); here too

::: plugins/documentation-card/gbp-documentation-card.h
@@ +28,3 @@
+G_DECLARE_FINAL_TYPE (GbpDocumentationCard, gbp_documentation_card, GBP, DOCUMENTATION_CARD, GtkPopover)
+
+void gbp_documentation_card_set_text (GbpDocumentationCard *self,

How about renaming this to gbp_documentation_card_set_info() and then as we add more fields/extend GbpDocumentationInfo it continues to be an accurate name.

Created attachment 357477 [details] [review]
documentation-card: card showing relevant information about a library function

I reduced the amount of work done in motion-notify-event handler. It was bit tricky since I took advantage of always knowing what's under the pointer and whether the card should be popped down or not. So I created some sort of tolerance,  the mouse can move a bit without the card getting lost so the user has the chance to move the pointer over the card.

I think I’m getting hang on dealing with the leaks but I still wasn’t sure on couple of occasions. Specially the g_autofree gchar *line = NULL; line 114 in gbp-devhelp-documentation-provider.c.

--@@ +95,3 @@
--+
--+  gtk_popover_set_modal (GTK_POPOVER (self), TRUE);
--+  gtk_label_set_width_chars (self->text, CARD_WIDTH);

--You might be able to set max-width-chars here instead, so that the popover can
--shrink, but request no width wider than the 80 characters.

The problem with the max-with-chars is that when the function name is short the text becomes very narrow. So I at least made card change width to 80 character only after the user clicks “Show more”.

Review of attachment 357477 [details] [review]:

Things look like they're coming along quite well!

::: libide/documentation/ide-documentation-info.c
@@ +70,3 @@
+ * Requests proposal for the index.
+ *
+ * Returns: (transfer full): An #IdeDocumentationProposal

This doesn't pass a full reference back, so either (transfer none), or g_object_ref (g_ptr_array_index (...)).

::: libide/documentation/ide-documentation-info.h
@@ +26,3 @@
+#define IDE_TYPE_DOCUMENTATION_INFO (ide_documentation_info_get_type())
+
+G_DECLARE_DERIVABLE_TYPE (IdeDocumentationInfo, ide_documentation_info, IDE, DOCUMENTATION_INFO, GObject)

Do you anticipate subclassing IdeDocumentationInfo? If not, we should make it G_DECLARE_FINAL_TYPE() and then remove the class structure below. (You'd also need to add struct _IdeDocumentationInfo { GObject parent_instance; } to the .c file).

@@ +36,3 @@
+
+typedef enum {
+  IDE_DOCUMENTATION_CONTEXT_NON,

We use "NONE" in other places, so we should be consistent.

::: libide/documentation/ide-documentation-proposal.c
@@ +41,3 @@
+
+IdeDocumentationProposal *
+ide_documentation_proposal_new (gchar  *uri)

const gchar *uri

@@ +48,3 @@
+}
+
+gchar *

const gchar * since the caller may not mutate the string.

@@ +58,3 @@
+}
+
+gchar *

const gchar *

@@ +68,3 @@
+}
+
+gchar *

const gchar *

@@ +87,3 @@
+  g_return_if_fail (IDE_IS_DOCUMENTATION_PROPOSAL (self));
+
+  priv->header = g_strdup (header);

This leaks priv->header. So let's do this pattern for strings. We only want to update the string (and notify if there are any signal listeners) if it changed. So we can use g_strcmp() which is a string compare but NULL-safe (unlike strcmp).

  if (g_strcmp0 (priv->header, header) != 0)
    {
      g_free (priv->header);
      priv->header = g_strdup (header);
      g_object_notify_by_pspec (G_OBJECT (self), properties [PROP_HEADER]);
    }

@@ +98,3 @@
+  g_return_if_fail (IDE_IS_DOCUMENTATION_PROPOSAL (self));
+
+  priv->text = g_strdup (text);

Same string pattern here.

::: libide/documentation/ide-documentation-proposal.h
@@ +35,3 @@
+G_DECLARE_DERIVABLE_TYPE (IdeDocumentationProposal, ide_documentation_proposal, IDE, DOCUMENTATION_PROPOSAL, GObject)
+
+IdeDocumentationProposal *ide_documentation_proposal_new            (gchar                      *url);

const gchar *uri

@@ +40,3 @@
+void                      ide_documentation_proposal_set_text       (IdeDocumentationProposal   *self,
+                                                                     const gchar                *text);
+gchar                    *ide_documentation_proposal_get_header     (IdeDocumentationProposal   *self);

const gchar * return type for all of the following

::: libide/documentation/ide-documentation.c
@@ +99,3 @@
+IdeDocumentationInfo *
+ide_documentation_get_info    (IdeDocumentation        *self,
+                               gchar                   *input,

const gchar *input

::: libide/documentation/ide-documentation.h
@@ +33,3 @@
+                      IdeObject)
+
+IdeDocumentationInfo    *ide_documentation_get_info    (IdeDocumentation        *self,

const gchar *input

::: plugins/devhelp/gbp-devhelp-documentation-provider.c
@@ +180,3 @@
+
+  self->proposal = ide_documentation_proposal_new (self->uri);
+  ide_documentation_proposal_set_header (self->proposal, g_string_free (g_steal_pointer (&header), FALSE));

By stealing the GString here, we leak the result (as set_header() should be expected to copy the string). Instead, just pass the string directly with header->str and text->str.

@@ +198,3 @@
+    return FALSE;
+
+  self->uri = dh_link_get_uri (link);

It looks like self->uri is left as a dangling pointer, so calling g_free() first would potentially become a double free. We should be extra careful to avoid leaving dangling pointers and NULL them after freeing. Being defensive like this helps future contributors avoid miss-using the structure.

Let's use g_clear_pointer (&self->uri, g_free) when possible, and remember to g_free() before assignment of the new value.

@@ +227,3 @@
+
+  parse_succ = xml_parse (self, tokens[0], tokens[1], info);
+  g_free (self->uri);

Try not to leave dangling pointers around, so g_clear_pointer (&self->uri, g_free);

@@ +244,3 @@
+
+  if (parse_succ)
+    ide_documentation_info_take_proposal (info, self->proposal);

Since we are taking the reference to self->proposal, we should clear our pointer to avoid a dangling pointer. Something like:

  ide_documentation_info_take_proposal (info, g_steal_pointer (&self->proposal));

You may also want to either assert or verify that self->proposal is non-NULL.

@@ +263,3 @@
+  context = ide_object_get_context (IDE_OBJECT (self));
+  self->documentation = ide_context_get_documentation (context);
+  self->keyword_model = dh_keyword_model_new ();

Maybe add a GObjectClass.finalize or GObjectClass.dispose which frees the keyword model?

::: plugins/documentation-card/gbp-documentation-card-view-addin.c
@@ +37,3 @@
+
+  guint                 timeout_id;
+  gint                  motion_handler_id;

The documentation for macros is quite poor, but g_signal_connect* returns a gulong.

@@ +97,3 @@
+  self->last_y = y;
+
+  self->timeout_id = 0;

We don't return TRUE from anywhere in this function, so just move the "self->timeout_id = 0;" to line 80 before we can possibly return.

You also might consider using G_SOURCE_REMOVE instead of FALSE, as they are equivalent but one is more obvious what is going on.

@@ +122,3 @@
+  gtk_text_view_get_iter_at_location (GTK_TEXT_VIEW (source_view), &begin, x, y);
+
+  while (g_unichar_islower (gtk_text_iter_get_char (&begin)) || g_unichar_isdigit(gtk_text_iter_get_char (&begin)) || gtk_text_iter_get_char (&begin) == '_')

Maybe break this out into a helper function like:

static gboolean
unichar_issymbol (gunichar ch)
{
  return g_unichar_isalnum (ch) || ch == '_';
}

and then use it from both loops

@@ +131,3 @@
+      break;
+
+  selected_text = gtk_text_buffer_get_text (GTK_TEXT_BUFFER (buffer), &begin, &end, FALSE);

gtk_text_iter_get_slice (&begin, &end); might be a little clearer to read and get the same effect.

@@ +167,3 @@
+                                                  g_object_unref);
+  else
+    search_document_cb (g_object_ref (self));

No need to pass a new reference now that gdk_threads_add_timeout_seconds_full() is used.

@@ +211,3 @@
+  ide_clear_source (&self->timeout_id);
+
+  if (self->motion_handler_id)

You can use "ide_clear_signal_handler (self->editor_view, &self->motion_handler_id);" here if you like instead of the if/disconnect/zero

@@ +215,3 @@
+      g_signal_handler_disconnect (self->editor_view, self->motion_handler_id);
+      self->motion_handler_id = 0;
+    }

Lets also cleanup the popover and editor_view pointers here with something like:

 gtk_widget_destroy (GTK_WIDGET (self->popover));
 self->popover = NULL;
 self->editor_view = NULL;

::: plugins/documentation-card/gbp-documentation-card.c
@@ +138,3 @@
+  self->pointer = gdk_seat_get_pointer (gdk_display_get_default_seat (display));
+
+  card_popup (g_object_ref (self));

No need to pass a reference

@@ +146,3 @@
+  g_return_if_fail (GBP_IS_DOCUMENTATION_CARD (self));
+
+  card_popdown (g_object_ref (self));

No need to pass a reference

Created attachment 357672 [details] [review]
documentation-card: card showing relevant information about a library function

Review of attachment 357672 [details] [review]:

This looks great, I just tested things and I'm very happy how it's working. I'm going to go ahead and merge this now so that, as you continue to iterate, it's easier to see what's being changed.

Some things you might want to work on as time permits:

 - Some whitespace cleanup (space after function names, alignment of {}, etc). I tried to avoid doing nits on this stuff in previous reviews so we didn't detract from the important design issues.
 - We might want to hook into the "request-documentation" signal on the IdeSourceView and show the popover in that case. This allows us to get documentation from keyboard shortcuts.
 - I noticed some warnings when closing the editor view (it looks like the timeout fires later on)


Gdk[ 21504]: CRITICAL: gdk_window_get_display: assertion 'GDK_IS_WINDOW (window)' failed
Gdk[ 21504]: CRITICAL: gdk_display_get_default_seat: assertion 'GDK_IS_DISPLAY (display)' failed
Gdk[ 21504]: CRITICAL: gdk_seat_get_pointer: assertion 'GDK_IS_SEAT (seat)' failed
Gdk[ 21504]: CRITICAL: gdk_window_get_device_position_double: assertion 'GDK_IS_WINDOW (window)' failed

Wooo! Code is in master, and ready for general testing!

I've pushed a small followup commit that should help be defensive against the
warnings I saw. I've noticed another small issue (that is my fault, unrelated
to this code) where I think we are leaking some editor views (meaning we don't
actually call the IdeEditorViewAddin.unload() vfunc).

Attachment 357672 [details] pushed as 59e527d - documentation-card: card showing relevant information about a library function

Created attachment 358729 [details] [review]
documentation-card: card showing relevant information about a library function

Comment on attachment 358729 [details] [review]
documentation-card: card showing relevant information about a library function

Excellent!

Pushed with a small fixup to use GtkPopoverClass.closed vfunc instead of a signal connection.