Re: [Groff] pic bug between 1.18.1 and 1.19.x

[Zvezdan Petkovic]

On Fri, Jun 17, 2005 at 01:14:18PM +0200, Werner LEMBERG wrote:
> 
> > Also, it happens only on pics that use for loops.
> 
> Aah, there was indeed a change of the code!

It turns out it wasn't related to the for loop.
I think I've found a good fix.

I've run pic from groff-1.18.1.1 on my extracted test file.
Then I ran pic from groff-1.19.2pre.  The diff showed a lot of lines
like this

-----8<-----
...
 \m[darkgreen]
-.sp -1
 \D't 2.000p'\h'-2.000p'
 .sp -1
-\h'3.204i'\v'1.190i'\D'p0.000i -1.099i -1.648i 0.000i 0.000i 1.099i'
+\h'3.204i'\v'1.190i'\D'p 0.000i -1.099i -1.648i 0.000i 0.000i 1.099i'
 .sp -1
 \m[]
-.sp -1
 \M[darkgreen]
-.sp -1
 \m[darkgreen]
-.sp -1
 \D't -1.000p'\h'1.000p'
 .sp -1
 .lf 11
...
----->8-----

The diff with \h... is OK.
But, the .sp -1 lines are missing in 1.19.2pre and causing all the
trouble obviously.

Then I compared carefully 1.18.1.1 and 1.19.2pre for all the files in
src/preproc/pic and noticed this at the bottom:

-----8<-----
...
@@ -478,13 +480,15 @@
 void troff_output::set_fill(double f)
 {
   if (driver_extension_flag && f != last_fill) {
-    printf("\\D'f %du'\\h'%du'\n.sp -1\n", int(f*FILL_MAX), -int(f*FILL_MAX));
+    // \D'Fg ...' emits a node only in compatibility mode,
+    // thus we add a dummy node
+    printf("\\&\\D'Fg %.3f'\n.sp -1\n", 1.0 - f);
     last_fill = f;
   }
   if (last_filled) {
     free(last_filled);
     last_filled = 0;
-    printf("\\M[]\n.sp -1\n");
+    printf("\\M[]\n");
   }
 }
 
@@ -494,13 +498,15 @@
     if (last_filled || last_outlined) {
       reset_color();
     }
+    // \m and \M emit a node in compatibility mode only,
+    // but that won't work anyway
     if (color_fill) {
-      printf("\\M[%s]\n.sp -1\n", color_fill);
-      last_filled = strdup(color_fill);
+      printf("\\M[%s]\n", color_fill);
+      last_filled = strsave(color_fill);
     }
     if (color_outlined) {
-      printf("\\m[%s]\n.sp -1\n", color_outlined);
-      last_outlined = strdup(color_outlined);
+      printf("\\m[%s]\n", color_outlined);
+      last_outlined = strsave(color_outlined);
     }
   }
 }
@@ -509,13 +515,13 @@
 {
   if (driver_extension_flag) {
     if (last_filled) {
-      printf("\\M[]\n.sp -1\n");
-      free(last_filled);
+      printf("\\M[]\n");
+      a_delete last_filled;
       last_filled = 0;
     }
     if (last_outlined) {
-      printf("\\m[]\n.sp -1\n");
-      free(last_outlined);
+      printf("\\m[]\n");
+      a_delete last_outlined;
       last_outlined = 0;
     }
   }
----->8-----

Now, I tried to fix the things by adding ".sp -1\n" to each "\\m[]\n"
and "\\M[]\n", for example

      printf("\\m[]\n.sp -1\n");

as in 1.18.1.1.  That has fixed the problem with the vertical movement
between the colour changes and pauses, but the picture was bad because
some elements were moved vertically back.

Then I tried the change given in the attached patch and the pics are now
all right.  I tried it on two large presentations and it looks OK.

Werner, you know the best why the change was made in the first place.
Do you find this fix correct?  Is it safe to assume it won't spoil other
things?

While waiting your answer I'll test it on my own recent documents and
see do they behave correctly.

        Zvezdan Petkovic

patch-troff_cpp
Description: Text document