Created attachment 332170 [details] [review]
Support for GEmblem in IdeTreeNode

Tested this by setting the emblem-field introduced in the patch to a static string and the emblemed icon is created without any issues. However, I am unsure whether this solution works if either the emblem and/or the icon-name is changed during run-time. Would love some feedback on how to test this properly.

Review of attachment 332170 [details] [review]:

Nice first patch!

Based on how I think I need to use this from Builder I've added some alternate API suggestions. Also, since you are just getting started on this codebase I've included some style nits to help you get the hang of our coding style.

Thanks for working on this and sorry for the long cycle for review (I was out all last week).

::: libide/tree/ide-tree-node.c
@@ +980,3 @@
+const gchar *
+ide_tree_node_get_emblem (IdeTreeNode *node)
+{

Needs:

  g_return_val_if_fail (IDE_IS_TREE_NODE (node), NULL);

@@ +985,3 @@
+
+void
+ide_tree_node_set_emblem (IdeTreeNode *node, const gchar *emblem)

Each parameter goes on it's own line (and align parameters like other functions in the file)

@@ +988,3 @@
+{
+  GQuark value = 0;
+

Also add a g_return_if_fail (IDE_IS_TREE_NODE (node)); before dereferencing node.

::: libide/tree/ide-tree-node.h
@@ -66,2 +66,3 @@
 void            ide_tree_node_set_use_markup        (IdeTreeNode            *self,
                                                      gboolean                use_markup);
+const gchar    *ide_tree_node_get_emblem            (IdeTreeNode            *self);

I can think of some cases where we might need more than one emblem.

For example, a file added emblem (as determined by the Version Control system) along with a file has errors emblem as determined by the Diagnostician.

How about something like:

  void ide_tree_node_add_emblem (IdeTreeNode *, const gchar *);
  void ide_tree_node_remove_emblem (IdeTreeNode *, const gchar *);
  void ide_tree_node_clear_emblems (IdeTreeNode *);
  gboolean ide_tree_node_has_emblem (IdeTreeNode *, const gchar *);

::: libide/tree/ide-tree.c
@@ -468,3 +468,3 @@
              gpointer         data)
 {
-  const gchar *icon_name;
+  const gchar *icon_name, *emblem_name;

Each parameter on a new line like:

  const gchar *icon_name;
  const gchar *emblem_name;

@@ -478,3 +478,3 @@
   gtk_tree_model_get (tree_model, iter, 0, &node, -1);
-  icon_name = node ? ide_tree_node_get_icon_name (node) : NULL;
-  g_object_set (cell, "icon-name", icon_name, NULL);
+  emblem_name = ide_tree_node_get_emblem(node);
+  if (emblem_name)

Creating a new GIcon on every render is probably more work than we want to do in the render path.

Instead, how about something like:

  ide_tree_node_get_gicon (IdeTreeNode *)

This would create the new GThemedIcon if necessary, and cache the result for future lookups. Then, whenever :icon-name or an emblem is added, we would invalidated the GIcon (and wait until ide_tree_node_get_gicon() is called again to create the replacement).

Created attachment 332886 [details] [review]
Support for GEmblem in IdeTreeNode

Here is the new implementation which supports multiple emblems and caches the GIcon object rather than re-rendering it every time pixbuf_func is called. I am still new to memory management involving GObjects, thus feedback on the implementation's memory management would be much appreciated.

Review of attachment 332886 [details] [review]:

::: libide/tree/ide-tree-node.c
@@ +59,3 @@
   PROP_ICON_NAME,
+  PROP_ICON,
+  PROP_EMBLEMS,

Drop PROP_EMBLEMS (reasoning below)

@@ +288,3 @@
+  if (!(node->icon) && icon_name)
+    {
+      icon = g_themed_icon_new (icon_name);

icon is leaked

@@ -270,0 +274,22 @@
+ * ide_tree_node_get_gicon:
+ *
+ * Fetch the GIcon, re-render if necessary
... 19 more ...

No need for a (const gchar *) cast, since it is void* and this isn't C++ code (where the cast would be required).

@@ +295,3 @@
+          GIcon *emblem_icon = g_themed_icon_new ((const gchar *) (element->data));
+          GEmblem *emblem = g_emblem_new (emblem_icon);
+          g_emblemed_icon_add_emblem (node->icon, emblem);

emblem is being leaked here

@@ -270,0 +274,25 @@
+ * ide_tree_node_get_gicon:
+ *
+ * Fetch the GIcon, re-render if necessary
... 22 more ...

Just use element->next.

@@ -308,0 +345,18 @@
+void
+ide_tree_node_set_icon (IdeTreeNode *node,
+                       GIcon *icon)
... 15 more ...

Better to open code the list iteration and and hold a pointer to the last element. Then you have O(1) append if the emblem_name is not found.

@@ -308,0 +345,68 @@
+void
+ide_tree_node_set_icon (IdeTreeNode *node,
+                       GIcon *icon)
... 65 more ...

This is incorrect. GList* is not a GObject* and therefore g_set_object() would result in undefined behavior (and eventually a crash).

Instead do something like:

  if (emblems != node->emblems)
    {
      g_list_free_full (self->emblems, g_object_unref);
      self->emblems = g_list_copy_deep (emblems, (GCopyFunc)g_object_ref, NULL);
    }

Drop the use of PROP_EMBLEMS since we don't want that as a property.

@@ -559,0 +667,5 @@
+    case PROP_ICON:
+      g_value_set_object (value, node->icon);
+      break;
... 2 more ...

Drop this, we'd have to either use a Pointer type or create a Boxed type for GList<GObject> as set_object() is incorrect.

@@ -606,0 +722,5 @@
+    case PROP_ICON:
+      ide_tree_node_set_icon (node, g_value_get_object (value));
+      break;
... 2 more ...

And drop this too

@@ -667,0 +793,13 @@
+  /**
+   * IdeTreeNode:icon:
+   *
... 10 more ...

And drop this.

Created attachment 335569 [details] [review]
Support for GEmblem in IdeTreeNode

Sorry for the slow response.
I reworked the patch according to your comments thus dropping the use of PROP_EMBLEM, fixed the memory leaks that you pointed out, and open-coded the list iteration in ide_tree_node_add_emblem() to make append O(1).
I was a bit unsure whether you wanted me to remove the PROP_ICON property all together. I still left it in but changed it to become read-only, since setting the icon-object would make adding and removing emblems quite a bit more complex.

I've been really busy getting 3.22 out the door, but I'll try to review the updated code this week while at LAS GNOME. Thanks again!

Thanks for working on this!

Attachment 335569 [details] pushed as 077fb44 - Support for GEmblem in IdeTreeNode