Created attachment 309073 [details] [review]
patch

This patch implements the feature.


There are couple of API questions for which I would welcome other opinions

1) I added a completion_item_new_with_properties that takes a vararg, since clearly we cannot have constructors for all possible combinations of icon types, text, markup etc. Is the name ok or is it better new_full ?

2) I wonder if we should just deprecate CompletionItem entirely and add a new GtkSourceSimpleProposal. The new class would just have a single new() constructor with the vararg


I like (2) since using "Simple" for a basic concrete class is in line with moder Glib/Gtk conventions, however is quite a bit of API churn for not so much gain... If we go that way, is there an easy way to alias the old CompletionItem to SimpleProposal or we need duplicate code?

Also note that this depends on two gtk patches: #753506 and #753510

Without those patches GtkCellRenderePixbuf is not able to fallback from Pixbuf, to GIcon, to icon-name.


If the patches are not accepted we will need to find a different way... a custom renderer?

I don't like a constructor with a vararg. g_object_new() can be used instead in such cases.

But I agree gtk_source_completion_item_new() is not great. The doc can recommend to use g_object_new().

g_object_new() forces you to pass a GType and is not search/completion friendly.

I think a vararg constructor is a fine convenience for C and introspected languages can easily achieve the same with g_object_new

I think a better API (if we can break the API, or if we create SimpleProposal) is to have a constructor that takes zero arguments, and have setter functions.

Or, instead of having setter functions, say to use g_object_set(), like it is the case for GtkTextTag (see the doc of gtk_text_tag_new()).

A library should try to not add too many exported symbols, since more symbols means more time to link the library. A wrapper around g_object_new() just to avoid the GType parameter is the kind of things that can easily be avoided.

Also, the constructor-wrapper around g_object_new() is not standard. I've never seen such a constructor! Constructors for GtkDialog are different, since it's a printf()-like constructor. The same for g_variant_new(), it takes a format string as a first argument.

So for the patch here, the simplest in my opinion is just to add the properties, add perhaps setters for the different icon properties, and saying to use g_object_new() if the user wants a different kind of icon property. Then for GtkSourceView 4 we can clean-up the API. Anyway for such a simple API, one way or another is almost the same, after all it's just creating a dumb object and assign values to some properties. g_object_new() and g_object_set() are already convenient APIs for the C language.

I dropped the constructor and pushed this.


To work around the GTK bug I used a cell_data_func instead of mapping the attributes directly

The API is good to me. The freeze is monday, so it's better to be sure.

Can you document the priority between the pixbuf, icon-name and gicon, if several are provided? I would write this in the classes/interfaces' main descriptions.

I would document that at the gtksourceview level you need to set only one of pixbuf/icon-name/gicon.

The fallback order is up to gtk and I would consider it an implementation detail

It's equal to me, but one way or the other it should be documented.