The output of that test is:

$ ./test
Added foo
Insert bar at pos 0
item 0: foo
item 1: bar

which does seem wrong.  I would expect bar to be before foo.

Christoph, can you write a patch to fix GLib’s behaviour here and add unit tests please? According to Debian code search (https://codesearch.debian.net/search?q=g_list_store_splice), only Nautilus uses g_list_store_splice(), so I think changing it to match the documented behaviour is OK, rather than maintaining backwards compatibility for the bug.

> Christoph, can you write a patch to fix GLib’s behaviour here and add unit tests please? According to Debian code search (https://codesearch.debian.net/search?q=g_list_store_splice), only Nautilus uses g_list_store_splice(), so I think changing it to match the documented behaviour is OK, rather than maintaining backwards compatibility for the bug.

Will do. afaics nautilus replaces the whole list so the bug isn't triggered.

It also inserts things in reverse, so the first item in the "additions" array will end up last in the store. I guess keep it that way and document?

Yikes, that seems like even more of a gotcha than the other. Should that be the behaviour? It seems very unhelpful, relative to the likelihood that users will expect the order to be preserved. If it was not documented before as reversing, maybe it should stop doing so...

I would definitely have expected it to splice in the new entries in the order they’re provided to the function.

Again, since g_list_store_splice() is only used in Nautilus, I’d be in favour of just fixing it to splice in order. We might need to update Nautilus to keep its behaviour unchanged, but I think that’s worthwhile.

Created attachment 371328 [details] [review]
g_list_store_splice: Add items in the order of the input  array and at the given position

Review of attachment 371328 [details] [review]:

This looks reasonable, but I’d like to see some more tests please. :-)  I also need to convince myself of the behaviour of GSequence iters; the documentation for them is fairly poor.

::: gio/tests/glistmodel.c
@@ +218,3 @@
 
+static void
+test_store_splice_replace_middle (void)

Please add a brief comment above the test explaining what it tests. For example:

```
/* Test that using splice() to replace the middle element in a list store works. */
static void
test_store_splice_replace_middle (void)
…
```

Same for the other test.

@@ +225,3 @@
+  GPtrArray *array;
+
+  store = g_list_store_new (G_TYPE_SIMPLE_ACTION);

Add a `g_test_bug ("795307");` above here, and make sure that g_test_bug_base() is called after g_test_init() in main(). Same for the other test.

@@ +323,3 @@
+                   test_store_splice_replace_middle);
+  g_test_add_func ("/glistmodel/store/splice-replace-all",
+                   test_store_splice_replace_all);

Other splice() tests which should be added:
 • A no-op (removing 0 items; while adding 0 items)
 • Removing 0 items; while adding 2 items at position 0, a middle position, and at the end
 • Removing 2 items at position 0, a middle position, and at the end; while adding 0 items
 • Removing 1 item; while re-adding the same 1 item at a different position (specifically to test that if an item has a refcount of 1 before calling splice(), its refcount shouldn’t drop to 0 in the middle of the splice())

(In reply to Philip Withnall from comment #7)
> Review of attachment 371328 [details] [review] [review]:
> 
> This looks reasonable, but I’d like to see some more tests please. :-)

OK, sounds good, I tried to cover only the cases I changed for now.

> I
> also need to convince myself of the behaviour of GSequence iters; the
> documentation for them is fairly poor.

Yeah, the function docs lack details, the "Sequences" description makes it a bit clearer. To summarize the important bits for this use case: iters are between items and point to the next item (their "position"), they remain valid as long their item is still there (the one at its "position"). For example with g_sequence_remove_range() the end iter remains valid because it points to the next item which isn't removed by it.

(In reply to Christoph Reiter (lazka) from comment #8)
> > I
> > also need to convince myself of the behaviour of GSequence iters; the
> > documentation for them is fairly poor.
> 
> Yeah, the function docs lack details, the "Sequences" description makes it a
> bit clearer. To summarize the important bits for this use case: iters are
> between items and point to the next item (their "position"), they remain
> valid as long their item is still there (the one at its "position"). For
> example with g_sequence_remove_range() the end iter remains valid because it
> points to the next item which isn't removed by it.

Yeah, after reading the introduction to GSequence, the ‘points to’ terminology became a lot clearer. Still, I pushed some fixes to the documentation: https://gitlab.gnome.org/GNOME/glib/commit/a8b4d516aa8f15eeb6e4c65fc0f908266f09f768

Created attachment 371521 [details] [review]
g_list_store_splice: Add items in the order of the input  array and at the given position

Created attachment 371522 [details] [review]
gliststore: Improve the test coverage

I've tried to improve the coverage overall based on the lcov output.

(In reply to Philip Withnall from comment #7)
>  • Removing 1 item; while re-adding the same 1 item at a different position
> (specifically to test that if an item has a refcount of 1 before calling
> splice(), its refcount shouldn’t drop to 0 in the middle of the splice())

I think that's not possible because (1) there is only one position, so one can only re-add at the same position and more importantly (2) all item getters are transfer-full so it's not possible add an item without holding a reference while doing so.

Review of attachment 371521 [details] [review]:

I (or you) can fix this before pushing.

::: gio/tests/glistmodel.c
@@ +322,3 @@
 {
   g_test_init (&argc, &argv, NULL);
+  g_test_bug_base ("http://bugzilla.gnome.org/");

Nitpick: https

Review of attachment 371522 [details] [review]:

This is fantastic! Feel free to change or ignore my nitpick below before pushing.

(When I mark something as a ‘nitpick’ in a review, it means that I think it would be nice to change, but am open to argument against the change, and it’s definitely not a blocker for merging the patch.)

::: gio/tests/glistmodel.c
@@ +370,3 @@
+          return FALSE;
+        }
+      g_object_unref (ptr);

Nitpick: Could simplify the return path here:

gboolean equal = TRUE;

for (i = 0; i < array->len && equal; i++)
  {
    GObject *ptr = g_list_model_get_item (model, i);
    equal = equal && (g_ptr_array_index (array, i) == ptr);
    g_object_unref (ptr);
  }

return equal;

(In reply to Philip Withnall from comment #13)
> Review of attachment 371521 [details] [review] [review]:
> 
> I (or you) can fix this before pushing.
> 
> ::: gio/tests/glistmodel.c
> @@ +322,3 @@
>  {
>    g_test_init (&argc, &argv, NULL);
> +  g_test_bug_base ("http://bugzilla.gnome.org/");
> 
> Nitpick: https

done

(In reply to Philip Withnall from comment #14)
> Nitpick: Could simplify the return path here:

good point, I went with this: https://gitlab.gnome.org/GNOME/glib/blob/fe9457dedd47edb3004a292cd0a05052e915f166/gio/tests/glistmodel.c#L362

----

Thanks for the reviews!

(hit enter to early) CCing ernestask, as this change might affect nautilus.

Thanks for the heads-up, will take a look.

One of the uses of g_list_store_splice() in Nautilus is bogus, as that code path is never executed, but the other one looks fine, as we append the items (the position is larger than the item count, so we get an end iterator). The order of insertions also doesn’t matter, since we immediately sort the store.