(bug) "--vertical-space yes" adds two blank lines instead of one

[shentino]

I would have let the previous issue's responder report this, but the truth is I encountered it a long time ago and had since forgotten about this option.

Please see issue #581 for more details, this is a separate bugtracker

Edit: Add hash to cause cross-link of issues

[geoffmcl]

@shentino thank you for this issue... as you suggest, it has been around for a while...

Probably even since the vertical-space option was expanded from a simple boolean type, to an autoboolean type, and PCondFlushLineSmart( doc, indent ); and TY_(PFlushLineSmart)( doc, indent ); were introduced... see commit f697978, circa Jul 13, 2015...

Note that --vertical-space auto is a special case where nearly all newlines are removed from the output... see #231, but started in #228, and several other cross-referenced issues...

These need to be read, reviewed to appreciate this vertical space issue...

But --vertical-space yes is also mixed up with the indent option...

Given a simple input:

<ul>
  <li>foo</li>
  <li>bar</li>
  <li>baz</li>
</ul>

With a config of --show-body-only yes --show-info no --vertical-space yes I get an output of -

F:\Projects\tidy-html5\build\win64>tidy5 --show-body-only yes --show-info no --vertical-space yes F:\Projects\tidy-test\test\input5\in_581.html
No warnings or errors were found.

<ul>
<li>foo</li>

<li>bar</li>

<li>baz</li>
</ul>

Which is perfect. A single newline is added...

Now I add -i indent and get -

F:\Projects\tidy-html5\build\win64>tidy5 -i --show-body-only yes --show-info no --vertical-space yes F:\Projects\tidy-test\test\input5\in_581.html
No warnings or errors were found.

<ul>
  <li>foo</li>


  <li>bar</li>


  <li>baz</li>
</ul>

So, in this case, it is the additional interaction with the --indent auto option that seems to add an additional newline...

That might be a clue to look for where this extra newline is added...

We need to also test <p>, <dt>, <dd>, ... and any other elements effected by this...

Appreciate any comment, patches or PR on this... thanks...

[geoffmcl]

@shentino Ok, I can sort of see where it happens... all in pprint.c module...

Line 2349 does a TY_(PFlushLineSmart)( doc, indent ); if classic mode... We have already output [indent]<li>foo</li>\n, per Line 2451, and this adds a second newline...

We sort of have what we want... that is an extra newline, if classic mode...

But then a little later, Line 2364, if indent: auto, that is indsmart, and classic both are on, then the third newline is added...

Maybe this third newline is no longer required, and can be removed... but maybe this is not the problem...

Perhaps someone will get the chance to comment out 2364 and 2365 and run some tests, and show the results... we would also need to conduct the full regression tests... thanks...

[geoffmcl]

@shentino have pushed a fix to the issue-582 branch... check it out...

$ cd tidy-html5
$ git pull
$ git checkout issue-582
$ cd build/cmake
$ ./build-me.sh

As a sample test case, try in_582-1.html using config cfg_582-1.txt...

As suspected, just removing lines 2364-2365 did the trick - patch -

diff --git a/src/pprint.c b/src/pprint.c
index 68e8d44..87568b0 100644
--- a/src/pprint.c
+++ b/src/pprint.c
@@ -2340,7 +2340,8 @@ void TY_(PPrintTree)( TidyDocImpl* doc, uint mode, uint indent, Node *node )
         else /* other tags */
         {
             Bool indcont  = ( cfgAutoBool(doc, TidyIndentContent) != TidyNoState );
-            Bool indsmart = ( cfgAutoBool(doc, TidyIndentContent) == TidyAutoState );
+            /* Issue #582 - Seems this is no longer used
+               Bool indsmart = ( cfgAutoBool(doc, TidyIndentContent) == TidyAutoState ); */
             Bool hideend  = cfgBool( doc, TidyOmitOptionalTags );
             Bool classic  = TidyClassicVS; /* #228 - cfgBool( doc, TidyVertSpace ); */
             uint contentIndent = indent;
@@ -2360,9 +2361,11 @@ void TY_(PPrintTree)( TidyDocImpl* doc, uint mode, uint indent, Node *node )
              *  Issue #180 - with the above PCondFlushLine, 
              *  this adds an uneccessary additional line!
              *  Maybe only if 'classic' ie --vertical-space yes 
+             *  Issue #582 - maybe this is no longer needed!
+             *  It adds a 3rd newline if indent: auto...
+             *  if ( indsmart && node->prev != NULL && classic)
+             *   TY_(PFlushLineSmart)( doc, indent );
             \*/
-            if ( indsmart && node->prev != NULL && classic)
-                TY_(PFlushLineSmart)( doc, indent );
 
             /* do not omit elements with attributes */
             if ( !hideend || !TY_(nodeHasCM)(node, CM_OMITST) ||

Have done a full regression test and it passes...

Look forward to further feedback... thanks...

[shentino]

I tested it and it works on my end. Please merge to mainline ASAP so my distro can pick it up.

[shentino]

Issue 585 might be related.

[geoffmcl]

@shentino, since @balthisar has merged PR #583, thanks, this seems to have fixed this so closing...

And note you have openned #596 to cover other vertical space matters... thanks...