add max-width option to DESCRIBE

[travv0]

Another proposal, this one will wrap the description text if the total line length is longer than max-width. Also fixes the padding for lines after the first line of the description being one character shorter than the first line's padding. Like my other pull request, I'll wait to update the tests until I know this is something you're actually interested in.

[libre-man]

I really like the change, awesome! I think it could be a bit more efficient though, so I would propose the following:

(defparameter *breakpoints* '(#\Space #\Tab #\Linefeed #\Return #\Newline #\Page #\Vt)
  "A list of characters that will be considered a breakpoint when printing")

(defun add-text-padding (str &key padding newline (max-width 80))
  "Add padding to text STR. Every line except for the first one, will be
prefixed with PADDING spaces. If NEWLINE is non-NIL, newline character will
be prepended to the text making it start on the next line with padding
applied to every single line."
  (let* ((padding-next-lines (max 0 (1- padding)))
         (pad            (make-string padding      :initial-element #\Space))
         (pad-next-lines (make-string padding-next-lines :initial-element #\Space))
         (cur-index 0)
         (string-end (length str))
         (cur-max-width (- max-width padding)))
    (with-output-to-string (s)
      (when newline
        (format s "~%~a" pad))
      (loop :while (< cur-index string-end)
            :do (let ((next-breakpoint (next-breakpoint
                                        str
                                        cur-index
                                        cur-max-width)))
                  (when (> cur-index 0)
                    (write-char #\Newline s)
                    (write-string pad-next-lines s))
                  (write-string str s :start cur-index :end next-breakpoint)
                  ;; We skip all whitespace, maybe we should not do that?
                  (setf cur-index (or
                                   (position-if (lambda (c) (not (member c *breakpoints*)))
                                                str
                                                :start next-breakpoint
                                                :end string-end)
                                   string-end)
                        cur-max-width (- max-width padding-next-lines)))))))

(defun next-breakpoint (str cur-index max-width)
  "Find the index of the next whitespace character in STR starting
  from START-FROM. If whitespace not found, returns length of STR."
  (let* ((string-end (length str))
         (chars-left (- string-end cur-index)))
    (if (> max-width chars-left)
        string-end
        (loop
          :with last-feasible
          :for index :from (1+ cur-index) :below string-end
          :until (and last-feasible (> index max-width))
          :when (member (char str index) *breakpoints*)
            :do (setf last-feasible index)
          :finally (return (or last-feasible string-end))))))

What do you think?

[libre-man]

Nit: please move all conditions for the cond to their own line, this improves the diff when we add things later on.

[libre-man]

For consistency please use keywords for the loop keywords.

[libre-man]

I think that if we encounter this case (i.e. when we split in the middle of a word) we should in the best case try to find a nice place to hyphenate (which is impossible as we don't know the language), or simply allow lines longer than our maximum (my preference).

[travv0]

I believe the current behavior (returning the length of the string if it doesn't find a break character) will allow lines longer than the maximum when that's the case.

[libre-man]

This way of looping over each character and calling line-break-needed-p for each character is quite inefficient as we are looking forward for the next whitespace char for every character. I've added a more efficient suggestion in the review.

[libre-man]

I would prefer to call add-text-padding with the actual max-width value and let it handle subtractions for the padding.

[travv0]

I added you as a collaborator so you can update add-text-padding to use your updated code. I believe we want (padding-next-lines (max 0 (1- padding))) to be (padding-next-lines (max 0 padding)) though or else each line after the first will be indented one less than the first line. Actually, looking at it again, I don't think we need a separate pad and pad-next-lines for anything, since I assume we'll always want all the lines aligned.