From 4b45adaf399d3063a539daff810da236a7a31216 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= Date: Thu, 25 Jan 2024 16:58:12 +0000 Subject: [PATCH 1/4] testsuite: Test inserting items at sections We are not catching bugs when inserting if we're right at a boundary. This because we never add or remove items from a section. We only ever add or remove whole sections. Introduce a test which inserts items at a random position inside of a section. --- testsuite/gtk/listitemmanager.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/testsuite/gtk/listitemmanager.c b/testsuite/gtk/listitemmanager.c index e11e3f85e0..07486e9e99 100644 --- a/testsuite/gtk/listitemmanager.c +++ b/testsuite/gtk/listitemmanager.c @@ -433,7 +433,7 @@ test_exhaustive (void) gboolean add = FALSE, remove = FALSE; guint position, n_items; - switch (g_test_rand_int_range (0, 6)) + switch (g_test_rand_int_range (0, 7)) { case 0: if (g_test_verbose ()) @@ -483,6 +483,34 @@ test_exhaustive (void) } break; + case 6: + { + n_items = g_list_model_get_n_items (G_LIST_MODEL (store)); + if (n_items > 0) + { + guint j = g_test_rand_int_range (0, n_items); + GListModel *source = g_list_model_get_item (G_LIST_MODEL (store), j); + guint source_size = g_list_model_get_n_items (G_LIST_MODEL (source)); + GStrvBuilder *builder = g_strv_builder_new (); + guint inclusion_size = g_test_rand_int_range (1, 11); + char **inclusion; + + for (j = 0; j < inclusion_size; j++) + g_strv_builder_add (builder, g_test_rand_bit () ? "A" : "B"); + inclusion = g_strv_builder_end (builder); + g_strv_builder_unref (builder); + + j = g_test_rand_int_range (0, source_size + 1); + gtk_string_list_splice (GTK_STRING_LIST (source), j, 0, (const char * const *) inclusion); + g_strfreev (inclusion); + + if (g_test_verbose ()) + g_test_message ("Adding %u items at position %u of a section which had %u items", + inclusion_size, j, source_size); + } + } + break; + default: g_assert_not_reached (); break; From b05000d8bdbe40771b59edd793574263ef6cb60f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= Date: Wed, 24 Jan 2024 17:17:54 +0000 Subject: [PATCH 2/4] listitemmanager: Remove section when adding at its end We are failing to go from this: [ BLUE ] [ RED ] ...to this: [ BLUE GREEN YELLOW ] [ RED ] ...where '[' and ']' represent section header and footer. Instead, the result is... [ BLUE ] [ GREEN YELLOW ] [ RED ] ... despite the first 3 items belonging to the same section according to the section model. This leaves the view in an inconsistent state and, ultimately, to crashes the non-removed footer. Indeed, when receiving items-changed(1,0,2), we call `append_items()` which inserts a new tile before the tile at `1` (which was RED), and then notices there is a HEADER right befo-re it, so it flags both it and the corresponding FOOTER as unmatched: [ BLUE ] ( GREEN-YELLOW RED ) ... where '(' and ')' represent unmatched header and footer. Problem is subsequent code in `release_items()` doesn't even touch the section boundary footer-header pair ('] ('), because they are belong in the tracked interval (visible items). And `ensure_items()` proceeds to match the header with a new footer, producing the result described above. To handle this correctly, `append_items()` must delete the section boundary, and flag as unmatched both the HEADER of the section before and the FOOTER of section after (whose respective footer and header has been marked for removal): ( BLUE . . GREEN-YELLOW RED ) ... where '.' represents tiles marked for removal. This way, `release_items()` will release the removed footer-header section boundary, and `ensure_items()` is going to reinstate new section remove the section boundary at the correct place, resulting in the expected behavior: [ BLUE GREEN YELLOW ] [ RED ] --- gtk/gtklistitemmanager.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/gtk/gtklistitemmanager.c b/gtk/gtklistitemmanager.c index cfa2f2b843..cf0c8b7d9b 100644 --- a/gtk/gtklistitemmanager.c +++ b/gtk/gtklistitemmanager.c @@ -968,10 +968,26 @@ gtk_list_item_manager_add_items (GtkListItemManager *self, if (section != NULL && section->type == GTK_LIST_TILE_HEADER) { + guint start, end; + GtkListTile *footer = gtk_list_tile_get_footer (self, section); + GtkListTile *previous_footer = gtk_list_tile_get_previous_skip (section); + + gtk_section_model_get_section (GTK_SECTION_MODEL (self->model), position, &start, &end); + + if (previous_footer != NULL && previous_footer->type == GTK_LIST_TILE_FOOTER && + position > start && position < end) + { + gtk_list_item_change_clear_header (change, §ion->widget); + gtk_list_tile_set_type (section, GTK_LIST_TILE_REMOVED); + gtk_list_tile_set_type (previous_footer, GTK_LIST_TILE_REMOVED); + + section = gtk_list_tile_get_header (self, previous_footer); + } + gtk_list_item_change_clear_header (change, §ion->widget); gtk_list_tile_set_type (section, GTK_LIST_TILE_UNMATCHED_HEADER); - gtk_list_tile_set_type (gtk_list_tile_get_footer (self, section), + gtk_list_tile_set_type (footer, GTK_LIST_TILE_UNMATCHED_FOOTER); } } From 3d88c44803f4fa1bf8a7cfdadcc91558385b8169 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= Date: Wed, 24 Jan 2024 12:44:50 +0000 Subject: [PATCH 3/4] listitemmanager: Don't doubly-recycle widget When there is a duplicate item in the hash table of deleted items, we: 1. Unparent the unparent the old `widget` value (gtk_widget_unparent is passed as `GDestroyNotify value_destroy_func` for the hastable). 2. Set the new `widget` value in the hashtable. 3. Also set the same `widget` in the recycled queue. This means the same widget is found in the 2 containers and, therefore, the same widget may be returned twice by gtk_list_item_change_get(). Alternatively, this means we may reuse the item by taking it from the hashtable and reassigning it to a tile, but then it ends up getting unparented by gtk_list_item_change_finish(). Or we don't take it at all and end up calling gtk_widget_unparent()` on it twice, which may result in use-after-free on the second call the parent was holding the last reference. This was introduced by 76d601631d45e790a30e4e56dcf46f59981e358f Previously, gtk_list_item_manager_release_list_item() would just emit the warning but otherwise do nothing. Let's restore that behavior. --- gtk/gtklistitemmanager.c | 1 - 1 file changed, 1 deletion(-) diff --git a/gtk/gtklistitemmanager.c b/gtk/gtklistitemmanager.c index cf0c8b7d9b..9a2f178704 100644 --- a/gtk/gtklistitemmanager.c +++ b/gtk/gtklistitemmanager.c @@ -117,7 +117,6 @@ gtk_list_item_change_release (GtkListItemChange *change, if (!g_hash_table_replace (change->deleted_items, gtk_list_item_base_get_item (GTK_LIST_ITEM_BASE (widget)), widget)) { g_warning ("Duplicate item detected in list. Picking one randomly."); - gtk_list_item_change_recycle (change, widget); } } From 9fb449793dfe0a645682fb35441fe0831c8a1e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= Date: Thu, 25 Jan 2024 17:18:05 +0000 Subject: [PATCH 4/4] listitemmanager: Fix section change handler mistake The statement is not doing what it was meant to do. gtk_list_item_manager_get_nth (self, position, &offset) returns the tile for a given position, and if the tile maps to more than 1 item, the offset indicates how far into that tile the given position is. So position - offset would give us the position of this tile. It doesn't make sense to subtract it from n_items. Instead, we should be adding the offset to compensate for having landed too early in the list, such that we successfully reach position + n_items. --- gtk/gtklistitemmanager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gtk/gtklistitemmanager.c b/gtk/gtklistitemmanager.c index 9a2f178704..82c6444cff 100644 --- a/gtk/gtklistitemmanager.c +++ b/gtk/gtklistitemmanager.c @@ -1625,7 +1625,7 @@ gtk_list_item_manager_model_sections_changed_cb (GListModel *model, gtk_list_item_change_clear_header (&change, &header->widget); gtk_list_tile_set_type (header, GTK_LIST_TILE_UNMATCHED_HEADER); - n_items -= MIN (n_items, position - offset); + n_items += offset; while (n_items > 0) { switch (tile->type)