[Gtk-sharp-list] patch: managed TreeModel impl

[Mike Kestner]

On Mon, 2004-05-24 at 17:47, Vladimir Vukicevic wrote:
> Updated patch attached; fixed a lot of the out/ref business based on 
> feedback from Mike.  We use TreeIter.Zero to indcate invalid (i.e. NULL) 
> passed-in TreeIters.

I'm not sure how excited I am about having this in Gtk#.  I think the
NodeStore approach is much more C# like, and this exposes way too much
of the underlying nativisms of tree models.

What exactly about NodeStore made it inappropriate for your work on the
DataBindings project, if you considered using it?  I'd rather finish
cooking that approach than put such a thin wrapper like this into Gtk#
if we can accomplish your goals that way.

I have some specific comments in-line:
> 
> ______________________________________________________________________
> Index: glib/Object.cs
> ===================================================================
> RCS file: /cvs/public/gtk-sharp/glib/Object.cs,v
> retrieving revision 1.64
> diff -u -u -r1.64 Object.cs
> --- glib/Object.cs	18 May 2004 05:06:10 -0000	1.64
> +++ glib/Object.cs	24 May 2004 21:58:39 -0000
> @@ -149,7 +149,7 @@
>  			if (g_types.Contains (t))
>  				return (GType) g_types [t];
>  			
> -			PropertyInfo pi = t.GetProperty ("GType", BindingFlags.DeclaredOnly | BindingFlags.Static | BindingFlags.Public);
> +			PropertyInfo pi = t.GetProperty ("GType", BindingFlags.DeclaredOnly | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic);
>  			if (pi != null)
>  				return (GType) pi.GetValue (null, null);

I'd rather fix this properly by making the generator produce protected
GType properties so we can drop the Public flag instead of just querying
all protection levels.

> Index: gtk/TreeIter.custom
> ===================================================================
> RCS file: /cvs/public/gtk-sharp/gtk/TreeIter.custom,v
> retrieving revision 1.3
> diff -u -u -r1.3 TreeIter.custom
> --- gtk/TreeIter.custom	12 Feb 2004 21:28:42 -0000	1.3
> +++ gtk/TreeIter.custom	24 May 2004 21:58:39 -0000
> @@ -19,3 +19,18 @@
>                                  ti._user_data2 == _user_data2 &&
>                                  ti._user_data3 == _user_data3;
>                  }
> +
> +                public IntPtr UserData1 {
> +                        get { return _user_data; }
> +                        set { _user_data = value; }
> +                }
> +                
> +                public IntPtr UserData2 {
> +                        get { return _user_data2; }
> +                        set { _user_data2 = value; }
> +                }
> +                
> +                public IntPtr UserData3 {
> +                        get { return _user_data3; }
> +                        set { _user_data3 = value; }

Ugh, ugh, ugh. ;-)  Stuff like this just makes me feel more strongly
that such a thin wrapper is the wrong way to go.

> +                }
> Index: gtk/TreeModel.custom
> ===================================================================
> RCS file: /cvs/public/gtk-sharp/gtk/TreeModel.custom,v
> retrieving revision 1.2
> diff -u -u -r1.2 TreeModel.custom
> --- gtk/TreeModel.custom	1 Nov 2003 12:00:26 -0000	1.2
> +++ gtk/TreeModel.custom	24 May 2004 21:58:39 -0000
> @@ -18,6 +18,7 @@
>  	/// <remarks>To be completed</remarks>
>  	bool IterNthChild (out Gtk.TreeIter iter, int n);
>  
> +#if false
>          void SetValue (Gtk.TreeIter iter, int column, bool value);
>          void SetValue (Gtk.TreeIter iter, int column, double value);
>  	void SetValue (Gtk.TreeIter iter, int column, int value);
> @@ -25,4 +26,5 @@
>  	void SetValue (Gtk.TreeIter iter, int column, float value);
>  	void SetValue (Gtk.TreeIter iter, int column, uint value);
>  	void SetValue (Gtk.TreeIter iter, int column, object value);
> +#endif

If these aren't needed, then they should be deleted, not #if'd.

>  	object GetValue(Gtk.TreeIter iter, int column);

> +    public virtual void GetValue (Gtk.TreeIter iter, int column, ref GLib.Value value) {
> +      object o = GetValue (iter, column);
> +      if (o == null) {
> +	value.Init (GLib.GType.Invalid);
> +	return;
> +      }
> +      GLib.GType ctype = GLibSharp.TypeConverter.LookupType (o.GetType());
> +      value.Init (ctype);
> +      value.Val = o;
> +    }

After such a thin wrapper elsewhere, why add a bunch of code here that's
going to be duplicated in subclasses anyway.  I should internalize
GLibSharp.TypeConverter, now that you mention it.  ;-)

> +   
> +    /* This is a copy of the TreeModel interface struct */
> +    internal struct TreeModelInterface {

You need to use the [StructLayout (LayoutKind.Sequential)] attr on all
these, or you will get unpredictable layout.

> +    // internal static void tree_model_iface_init (ref TreeModelInterface iface, IntPtr data) {
> +    internal static void tree_model_iface_init (IntPtr ifaceptr, IntPtr data) {
> +      TreeModelInterface iface = (TreeModelInterface) Marshal.PtrToStructure (ifaceptr, typeof(TreeModelInterface));
> +
> +      iface.get_flags = new TreeModelGetFlagsDelegate (TreeModelGetFlagsImpl);
> +      iface.get_n_columns = new TreeModelGetNColumnsDelegate (TreeModelGetNColumnsImpl);
> +      iface.get_column_type = new TreeModelGetColumnTypeDelegate (TreeModelGetColumnTypeImpl);
> +      iface.get_iter = new TreeModelGetIterDelegate (TreeModelGetIterImpl);
> +      iface.get_path = new TreeModelGetPathDelegate (TreeModelGetPathImpl);
> +      iface.get_value = new TreeModelGetValueDelegate (TreeModelGetValueImpl);
> +      iface.iter_next = new TreeModelIterNextDelegate (TreeModelIterNextImpl);
> +      iface.iter_children = new TreeModelIterChildrenDelegate (TreeModelIterChildrenImpl);
> +      iface.iter_has_child = new TreeModelIterHasChildDelegate (TreeModelIterHasChildImpl);
> +      iface.iter_n_children = new TreeModelIterNChildrenDelegate (TreeModelIterNChildrenImpl);
> +      iface.iter_nth_child = new TreeModelIterNthChildDelegate (TreeModelIterNthChildImpl);
> +      iface.iter_parent = new TreeModelIterParentDelegate (TreeModelIterParentImpl);
> +      iface.ref_node = new TreeModelRefNodeDelegate (TreeModelRefNodeImpl);
> +      iface.unref_node = new TreeModelUnrefNodeDelegate (TreeModelUnrefNodeImpl);
> +
> +      Marshal.StructureToPtr (iface, ifaceptr, false);
> +    }

Your delegates are all going to disappear as soon as the GC runs, and
then all those fptrs written to ifaceptr are going to be invalid. 
Boom.  

The problem is complex.  I don't think you can even just hold onto the
iface struct in a static variable either, because the class will get
GC'd as soon as the last instance "wrapper" is GC'd.  Any time the
native side is more persistent than the managed side, there has to be
some mechanism to hang onto the delegates that will persist as long as
the native side is around.  

This is basically why I haven't already implemented GInterfaces (or more
generally, structs-full-of-fptrs) already.  The static field hack will
work as long as the user hangs on to a ref of the ManagedTreeModel
subclass, but if they don't, and you know someone won't, ...

-- 
Mike Kestner <mkestner@ximian.com>