Re: cellrenderer GET_SIZE super method

[Torsten Schoenfeld <kaffeetisch gmx de>]

Kevin Ryde wrote:
> I struck a bit of trouble with Gtk2::Ex::Datasheet::DBI not having a
> GET_SIZE method in its Gtk2::Ex::Datasheet::DBI::CellRendererText
> package.  (RT 36996)
> 
> Nosing around the GtkCellRenderer.xs fallback GET_SIZE, I wondered if it
> could look up through the classes of the given object instead of using
> "caller".  The first superclass which had class->get_size func not equal
> to gtk2perl_cell_renderer_get_size would be the one to call, would it?

This is actually a very long-standing problem.  And your suggestion of looking
for the first non-Perl class in the ancestry might very well solve most of the
issues!

The attached caller-v1.patch was my first attempt at this from four years (!)
ago.  It did seem to fix most of the problems I saw, but it fails in one
important case, as muppet explained back then: you get endless loops when you
have a hierarchy where a Perl class inherits from a Perl class.  Something
like this:

  ...
  +- GtkCellRenderer
     +- Foo::RendererOne
        +- Foo::RendererTwo

When something calls the get_size vfunc on a Foo::RendererTwo,
Foo::RendererTwo::GET_SIZE is invoked if present.  If it's not present, or if
it chains up, then Foo::RendererOne::GET_SIZE is called.  If this one is not
present, or if it chains up, our fallback Gtk2::CellRenderer::GET_SIZE kicks
in.  The fallback uses G_OBJECT_TYPE() on the object, and since
G_OBJECT_TYPE() always returns the bottommost type, this yields the type of
Foo::RendererTwo.  The fallback then finds out that Foo::RendererOne is the
parent of Foo::RendererTwo and goes on to call Foo::RendererOne::GET_SIZE
which eventually gets us into Gtk2::CellRenderer::GET_SIZE again.  Endless loop.

So, if both subclasses either don't provide GET_SIZE or if they both chain up
so that Gtk2::CellRenderer::GET_SIZE is reached, we get an endless loop with
caller-v1.patch due to G_OBJECT_TYPE() always returning the bottommost type.
This was the original reason of using caller() instead of simply G_OBJECT_TYPE().

But as far as I can tell, your suggestion of chaining up to the first parent
that is not a Perl subclass does indeed fix this.  It's implemented and tested
in caller-v2.patch.  The patch contains some more rationale of why I think
this works.  The patch also contains a FIXME comment detailing a scenario
where this approach still results in an endless loop: a Perl class that
inherits from a C class that inherits from a Perl class.

But since this scenario is rather contrived, and since the current
caller()-using code will fail in this case just as well, and since your new
suggestion does fix another problem, I think we should commit caller-v2.patch.

Daniel, Kevin: can you verify that caller-v2.patch fixes the
Gtk2::Ex::Datasheet::DBI::CellRendererText issue even when you revert the
GET_SIZE "no-op" fix you applied already?

muppet: what do you think?

-- 
Bye,
-Torsten

Index: xs/GtkCellRenderer.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/xs/GtkCellRenderer.xs,v
retrieving revision 1.32
diff -u -d -p -r1.32 GtkCellRenderer.xs
--- xs/GtkCellRenderer.xs	7 Jan 2008 19:54:49 -0000	1.32
+++ xs/GtkCellRenderer.xs	29 Jun 2008 14:41:10 -0000
@@ -653,23 +653,16 @@ GET_SIZE (GtkCellRenderer * cell, ...)
 	Gtk2::CellRenderer::parent_start_editing = 7
     PREINIT:
 	GtkCellRendererClass * class;
-	GType thisclass, parent_class;
-	SV * saveddefsv;
+	GType this, parent;
     PPCODE:
-	/* may i ask who's calling? */
-	saveddefsv = newSVsv (DEFSV);
-	eval_pv ("$_ = caller;", 0);
-	thisclass = gperl_type_from_package (SvPV_nolen (DEFSV));
-	SvSetSV (DEFSV, saveddefsv);
-	if (!thisclass)
-		thisclass = G_OBJECT_TYPE (cell);
 	/* look up his parent */
-	parent_class = g_type_parent (thisclass);
-	if (! g_type_is_a (parent_class, GTK_TYPE_CELL_RENDERER))
+	this = G_OBJECT_TYPE (cell);
+	parent = g_type_parent (this);
+	if (! g_type_is_a (parent, GTK_TYPE_CELL_RENDERER))
 		croak ("parent of %s is not a GtkCellRenderer",
-		       g_type_name (thisclass));
+		       g_type_name (this));
 	/* that's our boy.  call one of his functions. */
-	class = g_type_class_peek (parent_class);
+	class = g_type_class_peek (parent);
 	switch (ix) {
 	    case 4: /* deprecated parent_get_size */
 	    case 0: /* GET_SIZE */

? t/GtkCellRendererIface-Chaining.t
Index: xs/GtkCellRenderer.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/xs/GtkCellRenderer.xs,v
retrieving revision 1.32
diff -u -d -p -r1.32 GtkCellRenderer.xs
--- xs/GtkCellRenderer.xs	7 Jan 2008 19:54:49 -0000	1.32
+++ xs/GtkCellRenderer.xs	29 Jun 2008 16:52:00 -0000
@@ -652,36 +652,63 @@ GET_SIZE (GtkCellRenderer * cell, ...)
 	Gtk2::CellRenderer::parent_activate      = 6
 	Gtk2::CellRenderer::parent_start_editing = 7
     PREINIT:
-	GtkCellRendererClass * class;
-	GType thisclass, parent_class;
-	SV * saveddefsv;
+	GtkCellRendererClass *parent_class = NULL;
+	GType this, parent;
     PPCODE:
-	/* may i ask who's calling? */
-	saveddefsv = newSVsv (DEFSV);
-	eval_pv ("$_ = caller;", 0);
-	thisclass = gperl_type_from_package (SvPV_nolen (DEFSV));
-	SvSetSV (DEFSV, saveddefsv);
-	if (!thisclass)
-		thisclass = G_OBJECT_TYPE (cell);
-	/* look up his parent */
-	parent_class = g_type_parent (thisclass);
-	if (! g_type_is_a (parent_class, GTK_TYPE_CELL_RENDERER))
-		croak ("parent of %s is not a GtkCellRenderer",
-		       g_type_name (thisclass));
-	/* that's our boy.  call one of his functions. */
-	class = g_type_class_peek (parent_class);
+	/* look up the parent.
+	 *
+	 * FIXME: this approach runs into an endless loop with a hierarchy
+	 * where a Perl class inherits from a C class which inherits from a
+	 * Perl class.  Like this:
+	 *
+	 *   ...
+	 *   +- GtkCellRenderer
+	 *      +- Foo::RendererOne		(Perl subclass)
+	 *         +- FooRendererTwo		(C subclass)
+	 *            +- Foo::RendererThree	(Perl subclass)
+	 *
+	 * yes, this is contrived.  But possible!
+	 */
+	this = G_OBJECT_TYPE (cell);
+	while ((parent = g_type_parent (this))) {
+		if (! g_type_is_a (parent, GTK_TYPE_CELL_RENDERER))
+			croak ("parent of %s is not a GtkCellRenderer",
+			       g_type_name (this));
+
+		parent_class = g_type_class_peek (parent);
+
+		/* check if this class isn't actually one of ours.  if it is a
+		 * Perl class, then we must not chain up to it: if it had a sub
+		 * defined for the current vfunc, we wouldn't be in this
+		 * fallback one here; so chaining up would result in the
+		 * fallback being called again.  this will lead to an endless
+		 * loop.
+		 *
+		 * so, if it's not a Perl class, we're done.  if it is,
+		 * continue in the while loop to the next parent. */
+		if (parent_class->get_size != gtk2perl_cell_renderer_get_size) {
+			break;
+		}
+
+		this = parent;
+	}
+
+	/* the ancestry will always contain GtkCellRenderer, so parent and
+	 * parent_class should never be NULL. */
+	assert (parent != NULL && parent_class != NULL);
+
 	switch (ix) {
 	    case 4: /* deprecated parent_get_size */
 	    case 0: /* GET_SIZE */
-		if (class->get_size) {
+		if (parent_class->get_size) {
 			gint x_offset, y_offset, width, height;
-			class->get_size (cell,
-			                 SvGtkWidget (ST (1)),
-					 SvGdkRectangle_ornull (ST (2)),
-					 &x_offset,
-					 &y_offset,
-					 &width,
-					 &height);
+			parent_class->get_size (cell,
+						SvGtkWidget (ST (1)),
+						SvGdkRectangle_ornull (ST (2)),
+						&x_offset,
+						&y_offset,
+						&width,
+						&height);
 			EXTEND (SP, 4);
 			PUSHs (sv_2mortal (newSViv (x_offset)));
 			PUSHs (sv_2mortal (newSViv (y_offset)));
@@ -691,41 +718,41 @@ GET_SIZE (GtkCellRenderer * cell, ...)
 		break;
 	    case 5: /* deprecated parent_render */
 	    case 1: /* RENDER */
-		if (class->render)
-			class->render (cell,
-			               SvGdkDrawable_ornull (ST (1)), /* drawable */
-				       SvGtkWidget_ornull (ST (2)), /* widget */
-				       SvGdkRectangle_ornull (ST (3)), /* background_area */
-				       SvGdkRectangle_ornull (ST (4)), /* cell_area */
-				       SvGdkRectangle_ornull (ST (5)), /* expose_area */
-				       SvGtkCellRendererState (ST (6))); /* flags */
+		if (parent_class->render)
+			parent_class->render (cell,
+					      SvGdkDrawable_ornull (ST (1)), /* drawable */
+					      SvGtkWidget_ornull (ST (2)), /* widget */
+					      SvGdkRectangle_ornull (ST (3)), /* background_area */
+					      SvGdkRectangle_ornull (ST (4)), /* cell_area */
+					      SvGdkRectangle_ornull (ST (5)), /* expose_area */
+					      SvGtkCellRendererState (ST (6))); /* flags */
 		break;
 	    case 6: /* deprecated parent_activate */
 	    case 2: /* ACTIVATE */
-		if (class->activate) {
+		if (parent_class->activate) {
 			gboolean ret;
-			ret = class->activate (cell,
-			                       SvGdkEvent (ST (1)),
-					       SvGtkWidget (ST (2)),
-					       SvGChar (ST (3)),
-					       SvGdkRectangle_ornull (ST (4)),
-					       SvGdkRectangle_ornull (ST (5)),
-					       SvGtkCellRendererState (ST (6)));
+			ret = parent_class->activate (cell,
+						      SvGdkEvent (ST (1)),
+						      SvGtkWidget (ST (2)),
+						      SvGChar (ST (3)),
+						      SvGdkRectangle_ornull (ST (4)),
+						      SvGdkRectangle_ornull (ST (5)),
+						      SvGtkCellRendererState (ST (6)));
 			EXTEND (SP, 1);
 			PUSHs (sv_2mortal (newSViv (ret)));
 		}
 		break;
 	    case 7: /* deprecated parent_start_editing */
 	    case 3: /* START_EDITING */
-		if (class->start_editing) {
+		if (parent_class->start_editing) {
 			GtkCellEditable * editable;
-			editable = class->start_editing (cell,
-				                         SvGdkEvent_ornull (ST (1)),
-						         SvGtkWidget (ST (2)),
-						         SvGChar (ST (3)),
-						         SvGdkRectangle_ornull (ST (4)),
-						         SvGdkRectangle_ornull (ST (5)),
-						         SvGtkCellRendererState (ST (6)));
+			editable = parent_class->start_editing (cell,
+								SvGdkEvent_ornull (ST (1)),
+								SvGtkWidget (ST (2)),
+								SvGChar (ST (3)),
+								SvGdkRectangle_ornull (ST (4)),
+								SvGdkRectangle_ornull (ST (5)),
+								SvGtkCellRendererState (ST (6)));
 			EXTEND (SP, 1);
 			PUSHs (sv_2mortal (newSVGtkCellEditable_ornull (editable)));
 		}
--- /dev/null	2008-04-13 20:57:27.000000000 +0200
+++ t/GtkCellRendererIface-Chaining.t	2008-06-29 18:10:19.000000000 +0200
@@ -0,0 +1,58 @@
+#!/usr/bin/perl
+
+# $Id$
+
+use Gtk2::TestHelper tests => 2;
+
+package CellRendererFoo;
+
+use Glib::Object::Subclass
+	Gtk2::CellRendererText::,
+	;
+
+our $hits = 0;
+
+sub GET_SIZE {
+	#warn __PACKAGE__;
+	$hits++;
+	if ($hits > 50) {
+		die 'Overflow';
+	}
+	shift->SUPER::GET_SIZE (@_);
+}
+
+package CellRendererBar;
+
+use Glib::Object::Subclass
+	CellRendererFoo::,
+	;
+
+our $hits = 0;
+
+sub GET_SIZE {
+	#warn __PACKAGE__;
+	$hits++;
+	shift->SUPER::GET_SIZE (@_);
+}
+
+package main;
+
+my $model = Gtk2::ListStore->new ('Glib::String');
+foreach (qw/foo fluffy flurble frob frobnitz ftang fire/) {
+	my $iter = $model->append;
+	$model->set ($iter, 0 => $_);
+}
+
+my $view = Gtk2::TreeView->new ($model);
+
+my $renderer = CellRendererBar->new;
+my $column = Gtk2::TreeViewColumn->new_with_attributes ('F-Words', $renderer,
+                                                        text => 0);
+$view->append_column ($column);
+
+my $window = Gtk2::Window->new;
+$window->add ($view);
+
+ok (eval { $window->show_all; 1; }, 'no overflow');
+ok ($CellRendererFoo::hits == $CellRendererBar::hits,
+    'both classes were hit just as often');