I meant:
gtk_source_gutter_insert (source_gutter,
                          my_gutter_renderer_new ("prop1", val1,
                                                  "prop2", val2
                                                  NULL));

I agree, it is not a common way to do it. Feel free to provide a patch for it, as we still can break the api without any problem.

BTW I'd rather this approach:

GtkSourceGutterRenderer* rend = my_gutter_renderer_new ("prop1", val1,
                                                        "prop2", val2
                                                        NULL);
gtk_source_gutter_insert (source_gutter, rend);
g_object_unref (rend);

and being the insert the one that takes care of reffing internally. I think it is more correct with other gtk+ apis around. Comments here?

Well, here I don't agree. Your version assumes that GtkSourceGutterRenderer would derive from GObject instead of GInitiallyUnowned, so creator owns a reference, which later needs to be dropped. That would be inconsistent with earlier used GtkCellRenderer.

ups, right, forgot about the GInitiallyUnowned.

We did this so that the renderers can be easily constructed with the 'view' and 'window-type' construct only properties correctly set without having implementers specifying them when creating the renderer. This works out neat in C, but I guess it's indeed not very cool for bindings. I don't know what the best way is to fix this.

Well, seems that instead of changing the current API, an addition would be best. Sort of:

void gtk_source_gutter_insert_renderer (GtkSourceGutter* gutter,
                                        GtkSourceGutterRenderer* renderer,
                                        int position);

C developers using this API should then do something like this:

GtkSourceView* view;
GtkTextWindowType window_type;
GtkSourceGutter* gutter = get_gutter ();
int position = get_position ();
int foo = get_foo ();
gchar* bar = get_bar ();

g_object_get (gutter,
              "view", &view,
              "window-type", window_type,
              NULL);
 
GtkSourceGutterRenderer renderer* = my_renderer_new ("view", view,
                                                     "window-type", window_type,
                                                     "foo", foo,
                                                     "bar", bar,
                                                     NULL);

gtk_source_gutter_insert_renderer (gutter, renderer, position);

g_object_unref (view);
g_free (bar);

Looks like an awkward API to use, so probably no one will use it in favor of already existing functions, but it is bindings friendly. With it I could do something like this:

class SourceGutterRenderer : public Gtk::Object
{
protected:
  SourceGutterRenderer (const Glib::ConstructParams& construct_params)
  : Gtk::Object (construct_params)
  {
    // This is called by a SourceGutterRenderer derived type which is itself
    // a wrapper around some C type (like SourceGutterRenderer{Pixbuf,Text}).
    // So derived class should call inserting itself into a gutter.
  }
  
  SourceGutterRenderer (const Glib::RefPtr<SourceGutter>& gutter,
                        int position)
  : Gtk::Object (Glib::ConstructParams
                  (some sort of class type,
                  "window-type",
                  gutter->property_window_type.get_value (),
                  "view",
                  gutter->property_view.get_value()))
  {
    // This constructor would be called by our own implementation of
    // SourceGutterRenderer. Here I insert this object to the gutter.
    gtk_source_gutter_insert_renderer (gutter->gobj (), gobj (), position);
  }
...
};

class SourceGutterRendererText : public SourceGutterRenderer
{
public:
  SourceGutterRendererText (const Glib::RefPtr<SourceGutter>& gutter,
                            int position)
  : SourceGutterRenderer (Glib::ConstructParams
                           (some sort of class type
                            "window-type",
                            gutter->property_window_type.get_value (),
                            "view",
                            gutter->property_view.get_value()))
  {
    gtk_source_gutter_insert_renderer (gutter->gobj (), gobj (), position);
  }
};

class MyRenderer : public SourceGutterRenderer
{
  MyRenderer(const Glib::RefPtr<SourceGutter>& gutter,
             int position,
             int foo,
             const Glib::ustring& bar)
  : SourceGutterRenderer (gutter, position)
  {
    // set foo and bar. renderer is already inserted into gutter.
  }
};

That way I can omit wrapping any renderer inserting functions, especially that gutter renderers are meant to be inserted into and owned by gutter from the start. The difference would be that in C renderers are constructed inside insertion and in C++ renderers are inserted inside constructors. The only wish I could have is adding getters for GtkSourceGutter's "view" and "window-type" properties.

Created attachment 175868 [details] [review]
Adds gtk_source_gutter_insert_renderer () function.

I was wondering if I should add another check in gtk_source_gutter_insert_renderer ():

g_return_if_fail (g_object_is_floating (renderer));

But that could be probably problematic.

Also, should it return anything? A gboolean if operation succeeded? Or passed renderer?

Jesse: why do renderers need to have "view" and "window-type" as construct properties?

They look like something that should be set only when the renderer is inserted in a gutter, not when it is constructed. I think that we could have the gutter that sets those two properties of the renderer when gutter_insert is called. At that point we can use the usual api where insert takes a renderer object and sinks it.

Krzesimir: do you feel like giving this a try?

paolo: the idea was that the only way to construct a renderer is through the gutter API, so that we can have the control over these properties, without having the implementer worry about that. And this resulted from the idea that from a design point of view it's nice to have the view and window-type as construct-only properties. Of course, we can simply change this (the only thing that I like about marking things construct-only is that the developer automatically knows this is something that can't be changed at run time).

we could have "view" and "window-type" not be public writable at all and just have a private _renderer_inserted() that is called by the gutter when the renderer is inserted.

I'm fine with that, maybe with a check that 'view' and 'window-type' are not set yet.

(In reply to comment #9)
> Krzesimir: do you feel like giving this a try?

Sure, I'll post a patch when done.

Created attachment 176028 [details] [review]
Changes gtk_source_gutter_insert() prototype.

Attaching a patch doing the stuff mentioned in earlier posts. I have some doubts:
1. Should gtk_source_gutter_insert() return anything?
2. Should a try to insert a previously inserted renderer be treated as a programming error (that is: should it be checked in g_return_if_fail())?)
3. I am wondering if GtkSourceGutterRenderer{Pixbuf,Text}s' _new functions have to be changed in any way?

Review of attachment 176028 [details] [review]:

Some minor comments inline. One thing that I think is missing is to set 'view' and 'window-type' to NULL and PRIVATE when removing a renderer from the gutter.

::: gtksourceview/gtksourcegutter.c
@@ +660,2 @@
 	{
+		return FALSE;

Please make this a g_return_val_if_fail because it will be more informative for a programmer to have a warning being printed

::: gtksourceview/gtksourcegutterrenderer.c
@@ +1224,3 @@
 
+void
+_gtk_source_gutter_renderer_insert (GtkSourceGutterRenderer *renderer,

Maybe rename this to _gtk_source_gutter_renderer_set_view

@@ +1228,3 @@
+                                    GtkTextWindowType        window_type)
+{
+	if (renderer->priv->view)

Not sure you really need this check, a return_if_fail would be better I think

::: gtksourceview/gtksourceview.c
@@ +937,3 @@
+	                                          "xalign", 1.0,
+	                                          "xpad", 3,
+	                                          NULL);

I think I prefer gtk_source_gutter_renderer_lines_new, followed by a g_object_set

@@ +947,3 @@
+	                                           "yalign", 0.5,
+	                                           "xalign", 0.5,
+	                                           NULL);

Same here

Created attachment 176029 [details] [review]
Changes gtk_source_gutter_insert() prototype - corrected.

Attached corrected patch. It also plugs to memleaks in removing and reordering renderers.

Erm, two memleaks.

Friendly ping. :)

Review of attachment 176029 [details] [review]:

I glanced at the patch and it looks good to me, just a minor nitpick below.

If Jesse I'd no objections I'd say to commit

::: gtksourceview/gtksourcegutter.c
@@ +162,2 @@
 static Renderer *
 renderer_new (GtkSourceGutter         *gutter,

I'd change the name of this function, is not a "new" anymore

I would leave it as it is, because it is indeed "new", but for internal Renderer struct. Note that there is also corresponding "free" function for it below.

Looks good to me, good catch with the memleaks. Commit it!

Pushed to master. Thanks.

BTW I'm getting some warnings due to the gutter patch in the test-widget.c

Hm, I don't remember any warnings there, but I'll see to it today.

I got no warnings:
make[2]: Entering directory `/home/kudi/usr/src/gtksourceview-3/tests'
  CC     test-languagemanager.o
  CC     test-widget.o
  CC     test-completion.o
  CC     test-printcompositor.o
  CC     test-buffer.o
  GEN    .gitignore
  CCLD   test-widget
  CCLD   test-completion
  CCLD   test-languagemanager
  CCLD   test-printcompositor
  CCLD   test-buffer
make[2]: Leaving directory `/home/kudi/usr/src/gtksourceview-3/tests'

There are actually anyway issues. For example, the line renderer has a constructed function, which expected the view to be set, but now that is no longer the case. I'd suggest to add 'view' as a parameter to lines_new (also check the other renderers maybe).

btw, the warnings are when you execute the test-widget or any other widget using gtksourceview.

I see - there is problem with connecting to "notify::buffer" signal. Maybe it should be moved from constructed to the virtual function called by a gutter when it is adding or removing a renderer to itself? Sort of:

void gtk_source_gutter_renderer_change_view (GtkSourceGutterRenderer *renderer,
                                             GtkTextView             *view,
                                             GtkTextWindowType        win_type);

Inside that function we could connect or disconnect or both depending on passed view. Or any other action a renderer needs.

Or other idea: renderer could have "gutter" property instead of "view" and "window-type" ones? Having a gutter we could then get view and window_type from it. Then such virtual function could be:
void gtk_source_gutter_renderer_change_gutter (GtkSourceGutterRenderer *renderer,
                                               GtkSourceGutter         *gutter);

Having it would allow us to remove a renderer from left gutter and place it into the right one or maybe into the one belonging to other view.

If that is not acceptable then I'll prepare a patch adding a "view" and maybe "window-type" parameters to lines_new and probably pixbuf_new for consistency.

I tried writing a patch yesterday which gives additional "view" parameter to _new functions of GtkSourceGutterRendererLines and GtkSourceGutterRendererMarks. Problems:
- "view" and probably "window-type" will have to be again writable.
- double disposal of GtkSourceView (first because of calling gtk_widget_destroy() and second because of dropping its reference count to 0, when disposing last renderer, which is disposed in gutter, which is disposed in view. GtkSourceView is not protected against such situation, and in the end test-widget spews warnings while closing about destroying something which is not an instance or something.
Now in gtk_source_view_dispose() there is:
if (view->priv->left_gutter)
{
  g_object_unref (view->priv->left_gutter);
  view->priv->left_gutter = NULL;
}
and (probably) should be:
if (view->priv->left_gutter)
{
  GtkSourceGutter *gutter = view->priv->left_gutter;
  view->priv->left_gutter = NULL;
  g_object_unref (gutter);
}
I'll check all classes' dispose() functions whether they handle being disposed again while being already during dispose correctly.
- "notify::buffer" is disconnected too late in lines and marks renderers and that leads to segfault, because buffer is created on destroyed textview - that was a problem also with GtkSourceView widget earlier. So this notification should be disconnected when unsetting view and that can be solved nicely by using GtkSourceGutterRenderer's new virtual function called by gutter when removing a renderer.

Created attachment 176647 [details] [review]
Use notify to inform gutter renderers of view change

Created attachment 176664 [details] [review]
Better management of view/buffer for gutters

Review of attachment 176664 [details] [review]:

One comment not related with the patch.

::: gtksourceview/gtksourcegutterrendererlines.c
@@ +29,3 @@
 	gint num_line_digits;
 
 	guint changed_handler_id;

shouldn't this be a glong?

Created attachment 176694 [details] [review]
Quite a large patch.

This is my patch I was pitching this week, but mostly yesterday, because earlier my day time job eaten all the time.

It changes some stuff, so maybe it should be splitted into some smaller patches.

Gutter renderers now keep gutter instead of view and window type.
Added two sort of private virtual functions for actions after setting a gutter and before unsetting a gutter.
Some anti-double-unref changes in dispose functions of view and gutter.
The probably ugly thing is that introduced some circular dependencies, so in some places I used struct _GtkSourceFoo instead of GtkSourceFoo.

If you prefer Jeese's patch then no problem for me. Meddling with it was fun.

I committed my patch before I saw your patch just so that we have current master at least working again. I think I prefer my patch, but I'm open for suggestions or arguments why one is better than the other.

It is hard for me to find arguments about my patch being better than yours. The only I could find is that with current patch I get a warning in test-widget:

(lt-test-widget:11926): GLib-GObject-WARNING **: instance of invalid non-instantiatable type `(null)'

(lt-test-widget:11926): GLib-GObject-CRITICAL **: g_signal_handler_disconnect: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

This means that a pointer we get is not NULL, but points to rubbish. It is issued in gutter_renderer_change_buffer() of GtkSourceGutterRendererLines. Probably GtkSourceGutterRenderer should have a weak ref or just weak pointer on the priv->buffer, so it could set to NULL when this is finalized. And such situation happens on the start:
gtk_source_view_new_with_buffer() first creates a new view with some default buffer. Renderers keep pointer to that buffer. Then in this function gtk_text_view_set_buffer() is called, so default buffer is disposed and finalized, but renderers don't know about it.

Also I strived to lower some redundancy - renderers keep reference to gutter, gutter keeps reference to view. Of course view also keeps reference to gutter, and gutter to renderers, so view breaks such reference cycles by calling g_object_run_dispose() on its gutters.

And having two virtual functions for gutter (or view) changes (post set and pre unset) in renderers allows finer tuning of renderers behavior. At the beginning I also used only one virtual function (change_view() or something), but I got some warnings because at one point I expected that priv->foo will still point to old foo, but in other place code expected priv->foo to point to new foo. Sorry for lack of details - now all of this is messed in my head.

I've also seen the warning, it should not be hard to fix though. I'm generally not a fan of direct cyclic dependencies, as such prefer to have a view in the gutter renderer and not a gutter. Note that it's not that much better because we currently have a GtkTextView as parameter for the gutter renderer to avoid need for forward declaring the GtkSourceView, bleh, C.

Wrt two virtual functions, pre and post, I think that's simply not needed. Even if it might give more fine-tuning, it seems hardly necessary.

Created attachment 176744 [details] [review]
Fixes the warning about invalid instance by making a weak pointer to buffer.

Ok, that's fine.

I'm attaching a patch fixing the warning.

Pushed the patch by accident. Anyway, I think it is fixed now.