Re: Patch: word-wrapping for 'help' output

[Mark Wielaard <mark at klomp dot org>]

Hi Tom,

On Fri, 2008-02-29 at 13:27 -0700, Tom Tromey wrote:
> This patch implements word-wrapping for frysk 'help' output.

Wow, this is really useful!

> frysk-core/frysk/hpd/ChangeLog:
> 2008-02-29  Tom Tromey  <tromey@redhat.com>
> 
> 	* ParameterizedCommand.java (help): Use getWordWrapWriter.  Set
> 	indent for wrapping.
> 	* NoOptsCommand.java (help): Use getWordWrapWriter.
> 	* MultiLevelCommand.java (help): use getWordWrapWriter.  Set
> 	indent for wrapping.
> 	* Command.java (help): Use getWordWrapWriter.
> 	* CLI.java (getWordWrapWriter): New method.
> 
> frysk-core/frysk/util/ChangeLog:
> 2008-02-29  Tom Tromey  <tromey@redhat.com>
> 
> 	* WordWrapWriter.java: New file.

Some very minor questions and suggestions. It is pretty good. Also I saw
no test regressions. Nice work.

> diff --git a/frysk-core/frysk/hpd/CLI.java b/frysk-core/frysk/hpd/CLI.java
> index 35bc9e5..67b3788 100644
> --- a/frysk-core/frysk/hpd/CLI.java
> +++ b/frysk-core/frysk/hpd/CLI.java
> @@ -61,6 +61,7 @@ import frysk.rt.ProcTaskIDManager;
>  import frysk.stepping.SteppingEngine;
>  import frysk.stepping.TaskStepEngine;
>  import frysk.util.CountDownLatch;
> +import frysk.util.WordWrapWriter;
>  import frysk.expr.Expression;
>  import frysk.expr.ScratchSymTab;
>  import frysk.expr.ExprSymTab;
> @@ -181,6 +182,10 @@ public class CLI {
>              idManager.manageProc(proc, this.taskID);
>      }
>  
> +    WordWrapWriter getWordWrapWriter() {
> +	return new WordWrapWriter(outWriter);
> +    }

This could use a method description.
The CLI doesn't have the PtyTerminal (created in fhpd.java).
It might make sense to pass that to the CLI so it can use
getTerminalWidth() here when constructing the WordWrapWriter.

> +/**
> + * This class is a PrintWriter which word-wraps the output.  It
> + * provides settings which control over the number of columns and
> + * indentation of wrapped lines.
> + */

An example would be nice.

> +public class WordWrapWriter extends PrintWriter {
> +    // Number of columns available.
> +    private int columns;
> +
> +    // Amount of indentation to use on wrapped line.
> +    private int wrapIndent;
> +
> +    // The break iterator, used to find the next line-break
> +    // opportunity.
> +    private BreakIterator iter;
> +
> +    // The current column.
> +    private int column;
> +
> +    public WordWrapWriter(Writer outStream, int columns, int wrapIndent, Locale locale) {
> +	// Always enable auto-flush.
> +	super(outStream, true);
> +	this.columns = columns;
> +	this.wrapIndent = wrapIndent;
> +	this.iter = BreakIterator.getWordInstance(locale);
> +    }
> +
> +    public WordWrapWriter(Writer outStream, int columns) {
> +	this(outStream, columns, 0, Locale.getDefault());
> +    }
> +
> +    public WordWrapWriter(Writer outStream) {
> +	this(outStream, 72, 0, Locale.getDefault());
> +    }

The defaults should really be documented.

> +    public void setWrapIndentFromColumn() {
> +	this.wrapIndent = this.column;
> +    }

This could really use a comment. It took me a while to realize why it is
useful.

> +    public void write(char[] buf, int offset, int len) {
> +	// A bit inefficient but we don't care much.
> +	write(new String(buf, offset, len));
> +    }
> +
> +    public void write(int b) {
> +	write(String.valueOf((char) b));
> +    }

A comment about which write methods there are in a PrintWriter would
have helped. I actually had to double check to see if you overrode them
all and whether the print() methods all delegate to the write() methods.

Thanks,

Mark