Re: Introducing two new markup-commands: draw-dashed-line and draw-dotted-line. (issue 7029045)

[thomasmorley65]

On 2012/12/31 14:59:45, david.nalesnik wrote:
> Harm--

> This looks great!  Thank you for the 'full-length option.  I can't be
alone in
> hating lines ending with incomplete dashes.

Hi David,

thanks for reviewing!

> All I have is a suggestion or two, and some quibbles.

> Besides what I've pointed out inline, I should mention that I got a
number of
> whitespace errors when I applied your patch.  There are a number of
spots where
> you should trim the ends of lines (mostly in the .scm file).

Done. (Hope so)


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm
> File scm/define-markup-commands.scm (right):


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode155
> scm/define-markup-commands.scm:155: If @code{full-length} is set
@code{#t}
> (default) the dashed-line extends to the
> set to #t

Done


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode156
> scm/define-markup-commands.scm:156: whole length given by @var{dest},
without
> white space at begin/end.
> beginning or end.

Done


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode169
> scm/define-markup-commands.scm:169: (let* (;; Get the line-thickness.
> I think your variable name is sufficient--no comment needed.

Ok. Deleted.


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode181
> scm/define-markup-commands.scm:181: (begin
> Branches of if expression should be aligned with test (here and
elsewhere).

Done


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode195
> scm/define-markup-commands.scm:195: (new-off (/ (- line-length corr (*
(+ 1
> guess) on)) guess))
> Why not use (1+ guess)

Done


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode231
> scm/define-markup-commands.scm:231: #f)
> Could omit the boolean--value will be unspecified.

Done


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode233
> scm/define-markup-commands.scm:233: ;; If `on´ or `off´ is negative,
or both,
> `on´ and `off´ equals zero a
> Possibly "...or the sum of `on' and `off' equals [is] zero..."

Changed


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode244
> scm/define-markup-commands.scm:244: #f)
> Here again, you could omit the alternate.

Done



https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode248
> scm/define-markup-commands.scm:248: (interval-widen (ordered-cons 0 x)
> half-thick)
> You do such a thorough job of commenting everything, but you don't
explain why
> you need `half-thick' here.

Comment added.


https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#newcode264
> scm/define-markup-commands.scm:264: Manual settings for @code{off} is
possible
> to get larger or smaller space
> "are possible"

Done.



https://codereview.appspot.com/7029045/