From f1c1c979c2ddc5a73a1859edbd74b261735bd531 Mon Sep 17 00:00:00 2001 From: Matthias Clasen Date: Wed, 24 Aug 2022 08:20:33 -0400 Subject: [PATCH 1/2] treelistmodel: Fix handling of collapsed nodes When we collapse a node, we clear out the children, but we were not disconnecting the signal handler on the child listmodel, leading to bad outcomes when that model is persistent and changing. Fixes: #4595 --- gtk/gtktreelistmodel.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/gtk/gtktreelistmodel.c b/gtk/gtktreelistmodel.c index 268aa449ef..ab053b323f 100644 --- a/gtk/gtktreelistmodel.c +++ b/gtk/gtktreelistmodel.c @@ -440,6 +440,20 @@ gtk_tree_list_model_items_changed_cb (GListModel *model, static void gtk_tree_list_row_destroy (GtkTreeListRow *row); +static void +gtk_tree_list_model_clear_node_children (TreeNode *node) +{ + if (node->model) + { + g_signal_handlers_disconnect_by_func (node->model, + gtk_tree_list_model_items_changed_cb, + node); + g_clear_object (&node->model); + } + + g_clear_pointer (&node->children, gtk_rb_tree_unref); +} + static void gtk_tree_list_model_clear_node (gpointer data) { @@ -448,15 +462,7 @@ gtk_tree_list_model_clear_node (gpointer data) if (node->row) gtk_tree_list_row_destroy (node->row); - if (node->model) - { - g_signal_handlers_disconnect_by_func (node->model, - gtk_tree_list_model_items_changed_cb, - node); - g_object_unref (node->model); - } - if (node->children) - gtk_rb_tree_unref (node->children); + gtk_tree_list_model_clear_node_children (node); } static void @@ -551,8 +557,7 @@ gtk_tree_list_model_collapse_node (GtkTreeListModel *self, n_items = tree_node_get_n_children (node); - g_clear_pointer (&node->children, gtk_rb_tree_unref); - g_clear_object (&node->model); + gtk_tree_list_model_clear_node_children (node); tree_node_mark_dirty (node); From 83bf1932180f55bab21fb83a38a7244a3484a9be Mon Sep 17 00:00:00 2001 From: Matthias Clasen Date: Tue, 23 Aug 2022 23:03:49 -0400 Subject: [PATCH 2/2] Add a test for treelistmodel row collapse Test that we can expand and collapse a row, and then add another child below it, without crashing. Adapted from the testcase in #4595. This tests the fix in the previous commit. --- testsuite/gtk/treelistmodel.c | 120 ++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/testsuite/gtk/treelistmodel.c b/testsuite/gtk/treelistmodel.c index bf3f12caf3..941ac3d306 100644 --- a/testsuite/gtk/treelistmodel.c +++ b/testsuite/gtk/treelistmodel.c @@ -261,6 +261,125 @@ test_remove_some (void) g_object_unref (tree); } +/* Test for https://gitlab.gnome.org/GNOME/gtk/-/issues/4595 */ +typedef struct _DemoNode DemoNode; + +struct _DemoNode { + GObject parent_instance; + char *value; + GListStore *children; +}; + +G_DECLARE_FINAL_TYPE (DemoNode, demo_node, DEMO, NODE, GObject); + +G_DEFINE_TYPE (DemoNode, demo_node, G_TYPE_OBJECT); + +static void +demo_node_init (DemoNode *node) +{ +} + +static void +demo_node_finalize (GObject *object) +{ + g_free (DEMO_NODE (object)->value); + + G_OBJECT_CLASS (demo_node_parent_class)->finalize (object); +} + +static void +demo_node_class_init (DemoNodeClass *klass) +{ + G_OBJECT_CLASS (klass)->finalize = demo_node_finalize; +} + +static DemoNode * +demo_node_new (const char *value, + GListStore *children) +{ + DemoNode *result; + + result = g_object_new (demo_node_get_type(), NULL); + result->value = g_strdup (value); + if (children) + result->children = g_object_ref (children); + + return result; +} + +static GListStore * +create_model (void) +{ + DemoNode *aa, *a, *b, *c; + GListStore *a_children, *root; + + aa = demo_node_new ("aa", NULL); + + a_children = g_list_store_new (demo_node_get_type ()); + g_list_store_append (a_children, aa); + + a = demo_node_new ("a", a_children); + b = demo_node_new ("b", NULL); + c = demo_node_new ("c", NULL); + + root = g_list_store_new (demo_node_get_type ()); + g_list_store_append (root, a); + g_list_store_append (root, b); + g_list_store_append (root, c); + + g_object_unref (aa); + g_object_unref (a_children); + g_object_unref (a); + g_object_unref (b); + g_object_unref (c); + + return root; +} + +static GListModel * +model_children (gpointer item, + gpointer unused) +{ + GListStore *children; + + children = DEMO_NODE (item)->children; + if (children) + return G_LIST_MODEL (g_object_ref (children)); + + return NULL; +} + +static void +test_collapse_change (void) +{ + GListStore *model; + GtkTreeListModel *treemodel; + DemoNode *a, *ab; + GtkTreeListRow *row; + + model = create_model (); + a = g_list_model_get_item (G_LIST_MODEL (model), 0); + + treemodel = gtk_tree_list_model_new (G_LIST_MODEL (model), + FALSE, + FALSE, + model_children, + NULL, + NULL); + + row = gtk_tree_list_model_get_row (treemodel, 0); + gtk_tree_list_row_set_expanded (row, TRUE); + gtk_tree_list_row_set_expanded (row, FALSE); + g_object_unref (row); + + ab = demo_node_new ("ab", NULL); + g_list_store_append (a->children, ab); + g_object_unref (ab); + + g_object_unref (treemodel); + g_object_unref (a); +} + int main (int argc, char *argv[]) { @@ -272,6 +391,7 @@ main (int argc, char *argv[]) g_test_add_func ("/treelistmodel/expand", test_expand); g_test_add_func ("/treelistmodel/remove_some", test_remove_some); + g_test_add_func ("/treelistmodel/collapse-change", test_collapse_change); return g_test_run (); }