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

Vladimir Vukicevic vladimir@pobox.com
Tue, 25 May 2004 14:19:55 -0700
Previous message: [Gtk-sharp-list] patch: managed TreeModel impl
Next message: [Gtk-sharp-list] patch: managed TreeModel impl
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Mike Kestner wrote:
> 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.

Both the ADO.NET DataTable/DataTableView stuff and the TreeModel are 
fairly low level; having to implement an ITreeNode interface on top of 
DataRow, and then having to create one of those instances for each row 
in the data table seems rife with problems.  With NodeStore, I can't 
dynamically generate the data based on what the TreeView requests -- I 
have to generate everything beforehand and pass it to NodeStore.  With a 
lower level TreeModel interface, I only need to query the (potentially 
huge) data table only for data rows that the TreeView actually needs.

I understand that Tree Models are pretty heavy internals bits; however, 
I think that having this type of interface exported would be a good 
thing, for people that want to use it.

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

Sounds good.

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

Yeah, I'm not really happy with this; I was thinking of another 
approach, where the iterator would essentially be "object", and the 
ManagedTreeModel would map it to/from TreeIter for you.  I need to 
understand the RefNode/UnrefNode bits better first.  (I'd keep a local 
hash of Ref'd iterators, just need to know when to remove bits from the 
hash.)

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

I agree. :)

>>     public abstract 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.  ;-)

Well, the idea being that you can override either the GetValue that 
returns an object, or a GetValue that uses the GLib.Value (I have a 
change locally that changes the object-returning abstract GetValue to 
virtual returning null).

>>+   
>>+    /* 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.

LayoutKind.Sequential should be the default layout kind for structs, at 
least according to the MS docs -- but yeah, it should be added on there.

>>+    // 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, ...

For some reason, I convinced myself that this would work -- however, to 
be fully correct, you're right; I need to keep a local reference to the 
struct, and also create a static GCHandle to it that I allocate in the 
_init and deallocate when the interface is destroyed.  Doing that should 
cause zero problems, I think.

I'll go ahead and make the changes you suggested; I still think that 
this would be a useful addition to Gtk#, as the TreeView is a pretty 
powerful widget and it would be unfortunate to not be able to fully take 
advantage of it from within C#.

Thanks,
	- Vlad
Previous message: [Gtk-sharp-list] patch: managed TreeModel impl
Next message: [Gtk-sharp-list] patch: managed TreeModel impl
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]