[libadwaita/wip/exalm/carousel-fix: 11/13] carousel: Rewrite reorder()

[Alexander Mikhaylenko <alexm src gnome org>]

commit 6576046d0af1eca4c45620c2c69eac05948e2a29
Author: Alexander Mikhaylenko <alexm gnome org>
Date:   Wed Dec 29 13:04:34 2021 +0500

    carousel: Rewrite reorder()
    
    The existing function was complete nonsense. It was taking the previous
    sibling and inserting before it. It was messing up position shifting. It
    was leaking a GList link. Its no-op check wasn't working in some cases.
    And it had no tests other than checking that the page count hasn't changed.
    
    Rewrite it, add proper tests.
    
    Fixes https://gitlab.gnome.org/GNOME/libadwaita/-/issues/324

 src/adw-carousel.c    | 78 ++++++++++++++++++++++++++--------------
 tests/test-carousel.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 147 insertions(+), 30 deletions(-)
---
diff --git a/src/adw-carousel.c b/src/adw-carousel.c
index 93b294e5..3b095d60 100644
--- a/src/adw-carousel.c
+++ b/src/adw-carousel.c
@@ -344,14 +344,6 @@ animate_child_resize (AdwCarousel *self,
   adw_animation_play (child->resize_animation);
 }
 
-static void
-shift_position (AdwCarousel *self,
-                double       delta)
-{
-  set_position (self, self->position + delta);
-  adw_swipe_tracker_shift_position (self->tracker, delta);
-}
-
 static void
 scroll_animation_value_cb (double       value,
                            AdwCarousel *self)
@@ -599,7 +591,8 @@ adw_carousel_size_allocate (GtkWidget *widget,
   double snap_point;
 
   if (self->position_shift != 0) {
-    shift_position (self, self->position_shift);
+    set_position (self, self->position + self->position_shift);
+    adw_swipe_tracker_shift_position (self->tracker, self->position_shift);
     self->position_shift = 0;
   }
 
@@ -1295,9 +1288,9 @@ adw_carousel_reorder (AdwCarousel *self,
                       GtkWidget   *child,
                       int          position)
 {
-  ChildInfo *info, *prev_info;
-  GList *link, *prev_link;
-  int old_position;
+  ChildInfo *info, *next_info;
+  GList *link, *next_link;
+  int old_position, n_pages;
   double closest_point, old_point, new_point;
 
   g_return_if_fail (ADW_IS_CAROUSEL (self));
@@ -1313,27 +1306,60 @@ adw_carousel_reorder (AdwCarousel *self,
   if (position == old_position)
     return;
 
-  old_point = ((ChildInfo *) link->data)->snap_point;
+  old_point = info->snap_point;
+  n_pages = adw_carousel_get_n_pages (self);
+
+  if (position < 0 || position > n_pages)
+    position = n_pages;
 
-  if (position < 0 || position >= adw_carousel_get_n_pages (self))
-    prev_link = g_list_last (self->children);
+  if (old_position == n_pages - 1 && position == n_pages)
+    return;
+
+  if (position == n_pages)
+    next_link = NULL;
+  else if (position > old_position)
+    next_link = get_nth_link (self, position + 1);
   else
-    prev_link = get_nth_link (self, position);
+    next_link = get_nth_link (self, position);
+
+  if (next_link) {
+    next_info = next_link->data;
+    new_point = next_info->snap_point;
 
-  prev_info = prev_link->data;
-  new_point = prev_info->snap_point;
-  if (new_point > old_point)
-    new_point -= prev_info->size;
+    /* Since we know position > old_position, it's not 0 so prev_info exists */
+    if (position > old_position) {
+      ChildInfo *prev_info = next_link->prev->data;
+
+      new_point = prev_info->snap_point;
+    }
+  } else {
+    GList *last_link = g_list_last (self->children);
+    ChildInfo *last_info = last_link->data;
+
+    new_point = last_info->snap_point;
+  }
 
   self->children = g_list_remove_link (self->children, link);
-  self->children = g_list_insert_before (self->children, prev_link, link->data);
+
+  if (next_link) {
+    self->children = g_list_insert_before_link (self->children, next_link, link);
+
+    gtk_widget_insert_before (child, GTK_WIDGET (self), next_info->widget);
+  } else {
+    self->children = g_list_append (self->children, info);
+    g_list_free (link);
+
+    gtk_widget_insert_before (child, GTK_WIDGET (self), NULL);
+  }
 
   if (closest_point == old_point)
-    shift_position (self, new_point - old_point);
-  else if (old_point > closest_point && closest_point >= new_point)
-    shift_position (self, info->size);
-  else if (new_point >= closest_point && closest_point > old_point)
-    shift_position (self, -info->size);
+    self->position_shift += new_point - old_point;
+  else if (old_point >= closest_point && closest_point >= new_point)
+    self->position_shift += info->size;
+  else if (new_point >= closest_point && closest_point >= old_point)
+    self->position_shift -= info->size;
+
+  gtk_widget_queue_allocate (GTK_WIDGET (self));
 }
 
 /**
diff --git a/tests/test-carousel.c b/tests/test-carousel.c
index 1a3e2d80..44cdb9b2 100644
--- a/tests/test-carousel.c
+++ b/tests/test-carousel.c
@@ -41,10 +41,6 @@ test_adw_carousel_add_remove (void)
   g_assert_cmpuint (adw_carousel_get_n_pages (carousel), ==, 3);
   g_assert_cmpint (notified, ==, 3);
 
-  adw_carousel_reorder (carousel, child3, 0);
-  g_assert_cmpuint (adw_carousel_get_n_pages (carousel), ==, 3);
-  g_assert_cmpint (notified, ==, 3);
-
   adw_carousel_remove (carousel, child1);
   g_assert_cmpuint (adw_carousel_get_n_pages (carousel), ==, 2);
   g_assert_cmpint (notified, ==, 4);
@@ -56,6 +52,100 @@ test_adw_carousel_add_remove (void)
   g_assert_finalize_object (carousel);
 }
 
+static void
+allocate_carousel (AdwCarousel *carousel)
+{
+  int width, height;
+
+  gtk_widget_measure (GTK_WIDGET (carousel), GTK_ORIENTATION_HORIZONTAL, -1,
+                      NULL, &width, NULL, NULL);
+  gtk_widget_measure (GTK_WIDGET (carousel), GTK_ORIENTATION_VERTICAL, -1,
+                      NULL, &height, NULL, NULL);
+  gtk_widget_allocate (GTK_WIDGET (carousel), width, height, 0, NULL);
+}
+
+static void
+assert_carousel_positions (AdwCarousel *carousel,
+                           GtkWidget   *child1,
+                           GtkWidget   *child2,
+                           GtkWidget   *child3,
+                           GtkWidget   *child4,
+                           double       position)
+{
+  allocate_carousel (carousel);
+  g_assert_true (adw_carousel_get_nth_page (carousel, 0) == child1);
+  g_assert_true (adw_carousel_get_nth_page (carousel, 1) == child2);
+  g_assert_true (adw_carousel_get_nth_page (carousel, 2) == child3);
+  g_assert_true (adw_carousel_get_nth_page (carousel, 3) == child4);
+  g_assert_cmpfloat (adw_carousel_get_position (carousel), ==, position);
+}
+
+static void
+test_adw_carousel_reorder (void)
+{
+  AdwCarousel *carousel = g_object_ref_sink (ADW_CAROUSEL (adw_carousel_new ()));
+  GtkWidget *child1, *child2, *child3, *child4;
+
+  child1 = gtk_label_new ("");
+  child2 = gtk_label_new ("");
+  child3 = gtk_label_new ("");
+  child4 = gtk_label_new ("");
+
+  adw_carousel_append (carousel, child1);
+  adw_carousel_append (carousel, child2);
+  adw_carousel_append (carousel, child3);
+  adw_carousel_append (carousel, child4);
+  allocate_carousel (carousel);
+
+  g_assert_cmpuint (adw_carousel_get_n_pages (carousel), ==, 4);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+
+  /* No-op */
+  adw_carousel_reorder (carousel, child1, 0);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+  adw_carousel_reorder (carousel, child2, 1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+  adw_carousel_reorder (carousel, child3, 2);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+  adw_carousel_reorder (carousel, child4, 3);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+
+  adw_carousel_reorder (carousel, child4, 4);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+  adw_carousel_reorder (carousel, child4, -1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+
+  adw_carousel_scroll_to (carousel, child1, FALSE);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+  adw_carousel_reorder (carousel, child2, 2);
+  assert_carousel_positions (carousel, child1, child3, child2, child4, 0);
+  adw_carousel_reorder (carousel, child2, 1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 0);
+
+  adw_carousel_scroll_to (carousel, child2, FALSE);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 1);
+  adw_carousel_reorder (carousel, child2, 2);
+  assert_carousel_positions (carousel, child1, child3, child2, child4, 2);
+  adw_carousel_reorder (carousel, child2, 1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 1);
+
+  adw_carousel_scroll_to (carousel, child3, FALSE);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 2);
+  adw_carousel_reorder (carousel, child2, 2);
+  assert_carousel_positions (carousel, child1, child3, child2, child4, 1);
+  adw_carousel_reorder (carousel, child2, 1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 2);
+
+  adw_carousel_scroll_to (carousel, child4, FALSE);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 3);
+  adw_carousel_reorder (carousel, child2, 2);
+  assert_carousel_positions (carousel, child1, child3, child2, child4, 3);
+  adw_carousel_reorder (carousel, child2, 1);
+  assert_carousel_positions (carousel, child1, child2, child3, child4, 3);
+
+  g_assert_finalize_object (carousel);
+}
+
 static void
 test_adw_carousel_interactive (void)
 {
@@ -204,6 +294,7 @@ main (int   argc,
   adw_init ();
 
   g_test_add_func("/Adwaita/Carousel/add_remove", test_adw_carousel_add_remove);
+  g_test_add_func("/Adwaita/Carousel/reorder", test_adw_carousel_reorder);
   g_test_add_func("/Adwaita/Carousel/interactive", test_adw_carousel_interactive);
   g_test_add_func("/Adwaita/Carousel/spacing", test_adw_carousel_spacing);
   g_test_add_func("/Adwaita/Carousel/allow_mouse_drag", test_adw_carousel_allow_mouse_drag);