Re: custom cell renderer start_editing refcount

[Kevin Ryde <user42 zip com au>]

I ended up with this for the tests, it's not terribly systematic.  It
might be better chopped around to loop over all the combinations of
builtin renderer, custom renderer, newly constructed editable or chained
superclass, then in turn reached by viewer click, start_editing method
or START_EDITING chain up ...

Index: xs/GtkCellRenderer.xs
===================================================================
--- xs/GtkCellRenderer.xs	(revision 2104)
+++ xs/GtkCellRenderer.xs	(working copy)
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003-2004 by the gtk2-perl team (see the file AUTHORS)
+ * Copyright (c) 2003-2004, 2009 by the gtk2-perl team (see the file AUTHORS)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -272,22 +272,57 @@
 		sv = POPs;
 		if (gperl_sv_is_defined (sv)) {
 			editable = SvGtkCellEditable (sv);
-			/* if the object returned here was newly created by
-			 * the called code, then the wrapper (pointed to by
-			 * sv) is the owner of the object.  if there are no
-			 * other references to the wrapper, the call to
-			 * FREETMPS below will actually result in finalization
-			 * of the GObject, which means we'll return a bad
-			 * pointer to our caller.  to prevent this situation,
-			 * we need to add a ref to the wrapper (that is, the
-			 * blessed sv stored in the GObject, not the reference
-			 * sv) to keep it alive across FREETMPS and thus
-			 * prevent premature destruction.
+#if GLIB_CHECK_VERSION (2, 10, 0)
+			/* (*start_editing)() is basically a constructor and
+			 * as such should return an object with a floating
+			 * reference for the caller to take over.
+			 *
+			 * For GtkTreeView and GtkIconView for example that
+			 * ref is sunk when gtk_tree_view_put() or
+			 * gtk_icon_view_put() call gtk_widget_set_parent()
+			 * to add "editable" as one of their container
+			 * children.  (Eventually to be dereffed in the
+			 * usual way by gtk_container_remove() from
+			 * gtk_tree_view_remove_widget() or
+			 * gtk_icon_view_remove_widget() at the end of
+			 * editing.)
+			 *
+			 * Perl code constructors like Gtk2::Foo->new or
+			 * Glib::Object->new sink any initial floating
+			 * reference when making the wrapper (either if
+			 * constructing in the START_EDITING code or from
+			 * something made or wrapped previously).  So must
+			 * explicitly add a floating ref for GtkTreeView etc
+			 * to take over.
+			 *
+			 * If START_EDITING code gives a new object in "sv"
+			 * and it's used nowhere else then FREETMPS below
+			 * will SvREFCNT_dec it to zero and send it to the
+			 * usual Glib::Object::DESTROY.  If there wasn't a
+			 * floating ref added here on the GObject then that
+			 * GObject would be destroyed before we ever got to
+			 * return it.  With the extra floating ref the
+			 * wrapper converts to undead (ie. unused from perl
+			 * for the time being) and the GObject has a
+			 * refcount of 1 and the floating flag set.
+			 *
+			 * It's conceivable there could be a floating ref
+			 * already at this point.  That was the case in the
+			 * past from chained-up perl SUPER::START_EDITING
+			 * for instance.  Though it's abnormal let's assume
+			 * any floating ref here is meant for the caller to
+			 * take over and therefore should be left unchanged.
 			 */
-			if (G_OBJECT (editable)->ref_count == 1 &&
-			    SvREFCNT (SvRV (sv)) == 1) {
-				SvREFCNT_inc (SvRV (sv));
+			if (! g_object_is_floating (editable)) {
+				g_object_ref (editable);
+				g_object_force_floating (G_OBJECT (editable));
 			}
+#else
+			if (! GTK_OBJECT_FLOATING (editable)) {
+				gtk_object_ref (GTK_OBJECT (editable));
+				GTK_OBJECT_SET_FLAGS (editable, GTK_FLOATING);
+			}
+#endif
 		} else {
 			editable = NULL;
 		}
@@ -540,8 +575,26 @@
 	GdkRectangle         * cell_area
 	GtkCellRendererState   flags
 
+## gtk_cell_renderer_start_editing() is normally a constructor,
+## returning a widget with a floating ref ready for the caller to take
+## over.  But the generated typemap for "interface" objects like
+## GtkCellEditable only treats it as GObject and doesn't sink when
+## making the perl wrapper, so cast up to GtkWidget to get that.
+## GtkWidget is a requirement of GtkCellEditable, so "editable" is
+## certain to be a widget.
+##
+## The returned widget is normally about to be put in a container
+## anyway, which sinks any floating ref, but sink it now to follow the
+## general rule that wrapped widgets at the perl level don't have a
+## floating ref left.  In particular this means if you start_editing()
+## and then strike an error or otherwise never add it to a container
+## it won't be a memory leak.
+##
 ##GtkCellEditable* gtk_cell_renderer_start_editing (GtkCellRenderer *cell, GdkEvent *event, GtkWidget *widget, const gchar *path, GdkRectangle *background_area, GdkRectangle *cell_area, GtkCellRendererState flags)
-GtkCellEditable_ornull *
+=for apidoc
+=for signature celleditable or undef = $cell->start_editing ($event, $widget, $path, $background_area, $cell_area, $flags)
+=cut
+GtkWidget_ornull *
 gtk_cell_renderer_start_editing (cell, event, widget, path, background_area, cell_area, flags)
 	GtkCellRenderer      * cell
 	GdkEvent             * event
@@ -550,6 +603,10 @@
 	GdkRectangle         * background_area
 	GdkRectangle         * cell_area
 	GtkCellRendererState   flags
+CODE:
+	RETVAL = GTK_WIDGET (gtk_cell_renderer_start_editing (cell, event, widget, path, background_area, cell_area, flags));
+OUTPUT:
+	RETVAL
 
 #if GTK_CHECK_VERSION (2, 4, 0)
 
@@ -757,7 +814,12 @@
 								SvGdkRectangle_ornull (ST (5)),
 								SvGtkCellRendererState (ST (6)));
 			EXTEND (SP, 1);
-			PUSHs (sv_2mortal (newSVGtkCellEditable_ornull (editable)));
+			/* Note newSVGtkWidget here instead of
+			 * newSVGtkCellEditable so as to take ownership of
+			 * any floating ref.  See comments with
+			 * gtk_cell_renderer_start_editing() above.
+			 */
+			PUSHs (sv_2mortal (newSVGtkWidget_ornull (GTK_WIDGET (editable))));
 		}
 		break;
 	    default:
Index: t/GtkCellRenderer.t
===================================================================
--- t/GtkCellRenderer.t	(revision 2104)
+++ t/GtkCellRenderer.t	(working copy)
@@ -2,7 +2,7 @@
 
 # $Id$
 
-use Gtk2::TestHelper tests => 13;
+use Gtk2::TestHelper tests => 20;
 use strict;
 
 package Mup::CellRendererPopup;
@@ -44,6 +44,23 @@
 sub on_activate { $hits_compat{activate}++;  shift->parent_activate (@_) }
 sub on_start_editing { $hits_compat{edit}++;  shift->parent_start_editing (@_) }
 
+package My::CellRendererNewEntry;
+use strict;
+use warnings;
+use Glib::Object::Subclass 'Gtk2::CellRendererText';
+my %hits_newentry;
+sub INIT_INSTANCE { $hits_newentry{init}++; }
+sub GET_SIZE { $hits_newentry{size}++;  shift->SUPER::GET_SIZE (@_) }
+sub RENDER { $hits_newentry{render}++;  shift->SUPER::RENDER (@_) }
+sub ACTIVATE { $hits_newentry{activate}++;  shift->SUPER::ACTIVATE (@_) }
+sub START_EDITING { $hits_newentry{edit}++;
+                    my $entry = Gtk2::Entry->new;
+                    $entry->signal_connect (destroy => \&_editable_destroy);
+                    return $entry;
+                  }
+sub _editable_destroy { $hits_newentry{editable_destroy}++ }
+ 
+
 ##########################################################################
 # driver code
 package main;
@@ -106,6 +123,25 @@
 	}, $model);
 $treeview->append_column ($column_compat);
 
+#
+# new entry cell renderer
+#
+my $renderer_newentry = My::CellRendererNewEntry->new;
+$renderer_newentry->set (mode => 'editable');
+my $column_newentry = Gtk2::TreeViewColumn->new_with_attributes
+    ('new-entry', $renderer_newentry, text => 0,);
+$treeview->append_column ($column_newentry);
+
+#
+# plain core CellRendererText
+#
+my $renderer_text = Gtk2::CellRendererText->new;
+$renderer_text->set (editable => 1);
+my $column_text = Gtk2::TreeViewColumn->new_with_attributes
+    ('core-text', $renderer_text, text => 0,);
+$treeview->append_column ($column_text);
+
+
 ##########################################################################
 
 $vbox->pack_start ($treeview, 1, 1, 0);
@@ -128,7 +164,33 @@
 
 $renderer->render ($window->window, $treeview, $rect, $rect, $rect, [qw(sorted prelit)]);
 ok(!$renderer->activate ($event, $treeview, "0", $rect, $rect, qw(selected)));
-isa_ok ($renderer->start_editing ($event, $treeview, "0", $rect, $rect, qw(selected)), "Gtk2::Entry");
+{
+  my $editable = $renderer->start_editing ($event, $treeview, "0", $rect, $rect, qw(selected));
+  isa_ok ($editable, "Gtk2::Entry");
+  my $destroyed = 0;
+  $editable->signal_connect (destroy => sub { $destroyed = 1 });
+  undef $editable;
+  is ($destroyed, 1,
+      'editable from start_editing using SUPER::START_EDITING destroyed when forgotten');
+}
+{
+  my $editable = $renderer_newentry->start_editing ($event, $treeview, "0", $rect, $rect, qw(selected));
+  isa_ok ($editable, "Gtk2::Entry");
+  my $destroyed = 0;
+  $editable->signal_connect (destroy => sub { $destroyed = 1 });
+  undef $editable;
+  is ($destroyed, 1,
+      'editable from start_editing using Gtk2::Entry->new destroyed when forgotten');
+}
+{
+  my $editable = $renderer_text->start_editing ($event, $treeview, "0", $rect, $rect, qw(selected));
+  isa_ok ($editable, "Gtk2::Entry");
+  my $destroyed = 0;
+  $editable->signal_connect (destroy => sub { $destroyed = 1 });
+  undef $editable;
+  is ($destroyed, 1,
+      'editable from start_editing on core GtkCellRendererText destroyed when forgotten');
+}
 
 $renderer->set_fixed_size (23, 42);
 is_deeply([$renderer->get_fixed_size], [23, 42]);
@@ -154,13 +216,24 @@
 	                       $column, 1);
 	$treeview->set_cursor (Gtk2::TreePath->new_from_string ('0'),
 	                       $column_compat, 1);
+	$treeview->set_cursor (Gtk2::TreePath->new_from_string ('0'),
+	                       $column_newentry, 1);
+
+        # and not editing any more
+	$treeview->set_cursor (Gtk2::TreePath->new_from_string ('0'),
+	                       $column_text, 0);
 };
 
 
 is_deeply ([ sort keys %hits ], [ qw/edit init render size/ ], 'callbacks encountered');
 is_deeply ([ sort keys %hits_compat ], [ qw/edit init render size/ ], 'callbacks encountered');
 
+# one start_editing programmatically, and one from set_cursor of $treeview
+is ($hits_newentry{edit}, 2);
+is ($hits_newentry{editable_destroy}, 2,
+   'the otherwise unreferenced GtkEntry should be destroyed');
+
 __END__
 
-Copyright (C) 2003-2005 by the gtk2-perl team (see the file AUTHORS for the
+Copyright (C) 2003-2005, 2009 by the gtk2-perl team (see the file AUTHORS for the
 full list).  See LICENSE for more information.