Add dots to tocItemMarkup

Add dots to tocItemMarkup

* scm/define-markup-commands.scm
  New markup command : fill-with-pattern

* ly/toc-init.ly
  Add dots to tocItemMarkup

[Carl, 2011-02-16 13:23:00]

Looks very good.

Just a couple more comments.

Thanks,

Carl

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:994: (define (nest-patterns pattern pattern-width
distance count patterns)
It might be even better if this were defined as a markup command, for example
pattern-markup.  Then anybody else (like me) could use it for making a repeating
pattern of markups (like a LyricExtender for shape notes).

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:1032: (x-offset (+ (* (- (- middle-width (*
patterns-number period)) pattern-width) (/ (1+ dir) 2)) (abs (car
pattern-x-extent)))))
Wrap to proper formatting at about 80 characters.

[Bertrand Bordage, 2011-02-16 14:26:45]

A few questions.

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:994: (define (nest-patterns pattern pattern-width
distance count patterns)
Ok. So, the input arguments will be "count", "pattern" and "distance" (in this
order) ? Or just "count" and "pattern" ?
With the property "word-space" instead of "distance"...

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:1021: }
Maybe a smaller example ? Shall we see more possibilities ? With a \override
#'(line-width . 50) for example.

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:1032: (x-offset (+ (* (- (- middle-width (*
patterns-number period)) pattern-width) (/ (1+ dir) 2)) (abs (car
pattern-x-extent)))))
It will be done (my Lord ;o) )

[Carl, 2011-02-16 14:36:46]

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:994: (define (nest-patterns pattern pattern-width
distance count patterns)
On 2011/02/16 14:26:45, Bertrand Bordage wrote:
> Ok. So, the input arguments will be "count", "pattern" and "distance" (in this
> order) ? Or just "count" and "pattern" ?

As a markup function, they should be count, pattern, distance.  The order
doesn't really matter.   My preference would be pattern, count, distance.

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:1021: }
On 2011/02/16 14:26:45, Bertrand Bordage wrote:
> Maybe a smaller example ? Shall we see more possibilities ? With a \override
> #'(line-width . 50) for example.

I think what's here is sufficient.  But I wouldn't be opposed to a change.

http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:1032: (x-offset (+ (* (- (- middle-width (*
patterns-number period)) pattern-width) (/ (1+ dir) 2)) (abs (car
pattern-x-extent)))))
On 2011/02/16 14:26:45, Bertrand Bordage wrote:
> (my Lord ;o) )
     ^^^^^^ -- Yikes!

[Bertrand Bordage, 2011-02-17 00:49:31]

Changes done !
It is more clean now.
I'm not sure if the commands are in the right categories.

I also noticed numerous commands have @code instead of @var and vice versa in
their descriptions. Center-align's args or fill-line's line-width for example.
Should I do a small patch to fix these ?

Regards,
Bertrand

[Bertrand Bordage, 2011-02-17 01:24:30]

Maybe add \pattern to the changelog ?

[Carl, 2011-02-17 03:19:04]

Looks good.  I had one comment.

Feel free to add an entry to the changelog.  It's done by editing the file
Documentation/changes.tely

Thanks,

Carl

http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3403: (define-markup-command (pattern layout
props pattern count space)
Now that I see it written -- should there be a direction argument for pattern?
Or at least should we document that it's a horizontal repeat?

http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3416: (if (zero? i)
Nicely done!

[Bertrand Bordage, 2011-02-18 21:01:11]

I added a "dir" argument to \pattern, so that we can get vertical repeats.
Also made a changelog item.

Bertrand

[Graham Percival (old account), 2011-02-22 07:29:13]

lgtm

[Neil Puttock, 2011-02-23 23:32:51]

http://codereview.appspot.com/4182056/diff/12001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/4182056/diff/12001/Documentation/changes.tely#n...
Documentation/changes.tely:65: New markup commands @code{\pattern} and
@code{\fill-with-pattern}
Please make this a full sentence.

http://codereview.appspot.com/4182056/diff/12001/ly/toc-init.ly
File ly/toc-init.ly (right):

http://codereview.appspot.com/4182056/diff/12001/ly/toc-init.ly#newcode30
ly/toc-init.ly:30: \fill-with-pattern . #1 #RIGHT \fromproperty #'toc:text
\fromproperty #'toc:page
Please keep the original as default and make this an option instead.

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3403: (define-markup-command (pattern layout
props pattern count space dir)
make `pattern' last arg

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3409: If @var{dir} is @code{CENTER}, patterns are
spread horizontally.
It would make more sense to use axis here (i.e., X/Y or 0/1), since there's no
distinction between UP/DOWN/LEFT/RIGHT.

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3422: (ly:stencil-extent (interpret-markup layout
props pattern) X))))
indent:

(pattern-width (interval-length
                (ly:stencil-extent (interpret-markup layout props pattern) X))))

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3425: (interpret-markup
indent:

(if (zero? i)
    (interpret-markup

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3431: (loop (1- i)
indent (goes with (zero? i) above)

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3434: (markup #:column (patterns pattern #:vspace
space))))))))
indent:

(if (zero? dir)
    (markup patterns pattern #:hspace space)
    (markup #:column (patterns pattern #:vspace space))))))))

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3437: (markup? number? ly:dir? markup? markup?)
group markup args together (i.e., `pattern' before `left')

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3472: #:translate (cons x-offset 0)
indent

http://codereview.appspot.com/4182056/diff/12001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3473: #:pattern pattern (1+ count) space CENTER
indent

[Bertrand Bordage, 2011-02-24 01:23:38]

New patch with Neil's changes.

[Graham Percival (old account), 2011-02-24 05:29:30]

Looks good!

Could you make one more version (changes below), then email me the
  git format-patch origin
so I can push it?

http://codereview.appspot.com/4182056/diff/15002/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/4182056/diff/15002/Documentation/changes.tely#n...
Documentation/changes.tely:65: Dots can be added to the table of contents items
using :
Please remove the space before the :

http://codereview.appspot.com/4182056/diff/15002/Documentation/notation/input...
File Documentation/notation/input.itely (right):

http://codereview.appspot.com/4182056/diff/15002/Documentation/notation/input...
Documentation/notation/input.itely:960: Dots can be added to fill the line
between an item and its page number :
Please remove the space before the :

http://codereview.appspot.com/4182056/diff/15002/Documentation/notation/input...
Documentation/notation/input.itely:962: @verbatim
Delete this

http://codereview.appspot.com/4182056/diff/15002/Documentation/notation/input...
Documentation/notation/input.itely:968: @lilypond[line-width=11.0\cm]
Please add "verbatim" to the @lilypond.

[Neil Puttock, 2011-02-24 15:31:46]

Hi Betrand,

LGTM, apart from a few minor details.

Cheers,
Neil

http://codereview.appspot.com/4182056/diff/15002/ly/toc-init.ly
File ly/toc-init.ly (right):

http://codereview.appspot.com/4182056/diff/15002/ly/toc-init.ly#newcode32
ly/toc-init.ly:32: tocItemWithDotsMarkup = \markup \fill-with-pattern #1 #RIGHT
.
this can be defined outside the \paper block

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3410: Otherwise they are spread vertically.
remove this line

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3421: (let* ((pattern-width (interval-length
(let ((

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3427: (prepend-alist-chain 'word-space 0
(prepend-alist-chain 'baseline-skip 0 props))
move to separate binding above (e.g., `new-props')

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3428: (if (zero? axis)
(if (= axis X)

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3431: (loop (1- i)
indent (goes with (zero? i) above)

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3432: (if (zero? axis)
(if (= axis X)

http://codereview.appspot.com/4182056/diff/15002/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3473: #:pattern (1+ count) X space pattern
I'm sorry I wasn't clearer about the indentation here:

(markup left
        #:with-dimensions (cons 0 middle-width) '(0 . 0)
        #:translate (cons x-offset 0)
        #:pattern (1+ count) X space pattern

[Graham Percival (old account), 2011-02-25 05:56:12]

LGTM.

In light of the changes, let's have one more round of giving chances for
reviews.