Re: cellrenderer GET_SIZE super method

[Torsten Schoenfeld <kaffeetisch gmx de>]

Torsten Schoenfeld wrote:

> 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.

Here's caller-v3.patch which adds a test case that fails with the current
caller()-using code and succeeds with the new stuff.  The test is derived from
the code Kevin posted on the RT issue.

-- 
Bye,
-Torsten

? 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 17:52:08 -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 19:49:08.000000000 +0200
@@ -0,0 +1,110 @@
+#!/usr/bin/perl
+
+# $Id$
+
+use Gtk2::TestHelper tests => 3;
+
+# --------------------------------------------------------------------------- #
+
+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 CellRendererEmpty;
+
+use Glib::Object::Subclass
+	Gtk2::CellRendererText::,
+	;
+
+package ProxyDialog;
+
+use Glib::Object::Subclass
+	Gtk2::Dialog::
+	;
+
+sub INIT_INSTANCE {
+	my ($self) = @_;
+
+	my $vbox = $self->vbox;
+
+	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);
+	$vbox->add ($view);
+
+	my $renderer = CellRendererEmpty->new;
+	my $column = Gtk2::TreeViewColumn->new_with_attributes ('F-Words', $renderer,
+	                                                        text => 0);
+	$view->append_column ($column);
+
+	# This eventually results in a call to CellRendererEmpty::GET_SIZE.
+	$self->show_all;
+}
+
+# --------------------------------------------------------------------------- #
+
+package main;
+
+# Test that Perl renderers can chain up without endless loops ensuing.  Even if
+# a Perl renderer inherits from a Perl renderer.
+{
+	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');
+}
+
+# Test that calls to vfuncs from strange places (like
+# ProxyDialog::INIT_INSTANCE) don't confuse the fallback functions in
+# Gtk2::CellRenderer.
+{
+	ok (eval { my $dialog = ProxyDialog->new; 1; }, 'no exception');
+}