Uploaded image for project: 'Commons CLI'
  1. Commons CLI
  2. CLI-151

HelpFormatter wraps incorrectly on every line beyond the first

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2
    • Component/s: Help formatter
    • Labels:
      None

      Description

      The method findWrapPos(...) in the HelpFormatter is a couple of bugs in the way that it deals with the "startPos" variable. This causes it to format every line beyond the first line by "startPos" to many characters, beyond the specified width.

      To see this, create an option with a long description, and then use the help formatter to print it. The first line will be the correct length. The 2nd, 3rd, etc lines will all be too long.

      I don't have a patch (sorry) - but here is a corrected version of the method.

      I fixed it in two places - both were using "width + startPos" when they should have been using width.

       protected int findWrapPos(String text, int width, int startPos)
          {
              int pos = -1;
      
              // the line ends before the max wrap pos or a new line char found
              if (((pos = text.indexOf('\n', startPos)) != -1 && pos <= width)
                  || ((pos = text.indexOf('\t', startPos)) != -1 && pos <= width))
              {
                  return pos+1;
              }
              else if ((width) >= text.length())
              {
                  return -1;
              }
      
      
              // look for the last whitespace character before startPos+width
              pos = width;
      
              char c;
      
              while ((pos >= startPos) && ((c = text.charAt(pos)) != ' ')
                     && (c != '\n') && (c != '\r'))
              {
                  --pos;
              }
      
              // if we found it - just return
              if (pos > startPos)
              {
                  return pos;
              }
              
              // must look for the first whitespace chearacter after startPos 
              // + width
              pos = startPos + width;
      
              while ((pos <= text.length()) && ((c = text.charAt(pos)) != ' ')
                     && (c != '\n') && (c != '\r'))
              {
                  ++pos;
              }
      
              return (pos == text.length())        ? (-1) : pos;
          }
      

        Issue Links

          Activity

          Hide
          jlmuir J. Lewis Muir added a comment -

          I have observed this same problem.

          I'm not sure the suggested fix to findWrapPos is correct. When I tried it, the testFindWrapPos test case failed with

          [junit] wrap position 2 expected:<-1> but was:<16>
          [junit] junit.framework.AssertionFailedError:
          [junit]     wrap position 2 expected:<-1> but was:<16>
          [junit]   at org.apache.commons.cli.HelpFormatterTest.testFindWrapPos(
          [junit]     HelpFormatterTest.java:62)
          

          I think the problem is actually in HelpFormatter.renderWrappedText (HelpFormatter.java line 812). It calls findWrapPos with a start position of nextLineTabStop, but that's not right; it should be a start position of 0.

          The line wrapping works for me after making this correction. I will attach a patch containing this fix as well as an addition to the test cases to validate it.

          Show
          jlmuir J. Lewis Muir added a comment - I have observed this same problem. I'm not sure the suggested fix to findWrapPos is correct. When I tried it, the testFindWrapPos test case failed with [junit] wrap position 2 expected:<-1> but was:<16> [junit] junit.framework.AssertionFailedError: [junit] wrap position 2 expected:<-1> but was:<16> [junit] at org.apache.commons.cli.HelpFormatterTest.testFindWrapPos( [junit] HelpFormatterTest.java:62) I think the problem is actually in HelpFormatter.renderWrappedText ( HelpFormatter.java line 812). It calls findWrapPos with a start position of nextLineTabStop , but that's not right; it should be a start position of 0. The line wrapping works for me after making this correction. I will attach a patch containing this fix as well as an addition to the test cases to validate it.
          Hide
          jlmuir J. Lewis Muir added a comment -

          Line wrapping fix.

          Show
          jlmuir J. Lewis Muir added a comment - Line wrapping fix.
          Hide
          bayard Henri Yandell added a comment -

          Looks good to me - thanks for the report and the patch.

          svn ci -m "Applying J. Lewis Muir's patch from CLI-151 fixing HelpFormatter so it wraps properly on multiple lines"

          Sending src/java/org/apache/commons/cli/HelpFormatter.java
          Sending src/test/org/apache/commons/cli/HelpFormatterTest.java
          Transmitting file data ..
          Committed revision 654428.

          Show
          bayard Henri Yandell added a comment - Looks good to me - thanks for the report and the patch. — svn ci -m "Applying J. Lewis Muir's patch from CLI-151 fixing HelpFormatter so it wraps properly on multiple lines" Sending src/java/org/apache/commons/cli/HelpFormatter.java Sending src/test/org/apache/commons/cli/HelpFormatterTest.java Transmitting file data .. Committed revision 654428.
          Hide
          bayard Henri Yandell added a comment -

          This caused OutOfMemoryExceptions in some cases, so reopening.

          Show
          bayard Henri Yandell added a comment - This caused OutOfMemoryExceptions in some cases, so reopening.
          Hide
          bayard Henri Yandell added a comment -

          I've sat and looked at the code fresh, and I agree with the 0 as a fix here. Digging into the new failing test, I think the errors are in other code - so closing this out again.

          Show
          bayard Henri Yandell added a comment - I've sat and looked at the code fresh, and I agree with the 0 as a fix here. Digging into the new failing test, I think the errors are in other code - so closing this out again.

            People

            • Assignee:
              Unassigned
              Reporter:
              armbrust Dan Armbrust
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development