Re: Fixing GtkTreeModel:row-deleted once and for all

[Matthias Clasen <matthias clasen gmail com>]

On Mon, May 9, 2011 at 6:29 AM, Kristian Rietveld <kris gtk org> wrote:
> Hi all,
>
> The following two commits sparked my interest to look into the GtkTreeModel:row-deleted saga again, which has been haunting me for the last 9 years:
>
>  0c3da06 gtk_tree_model_filter_row_deleted: don't emit signals too early
>  f632956 Fixed GtkTreeModel::row-deleted documentation
>
> This because I have always been fully convinced that models implementing reference counting (which both GtkTreeModelFilter and GtkTreeModelSort do) could not safely emit row-deleted after deleting the node from the internal data structure, but rather had to do this before modifying the internal data structures.  This belief came from the following comment in gtktreemodelsort.c, added in February 2002 and see also a mailing list discussion from 2007[1]:
>
>  /* we _need_ to emit ::row_deleted before we start unreffing the node
>   * itself. This is because of the row refs, which start unreffing nodes
>   * when we emit ::row_deleted
>   */
>
> Upon further investigation, this statement is false, because nodes that have been deleted are never unreffed after emission of row-deleted. This is mentioned in the documentation for gtk_tree_model_unref_node().  The issue for which the above comment was added, has actually been fixed for real in April 2002 in commit 873e9ce4.  By no longer calling unref for the deleted node, this commit fixed the problem at hand.  Unfortunately, I did not manage to connect these two commits to each other back then (can't blame myself, since I was only 17 at that time).
>
>
> Fixing this
> ===
>
> Based on these results, I started a local branch in which I have also fixed GtkTreeModelSort to emit row-deleted *after* the internal data structures have been updated.  All GtkTreeModel unit tests have been consolidated in a single binary and more tests have been added.  More importantly, GtkTreeModelFilter has been corrected to never call unref on deleted child nodes.
>
> Before I merge these fixes into master, I plan to also do the following to verify that everything will finally work correctly within this framework:
>
> 1) Review and/or fix the following bugs, and bring them under unit test:
>
>  - 621076  GtkTreeModelFilter does not emit all signals in some situations
>  - 616871  GtkTreeModelFilter wasting CPU in g_array_remove_index()
>  - 611922  gtk_tree_model_sort_ref_node() is too slow
>  - 351814  GtkTreeModelFilter: row-deleted should probably emit signal first, then decrease visible nodes
>  - 540605  Possible leak in GtkTreeModelFilter
>
> If you know of any bug that is related to this, please let me know.
>
> 2) Bring reference counting under unit test, including the internal reference counting done by GtkTreeModelSort and GtkTreeModelFilter.  For this, I will likely write a custom model that does reference counting from which the reference counts of given nodes can be read out.  We can use this information to verify whether the parent model or parent view is referencing the correct nodes and also if references are released when required.
>
> 3) Document how GtkTreeModel's ref_node and unref_node are supposed to be used and why they are there.  This is unclear to many people.
>
>
> I hope this will rid us of nasty GtkTreeModelSort/GtkTreeModelFilter bugs for a long while as well as increase the maintainability of this code.
>

Sounds like an excellent plan to me !