bugGNU roff - Bugs: bug #60673, adjustment algorithm should...

 
 

bug #60673: adjustment algorithm should disregard nonadjusted lines in its alternation pattern

Submitter:  Dave <barx>
Submitted:  Wed 26 May 2021 03:47:19 AM UTC
   
 
Category:  Core Severity:  1 - Wish
Item Group:  Feature change Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 04 Sep 2021 04:40:28 PM UTC, comment #7: 

I think the changes I have pushed today resolve the issues raised in this bug.  The ones in bug #57836 are not addressed.

My solution turned out to be different, simpler, and (perhaps) more elegant, but that was through accident rather than cleverness because I was setting out to resolve a different problem entirely.

Long story short: adjustment parity is not changed on lines where no adjustment will take place.

...aaaaaand I see a typo in the commit message.  Sigh.


commit b93eacd8d7c4fb5ac587c629a38d8a3732c03d76
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sun Sep 5 00:07:50 2021 +1000

    src/roff/troff/env.cpp: Refactor slightly.

    * src/roff/troff/env.cpp (distribute_space): Refactor slightly.
      - Rename `force_reverse` to `force_reverse_node_list`.
      - Rename `reverse` to `do_reverse`.
      - Demote both of the above from `int` to `bool`.
      - Use Boolean literals with them.
      - Add assertions to enforce positive values of `nspaces` and
        `desired_space`.
      - Remove now-rendundant test for `nspaces` being positive.
      - Add explanatory comments.

      (environment::wrap_up_field): Update call sites of `distribute_space`
      when non-default value of `force_reverse_node_list` is supplied.

commit 69efbe0a69a8e7de8904d78e3de8c7e8a58a8b92
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sat Sep 4 23:20:54 2021 +1000

    [troff]: Don't adjust nonadjustable lines.

    This means that the direction from which an output line in adjustment
    mode "b" (or its "n" synonym) is filled with supplemental space is not
    changed if that output line does not require adjustment.  This will
    result in whitespace changes to documents using that adjustment mode,
    and these changes will be plainly visible on low-resolution output
    devices like terminals.

    To illustrate, in the following "A" means an output line requiring
    adjustment; "F" a line that is "full" and does not; and "L" and "R"
    indicate distribution of adjustment spaces from the left and right,
    respectively.

    groff 1.22.4    groff 1.23.0
    ------------    ------------
    A    L          A    L
    A    R          A    R
    F    L          F    R
    A    R          A    L

    * src/roff/troff/env.cpp (distribute_space): Return early if either the
      amount of desired space to be distributed or the count of space nodes
      in the output line to distribute it among is zero.

    * tmac/tests/an_TH-repairs-ad-damage.sh: Update test to expect space to
      be distributed differently.

    Fixes <https://savannah.gnu.org/bugs/?61089> and
    <https://savannah.gnu.org/bugs/index.php?60673>.


G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 01:42:20 PM UTC, comment #6: 

Confirmed; my change to fix bug #60189 also addresses this issue as a collateral benefit.


--- 60673-groff-1.22.4  2021-09-04 23:38:39.425040471 +1000
+++ 60673-groff-branden-HEAD    2021-09-04 23:37:42.812757531 +1000
@@ -1,14 +1,14 @@
 While  the  example  in  bug  57836’s original report is somewhat
 contrived and a bit of an edge case in real life, there turns out
-to  be  a  more  innate  bug  in grotty’s balancing algorithm. As
+to be a more innate  bug  in  grotty’s  balancing  algorithm.  As
 mentioned before (and easily observable), when grotty adds spaces
 to  a  line  in  the  process  of justifying it, the algorithm it
 utilizes adds spaces from opposite ends of each line. But when it
-adds  this  space,  it  does  not take into account lines with no
+adds this space, it does not take  into  account  lines  with  no
 adjustment at all required. Therefore if space only need be added
 to  every  other  line  of  the text, all the space ends up being
 added to the same end of the line, degrading the uniform grayness
-of  the  output,  as  can  be  seen in this example. There is one
+of the output, as can be seen  in  this  example.  There  is  one
 fairly simple way to address this: grotty shouldn’t "count" lines
 that  don’t  need  to  be  adjusted; instead, it should apply the
 alternation pattern only to those lines that do need adjustment.


G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 12:43:52 PM UTC, comment #5: 

I meant to say bug #61089, of course, so that it would be linky.

G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 12:43:22 PM UTC, comment #4: 

Heads up.

It looks like my planned changes to address #60189 are going to affect this.

You may want to play around with this.


diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 4af8741e1..5e7017b8f 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2053,6 +2053,10 @@ static node *node_list_reverse(node *n)
 static void distribute_space(node *n, int nspaces, hunits desired_space,
                             int force_reverse = 0)
 {
+  if (desired_space.is_zero() || nspaces == 0) {
+    debug("distribute_space(): no work to do");
+    return;
+  }
   static int reverse = 0;
   if (force_reverse || reverse)
     n = node_list_reverse(n);
@@ -2070,7 +2074,6 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
     (void)node_list_reverse(n);
   if (!force_reverse)
     reverse = !reverse;
-  assert(desired_space.is_zero() && nspaces == 0);
 }

 void environment::possibly_break_line(int start_here, int forced)


(The debug() diagnostic will not survive into the push.  It shows up a LOT in our in-tree document corpus.)

I haven't thought deeply enough to be sure, but the above change might actually resolve this bug.  What do you think?

G. Branden Robinson <gbranden>
Group administrator
Fri 20 Aug 2021 10:53:20 AM UTC, comment #3: 

comment #2:

> The patch still needs work:

An additional item to add to this list:

  • Following the style env.cpp is migrating to, inhibit_reverse should probably be a bool rather than an int.  (But then, so should distribute_space()'s existing "reverse" and "force_reverse.")
Dave <barx>
Group Member
Mon 31 May 2021 02:14:44 AM UTC, comment #2: 

I've now tested the patch, in both nroff and troff, on some of my own documents, and verified that it

  • has the desired effect in nroff, and
  • does not affect troff output.


By "does not affect," I mean does not visibly change.  Diffs of PostScript output (comparing the patched and unpatched distribute_space()) are noisy because of the effect of bug #60707, which "shifts" many horizontal positions by 1/1000th of a point.

The patch still needs work:

  • The new code should include some comments explaining itself.  Yes, this contravenes the style of the rest of env.cpp.
  • The documentation needs to be amended as noted in comment #1.


But before I put any more work into it, I'd like some feedback on whether others think the approach is sound or have better ideas how to address the problem.

Dave <barx>
Group Member
Wed 26 May 2021 06:34:50 AM UTC, comment #1: 

Based on aforementioned code snippet in bug #60665, it looks like fixing this may be as simple as three lines of code:


diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 1be86470..312c0f7d 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2087,6 +2087,7 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
                             int force_reverse = 0)
 {
   static int reverse = 0;
+  int inhibit_reverse;
   if (force_reverse || reverse)
     n = node_list_reverse(n);
   if (!force_reverse && nspaces > 0 && spread_limit >= 0
@@ -2097,11 +2098,12 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
     if (Ems > spread_limit)
       output_warning(WARN_BREAK, "spreading %1m per space", Ems);
   }
+  inhibit_reverse = desired_space.is_zero();
   for (node *tem = n; tem; tem = tem->next)
     tem->spread_space(&nspaces, &desired_space);
   if (force_reverse || reverse)
     (void)node_list_reverse(n);
-  if (!force_reverse)
+  if (!force_reverse && !inhibit_reverse)
     reverse = !reverse;
   assert(desired_space.is_zero() && nspaces == 0);
 }


Caveats:

  • This is completely untested beyond confirming that it creates the "desired" output handcrafted in comment #0.  Someday I'll have to get around to learning how to use the test suite we have now.
  • My C is rusty and my C++ skills nonexistent, so this may not be the most idiomatic way to do this.
  • As #60665 observes, the alternating behavior is already a change from AT&T troff necessitating a note in the documentation; this would be a further change that should also be documented.
Dave <barx>
Group Member
Wed 26 May 2021 03:47:19 AM UTC, original submission:  

This was originally reported in a comment in bug #57836, but I now feel it's a separate enough problem to merit its own bug report.

The text below, mostly unaltered from that previous report, characterizes this as "grotty" behavior, but it turns out, as Branden explained in bug #60665, that the relevant code is device independent; it's just that the line-alternation pattern is effectively invisible in typeset output.


$ nroff tty | uniq
While  the  example  in  bug  57836's original report is somewhat
contrived and a bit of an edge case in real life, there turns out
to  be  a  more  innate  bug  in grotty's balancing algorithm. As
mentioned before (and easily observable), when grotty adds spaces
to  a  line  in  the  process  of justifying it, the algorithm it
utilizes adds spaces from opposite ends of each line. But when it
adds  this  space,  it  does  not take into account lines with no
adjustment at all required. Therefore if space only need be added
to  every  other  line  of  the text, all the space ends up being
added to the same end of the line, degrading the uniform grayness
of  the  output,  as  can  be  seen in this example. There is one
fairly simple way to address this: grotty shouldn't "count" lines
that  don't  need  to  be  adjusted; instead, it should apply the
alternation pattern only to those lines that do need adjustment.

$ cat tty
.nh
.ss 12 0
While the example in bug 57836's original report is somewhat
contrived and a bit of an edge case in real life, there turns out
to be a more innate bug in grotty's balancing algorithm. As
mentioned before (and easily observable), when grotty adds spaces
to a line in the process of justifying it, the algorithm it
utilizes adds spaces from opposite ends of each line. But when it
adds this space, it does not take into account lines with no
adjustment at all required. Therefore if space only need be added
to every other line of the text, all the space ends up being
added to the same end of the line, degrading the uniform grayness
of the output, as can be seen in this example. There is one
fairly simple way to address this: grotty shouldn't "count" lines
that don't need to be adjusted; instead, it should apply the
alternation pattern only to those lines that do need adjustment.
$


Here is the same paragraph adjusted by hand using the suggested change to the algorithm.  The overall grayness does not change from one side of the column to the other as it did before; it's much more pleasing to the eye, while being no harder (or easier) to read.  (If you're reading this in an email client that uses a proportional typeface, neither example will display properly.  Look at them in the bug tracker.)


While  the  example  in  bug  57836's original report is somewhat
contrived and a bit of an edge case in real life, there turns out
to be a more innate  bug  in  grotty's  balancing  algorithm.  As
mentioned before (and easily observable), when grotty adds spaces
to  a  line  in  the  process  of justifying it, the algorithm it
utilizes adds spaces from opposite ends of each line. But when it
adds this space, it does not take  into  account  lines  with  no
adjustment at all required. Therefore if space only need be added
to  every  other  line  of  the text, all the space ends up being
added to the same end of the line, degrading the uniform grayness
of the output, as can be seen  in  this  example.  There  is  one
fairly simple way to address this: grotty shouldn't "count" lines
that  don't  need  to  be  adjusted; instead, it should apply the
alternation pattern only to those lines that do need adjustment.


Dave <barx>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by barx (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-09-04 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-09-04 gbranden StatusNone In Progress
        Assigned toNone gbranden

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code