add stencil-whiteout-outline function

add stencil-whiteout-outline function


clean up code formatting in stencil-whiteout

[pwm, 2015-05-24 17:30:27]

Please review, thanks.  

See discussion here for background and usage demo:
http://lists.gnu.org/archive/html/lilypond-user/2015-05/msg00522.html

I have not yet done a regtest, but will add one if this is reviewed favorably
for inclusion in LilyPond.

[Keith, 2015-05-24 22:56:44]

We certainly want this in LilyPond, and for it to be easy to use.

I think whiteout-stencil should simply call this function, with the defaults
suggested, so that the existing
\set Xx.whiteout=##t and \markup\whiteout use this new method.

We could provide access to the old whiteout method with 
\set Xx.block-whiteout and \markup\block-whiteout
for the former extent-based whiteout.

Those users who want whiteout with different parameters could use the function
explicitly 
  \override Staff.TimeSignature.stencil =
   #(lambda (grob)
     (stencil-whiteout-outline
       (ly:time-signature::print grob)
       0.7 cyan 45 1))

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode700
scm/stencil.scm:700: (angle-increments 12) (radial-increments 3))
The angle-increments seem more effective than radial-increments for creating a
filled-in shadow.  Glyphs are built mostly from lines, and a smooth circular
sweep of the lines is quite effective.  We would need radial-increments to fill
in the donut around things like stacatto dots, but only if the offset is more
than the dot-diameter -- more than 0.5staff-space plus the dot diameter before
we notice a staff-line peek through the hole.
I am so far happy testing (angle-increments 16) (radial-increments 1) with 16
copies instead of 36.

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749
scm/stencil.scm:749: (ly:stencil-extent stil Y))))))
The whiteout-stencil still affects the position of objects outside the staff,
because LilyPond uses the outlines stencils (not extents) for several spacing
tasks.

There are other situations (\page-ref and \with-dimensions) where we need one
stencil for spacing, and another for final printing, so we delay the formation
of the final-printing stencil.   That method seems appropriate here as well:

         (let ((whiteout-expr
                (ly:stencil-expr
                 (stencil-with-color
                  (radial-plot offset stil empty-stencil)
                  color))))
          (ly:stencil-add
            (ly:make-stencil
              `(delay-stencil-evaluation ,(delay whiteout-expr)))
            stil)))))

[Valentin Villenave, 2015-05-25 13:06:26]

On 2015/05/24 22:56:44, Keith wrote:
> I think whiteout-stencil should simply call this function, with the defaults
> suggested, so that the existing
> \set Xx.whiteout=##t and \markup\whiteout use this new method.

I’ve always been unhappy with the current method for whiteout (which I have also
been using in rounded-box-stencil, nevertheless). LilyPond scores’ background is
_not_ white, but alpha transparent. Look for example at the way Frescobaldi
displays PDF scores by default, or at the picture attached to
http://lists.gnu.org/archive/html/lilypond-user/2015-05/msg00522.html

So, ideally \whiteout should mimic this behavior rather than cover stuff with a
layer of white paint. Not sure how to do that, though.

[Carl, 2015-05-25 20:43:30]

On 2015/05/25 13:06:26, Valentin Villenave wrote:

> 
> So, ideally \whiteout should mimic this behavior rather than cover stuff with
a
> layer of white paint. Not sure how to do that, though.

In my opinion, whiteout needs to create a layer of white paint.  That's what
whiteout means.  It's like using a bottle of liquid paper (which many of you may
not even know about), and covers whatever is behind it, including the
background.

Thanks,

Carl

[Carl, 2015-05-25 20:47:33]

I like what you've done.

I like Keith's suggestion for delayed stencil evaluation.

Thanks,

Carl

[Keith, 2015-05-25 21:54:55]

On 2015/05/25 20:43:30, Carl wrote:
> 
> In my opinion, whiteout needs to create a layer of white paint.  That's what
> whiteout means. 

Well, "whiteout" is the name chosen by a couple modern Dutch programmers, but we
don't really want whiteout.
We want that thing that happens when you fill the neighboring groves on the
engraving plate so they don't take up ink, letting the natural color of the
paper show through on the prints.

If there was a mechanism to set the alpha-channel to zero rather than set the
color to white, that might be nice, but I do not think that facility is
available in the back-ends converting stencils to .svg or .pdf.

[Keith, 2015-05-25 22:24:01]

The additional copies of the stencils do make the PDF files noticeably larger. 
8% larger when I used 16-copy whiteout around all dynamics and text marks in a
string quartet score.

The compilation time increased by 2%, desipte the repeated computations of sines
and cosines. 

With \pointAndClickOn, the file-size increased by 30% when putting the new
white-out on dynamics and text marks.  Each copy of the stencil gets a
grob-cause mark, and these take up a lot of space.

If we use this for the built-in Xx.whiteout=##t we would have to re-order the
operations in grob.cc:get_print_stencil() so that the 'whiteout option is
applied before 'color and 'grob-cause are added.

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749
scm/stencil.scm:749: (ly:stencil-extent stil Y))))))
When I tried making stencil-whitéout' call the new improved function, I had
trouble with 
  \override Staff.Clef.whiteout = ##t
  \override Staff.Clef.color = #red
the whiteout was also red because the code in grob.cc applies the color before
handing the stencil to stencil-whiteout-outline, and in composed stencils with
color applied at different levels, the innermost 'color applies.

I added code to remove any 'color at top-level so I can continue using this. 
Ideally we would want code to remove any 'color from the stencil before using it
as whiteout, but it might be more appropriate to apply 'whiteout before 'color
in grob.cc.

        (let* ((stil-expr (ly:stencil-expr stil))
               (clean-stil (ly:make-stencil
                             (if (and (pair? stil-expr)
                                      (eq? (car stil-expr) 'color))
                               (caddr stil-expr)
                               stil-expr)))
               (whiteout-stil (stencil-with-color 
                                (radial-plot offset clean-stil empty-stencil)
                                color)))
          (ly:stencil-add
            (ly:make-stencil `(delay-stencil-evaluation 
                                ,(delay (ly:stencil-expr whiteout-stil))))
            stil)))))

[pwm, 2015-05-27 00:01:00]

Thanks for the reviews.

On 2015/05/24 22:56:44, Keith wrote:
> I think whiteout-stencil should simply call this function, with the defaults
> suggested, so that the existing
> \set Xx.whiteout=##t and \markup\whiteout use this new method.
> 
> We could provide access to the old whiteout method with 
> \set Xx.block-whiteout and \markup\block-whiteout
> for the former extent-based whiteout.

Sounds good to me.  In that case... would it make sense to name this new
function "stencil-whiteout" so that it replaces the current function, and then
the current function could be renamed "stencil-box-whiteout" or similar?  Then
we would have:

\set Xx.box-whiteout \markup \box-whiteout

I like "box" rather than "block" since "block" can more easily be misread as a
verb and "box" is shorter.

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode700
scm/stencil.scm:700: (angle-increments 12) (radial-increments 3))
On 2015/05/24 22:56:43, Keith wrote:
> I am so far happy testing (angle-increments 16) (radial-increments 1) with 16
> copies instead of 36.

I tried this out, and found it to work well, even on glyphs with thin lines like
treble clef and time signatures.

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749
scm/stencil.scm:749: (ly:stencil-extent stil Y))))))
On 2015/05/24 22:56:43, Keith wrote:
> The whiteout-stencil still affects the position of objects outside the staff,
> because LilyPond uses the outlines stencils (not extents) for several spacing
> tasks.
> 
> There are other situations (\page-ref and \with-dimensions) where we need one
> stencil for spacing, and another for final printing, so we delay the formation
> of the final-printing stencil.   That method seems appropriate here as well:
> 
>          (let ((whiteout-expr
>                 (ly:stencil-expr
>                  (stencil-with-color
>                   (radial-plot offset stil empty-stencil)
>                   color))))
>           (ly:stencil-add
>             (ly:make-stencil
>               `(delay-stencil-evaluation ,(delay whiteout-expr)))
>             stil)))))

Thanks.  I was not aware of this, but glad there's an easy fix.

https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749
scm/stencil.scm:749: (ly:stencil-extent stil Y))))))
On 2015/05/25 22:24:01, Keith wrote:
> When I tried making stencil-whitéout' call the new improved function, I had
> trouble with 
>   \override Staff.Clef.whiteout = ##t
>   \override Staff.Clef.color = #red
> the whiteout was also red because the code in grob.cc applies the color before
> handing the stencil to stencil-whiteout-outline, and in composed stencils with
> color applied at different levels, the innermost 'color applies.
> 
> I added code to remove any 'color at top-level so I can continue using this. 
> Ideally we would want code to remove any 'color from the stencil before using
it
> as whiteout, but it might be more appropriate to apply 'whiteout before 'color
> in grob.cc.

I am not that comfortable working in the C++ side of things, so if we go that
route (and that sounds like the better approach) I won't be much help.

[pwm, 2015-05-27 00:06:43]

On 2015/05/25 22:24:01, Keith wrote:
> The additional copies of the stencils do make the PDF files noticeably larger.

> 8% larger when I used 16-copy whiteout around all dynamics and text marks in a
> string quartet score.
> 
> The compilation time increased by 2%, desipte the repeated computations of
sines
> and cosines. 
> 
> With \pointAndClickOn, the file-size increased by 30% when putting the new
> white-out on dynamics and text marks.  Each copy of the stencil gets a
> grob-cause mark, and these take up a lot of space.
> 
> If we use this for the built-in Xx.whiteout=##t we would have to re-order the
> operations in grob.cc:get_print_stencil() so that the 'whiteout option is
> applied before 'color and 'grob-cause are added.

Thanks for looking closer at the performance and file size implications.  Sounds
like editing the C++ is the way to minimize these effects.

[pwm, 2015-05-27 00:09:55]

On 2015/05/25 13:06:26, Valentin Villenave wrote:
> I’ve always been unhappy with the current method for whiteout (which I have
also
> been using in rounded-box-stencil, nevertheless). LilyPond scores’ background
is
> _not_ white, but alpha transparent. Look for example at the way Frescobaldi
> displays PDF scores by default, or at the picture attached to
> http://lists.gnu.org/archive/html/lilypond-user/2015-05/msg00522.html
> 
> So, ideally \whiteout should mimic this behavior rather than cover stuff with
a
> layer of white paint. Not sure how to do that, though.

Me neither...  In any case, this seems like a separate issue.

[pkx166h, 2015-05-28 12:21:08]

Paul, I've set the Google Tracker (#4418) to 'needs_work' based on the comments
here

[pwm, 2015-05-29 03:16:23]

edits from first round of review

[paul_paulwmorris.com, 2015-05-29 04:03:06]

> On May 28, 2015, at 8:21 AM, pkx166h@gmail.com wrote:
> 
> Paul, I've set the Google Tracker (#4418) to 'needs_work' based on the
> comments here

Thanks James.  I’ve uploaded a second patch set for review.  
https://codereview.appspot.com/236480043/

(git-cl gave me “Incorrect username or password”  I assume this is due to the
problem mentioned in the other thread with login credentials.  Everything looks
ok on Reitveld.)

The patch set uses the new whiteout function as the new default.  The previous
function and markup command are renamed "whiteout-box".  (This puts whiteout and
whiteout-box next to each other in alphabetical listings, whereas "box-whiteout"
wouldn’t have.)  

I’ve incorporated Keith’s feedback, both about delaying the stencil formation in
stencil.scm and about rearranging the order of things in get_print_stencil in
grob.cc.  In my tests this did fix the color and file size issues.

Question: should the whiteout property be a number rather than a boolean, so
that it sets the thickness of the white outline (and #0 replaces ##f)?  Or
should the whiteout property accept either a number or a boolean?  That would
make it easier for users to simply get the default (0.3) using ##t, or to set it
to some other amount.  (A convert-ly rule may be needed… maybe in any case to
change whiteout to whiteout-box? ...or maybe not?)

-Paul

[pkx166h, 2015-05-29 07:20:59]

Passes make, make check - reg test diff attached to the tracker (issue 4418)

I'll run a full make doc, so assume no update means it has passed

[Keith, 2015-05-31 01:07:47]

I realize I am changing my opinion here on a few points:
1) leave the old whiteout as a simple box and give the new function a new name
2) enabling either type of whiteout through a single property looks better
3) the thickness should be in terms of line-thickness rather than staff-space

I hope you don't mind figuring out which of these were the wiser opinions.  I'll
just say this all looks good to me.

On 2015/05/29 04:03:06, paul_paulwmorris.com wrote:
>
> Question: should the whiteout property be a number rather than a boolean, so
> that it sets the thickness of the white outline (and #0 replaces ##f)?  Or
> should the whiteout property accept either a number or a boolean? 

Yes, and more yes to the second option.  There is a type-checking predicate
'boolean-or-symbol?' from which you could make the analogous
'boolean-or-number?'.
Existing uses of Xx.whiteout=##t could get the simple box, while Xx.whiteout=1
gives the outline style.  I suspect the users who want the outline will often
want to adjust the thickness.

Most thicknesses are in terms of the staff-line thickness.  Using this unit, the
range of thicknesses for which the function was intended, and works best without
excessive copies of the stencil, are simple numbers 
  \override Xx.whiteout = 4 %{ staffline thicknesses, rather than =#0.3
staff-spaces %}
Also, the whiteout border will more likely do the right thing if it scales with
the staffline thickness, rather than staff-space, when people scale the output
to make a pocket score for example.

Inconveniently for this patch, the stencil-translate works in units of
staff-spaces, and by design the stencil language keeps some things in units of
line-thickness, others in staff-spaces.  Each grob or markup is built for a
particular layout, so we can convert units where the grob or markup asks for
whiteout (see below).

> (A convert-ly rule may be needed… maybe in any case to
> change whiteout to whiteout-box? ...or maybe not?)

Looking at the existing uses of \whiteout Xxx.whiteout, it does seem best to
leave the plain box whiteout in existing scores.  Convert-ly to \whiteout-box
one way to preserve the behavior, but it would be better to leave the old markup
function \whiteout for the old behavior, and add \whiteout-outline.

https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rh...
File Documentation/notation/rhythms.itely (right):

https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rh...
Documentation/notation/rhythms.itely:555: collide with other objects in a staff.
At first I thought the docs should use the new outline form of whiteout, but
here I see that the box is perfect for this case.

(The text should say ".. properties of objects that should cause a gap in ties."
 Later we might build this into the grob definitions.)

https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc#newcode162
lily/grob.cc:162: 
We could let  \override Xx.whiteout = 3
give a white outline 3 times the staffline thickness, by looking up the
staffline thickness here.
if scm_is_number(get_property("whiteout") {
  SCM wh_proc = y_lily_module_constant ("stencil-whiteout-outline");
  Real thickness = robust_scm2double (get_property("whiteout"), 3.0)
       * layout ()->get_dimension (ly_symbol2scm ("line-thickness"));
  retval = *unsmob<Stencil>( scm_call_2(wh_proc,
                                       retval.smobbed_copy,
                                       thickness));
}

https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-command...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-command...
scm/define-markup-commands.scm:722: (stencil-whiteout (interpret-markup layout
props arg)))
Whatever the name for \whiteout-outline  you can let it read the 'thickness'
property, to use the interface for optional arguments that we have in the markup
language
 g^\markup \override #'(thickness . 5) \whiteout-outline "hello"

(define-markup-command (whiteout-outline layout props arg)
  (markup?)
  #:category other
  #:properties ((thickness 4))
  (stencil-whiteout-outline 
     (interpret-markup layout props arg)
      (* thickness 
         (ly:output-def-lookup layout 'line-thickness))))

[simon.albrecht, 2015-05-31 12:03:12]

Hello,

Am 31.05.2015 um 03:07 schrieb k-ohara5a5a@oco.net:
> I realize I am changing my opinion here on a few points:
>
[…]
>
>> (A convert-ly rule may be needed… maybe in any case to
>> change whiteout to whiteout-box? ...or maybe not?)
>
> Looking at the existing uses of \whiteout Xxx.whiteout, it does seem
> best to leave the plain box whiteout in existing scores. Convert-ly to
> \whiteout-box one way to preserve the behavior, but it would be better
> to leave the old markup function \whiteout for the old behavior, and add
> \whiteout-outline.
If I may chime in here: I think this is unnecessarily conservative. The 
new method for whiteout is certainly much more elegant and more what one 
would expect in the first place, than the method we have been using. 
It’s rather going to be special cases in which \whiteout-box would be 
preferable, and in most cases it will be \whiteout-outline, not the 
other way around. \whiteout-outline is just a bit too clumsy to be used 
for what is a real improvement.
So my vote goes toward \whiteout-box for the old and simply \whiteout 
for the new markup command.
Concerning the grob property: what about .whiteout = #'box for the old 
and .whiteout = #⟨x⟩ for the new behaviour, x being the thickness, 
measured in line-thickness?

Regards, Simon

[Keith, 2015-05-31 17:11:24]

On Sun, 31 May 2015 05:03:05 -0700, Simon Albrecht <simon.albrecht@mail.de>
wrote:

>>> (A convert-ly rule may be needed… maybe in any case to
>>> change whiteout to whiteout-box? ...or maybe not?)
>>
>> Looking at the existing uses of \whiteout Xxx.whiteout, it does seem
>> best to leave the plain box whiteout in existing scores. Convert-ly to
>> \whiteout-box one way to preserve the behavior, but it would be better
>> to leave the old markup function \whiteout for the old behavior, and add
>> \whiteout-outline.
> If I may chime in here: I think this is unnecessarily conservative. The
> new method for whiteout is certainly much more elegant and more what one
> would expect in the first place, than the method we have been using.
> It’s rather going to be special cases in which \whiteout-box would be
> preferable, and in most cases it will be \whiteout-outline, not the
> other way around. \whiteout-outline is just a bit too clumsy to be used
> for what is a real improvement.

The markup-function name could easily be shortened so something like \outline.

Searching mutopiaproject.org for 'whiteout' I found a few uses like this
   \markup \whiteout \pad-markup #0.4 \dynamic ff
which work differently with the new behavior so the padding no longer affects
the white-out dimension.  That is what made me more conservative.  A convert-ly
rule to preserve the old behavior would be fine.

[pwm, 2015-06-01 15:50:23]

On 2015/05/31 01:07:47, Keith wrote:
> I realize I am changing my opinion here on a few points:
> 1) leave the old whiteout as a simple box and give the new function a new name
> 2) enabling either type of whiteout through a single property looks better
> 3) the thickness should be in terms of line-thickness rather than staff-space
>
> I hope you don't mind figuring out which of these were the wiser opinions. 
I'll
> just say this all looks good to me.

Ok, see below.
 
> On 2015/05/29 04:03:06, http://paul_paulwmorris.com wrote:
> >
> > Question: should the whiteout property be a number rather than a boolean, so
> > that it sets the thickness of the white outline (and #0 replaces ##f)?  Or
> > should the whiteout property accept either a number or a boolean? 
> 
> Yes, and more yes to the second option.  There is a type-checking predicate
> 'boolean-or-symbol?' from which you could make the analogous
> 'boolean-or-number?'.

Ok, sounds good.

> Existing uses of Xx.whiteout=##t could get the simple box, while Xx.whiteout=1
> gives the outline style.  I suspect the users who want the outline will often
> want to adjust the thickness.

I see the appeal of having just one property and avoiding a convert-ly rule.  It
does seem a little odd if the kind of whiteout changes whether the property is a
number or boolean (but that may just be a matter of getting used to it).  One
potential problem is if we wanted to allow the old box whiteout function to also
take an optional thickness argument to provide some "padding" for the whiteout
box around its grob/stencil.  I don't see a good way to do that with one
property, but if there were two properties then it would be easy.  

> Most thicknesses are in terms of the staff-line thickness.  Using this unit,
the
> range of thicknesses for which the function was intended, and works best
without
> excessive copies of the stencil, are simple numbers 
>   \override Xx.whiteout = 4 %{ staffline thicknesses, rather than =#0.3
> staff-spaces %}
> Also, the whiteout border will more likely do the right thing if it scales
with
> the staffline thickness, rather than staff-space, when people scale the output
> to make a pocket score for example.
> 
> Inconveniently for this patch, the stencil-translate works in units of
> staff-spaces, and by design the stencil language keeps some things in units of
> line-thickness, others in staff-spaces.  Each grob or markup is built for a
> particular layout, so we can convert units where the grob or markup asks for
> whiteout (see below).

Using staffline thickness does seem like the better approach.  I need to look
closer at the implementation details.

> > (A convert-ly rule may be needed… maybe in any case to
> > change whiteout to whiteout-box? ...or maybe not?)
> 
> Looking at the existing uses of \whiteout Xxx.whiteout, it does seem best to
> leave the plain box whiteout in existing scores.  Convert-ly to \whiteout-box
> one way to preserve the behavior, but it would be better to leave the old
markup
> function \whiteout for the old behavior, and add \whiteout-outline.

Here I'd prefer a convert-ly rule for \whiteout --> \whiteout-box, and then we
can use \whiteout for the new outline style whiteout.  

To sum up, you're suggesting:

Grob Property:
  whiteout (T/F for old box style, numbers for new outline style)
Markup functions:
  \whiteout (old box style)
  \whiteout-outline (new style)
SCM Functions: 
  stencil-whiteout (old box style)
  stencil-whiteout-outline (new style)
Convert-ly: 
  none

But I think I prefer the following which has a consistent parallel structure
across all the categories and where the new style of whiteout is named in a way
that conveys that it is the default, with the old box style getting a more
specialized name.  This requires convert-ly rules and for users to learn the new
names, but I think it's worth it.

Grob Properties:
  whiteout (new style, T/F or numbers)
  whiteout-box (old style, T/F, could allow numbers for padding)
Markup functions:
  \whiteout (new style)
  \whiteout-box (old style)
SCM Functions:
  stencil-whiteout (new style)
  stencil-whiteout-box (old style, optional number arg for padding)
Convert-ly: 
  markups: \whiteout --> \whiteout-box
  grob properties: whiteout --> whiteout-box

(Hmmm... seems a convert-ly rule that would replace all "whiteout" with
"whiteout-box" would do the trick here for grob properties, markup functions,
and scm functions.)

[pwm, 2015-06-01 15:59:21]

https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rh...
File Documentation/notation/rhythms.itely (right):

https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rh...
Documentation/notation/rhythms.itely:555: collide with other objects in a staff.
On 2015/05/31 01:07:46, Keith wrote:
> At first I thought the docs should use the new outline form of whiteout, but
> here I see that the box is perfect for this case.
> 
> (The text should say ".. properties of objects that should cause a gap in
ties."
>  Later we might build this into the grob definitions.)

Ok, I'll make that change to the text.

https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc#newcode162
lily/grob.cc:162: 
On 2015/05/31 01:07:46, Keith wrote:
> We could let  \override Xx.whiteout = 3
> give a white outline 3 times the staffline thickness, by looking up the
> staffline thickness here.
> if scm_is_number(get_property("whiteout") {
>   SCM wh_proc = y_lily_module_constant ("stencil-whiteout-outline");
>   Real thickness = robust_scm2double (get_property("whiteout"), 3.0)
>        * layout ()->get_dimension (ly_symbol2scm ("line-thickness"));
>   retval = *unsmob<Stencil>( scm_call_2(wh_proc,
>                                        retval.smobbed_copy,
>                                        thickness));
> } 

Ok, thanks for this code.  I'll work it into the next patch set.

https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-command...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-command...
scm/define-markup-commands.scm:722: (stencil-whiteout (interpret-markup layout
props arg)))
On 2015/05/31 01:07:47, Keith wrote:
> Whatever the name for \whiteout-outline  you can let it read the 'thickness'
> property, to use the interface for optional arguments that we have in the
markup
> language
>  g^\markup \override #'(thickness . 5) \whiteout-outline "hello"
> 
> (define-markup-command (whiteout-outline layout props arg)
>   (markup?)
>   #:category other
>   #:properties ((thickness 4))
>   (stencil-whiteout-outline 
>      (interpret-markup layout props arg)
>       (* thickness 
>          (ly:output-def-lookup layout 'line-thickness))))

Sounds good.  Thanks for the code to do this.

[simon.albrecht, 2015-06-01 17:08:28]

LGTM.
~ Simon

Am 01.06.2015 um 17:50 schrieb paulwmorris@gmail.com:
> But I think I prefer the following which has a consistent parallel
> structure across all the categories and where the new style of whiteout
> is named in a way that conveys that it is the default, with the old box
> style getting a more specialized name.  This requires convert-ly rules
> and for users to learn the new names, but I think it's worth it.
>
> Grob Properties:
>   whiteout (new style, T/F or numbers)
>   whiteout-box (old style, T/F, could allow numbers for padding)
> Markup functions:
>   \whiteout (new style)
>   \whiteout-box (old style)
> SCM Functions:
>   stencil-whiteout (new style)
>   stencil-whiteout-box (old style, optional number arg for padding)
> Convert-ly:
>   markups: \whiteout --> \whiteout-box
>   grob properties: whiteout --> whiteout-box
>
> (Hmmm... seems a convert-ly rule that would replace all "whiteout" with
> "whiteout-box" would do the trick here for grob properties, markup
> functions, and scm functions.)
>
>
> https://codereview.appspot.com/236480043/

[pwm, 2015-06-01 20:23:59]

edits from second round of review

[pwm, 2015-06-01 20:55:00]

New patch set uploaded for review (3rd one):
https://codereview.appspot.com/236480043

Please review, especially the convert-ly rule, grob.cc, and doc strings.

Includes commits:

    convert-ly: add whiteout -> whiteout-box

    rhythms.itely: improve text for whiteout-box

    let \whiteout use the thickness property

    make whiteout grob property boolean-or-number?

    grob.cc: allow numbers in whiteout property

    stencil.scm: fix typo

    add boolean-or-number? predicate

[simon.albrecht, 2015-06-01 21:31:34]

LGTM. Thank you!

[dak, 2015-06-01 23:10:21]

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#ne...
python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str)
That seems like a rather overenthusiastic conversion pattern.  Did you actually
try running
scripts/auxiliar/update-with-convert-ly.sh
with this?

[pwm, 2015-06-02 01:44:17]

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#ne...
python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str)
On 2015/06/01 23:10:20, dak wrote:
> That seems like a rather overenthusiastic conversion pattern.  

Ah yes, good point.

> Did you actually
> try running
> scripts/auxiliar/update-with-convert-ly.sh
> with this?

Um... well, no...  I got in a hurry and didn't follow the CG thoroughly enough. 
Let me give it another try.

(I had actually already edited all occurrences manually (except for snippets),
and so wasn't thinking about convert-ly being run on the documentation.  I now
see the recommended way is to start with convert-ly and then follow up with any
needed manual edits.)

I suppose the most robust approach would be to have more than one rule to match:

  "\whiteout" for the markup command
  ".whiteout" for grob property overrides
  "#'whiteout" for the older property syntax
  "stencil-whiteout" for the scheme function.

[pwm, 2015-06-02 02:29:41]

I updated convertrules.py as follows:

@rule ((2, 19, 22), """whiteout -> whiteout-box""")
def conv(str):
    str = re.sub (r"\whiteout", r"\whiteout-box", str)
    str = re.sub (r".whiteout", r".whiteout-box", str)
    str = re.sub (r"#'whiteout", r"#'whiteout-box", str)
    str = re.sub (r"stencil-whiteout", r"stencil-whiteout-box", str)
    return str

but when I run 
scripts/auxiliar/update-with-convert-ly.sh
I get this:

make: *** No rule to make target `python-modules'.  Stop.
xargs: ./out/bin/convert-ly: No such file or directory
xargs: ./out/bin/convert-ly: No such file or directory

I'm running LilyDev3 in a virtual machine.  (Feeling like I'm in over my head
with this one.)

[Keith, 2015-06-02 05:00:30]

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#ne...
python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str)
On 2015/06/02 01:44:17, pwm wrote:
> I suppose the most robust approach would be to have more than one rule to
match:

Separate rules or a pattern that catches the prefixes-- whatever works to update
the LilyPond input without changing comments about the whiteout.

And be a bit defensive against cases that you can imagine.  You do not want to
convert whiteout-box -> whiteout-box-box, and to be thorough you might want to
catch uses with no spaces like \markup\whiteout1 , although with the new
\whiteout being an improvement over the old you can err on the side of being
conservative.

Scheme functions that do not appear in the docs, such as 'stencil-whiteout',
probably do not need convert-ly rules, but if the conversion is easy to do
without false-positives it should be fine.

https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-propertie...
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-propertie...
scm/define-grob-properties.scm:1147: equivalent to a value of 3.  Usually #f by
default.")
The 'boolean-or-number' parameter made sense for getting two types of behavior
through one parameter, but it is unusual to have #t be a synonym for 3.  '3' is
easier to type than '##t'.   

If this is a backward-compatibility help for people whose fingers know \override
Xx #'whiteout = ##t , it would be less mysterious to say "For compatibility with
former behavior (now available with @code{whiteout-box}) the value #t is treated
as 3.0."

Isn't it just "multiple of the staff-line thickness"? That is, if you
#(set-global-staff-size 5) doesn't the white-out stay relatively thicker, to
match the relatively thicker staff-lines?

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
scm/define-markup-commands.scm:710: #:properties ((thickness 4))
I just guessed the number 3 staffline thicknesses, and apparently typo'd it as
4.  If you like that default, fine.

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
scm/define-markup-commands.scm:720: \\whiteout whiteout
  \\override #'(thickness . 1.5) \\whiteout whiteout
otherwise there is no indication of what thickness does,
and the default thickness around 'whiteout' looks jagged on the black
background.

[pwm, 2015-06-07 17:10:37]

edits from 3rd round of review

[pwm, 2015-06-07 17:36:17]

Just uploaded patch set 4.

It includes improved convert-ly rules.  I ran 
scripts/auxiliar/update-with-convert-ly.sh
with these rules on both a clean branch and on the branch where I'm working on
this functionality.  It worked as expected for both.

This patch set also includes a changes entry and addresses Keith's comments from
the last set.

I had to update one snippet manually ("Using the whiteout property") to update
the doc string to "whiteout-box". I then copied it to the snippets/new directory
and ran the snippet update script, all as described in the CG.  It seems like
that went as it should.

Thanks for reviewing,
-Paul

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#ne...
python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str)
On 2015/06/02 05:00:30, Keith wrote:
> 
> Separate rules or a pattern that catches the prefixes-- whatever works to
update
> the LilyPond input without changing comments about the whiteout.
> 
> And be a bit defensive against cases that you can imagine.  You do not want to
> convert whiteout-box -> whiteout-box-box, and to be thorough you might want to
> catch uses with no spaces like \markup\whiteout1 , although with the new
> \whiteout being an improvement over the old you can err on the side of being
> conservative.
> 
> Scheme functions that do not appear in the docs, such as 'stencil-whiteout',
> probably do not need convert-ly rules, but if the conversion is easy to do
> without false-positives it should be fine.

Ok, thanks, fixed in next patch set (4th).

https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-propertie...
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-propertie...
scm/define-grob-properties.scm:1147: equivalent to a value of 3.  Usually #f by
default.")
On 2015/06/02 05:00:30, Keith wrote:
> The 'boolean-or-number' parameter made sense for getting two types of behavior
> through one parameter, but it is unusual to have #t be a synonym for 3.  '3'
is
> easier to type than '##t'.   
> 
> If this is a backward-compatibility help for people whose fingers know
\override
> Xx #'whiteout = ##t , it would be less mysterious to say "For compatibility
with
> former behavior (now available with @code{whiteout-box}) the value #t is
treated
> as 3.0."

Ok, I've fixed the wording in the next patch set (4th).  I was thinking about
backwards compatibility like you describe and also that it's easier on users if
they can get a sensible default with ##t without having to remember a number and
what the units are.  Also for parity with whiteout-box which accepts ##t for its
default (and may take a number in the future to customize its outline
thickness).

> Isn't it just "multiple of the staff-line thickness"? That is, if you
> #(set-global-staff-size 5) doesn't the white-out stay relatively thicker, to
> match the relatively thicker staff-lines?

Yep, done.

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
scm/define-markup-commands.scm:710: #:properties ((thickness 4))
On 2015/06/02 05:00:30, Keith wrote:
> I just guessed the number 3 staffline thicknesses, and apparently typo'd it as
> 4.  If you like that default, fine.

Ok, I tried both and went with 3 in the next patch set (4th).

https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-command...
scm/define-markup-commands.scm:720: \\whiteout whiteout
On 2015/06/02 05:00:30, Keith wrote:
>   \\override #'(thickness . 1.5) \\whiteout whiteout
> otherwise there is no indication of what thickness does,
> and the default thickness around 'whiteout' looks jagged on the black
> background.

Done.

[Keith, 2015-06-07 20:43:54]

lgtm, if you can narrow the convert-ly rule a bit and test it (the
semi-automated 'patchy' testing doesn't use convert-ly)

Also, I think you need to add
Documentation/snippets/new/blanking-staff-lines-using-the--whiteout-command.ly
and let the makelsr script update the copy in /snippets/, but I'm always
confused about that so just check the Contibutors' Guide

https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tel...
Documentation/changes.tely:66: now create a white background that follows the
outline of the
Better to be honest
"that follows the outline of"
 -> "built from multiple displaced copies of"

https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tel...
Documentation/changes.tely:69: @code{\whiteout-box} markup command.  Setting the
@code{whiteout}
Later, everything from "Setting the ..." on could be replaced by a one-measure
@lilypond[quote] example.

https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py#ne...
python/convertrules.py:3766: str = re.sub (r"\\whiteout(?!-box)",
r"\\whiteout-box", str)
Somebody has probably defined \whiteout-custom or similar so you should match
more narrowly   \\whiteout(?![a-z_-])

[pkx166h, 2015-06-08 06:07:47]

On 2015/06/07 20:43:54, Keith wrote:
> lgtm, if you can narrow the convert-ly rule a bit and test it (the
> semi-automated 'patchy' testing doesn't use convert-ly)
> 
> Also, I think you need to add
> Documentation/snippets/new/blanking-staff-lines-using-the--whiteout-command.ly
> and let the makelsr script update the copy in /snippets/, but I'm always
> confused about that so just check the Contibutors' Guide

Yes, do not edit the snippet directly in ../snippets/.. or add it directly to
the snippets/.. directory.

Put any new snippet in snippets/new/.. and edit any existing snippet also in the
snippets/new/.

The makelsr.py script (located in scripts/auxiliar/) is then run and it will
scan the snippets/new dir and do what it needs to do (update existing snippets
or add new ones to the 'snippets' directory as well as adding them to the
relevant texi files for the snippets.pdf/html files).

If you are testing the patch yourself (even if you are not doing a full make
doc) you will need to run the makelsr script before testing, however it is
usually best practice to NOT apply the patch for review with the makelsr run,
but generally run the makelsr after the patch has been pushed and push that
separately. It makes it easier to revert or keep track of changes - some makelsr
changes can produce a lot of tiny changes.

James

[pwm, 2015-06-08 17:42:43]

https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tel...
Documentation/changes.tely:66: now create a white background that follows the
outline of the
On 2015/06/07 20:43:54, Keith wrote:
> Better to be honest
> "that follows the outline of"
>  -> "built from multiple displaced copies of"

how about 
"built from multiple displaced copies of the glyph in order to approximate the
contours of its outline."
That indicates both what's better about the new behavior and what's actually
happening to achieve it.

https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py
File python/convertrules.py (right):

https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py#ne...
python/convertrules.py:3766: str = re.sub (r"\\whiteout(?!-box)",
r"\\whiteout-box", str)
On 2015/06/07 20:43:54, Keith wrote:
> Somebody has probably defined \whiteout-custom or similar so you should match
> more narrowly   \\whiteout(?![a-z_-])

Ok, sounds good.

[paul_paulwmorris.com, 2015-06-08 18:24:14]

> On Jun 8, 2015, at 2:07 AM, pkx166h@gmail.com wrote:
> 
> On 2015/06/07 20:43:54, Keith wrote:
>> lgtm, if you can narrow the convert-ly rule a bit and test it (the
>> semi-automated 'patchy' testing doesn't use convert-ly)
> 
>> Also, I think you need to add
> 
>>
Documentation/snippets/new/blanking-staff-lines-using-the--whiteout-command.ly
>> and let the makelsr script update the copy in /snippets/, but I'm
>> always
>> confused about that so just check the Contibutors’ Guide

I wondered about that.  The CG indicates that if you need to *manually* edit a
snippet, then do it by copying the file to /snippets/new, run makelsr, etc.  

The "blanking-staff-lines-using-the—whiteout-command.ly
<http://whiteout-command.ly/>" snippet didn’t need any *manual* changes after
running “update-with-convert-ly.sh”, so that’s why I didn’t put it into /new/
(like I did with the other snippet that required a manual edit to the doc
string).  

Is the CG correct on this?  CG 10.9.5 reads:

"Where the convert-ly rule is not able to automatically update snippets in
Documentation/snippets/, those snippets must be manually updated. Those snippets
should be copied to Documentation/snippets/new."


> Yes, do not edit the snippet directly in ../snippets/.. or add it
> directly to the snippets/.. directory.

I see, thanks, but what to do if running “update-with-convert-ly.sh” changes
snippets in the /snippets/ directory for you?

Sounds like I should not 'git commit' those changed files in /snippets/, and
then make the needed changes by copying the files to /snippets/new/.  Then maybe
there’s some way to discard or revert the automatic changes to the files in the
/snippets/ directory?

> If you are testing the patch yourself (even if you are not doing a full
> make doc) you will need to run the makelsr script before testing,
> however it is usually best practice to NOT apply the patch for review
> with the makelsr run, but generally run the makelsr after the patch has
> been pushed and push that separately. It makes it easier to revert or
> keep track of changes - some makelsr changes can produce a lot of tiny
> changes.

Ok.  Sounds like the CG would benefit from some updating on this point, as it
currently simply says to run makelsr (in 10.9.5) before submitting for review
(in 10.9.9).

Sounds like you are saying there’s a way to run makelsr to test a patch myself
but then discard those changes so that what is reviewed doesn’t include the
results of makelsr?  (I’m not sure how to do that…)  And then do a separate
follow-up patch that has the makelsr changes.

Anyway, it sounds like I should just add the edited snippets to /snippets/new/
and not run makelsr as part of this patch/issue.

Thanks for the help sorting this out.  I think the next step for this issue is
for me to create a new branch and redo all the changes on that branch with
better commit messages and now that I have a better understanding of the
convert-ly and snippet procedures.  Then upload that patch set for a (hopefully)
final review.

-Paul

[dak, 2015-06-08 18:30:09]

Paul Morris <paul@paulwmorris.com> writes:

> On Jun 8, 2015, at 2:07 AM, pkx166h@gmail.com wrote:
>
>> Yes, do not edit the snippet directly in ../snippets/.. or add it
>> directly to the snippets/.. directory.
>
> I see, thanks, but what to do if running “update-with-convert-ly.sh”
> changes snippets in the /snippets/ directory for you?

Nothing.  update-with-convert-ly.sh is run after every LSR reimport
anyway.

-- 
David Kastrup

[pkx, 2015-06-09 12:16:57]

On 08/06/15 19:23, Paul Morris wrote:
> Ok.  Sounds like the CG would benefit from some updating on this
> point, as it currently simply says to run makelsr (in 10.9.5)
> before submitting for review (in 10.9.9).

Yes it could be improved.

> 
> Sounds like you are saying there’s a way to run makelsr to test a
> patch myself but then discard those changes so that what is
> reviewed doesn’t include the results of makelsr?


Yes. It's not the end of the world if you didn't do that, assuming the
changes that makelsr ends up making itself aren't too large - but you
should then make sure the commit message includes the fact that you
ran makelsr

 (I’m not sure how to do that…)  And then do a separate follow-up
patch that has the makelsr changes.
> 
> Anyway, it sounds like I should just add the edited snippets to
> /snippets/new/ and not run makelsr as part of this patch/issue.


What I do to manually test, assuming someone has a patch with a
snippet/new change or addition but have nit run makelsr is this.

i. Run make, make test-baseline on current master to set up the reg tests.
ii. Apply patch
iii. Make sure that I 'git add' any 'new' snippets (I check using git
status)
iv. Run makelsr (it's important to remember if you use an out of tree
build - i.e. make your own build dir to make, make check etc - then
you have to run makelsr in the top level of LILYPOND_GIT and not in
the build dir itself (else it fails).
v. Once you have run makelsr then I check again with git status and
add all the new stuff that gets generated in snippets (from snippets/new)
v. make, make check
vi. make doc (that can take a while - usually step 'v's' make command
is enough to know if you have mucked something up in the doc with
regard to pointing to a snippet incorrectly in one of the manuals or
if you have munged up a snippet file name; no spaces for example).

I always make doc when I test the patch.

James

[dak, 2015-06-09 12:31:00]

James Lowe <pkx@gnu.org> writes:

> On 08/06/15 19:23, Paul Morris wrote:
>> Ok.  Sounds like the CG would benefit from some updating on this
>> point, as it currently simply says to run makelsr (in 10.9.5)
>> before submitting for review (in 10.9.9).
>
> Yes it could be improved.
>
>> 
>> Sounds like you are saying there’s a way to run makelsr to test a
>> patch myself but then discard those changes so that what is
>> reviewed doesn’t include the results of makelsr?
>
>
> Yes. It's not the end of the world if you didn't do that, assuming the
> changes that makelsr ends up making itself aren't too large - but you
> should then make sure the commit message includes the fact that you
> ran makelsr
>
>  (I’m not sure how to do that…)

Always do that in a separate commit with only the changes from running
makelsr, like with

git commit -m "Run scripts/auxiliar/makelsr.py" -a

In general, always keep automated changes and manual changes in separate
commits in order to reduce clutter.

-- 
David Kastrup

[paul_paulwmorris.com, 2015-06-10 03:14:02]

> On Jun 9, 2015, at 8:30 AM, David Kastrup <dak@gnu.org> wrote:
> 
> Always do that in a separate commit with only the changes from running
> makelsr, like with
> 
> git commit -m "Run scripts/auxiliar/makelsr.py" -a
> 
> In general, always keep automated changes and manual changes in separate
> commits in order to reduce clutter.

Ok, thanks David and James for the explanations.  It’s very helpful.  I'll see
if I can get another patch set up in the next few days.  (I’ll need to redo the
commits for better commit history)

-Paul

[pwm, 2015-06-10 06:00:15]

Narrow convert-ly rule, edit changes entry

[pwm, 2015-06-10 06:06:41]

Please review.  Per feedback I narrowed and tested the convert-ly rule and
edited the changes entry. (I also redid the commits for better commit history.)

-Paul

[pkx166h, 2015-06-14 08:56:46]

Patch on countdown for June 17th

[pkx166h, 2015-06-17 04:31:24]

Patch counted down - please push