Creates a Flag grob.

Creates a Flag grob.

[MikeSol, 2011-08-21 21:26:06]

This should do the trick.

The regtests don't really help as they show tons of fake changes.  I'll try Han
Wen's suggestion for hacking output-distance.py in the next week if I am still
having trouble.

The only thing that currently seems broken is stem-tremolo-note-column.ly.  This
is because, as I state in my patch on stem-tremolos, the stem tremolo pure
height function is broken.  So, I don't consider this a regression...

The order that this stuff would be pushed is:

This patch
The stem changes
My rewrite for stem-tremolo pure height stuff, which would be rewritten yet
again to be cleaner and more accurate in function of the pure height changes to
stem.cc

Cheers,
MS

[hanwenn, 2011-08-22 01:56:23]

quick remarks

- Why are the flags called "maybe"flags in the engraver?
- Currently, the stem already does width/2 X-offset, can't you
piggyback on that?
- You're copying me and jan's name in the header. If anythingn, you
should probably put your own.

(we could consider just using "The lilypond authors" as name at the
top everywhere).


On Sun, Aug 21, 2011 at 6:26 PM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> This should do the trick.
>
> The regtests don't really help as they show tons of fake changes.  I'll
> try Han Wen's suggestion for hacking output-distance.py in the next week
> if I am still having trouble.
>
> The only thing that currently seems broken is
> stem-tremolo-note-column.ly.  This is because, as I state in my patch on
> stem-tremolos, the stem tremolo pure height function is broken.  So, I
> don't consider this a regression...
>
> The order that this stuff would be pushed is:
>
> This patch
> The stem changes
> My rewrite for stem-tremolo pure height stuff, which would be rewritten
> yet again to be cleaner and more accurate in function of the pure height
> changes to stem.cc
>
> Cheers,
> MS
>
> Description:
> Creates a Flag grob.
>
> Please review this at http://codereview.appspot.com/4922042/
>
> Affected files:
>  M input/regression/color.ly
>  M input/regression/flags-default.ly
>  M input/regression/flags-in-scheme.ly
>  M input/regression/flags-straight-stockhausen-boulez.ly
>  M input/regression/flags-straight.ly
>  M input/regression/graphviz.ly
>  M input/regression/grid-lines.ly
>  M input/regression/les-nereides.ly
>  M input/regression/mozart-hrn3-defs.ily
>  M input/regression/quote-overrides.ly
>  M lily/beam-quanting.cc
>  M lily/dot-column.cc
>  A lily/flag.cc
>  M lily/include/stem.hh
>  M lily/stem-engraver.cc
>  M lily/stem.cc
>  M lily/tie-formatting-problem.cc
>  M ly/engraver-init.ly
>  M ly/grace-init.ly
>  M ly/gregorian.ly
>  M ly/property-init.ly
>  M scm/define-grob-properties.scm
>  M scm/define-grobs.scm
>  M scm/flag-styles.scm
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
>



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[MikeSol, 2011-08-22 06:16:47]

On 2011/08/22 01:56:23, hanwenn wrote:
> quick remarks
> 
> - Why are the flags called "maybe"flags in the engraver?

Because we don't know if they are for-real flags until auto beaming has finished
doing its thing (and all of the flags with beams are killed).  I can change it
to flags_ or flags_to_maybe_kill_.

> - Currently, the stem already does width/2 X-offset, can't you
> piggyback on that?

Yes (I'll write this into the code and you'll see it in a new patch after I get
more comments).

> - You're copying me and jan's name in the header. If anythingn, you
> should probably put your own.

OK - I figured put yours as all I did was copy and paste the code, but I can do
this.

> (we could consider just using "The lilypond authors" as name at the
> top everywhere).

I'm a fan of this.  git blame already shows who did what (that said, I was git
blaming the other day and a lot of things show up as Graham Percival because of
the great indenting push of 2011 - is there a way to see multiple layers in git
blame?).

Cheers,
MS

[Reinhold, 2011-08-22 10:06:36]

http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly
File input/regression/color.ly (right):

http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly#newcode24
input/regression/color.ly:24: \override Flag #'color = #blue
Why don't you choose a different color to check that the two grobs are really
handled differently?

http://codereview.appspot.com/4922042/diff/1/input/regression/flags-default.ly
File input/regression/flags-default.ly (right):

http://codereview.appspot.com/4922042/diff/1/input/regression/flags-default.l...
input/regression/flags-default.ly:32: \override Flag #'flag-style = #'mensural
Can't we rename that to to style now?

http://codereview.appspot.com/4922042/diff/1/lily/flag.cc
File lily/flag.cc (right):

http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode54
lily/flag.cc:54: '() or "grace").  */
That comment should be moved down to stroke-style

http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode57
lily/flag.cc:57: SCM flag_style_scm = me->get_property ("flag-style");
Rename property to style

http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode97
lily/flag.cc:97: if (scm_is_string (stroke_style_scm))
Sooner or later, that should be moved out from here into its own grob. Then we
can also have slashed beamed grace notes (which don't have any flag).

http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode149
lily/flag.cc:149: "flag-style "
Rename that to style.

http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc
File lily/tie-formatting-problem.cc (right):

http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc#n...
lily/tie-formatting-problem.cc:177: boxes.push_back (Box (flag->extent
(x_refpoint_, X_AXIS), flag->extent (commony, Y_AXIS)));
Line too long

http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787
ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag
rename to style

http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787
ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag
Do we need/want a convert-ly warning for \override Stem #'flag-style?

http://codereview.appspot.com/4922042/diff/1/ly/property-init.ly
File ly/property-init.ly (right):

http://codereview.appspot.com/4922042/diff/1/ly/property-init.ly#newcode429
ly/property-init.ly:429: \revert TabVoice.Flag #'flag-style
rename to style

http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:276: (flag-style ,symbol? "A symbol determining
what style of flag
rename to style? Unfortunately, then we don't have any documentation any more
about the valid values... :(

[mikesol_ufl.edu, 2011-08-22 10:20:53]

On Aug 22, 2011, at 12:06 PM, reinhold.kainhofer@gmail.com wrote:

> 
> http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly
> File input/regression/color.ly (right):
> 
>
http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly#newcode24
> input/regression/color.ly:24: \override Flag #'color = #blue
> Why don't you choose a different color to check that the two grobs are
> really handled differently?
> 

Done.

> 
> http://codereview.appspot.com/4922042/diff/1/lily/flag.cc
> File lily/flag.cc (right):
> 
> http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode54
> lily/flag.cc:54: '() or "grace").  */
> That comment should be moved down to stroke-style
> 

Done.

> http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode97
> lily/flag.cc:97: if (scm_is_string (stroke_style_scm))
> Sooner or later, that should be moved out from here into its own grob.
> Then we can also have slashed beamed grace notes (which don't have any
> flag).

Agreed.

> http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc
> File lily/tie-formatting-problem.cc (right):
> 
>
http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc#n...
> lily/tie-formatting-problem.cc:177: boxes.push_back (Box (flag->extent
> (x_refpoint_, X_AXIS), flag->extent (commony, Y_AXIS)));
> Line too long
> 

Fixed.

> http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787
> ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag
> Do we need/want a convert-ly warning for \override Stem #'flag-style?
> 

Yes - I'd put this in a separate patch/issue (unless people think it belongs
here - it seems that convert-ly rules usually come after patches get pushed).

> http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#n...
> scm/define-grob-properties.scm:276: (flag-style ,symbol? "A symbol
> determining what style of flag
> rename to style? Unfortunately, then we don't have any documentation any
> more about the valid values... :(

You've hit the proverbial nail on its proverbial head.  It is for this reason
that I'm wary about renaming.

New patchset up, and thanks for your comments!

Cheers,
MS

[hanwenn, 2011-08-22 12:52:05]

On Mon, Aug 22, 2011 at 7:20 AM, Mike Solomon <mikesol@ufl.edu> wrote:

>
http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#n...
> scm/define-grob-properties.scm:276: (flag-style ,symbol? "A symbol
> determining what style of flag
> rename to style? Unfortunately, then we don't have any documentation any
> more about the valid values... :(
>
> You've hit the proverbial nail on its proverbial head.  It is for this
> reason that I'm wary about renaming.

Yes, but please rename anyway.  You could put the 'style values in the
doc of flag-interface.


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[MikeSol, 2011-08-22 13:07:52]

On 2011/08/22 12:52:05, hanwenn wrote:
> On Mon, Aug 22, 2011 at 7:20 AM, Mike Solomon <mailto:mikesol@ufl.edu> wrote:
> 
> >
>
http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#n...
> > scm/define-grob-properties.scm:276: (flag-style ,symbol? "A symbol
> > determining what style of flag
> > rename to style? Unfortunately, then we don't have any documentation any
> > more about the valid values... :(
> >
> > You've hit the proverbial nail on its proverbial head. &nbsp;It is for this
> > reason that I'm wary about renaming.
> 
> Yes, but please rename anyway.  You could put the 'style values in the
> doc of flag-interface.
> 
> 
> -- 
> Han-Wen Nienhuys - mailto:hanwen@xs4all.nl - http://www.xs4all.nl/%7Ehanwen

Done.

Cheers,
MS

[Neil Puttock, 2011-08-24 21:53:36]

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc
File lily/flag.cc (right):

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode147
lily/flag.cc:147: "what style of flag glyph is typeset on a"
" what

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode148
lily/flag.cc:148: "@code{Stem}.  Valid options include @code{'()}"
" @code{Stem}.

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode149
lily/flag.cc:149: "for standard flags, @code{'mensural} and"
" for

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode150
lily/flag.cc:150: "@code{'no-flag}, which switches off the flag.",
" @code{'no-flag}

http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode154
lily/flag.cc:154: "stem "
unused

http://codereview.appspot.com/4922042/diff/17001/lily/stem-engraver.cc
File lily/stem-engraver.cc (right):

http://codereview.appspot.com/4922042/diff/17001/lily/stem-engraver.cc#newcod...
lily/stem-engraver.cc:191: if (scm_is_string (get_property ("whichBar")))
doc whichBar as read property

http://codereview.appspot.com/4922042/diff/17001/lily/stem.cc
File lily/stem.cc (left):

http://codereview.appspot.com/4922042/diff/17001/lily/stem.cc#oldcode1110
lily/stem.cc:1110: "flag "
restore

http://codereview.appspot.com/4922042/diff/17001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/4922042/diff/17001/ly/engraver-init.ly#newcode678
ly/engraver-init.ly:678: (Voice Flag font-size -3)
also add to make-voice-props-set

http://codereview.appspot.com/4922042/diff/17001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (left):

http://codereview.appspot.com/4922042/diff/17001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:275: (flag ,ly:stencil? "A function returning the
full flag stencil
move to internal properties and change type-predicate and docstring, e.g.,

(flag ,ly:grob? "A pointer to a @code{Flag} object.")

[mikesol_ufl.edu, 2011-08-25 10:03:40]

On Aug 24, 2011, at 11:53 PM, n.puttock@gmail.com wrote:

> 

Thanks for your suggestions Neil!

I'm holding off on pushing the patch because I have noticed a spacing
discrepancy in a few regtests.  See the attached, where "old" is current master
and "new" is with my patch.  The spacing between the last 16th note and the
quarter is too tight.

I have verified via pacifier prints that the pure heights of the flag are being
taken from the stencil function and are going into the spacing engine in
separation-item.cc.  However, in theory, this patch should have null effect on
the minimum and ideal distances of paper columns.  This is not the case (see
attached).

Happily (bizarrely), if I add the flag to the NoteColumn's elements grob-array
in the rhythmic-column engraver, this problem goes away in the opposite sense:
the paper column's minimum and ideal distances become wider (see attached).  I'm
tending towards doing this, but it also seems subpar (unless someone can vouch
that the previous spacing somehow mishandled flags).

If anyone has any intuition as to why this is happening, I'd be much obliged if
you'd share it with me.

Cheers,
MS

[mikesol_ufl.edu, 2011-08-25 10:51:44]

On Aug 25, 2011, at 12:03 PM, Mike Solomon wrote:

> On Aug 24, 2011, at 11:53 PM, n.puttock@gmail.com wrote:
> 
>> 
> 
> Thanks for your suggestions Neil!
> 
> I'm holding off on pushing the patch because I have noticed a spacing
discrepancy in a few regtests.  See the attached, where "old" is current master
and "new" is with my patch.  The spacing between the last 16th note and the
quarter is too tight.
> 
> I have verified via pacifier prints that the pure heights of the flag are
being taken from the stencil function and are going into the spacing engine in
separation-item.cc.  However, in theory, this patch should have null effect on
the minimum and ideal distances of paper columns.  This is not the case (see
attached).
> 
> Happily (bizarrely), if I add the flag to the NoteColumn's elements grob-array
in the rhythmic-column engraver, this problem goes away in the opposite sense:
the paper column's minimum and ideal distances become wider (see attached).  I'm
tending towards doing this, but it also seems subpar (unless someone can vouch
that the previous spacing somehow mishandled flags).
> 
> If anyone has any intuition as to why this is happening, I'd be much obliged
if you'd share it with me.
> 
> Cheers,
> MS

Addendum - it appears that the width of the paper column of a 16th note + flag
in current master is 2.1522 whereas in with my flag work it is 2.2172.  This is
the length of the discrepancy between paper column ideal and minimum distances
when the flag is added to the note head's element list.  It is also the length
(0.0650) of the x offset of the flag with respect to its x parent, the stem.

Current master's width is correct, but oddly, the two print out the same results
on the page (at least they look the same in terms of the flags x offset with
respect to the stem).  That means that, somehow, the flag's x offset with
respect to its parent is being tacked on in a way it shouldn't.

I'll keep digging (as best I can) and keep reporting results as I find them.

Cheers,
MS

[hanwenn, 2011-08-25 13:17:27]

On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon <mikesol@ufl.edu> wrote:

> I have verified via pacifier prints that the pure heights of the flag are
being taken from the stencil function and are going into the spacing engine in
separation-item.cc.  However, in theory, this patch should have null effect on
the minimum and ideal distances of paper columns.  This is not the case (see
attached).

? it's not the heights but the widths that should go into separation-item?

Have you verified that the new flag grob gets taken into account in
note-spacing.cc ?


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mikesol_ufl.edu, 2011-08-25 19:08:12]

On Aug 25, 2011, at 3:17 PM, Han-Wen Nienhuys wrote:

> On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> I have verified via pacifier prints that the pure heights of the flag are
being taken from the stencil function and are going into the spacing engine in
separation-item.cc.  However, in theory, this patch should have null effect on
the minimum and ideal distances of paper columns.  This is not the case (see
attached).
> 
> ? it's not the heights but the widths that should go into separation-item?
> 
> Have you verified that the new flag grob gets taken into account in
> note-spacing.cc ?
> 

I found the problem after a couple hours of banging my head against the wall.

The Stem::width function uses Stem::flag to get the flag, whereas the stencil
uses Stem::get_translated_flag.  Thus, the flag used for widths is not
translated.  As the flag's bounding box in the font is shifted by a half stem
length to the right of where the glyph actually is, and as the translated flag
was always shifted by half a stem width, this is what made it work (albeit
kludgily).

I'll work this into the patch tonight or tomorrow.

Cheers,
MS

[mikesol_ufl.edu, 2011-08-25 20:43:04]

On Aug 25, 2011, at 9:08 PM, Mike Solomon wrote:

> On Aug 25, 2011, at 3:17 PM, Han-Wen Nienhuys wrote:
> 
>> On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>> 
>>> I have verified via pacifier prints that the pure heights of the flag are
being taken from the stencil function and are going into the spacing engine in
separation-item.cc.  However, in theory, this patch should have null effect on
the minimum and ideal distances of paper columns.  This is not the case (see
attached).
>> 
>> ? it's not the heights but the widths that should go into separation-item?
>> 
>> Have you verified that the new flag grob gets taken into account in
>> note-spacing.cc ?
>> 
> 
> I found the problem after a couple hours of banging my head against the wall.
> 
> The Stem::width function uses Stem::flag to get the flag, whereas the stencil
uses Stem::get_translated_flag.  Thus, the flag used for widths is not
translated.  As the flag's bounding box in the font is shifted by a half stem
length to the right of where the glyph actually is, and as the translated flag
was always shifted by half a stem width, this is what made it work (albeit
kludgily).
> 
> I'll work this into the patch tonight or tomorrow.
> 
> Cheers,
> MS

Hey all,

I've uploaded a new patch that reproduces the spacing calculations from the
current master with a TODO item to fix this.  I'm comfortable pushing this
(after running the regtests again) in its current form even though it does not
fix this problem - it simply transfers it to the new code.  I would like to fix
the problem, though, in the not-too-distant future.

Cheers,
MS

[MikeSol, 2011-08-27 14:34:42]

Pushed as f0978ed121192fee9bdf2453a325d98693148acf.

Cheers,
MS