[dia] Fix Beziergon behavior after restructuring

[Hans Breuer <hans src gnome org>]

commit ebe144ce9d956470261fcab32b3e2ffb634754cf
Author: Hans Breuer <hans breuer org>
Date:   Fri Dec 13 23:00:03 2013 +0100

    Fix Beziergon behavior after restructuring
    
    Adding and removing points (segments) to the Beziergon did not work as
    before due to changes from 62dab7da68. Actually that commit did not take
    into account, that bezierconn_closest_segment() returned 0 for the first
    segment but beziershape_closest_segment() returned 1.
    Now the segment index is zero-based as expected and the Beziergon code
    use the appropriate offsets to deal with it.
    Also a unit-test is specifically written for Add/Remove Segment to not
    introduce such bugs with upcoming restructuring.

 lib/bezier-common.c          |   13 +++--
 lib/bezier_conn.c            |   10 ++--
 lib/beziershape.c            |   20 +++----
 objects/standard/beziergon.c |    2 +-
 tests/test-objects.c         |  121 +++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 142 insertions(+), 24 deletions(-)
---
diff --git a/lib/bezier-common.c b/lib/bezier-common.c
index 2a04969..6c2d118 100644
--- a/lib/bezier-common.c
+++ b/lib/bezier-common.c
@@ -140,16 +140,17 @@ beziercommon_closest_segment (BezierCommon *bezier,
 
   closest = 0;
   last = bezier->points[0].p1;
-  for (i = 0; i < bezier->num_points - 1; i++) {
-    real new_dist = distance_bez_seg_point(&last, &bezier->points[i+1], line_width, point);
+  /* the first point is just move-to so there is no need to consider p2,p3 of it */
+  for (i = 1; i < bezier->num_points; i++) {
+    real new_dist = distance_bez_seg_point(&last, &bezier->points[i], line_width, point);
     if (new_dist < dist) {
       dist = new_dist;
-      closest = i;
+      closest = i - 1;
     }
-    if (bezier->points[i+1].type == BEZ_CURVE_TO)
-      last = bezier->points[i+1].p3;
+    if (bezier->points[i].type == BEZ_CURVE_TO)
+      last = bezier->points[i].p3;
     else
-      last = bezier->points[i+1].p1;
+      last = bezier->points[i].p1;
   }
   return closest;
 }
diff --git a/lib/bezier_conn.c b/lib/bezier_conn.c
index ea96e95..c8c4c57 100644
--- a/lib/bezier_conn.c
+++ b/lib/bezier_conn.c
@@ -189,7 +189,7 @@ bezierconn_move_handle (BezierConn *bezier,
        point_sub(&pt, &bezier->bezier.points[comp_nr].p3);
        if (point_len(&pt) > 0)
          point_normalize(&pt);
-       else { pt.x = 1.0; pt.y = 0.0; }          
+       else { pt.x = 1.0; pt.y = 0.0; }
        point_scale(&pt, -len);
        point_add(&pt, &bezier->bezier.points[comp_nr].p3);
        bezier->bezier.points[comp_nr+1].p1 = pt;
@@ -220,7 +220,7 @@ bezierconn_move_handle (BezierConn *bezier,
        point_sub(&pt, &bezier->bezier.points[comp_nr-1].p3);
        if (point_len(&pt) > 0)
          point_normalize(&pt);
-       else { pt.x = 1.0; pt.y = 0.0; }          
+       else { pt.x = 1.0; pt.y = 0.0; }
        point_scale(&pt, -len);
        point_add(&pt, &bezier->bezier.points[comp_nr-1].p3);
        bezier->bezier.points[comp_nr-1].p2 = pt;
@@ -504,12 +504,14 @@ bezierconn_remove_segment (BezierConn *bezier, int pos)
   ConnectionPoint *cpt1, *cpt2, *cpt3;
   BezPoint old_point;
   BezCornerType old_ctype;
-  int next = pos+1;
+  int next;
 
   g_assert(pos > 0);
   g_assert(bezier->bezier.num_points > 2);
 
-  if (pos == bezier->bezier.num_points-1) pos--;
+  if (pos == bezier->bezier.num_points-1)
+    pos--;
+  next = pos+1;
 
   old_handle1 = bezier->object.handles[3*pos-2];
   old_handle2 = bezier->object.handles[3*pos-1];
diff --git a/lib/beziershape.c b/lib/beziershape.c
index 172588d..1c5a97e 100644
--- a/lib/beziershape.c
+++ b/lib/beziershape.c
@@ -452,14 +452,11 @@ beziershape_add_segment (BezierShape *bezier,
 
   g_return_val_if_fail (segment >= 0 && segment < bezier->bezier.num_points, NULL);
 
-  if (segment == 0) /* don't want to add this, just take the next one */
-    ++segment;
-
-  if (segment != 1)
-    startpoint = bezier->bezier.points[segment-1].p3;
+  if (bezier->bezier.points[segment].type == BEZ_CURVE_TO)
+    startpoint = bezier->bezier.points[segment].p3;
   else 
-    startpoint = bezier->bezier.points[0].p1;
-  other = bezier->bezier.points[segment].p3;
+    startpoint = bezier->bezier.points[segment].p1;
+  other = bezier->bezier.points[segment+1].p3;
   if (point == NULL) {
     realpoint.p1.x = (startpoint.x + other.x)/6;
     realpoint.p1.y = (startpoint.y + other.y)/6;
@@ -488,10 +485,10 @@ beziershape_add_segment (BezierShape *bezier,
   new_cp2 = g_new0(ConnectionPoint, 1);
   new_cp1->object = &bezier->object;
   new_cp2->object = &bezier->object;
-  add_handles(bezier, segment, &realpoint, corner_type,
+  add_handles(bezier, segment+1, &realpoint, corner_type,
              new_handle1, new_handle2, new_handle3, new_cp1, new_cp2);
   return beziershape_create_point_change(bezier, TYPE_ADD_POINT,
-                                        &realpoint, corner_type, segment,
+                                        &realpoint, corner_type, segment+1,
                                         new_handle1, new_handle2, new_handle3,
                                         new_cp1, new_cp2);
 }
@@ -512,12 +509,13 @@ beziershape_remove_segment (BezierShape *bezier, int pos)
   BezCornerType old_ctype;
   int next = pos+1;
 
-  g_assert(pos > 0);
+  g_return_val_if_fail (pos > 0 && pos < bezier->bezier.num_points, NULL);
   g_assert(bezier->bezier.num_points > 2);
-  g_assert(pos < bezier->bezier.num_points);
 
   if (pos == bezier->bezier.num_points - 1)
     next = 1;
+  else if (next == 1)
+    next = bezier->bezier.num_points - 1;
 
   old_handle1 = bezier->object.handles[3*pos-3];
   old_handle2 = bezier->object.handles[3*pos-2];
diff --git a/objects/standard/beziergon.c b/objects/standard/beziergon.c
index 7fde315..2f1c6fd 100644
--- a/objects/standard/beziergon.c
+++ b/objects/standard/beziergon.c
@@ -469,7 +469,7 @@ beziergon_delete_segment_callback (DiaObject *obj, Point *clicked, gpointer data
   ObjectChange *change;
   
   seg_nr = beziergon_closest_segment(bezier, clicked);
-  change = beziershape_remove_segment(&bezier->bezier, seg_nr);
+  change = beziershape_remove_segment(&bezier->bezier, seg_nr+1);
 
   beziergon_update_data(bezier);
   return change;
diff --git a/tests/test-objects.c b/tests/test-objects.c
index ef09b59..8280dc8 100644
--- a/tests/test-objects.c
+++ b/tests/test-objects.c
@@ -518,7 +518,7 @@ _test_object_menu (gconstpointer user_data)
     g_test_message ("SKIPPED (n.i.)!");
   } else {
     DiaMenu *menu = (o->ops->get_object_menu)(o, &from); /* clicked_pos should not matter much */
-    /* strangley enough I found a crash with menu==NULL today ;) */
+    /* strangely enough I found a crash with menu==NULL today ;) */
     int num_items = menu ? menu->num_items : 0;
 
     for (i = 0; i < num_items; ++i) {
@@ -623,6 +623,109 @@ _test_distance_from (gconstpointer user_data)
   o->ops->destroy (o);
   g_free (o);
 }
+
+#include "lib/prop_geomtypes.h" /* BezPointarrayProperty */
+
+/* there is no v-table entry for Add/Delete (Corner|Segment)
+ * so we have to search the object menu
+ */
+static ObjectChange *
+_change_point (DiaObject *o, const gchar *verb, Point *pt)
+{
+  int i;
+  DiaMenu *menu = (o->ops->get_object_menu)(o, pt); /* clicked_pos should not matter much */
+  /* strangely enough I found a crash with menu==NULL today ;) */
+  int num_items = menu ? menu->num_items : 0;
+
+  for (i = 0; i < num_items; ++i) {
+    DiaMenuItem *item = &menu->items[i];
+
+    if (item->text && strncmp (item->text, verb, strlen(verb)) == 0) {
+      if (item->callback && (item->active & DIAMENU_ACTIVE))
+       return (item->callback)(o, pt, item->callback_data);
+    }
+  }
+  return NULL;
+}
+
+static void
+_test_segments (gconstpointer user_data)
+{
+  const DiaObjectType *type = (const DiaObjectType *)user_data;
+  Handle *h1 = NULL, *h2 = NULL;
+  Point from = {0, 0};
+  DiaObject *o = type->ops->create (&from, type->default_user_data, &h1, &h2);
+  Property *prop;
+
+  /* only few object support to add/remove objects, filter to these objects first */
+  if ((prop = object_prop_by_name_type (o, "bez_points", PROP_TYPE_BEZPOINTARRAY)) != NULL) {
+    /* get the points array to compare against */
+    GArray *d1 = ((BezPointarrayProperty *)prop)->bezpointarray_data;
+    /* add and delete some points */
+    int n;
+    for (n = 0; n < d1->len - 1; ++n) {
+      ObjectChange *ch1, *ch2;
+#define SELECT_BP(d,j,k) g_array_index(d, BezPoint, j).type == BEZ_CURVE_TO ? g_array_index(d, BezPoint, 
j).p3.##k : g_array_index(d, BezPoint, j).p1.##k
+      from.x = (SELECT_BP(d1,n,x) + SELECT_BP(d1,n+1,x)) / 2;
+      from.y = (SELECT_BP(d1,n,y) + SELECT_BP(d1,n+1,y)) / 2;
+#undef SELECT_BP
+      if ((ch1 = _change_point (o, "Add ", &from)) != NULL) {
+       int i;
+       Property *prop2 = object_prop_by_name_type (o, "bez_points", PROP_TYPE_BEZPOINTARRAY);
+       GArray *d2 = ((BezPointarrayProperty *)prop2)->bezpointarray_data;
+       g_assert (d1->len == d2->len - 1);
+       ch2 = _change_point (o, "Delete ", &from);
+       prop2->ops->free (prop2);
+       /* adding and deleting the same point shall lead to the initial state */
+       prop2 = object_prop_by_name_type (o, "bez_points", PROP_TYPE_BEZPOINTARRAY);
+       d2 = ((BezPointarrayProperty *)prop2)->bezpointarray_data;
+       for (i = 0; i < d1->len; ++i) {
+         BezPoint *bp1 = &g_array_index(d1, BezPoint, i);
+         BezPoint *bp2 = &g_array_index(d2, BezPoint, i);
+         g_assert_cmpfloat (fabs (bp1->p1.x - bp2->p1.x), <, EPSILON); /* for all types of BezPoint */
+         g_assert_cmpfloat (fabs (bp1->p1.y - bp2->p1.y), <, EPSILON); /* - " - */
+         g_assert (bp1->type == bp2->type);
+         if (bp1->type != BEZ_CURVE_TO)
+           continue;
+         g_assert_cmpfloat (fabs (bp1->p2.x - bp2->p2.x), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p2.y - bp2->p2.y), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p3.x - bp2->p3.x), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p3.y - bp2->p3.y), <, EPSILON);
+       }
+       prop2->ops->free (prop2);
+       /* Check if undo is reconstructing the object, too */
+       (ch2->revert)(ch2, o);
+       _object_change_free(ch2);
+       (ch1->revert)(ch1, o);
+       _object_change_free(ch1);
+       prop2 = object_prop_by_name_type (o, "bez_points", PROP_TYPE_BEZPOINTARRAY);
+       d2 = ((BezPointarrayProperty *)prop2)->bezpointarray_data;
+       for (i = 0; i < d1->len; ++i) {
+         BezPoint *bp1 = &g_array_index(d1, BezPoint, i);
+         BezPoint *bp2 = &g_array_index(d2, BezPoint, i);
+         g_assert_cmpfloat (fabs (bp1->p1.x - bp2->p1.x), <, EPSILON); /* for all types of BezPoint */
+         g_assert_cmpfloat (fabs (bp1->p1.y - bp2->p1.y), <, EPSILON); /* - " - */
+         g_assert (bp1->type == bp2->type);
+         if (bp1->type != BEZ_CURVE_TO)
+           continue;
+         g_assert_cmpfloat (fabs (bp1->p2.x - bp2->p2.x), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p2.y - bp2->p2.y), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p3.x - bp2->p3.x), <, EPSILON);
+         g_assert_cmpfloat (fabs (bp1->p3.y - bp2->p3.y), <, EPSILON);
+       }
+       prop2->ops->free (prop2);
+      }
+    }
+  } else if ((prop = object_prop_by_name_type (o, "points", PROP_TYPE_POINTARRAY)) != NULL) {
+  } else {
+    g_test_message ("n.a. ");
+  }
+  if (prop)
+    prop->ops->free (prop);
+  /* finally */
+  o->ops->destroy (o);
+  g_free (o);
+}
 /*
  * A dictionary interface to all registered object(-types)
  */
@@ -673,15 +776,29 @@ _ot_item (gpointer key,
   g_test_add_data_func (testpath, type, _test_distance_from);
   g_free (testpath);
 
+  testpath = g_strdup_printf ("%s/%s/%s", base, name, "Segments");
+  g_test_add_data_func (testpath, type, _test_segments);
+  g_free (testpath);
+
   ++num_objects;
 }
 
+#ifdef G_OS_WIN32
+#define Rectangle win32Rectangle
+#include <windows.h>
+#endif
+
 int
 main (int argc, char** argv)
 {
   GList *plugins = NULL;
   int ret = 0;
-  
+
+#ifdef G_OS_WIN32
+  /* No dialog if it fails, please. */
+  SetErrorMode(SetErrorMode(0) | SEM_NOGPFAULTERRORBOX);
+#endif
+
   /* not using gtk_test_init() means we can only test non-gtk facilities of objects */
   g_type_init ();
   g_test_init (&argc, &argv, NULL);