custom cell renderer start_editing refcount

[Kevin Ryde <user42 zip com au>]

I had a go at making an editable cell renderer and I find the destroy
signal on the GtkEntry "editable" widget I return from START_EDITING is
not called.

Sample program below, click alterately between the two cells to edit,
and notice the destroy for $entry is not called.

I suspect the SvREFCNT_inc in gtk2perl_cell_renderer_start_editing()
keeps it alive forever.  The best I can nut out is that there ought to
be a floating reference ready for the View container to take over.  The
normal Gtk2::Entry->new has sunk the initial so I think another should
be created.

With the change below the destroy runs as I hoped.  Does it sound about
right? -- before digging out what it should look like for glib pre-2.10.


(The "Example 3" in the GObject reference is slightly related to what I
think is going on.  It shows sinking the floating ref and then putting
it back - though the unref there may be 1 too many.)

Incidentally, if you create the editable by chaining up to
$renderer->SUPER::START_EDITING() the destroy runs ok.  But I believe
that's only because the chain-up code at the end of GtkCellRenderer.xs
uses newSVGtkCellEditable_ornull() to wrap, and that macro does
gperl_new_object() with FALSE for "own", so it doesn't sink.  That
leaves the object with a refcount of 2 -- 1 still floating and 1 from
the wrapper -- so the condition for the SvREFCNT_inc bit in
gtk2perl_cell_renderer_start_editing() is not met.  With the change I'm
proposing a count of 2 with 1 floating and 1 from the wrapper would be
the normal situation; with the 1 from the wrapper quite possibly dropped
at the FREETMPS stage.


package My::CellRendererText;
use strict;
use warnings;
use Gtk2;
use Scalar::Util;

print Gtk2->VERSION,"\n";

use Glib::Object::Subclass
  'Gtk2::CellRendererText';

sub START_EDITING {
  my ($self, @args) = @_;

  my $entry = Gtk2::Entry->new;
  print "START_EDITING with $entry\n";
  $entry->signal_connect (destroy => \&_do_destroy);
  return $entry;
}

sub _do_destroy {
  my ($entry) = @_;
  print "destroy $entry\n";
}

package main;
use strict;
use warnings;
use Gtk2 '-init';

my $liststore = Gtk2::ListStore->new ('Glib::String');
$liststore->set_value ($liststore->append, 0 => 'abc');
$liststore->set_value ($liststore->append, 0 => 'def');

my $toplevel = Gtk2::Window->new('toplevel');
$toplevel->signal_connect (destroy => sub { Gtk2->main_quit });

my $iconview = Gtk2::IconView->new;
$iconview->set (model => $liststore);
$toplevel->add ($iconview);

my $cellrenderer = My::CellRendererText->new (editable => 1);
$iconview->pack_start ($cellrenderer, 0);
$iconview->add_attribute ($cellrenderer, text => 0);

$toplevel->show_all;
Gtk2->main;
exit 0;

Index: GtkCellRenderer.xs
===================================================================
--- GtkCellRenderer.xs	(revision 2095)
+++ GtkCellRenderer.xs	(working copy)
@@ -272,6 +272,53 @@
 		sv = POPs;
 		if (gperl_sv_is_defined (sv)) {
 			editable = SvGtkCellEditable (sv);
+
+#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_is_floating (editable)) {
+				g_object_ref (editable);
+				g_object_force_floating (G_OBJECT (editable));
+			}
+#else
 			/* 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
@@ -288,6 +335,7 @@
 			    SvREFCNT (SvRV (sv)) == 1) {
 				SvREFCNT_inc (SvRV (sv));
 			}
+#endif
 		} else {
 			editable = NULL;
 		}