Re: Patches for the sheet branch

[Ben Pfaff]

John Darrington <address@hidden> writes:

> On Sat, Jul 07, 2012 at 11:13:49AM -0700, Ben Pfaff wrote:
>      
>      Do you have a rule of thumb that explains what should go in
>      destroy, dispose, and finalize?  The difference between destroy
>      and dispose seems especially difficult to grasp.  
>
> Destroy is a method of GtkObject which has been deprecated in Gtk2 for
> some time, and is absent from Gtk3.  So destroy should not be used in
> new code.
>
> According to the Gtk docs, dispose should do nothing except unref objects.
> Unrealize should  of course undo everthing that was done by realize.  It
> should also realease any Gdk resources (such as colours etc).  Finaliue
> shoud not do any unreffing.
>
> In my experience, for GtkWidgets, finalise is not realy necessary since
> everything can be done in unrealise.  GObjects, which are not GtkWidgets,
> however may need finalise to actually free memory etc.  Note that Gtk
> engine guarantees that it will call no signal or method on your object 
> between the dispose and finalise invocations.  However that guarantee is
> void if a dependent object erroneously calls such a method (either 
> directly or indirectly) in its dispose method.
>      

So is this an accurate summary of your take?

        - Don't implement GtkObject.destroy in new code.

        - Implement unrealize to undo everything done by realize and
          release Gdk resources.

        - Implement dispose to just unref objects.

        - Implement finalize to do other cleanup.


>      Some of the commits adjust refcounting of the UI manager.  The
>      commit messages mention that this fixes problems but don't
>      explain further.  Can you say more?
>
> I changed the var_sheet and data_sheet to keep a reference to the uim
> objects which they return from their _get_ui_manager objects.  One
> of the comments in the code (which I removed in another commit), mentioned
> that there were problems when the final reference to uim was unreffed.
> This change fixed that problem, by ensureing that it wasn't finalised,
> until the object which owned it was disposed.  In fact, I think that a 
> later commit, made this change unecessary - at least it should be, once
> all the dispose/finalise methods are in order.. But it can't to a lot of 
> harm keeping this extra ref.

OK.  That makes enough sense to me.

Would you mind organizing the commits or editing the commit
messages to better explain the above?

>      The patch "Reduce the flicker when redrawing the toolbar and
>      menubar." appears to leak a reference in
>      psppire_data_editor_split_window().  At least, I see nothing that
>      will eventually unref de->old_vbox_widget.
>
> I think you're right.  But when I add a call to _unref in dipose, it 
> provokes a Critical.  This needs more investigation. I think the problem
> is that make_{single,split}_datasheet returns a floating object.  They
> should probably call g_object_ref_sink.

OK.  Can you add a comment in psppire_data_editor_split_window()
mentioning the leak?  Otherwise we'll forget about it.

>      P.S. "git am" told me that some of the patches add trailing
>      whitespace, not that that's really a big deal.
>
> We should probably run a sed script to remove all such whitespace.

I don't usually see the point in bothering with that, but I won't
object if you want to do it.

Thanks,

Ben.