Set up indent-tabs-mode in lexer.ll and parser.yy (issue 6551050)

[joeneeman]

lgtm

http://codereview.appspot.com/6551050/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: parser.yy: remove STRING_IDENTIFIER token (issue 6542057)

[joeneeman]

lgtm

http://codereview.appspot.com/6542057/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

note-collision: retain upper voice dot when merging dots (issue 6550056)

[joeneeman]

lgtm


http://codereview.appspot.com/6550056/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/6550056/diff/1/lily/note-collision.cc#newcode251
lily/note-collision.cc:251: dot_wipe_head = head_down;
Comment, please? (eg. it would make a nice addition to the existing
comment at line 232)

http://codereview.appspot.com/6550056/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[joeneeman]

On 2012/09/02 06:25:58, MikeSol wrote:

On 2012/09/01 23:58:37, Keith wrote:
 I might have a test case for you at
 http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776

 It seems you copy each slur into a slur-stub, and from those keep

only the

 ones with cross-staff set. Then when figuring system skylines you

insert all

 Grobs with the slur-stub-interface into the skylines for each staff.

 Why not insert the original Slur into the skylines, if it has

cross-staff

 instead of the SlurStub.  For that matter, why not insert all Grobs

marked

 cross-staff?

 all the cross-staff



It's not a copy of the original slur because it is using pure heights

and

offsets.


I realize I'm a bit late to this party, but why do you need a separate
grob instead of just a Slur::get_maybe_pure_vertical_skylines function?


http://codereview.appspot.com/6498077/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

On 2012/08/17 19:25:29, Keith wrote:

On Fri, 17 Aug 2012 10:16:25 -0700, mailto:mts...@gmail.com wrote:







http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode780

 lily/axis-group-interface.cc:780: while (dirty);
 On 2012/08/17 08:12:56, Keith wrote:
 I am beginning to understand the new code. Would you consider again

using

 distance() instead of the repeated calls to intersects() in the

while(dirty)

 loop ?  It might look simpler now that you've been away from the

code

 for awhile.



 floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
 ceilings[j] = forest[j][DOWN]~distance~pair[UP]



 You know the solution will be to move an amount equal to one of the
floors[j],
 and you want to find the smallest that fits, so make
trials = sort (floors)
 and find the smallest among trials that satisfies
 trials[n]  0
   for all j (trials[n]  floors[j]
 || trials[n]  ceilings[j] )



 I am not adverse to using distance if possible - my trouble case is

the

 following.

 A

  B

 Image that object A above is placed first and LilyPond is now
 considering whether it should leave B where it is or place it above

A.

 The distance function would dictate that it needs to be shifted

above A

 because its DOWN skyline is below A's UP.



We need not let the distance function between A[UP] and B[DOWN]

dictate, because

having positive distance between A[DOWN] and B[UP] is another

solution.


Object AA, a member of the forest, sets two constraints
 floor = padding  -  AA[UP] ~distance~ BB[DOWN]
 ceiling = AA[DOWN] ~distance~ BB[UP]  - padding
and we are checking that a trial location satisfies
 (trial = floor) OR (trial = ceiling)



I suppose you show BB having been placed just above the staff, and

that it still

lies below the ceiling set by AA, so this trial position satisfies the

first

test in the OR above, so we win.



All these distances can be computed once, before raising the skylines

at all,

because they tell you right away what trial values of raising BB will

clear AA.

That would be much simpler for people like me to understand than

computing the

four intersects() conditions, after having moved the skylines by zero

or more

bump(s).



I've implemented this method in dev/jneem-skylines and it looks pretty
neat. Next, padding...

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

On 2012/08/21 07:42:38, Keith wrote:

On 2012/08/18 10:12:00, MikeSol wrote:



 \relative c'' {
\override TupletBracket #'outside-staff-priority = #1
\override TupletNumber #'font-size = #5
\times 2/3 { a4\trill a\trill^foo a\trill }
 }

 I added this as a regtest.  If you comment out the rider business,
 you'll see that foo is printed on top of the tuplet number.



You gave TupletBracket a priority, but not TupletNumber.  Wouldn't it

make more

sense to have
   \override TupletNumber #'outside-staff-priority = #1
as well ?



If I strip out the special-case code

http://codereview.appspot.com/6462087

then all the Grobs honor the priorities I set for them.



Have you tried setting TupletNumber's priority to be smaller than
TupletBracket? It results TupletNumber moving twice as far as it should
(making it collide with foo), because first it translates itself and
then it gets moved again because TupletBracket moves, and its position
is measured relative to TupletBracket.

It would also be possible for some grob to insert itself between the
TupletBracket and the TupletNumber, which isn't particularly nice
either. So I think some form of rider functionality is necessary. I do
it a little differently in my branch, which even allows the TupletNumber
to affect the position of the TupletBracket.

Cheers,
Joe


http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode651
lily/axis-group-interface.cc:651: ---/
I've experimented with disabling the horizontal skylines, and it didn't
actually make any difference to the regtests. Could you add something
that exercises this?

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: are avoided
Have you measured the costs of not having exempt?

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

layout.cc: do not draw empty boxes (issue 6450113)

[joeneeman]

lgtm

http://codereview.appspot.com/6450113/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: [XY]-core-extent and general_alignment (issue 2613) (issue 6308093)

[joeneeman]

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc#newcode206
lily/self-alignment-interface.cc:206: grob_alignment = scm_to_double
(scm_cdr (grob_alignment_property));
Use robust_scm2double

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc#newcode221
lily/self-alignment-interface.cc:221: : me-core_extent (me, a);
What about:
if (which_grob_extent == ly_symbol2scm (extent))
  grob_extent = me-extent (me, a);
else
  grob_extent = ly_scm2interval(me-get_property (which_grob_extent);

Then you can get rid of all the core-extent-related caching code.

http://codereview.appspot.com/6308093/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: lilypond-book: Set include path for --output option (issue 2423). (issue 5846075)

[joeneeman]

http://codereview.appspot.com/5846075/diff/1/scripts/lilypond-book.py
File scripts/lilypond-book.py (right):

http://codereview.appspot.com/5846075/diff/1/scripts/lilypond-book.py#newcode639
scripts/lilypond-book.py:639: global_options.include_path.insert (0,
inverse_relpath (original_dir, global_options.output_dir))
Wouldn't it be easier if you just added the absolute path to
original_dir?

http://codereview.appspot.com/5846075/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362
lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f,
-infinity_f, iv[LEFT] - 2 * horizon_padding));
push_back is constant time for STL lists. No need to push_front and then
reverse.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393
lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis
horizon_axis, Direction sky)
This isn't quite what I had in mind (for one thing, it means that the
caller has to be aware of buildings, calculating their slope, etc.)

what about
Skyline::Skyline (vectorpairPoint, Point  const segments, Axis,
Direction)?

it works similarly to Skyline::Skyline(vectorBox...) except that the
resulting skyline shows the outline of the given set of line segments.

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647
lily/skyline.cc:647: out.merge (to_merge);
merge is linear, so this loop is quadratic.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc#newcode659
lily/axis-group-interface.cc:659: vectorGrob * *riders)
I don't understand why riders are necessary. Is it because of this
cyclic dependence stuff?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode467
lily/skyline.cc:467: Strips all old sloped material, adds new.
You're assuming that all sloped material came from skyline padding.
That's true right now, but there's no reason that it will continue to be
true. It's probably better to avoid adding padding at creation time
altogether, and instead to add it when calling Skyline::distance.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
This is linear in the number of buildings, but it should be constant.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
Ditto

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc#newcode543
lily/stencil-integral.cc:543: SCM glyph_info = scm_hashq_ref (font_list,
scm_string_to_symbol (glyph), SCM_EOL);
There isn't much error-checking here. What if the user substitutes an
unofficial font which isn't in the list?

By the way, lilypond's fonts embed extra data in font tables (look for
LILC and LILY in open-type-font.cc). That may be a better way to embed
this information than by putting it in a scm file. For example, it would
allow unofficial fonts to support integrals by embedding an extra table.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[joeneeman]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc#newcode659
lily/axis-group-interface.cc:659: vectorGrob * *riders)
On 2012/02/16 10:08:39, MikeSol wrote:

On 2012/02/16 08:09:10, joeneeman wrote:
 I don't understand why riders are necessary. Is it because of this

cyclic

 dependence stuff?



Not cyclical, but imagine that a grob (say tuplet bracket, for

example) has its

outside staff priority set whereas one of its Y_AXIS children (say

tuplet

number, for example), doesn't.  If the tuplet number's skyline is

added to the

skyline_pair using add boxes before its parent is shifted, it will be

placed too

low in the skyline.  Thus, it must be added to the skyline only after

its

parent's outside-staff-priority has been set to SCM_BOOL_F.  This is

why riders

are used.


Could you document this reasoning, please?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
On 2012/02/16 10:08:39, MikeSol wrote:

On 2012/02/16 08:09:10, joeneeman wrote:
 This is linear in the number of buildings, but it should be

constant.


How would one do this?


The buildings are in increasing order, so this should work:

Real last_end = -infinity_f;
for (...)
  {
if (i-y_intercept_  -infinity_f)
  return last_end;
last_end = i-end_;
  }
return infinity_f;

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
On 2012/02/16 10:08:39, MikeSol wrote:

On 2012/02/16 08:09:10, joeneeman wrote:
 Ditto



Ditto


...and here it's a doubly-linked list so you can iterate backwards.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: some comments and complaints on the code (issue 5651069)

[joeneeman]

http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#newcode211
lily/accidental-placement.cc:211: * @return A vector of
Accidental_placement_entrys
On 2012/02/11 12:27:31, Milimetr88 wrote:

Do you mean @param and @return or the comment to the function? What

comment

would you propose?


I'm complaining about the @return comment, since it's redundant (ie. no
need to change it, just remove that line). I'm ok with the @param
comment, because you can't tell from the function signature that accs is
supposed to be a list.

http://codereview.appspot.com/5651069/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: some comments and complaints on the code (issue 5651069)

[joeneeman]

http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#newcode211
lily/accidental-placement.cc:211: * @return A vector of
Accidental_placement_entrys
Please don't just comment on the type (unless it's SCM in which case you
can say which scheme type it is).

http://codereview.appspot.com/5651069/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds some info about pure properties to the CG. (issue 5364048)

[joeneeman]

On 2011/11/21 07:44:36, mike_apollinemike.com wrote:

I'm going to leave it this way if that's all right with you for the
purely selfish reason that I grasp these lists better in terms of what



they contain.


That's definitely ok with me. Thanks for taking the time to document
this!


http://codereview.appspot.com/5364048/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds some info about pure properties to the CG. (issue 5364048)

[joeneeman]

http://codereview.appspot.com/5364048/diff/4001/Documentation/contributor/programming-work.itexi
File Documentation/contributor/programming-work.itexi (right):

http://codereview.appspot.com/5364048/diff/4001/Documentation/contributor/programming-work.itexi#newcode1836
Documentation/contributor/programming-work.itexi:1836: LilyPond).
Purity is not only about returning predictable values. It's also about
not having any side effects.

http://codereview.appspot.com/5364048/diff/4001/Documentation/contributor/programming-work.itexi#newcode1844
Documentation/contributor/programming-work.itexi:1844: meaningful notion
of estimation.  For example, even if a color was
I would argue that the reason for lacking pure estimation for color is
that we haven't found a use for it rather than because of any intrinsic
properties of color.

http://codereview.appspot.com/5364048/diff/4001/Documentation/contributor/programming-work.itexi#newcode1888
Documentation/contributor/programming-work.itexi:1888: taken from the
stencil.
Actually, the requirement is different: the print function must not have
any side effects. The print function doesn't necessarily have to deliver
the same result at all stages in the compilation process. For example,
if some side-effect modifies the font size of a grob mid-compilation,
then note-head::print will deliver a stencil of a different height.

http://codereview.appspot.com/5364048/diff/4001/Documentation/contributor/programming-work.itexi#newcode1901
Documentation/contributor/programming-work.itexi:1901: @end itemize
I think the preceding four lists can be understood as follows: if you
have a Grob and you want to estimate its height, what can you do?

 - If its Y-extent callback is pure, just use it (pure-functions)
 - If its Y-extent callback is ly:grob::stencil-height and the stencil
callback is pure, then it's safe to use ly:grob::stencil-height
(pure-print-functions)
 - If we have a manually-created pure version of its Y-extent callback,
use that (pure-conversions-alist and pure-print-to-height-conversions)

http://codereview.appspot.com/5364048/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

grob.cc: rewrite O(n^2) algorithm in Grob::common_refpoint algorithm to O(n) (issue 5371050)

[joeneeman]

http://codereview.appspot.com/5371050/diff/1/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/5371050/diff/1/lily/grob.cc#newcode536
lily/grob.cc:536: c = c-dim_cache_[a].parent_;
After this loop, balance will be 0, right? So the next loop won't do
anything...

http://codereview.appspot.com/5371050/diff/1/lily/grob.cc#newcode542
lily/grob.cc:542: while (c != d) {
{ on new line

http://codereview.appspot.com/5371050/diff/1/lily/grob.cc#newcode542
lily/grob.cc:542: while (c != d) {
The old version would return 0 if there was no common refpoint (which is
possible if the grob hierarchy hasn't been fully built yet). I'd suggest
a while (c  c != d) to keep the old behavior.

http://codereview.appspot.com/5371050/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: grob.cc: rewrite O(n^2) algorithm in Grob::common_refpoint algorithm to O(n) (issue 5371050)

[joeneeman]

Ok, lgtm

http://codereview.appspot.com/5371050/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Caches pure translations for full score. (issue 5349043)

[joeneeman]

looks good! I never realized this case was such a bottleneck.


http://codereview.appspot.com/5349043/diff/1/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/5349043/diff/1/lily/align-interface.cc#newcode320
lily/align-interface.cc:320: SCM l = SCM_EOL;
Given that you've gone to the trouble of writing float_list_to_vector,
maybe you could also write float_vector_to_list. And I think they belong
somewhere more public, too.

http://codereview.appspot.com/5349043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: lily/bezier.cc: Avoid a floating point compare (issue 5121042)

[joeneeman]

lgtm

http://codereview.appspot.com/5121042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Several fixes for annotate-spacing. (issue 4950071)

[joeneeman]

Reviewers: J_lowe, Neil Puttock,

Message:
Thanks, all fixed.

Description:
Several fixes for annotate-spacing.

- Fixes annotate-spacing to use the new spacing names.
- Annotates spacing between staves as well as spacing between systems.
- Fixes some collisions between annotations.
- Padding (between systems, titles and staves) is properly annotated,
  with the annotation occuring at the horizontal position where the
  collision would actually happen. If the padding is the cause of the
  vertical spacing, it is highlighted in green.

Please review this at http://codereview.appspot.com/4950071/

Affected files:
  M lily/include/skyline-pair.hh
  M lily/include/skyline.hh
  M lily/include/system.hh
  A lily/page-layout-problem-scheme.cc
  M lily/skyline-pair.cc
  M lily/skyline.cc
  M lily/system.cc
  scm/paper-system.scm
  scm/stencil.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 380: Try to auto-detect cyclic references in header fields (issue 4951073)

[joeneeman]

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

http://codereview.appspot.com/4951073/diff/3001/scm/define-markup-commands.scm#newcode1897
scm/define-markup-commands.scm:1897: (interpret-markup layout (cons
(list (list symbol `(,property-recursive-markup symbol))) props) m)
props is an alist, right? In that case, I think you want
(interpret-markup layout (cons (cons symbol `(,...)) props) m)

http://codereview.appspot.com/4951073/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add support for custom ledger positions, using two new staff-symbol properties (issue 4974075)

[joeneeman]

http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc#newcode362
lily/ledger-line-spanner.cc:362: ledgers.add_stencil (ledger_line);
Since you no longer seem to be using the brew_ledger_lines function,
please get rid of it.

http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc
File lily/staff-symbol.cc (right):

http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode82
lily/staff-symbol.cc:82: int l = line_positions.size ();
Could you use a different name? l looks an awful lot like 1...

http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode105
lily/staff-symbol.cc:105: int l = scm_to_int (scm_length
(line_positions));
and again...

http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode117
lily/staff-symbol.cc:117: int l = Staff_symbol::line_count (me);
ok, this one isn't your fault, but I still think it would be good to
change it.

http://codereview.appspot.com/4974075/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix 456: Also check for laissez-vibrer events attached to single heads inside a chord (issue 4969069)

[joeneeman]

lgtm

http://codereview.appspot.com/4969069/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Several fixes for annotate-spacing. (issue 4724041)

[joeneeman]

Reviewers: lemzwerg, Neil Puttock, Reinhold,


http://codereview.appspot.com/4724041/diff/1/scm/paper-system.scm
File scm/paper-system.scm (right):

http://codereview.appspot.com/4724041/diff/1/scm/paper-system.scm#newcode112
scm/paper-system.scm:112: (ly:output-def-lookup layout
'system-system-spacing
On 2011/09/05 21:10:38, Reinhold wrote:

What's the case with all the other *-*-spacing variables? In

particular,

-) top-markup-spacing
-) top-system-spacing
-) score-system-spacing


Thanks for the bump. I've made many changes since this issue  was
created (and rebased several times), so I'm closing this issue and
making another one. FWIW, the new patch takes care of
score-system-spacing and top-*-spacing is handled elsewhere (in
annotate-top-space).

Description:
Several fixes for annotate-spacing.

Fixes annotate-spacing to use the new spacing names.
Annotates spacing between staves as well as spacing between systems.
Fixes some collisions between annotations.

Please review this at http://codereview.appspot.com/4724041/

Affected files:
  M lily/include/system.hh
  M lily/system.cc
  M scm/paper-system.scm
  M scm/stencil.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)

[joeneeman]

lgtm


http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode593
lily/grob.cc:593: return get_maybe_root_vertical_alignment (g, 0);
I still think this should say
return g-get_system ()-get_vertical_alignment ();
because there are several grobs that implement Align_interface and you
want to make sure you get the right one.

http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc#newcode4
lily/span-bar-stub-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen
Nienhuys han...@xs4all.nl
You're looking different today, Han-Wen.

http://codereview.appspot.com/4917046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Changes variable names in include/beam-scoring-problem.hh and beam-quanting.cc (issue 4961041)

[joeneeman]

http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-problem.hh
File lily/include/beam-scoring-problem.hh (right):

http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-problem.hh#newcode114
lily/include/beam-scoring-problem.hh:114: TODO - use trailing _ on data
members.
I think you can delete this TODO now.

http://codereview.appspot.com/4961041/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)

[joeneeman]

Correct me if I'm wrong, but this is my understanding: wherever there's
a SpanBar, you're creating SpanBarStubs between every relevant pair of
staves. These don't actually get printed; they're just there for the
pure-height (because the SpanBar height is pretty much the whole system,
so it doesn't tell you where the gaps are).

If that's correct, I have two broad comments: it's worth commenting
somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of
SpanBarStub is for pure-height only. But more importantly, isn't SpanBar
now obsoleted by SpanBarStub? That is, you can just remove the SpanBar
altogether and print the SpanBarStubs.


http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode363
lily/align-interface.cc:363: Align_interface::get_vertical_alignment
(Grob *g)
Our convention is that in
Align_interface::foo_bar (Grob *g)
the Grob *g should be something that implements Align_interface. So this
function should really be Grob::get_vertical_alignment. Same for the
functions below it.

http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode374
lily/align-interface.cc:374: Align_interface::get_vertical_axis_group
(Grob *g)
Seems to me that what you really want here is the top-level
VerticalAlignment (as opposed to, say, a BassFigureAlignment, which also
implements Axis_group_interface and Align_interface). Try g-get_system
()-get_vertical_alignment () instead.

http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc#newcode116
lily/span-bar-stub-engraver.cc:116: //it-set_parent (y_parents[j],
Y_AXIS);
Obsolete code?

http://codereview.appspot.com/4917046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses Y-offset for stem tremolos instead of translated stencil. (issue 4867043)

[joeneeman]

lgtm


http://codereview.appspot.com/4867043/diff/1/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4867043/diff/1/lily/beam.cc#newcode1797
lily/beam.cc:1797: Beam::middle_stem_estimation (Grob *me, Direction
dir)
As far as I can tell, this function estimates the range of stem
end-positions in this beam. Why is it called middle_stem_estimation?

http://codereview.appspot.com/4867043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Does better polynomial calculations for avoid objects. (issue 4860042)

[joeneeman]

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc
File lily/bezier.cc (right):

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc#newcode239
lily/bezier.cc:239: return p.minmax (sol[LEFT][0], sol[RIGHT][0], d !=
LEFT);
On 2011/08/19 07:03:50, MikeSol wrote:

On 2011/08/18 21:36:45, joeneeman wrote:
 If there are multiple intersections with (say) r, then

Polynomial::solve

doesn't
 seem to return them in any useful order. So sol[RIGHT][0] is really

just an

 arbitrary solution, isn't it?



True, but this seems no worse than line 81 where ts[0] is returned.

Not that

this is a good excuse...


Right, but what really bothers me is that you're then using
sol[RIGHT][0] as though it means something. So what this function seems
to do (suppose ax=X_AXIS) is to take an arbitrary point where the curve
intersects x=r and an arbitrary point where the curve intersects x=l and
then finds the maximum y value of the curve between those two points.
But there's no guarantee that that maximum is what you're after. For
example, if the curve intersects x=r before it intersects x=l then it
seems to me that Polynomial::minmax will return something weird. And if
there are multiple intersections with either line, there are a lot of
different answers that could come out.

http://codereview.appspot.com/4860042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Does better polynomial calculations for avoid objects. (issue 4860042)

[joeneeman]

http://codereview.appspot.com/4860042/diff/1/flower/polynomial.cc
File flower/polynomial.cc (right):

http://codereview.appspot.com/4860042/diff/1/flower/polynomial.cc#newcode65
flower/polynomial.cc:65: Polynomial::minmax (Real l, Real r, bool dir)
const
Perhaps bool max instead of bool dir?

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc
File lily/bezier.cc (right):

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc#newcode87
lily/bezier.cc:87: Axis other = Axis ((a + 1) % NO_AXES);
Use the other_axis function.

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc#newcode217
lily/bezier.cc:217: Bezier::minmax (Axis ax, Real l, Real r, Direction
d) const
What is this function supposed to do?

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc#newcode219
lily/bezier.cc:219: Axis other = Axis ((ax + 1) % NO_AXES);
other_axis

http://codereview.appspot.com/4860042/diff/1/lily/bezier.cc#newcode239
lily/bezier.cc:239: return p.minmax (sol[LEFT][0], sol[RIGHT][0], d !=
LEFT);
If there are multiple intersections with (say) r, then Polynomial::solve
doesn't seem to return them in any useful order. So sol[RIGHT][0] is
really just an arbitrary solution, isn't it?

http://codereview.appspot.com/4860042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Remove special case in staff-spacing (issue4188051)

[joeneeman]

http://codereview.appspot.com/4188051/diff/4001/lily/include/spacing-spanner.hh
File lily/include/spacing-spanner.hh (right):

http://codereview.appspot.com/4188051/diff/4001/lily/include/spacing-spanner.hh#newcode45
lily/include/spacing-spanner.hh:45: static Real cushion_;
I think this should be a grob property rather than a static variable.

http://codereview.appspot.com/4188051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: modifying default behaviour of tremolo slashes (issue4636081)

[joeneeman]

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm (constant);
On 2011/07/23 19:58:19, Janek Warchol wrote:

On 2011/07/21 08:39:53, joeneeman wrote:
 These two lines are redundant.



Isn't their purpose to guard against undefined (empty) style property?


If style is empty (or not a symbol) then (style == ly_symbol2scm
(varying)) will be false and you'll just return 'default.

http://codereview.appspot.com/4636081/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: modifying default behaviour of tremolo slashes (issue4636081)

[joeneeman]

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode42
lily/stem-tremolo.cc:42: style = ly_symbol2scm (constant);
You can remove these two lines and use
style == ly_symbol2scm (varying)
below

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm (constant);
These two lines are redundant.

http://codereview.appspot.com/4636081/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 153: \once\set properly restores the context property (issue4810042)

[joeneeman]

If you do

\set #'foo = #1
c
\once \set #'foo = #2
c
\once \set #'foo = #3
c

then the final value of foo should be 1. But with this code, I think it
will be 2.

http://codereview.appspot.com/4810042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: modifying default behaviour of tremolo slashes (issue4636081)

[joeneeman]

Actually, non-rectangular, constant-sloped beams used to be the default,
but I changed them (some years ago now) to be rectangular and parallel
to the beam, since that's what most of my scores have. My music
collection isn't with me right now, but I can confirm at least that
BH's urtext edition of Bartok's solo violin sonata uses rectangular,
parallel-to-the-beam tremolos.

Cheers,
Joe

http://codereview.appspot.com/4636081/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)

[joeneeman]

ok, lgtm


http://codereview.appspot.com/4517136/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing with empty contexts; issue 1669 (issue4515158)

[joeneeman]

On 2011/06/09 02:55:54, Keith wrote:

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc

File lily/align-interface.cc (right):



http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode222

lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2011/06/07 04:48:01, joeneeman wrote:
 If pure is true, there may be some staves that are going to be

removed.

However,
 these staves won't be removed until after line-breaking.



... and, as you say, these staves are not yet removed when

Y-extent-estimates

for the page breaker are calculated, so this patch messes up the page

breaker.


If the page breaker depends on empty, but living, staves disappearing

for

vertical spacing purposes, then maybe empty staves should be treated

that way

consistently.


Or maybe we should just be checking for hara-kiri instead of checking
whether the extent is empty... I'll try putting together a patch for
this, since it would be less confusing than the current situation.


The obvious alternative would be to have the empty staves decide

whether to

suicide before page breaking, but my suspicion is that is awkward, or

it would

have been done that way at first.


Yes: we don't know whether a staff is empty until after line breaking.


http://codereview.appspot.com/4515158/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Spacing with empty contexts; issue 1669 (issue4515158)

[joeneeman]

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode222
lily/align-interface.cc:222: dy = max (dy, min_distance);
If pure is true, there may be some staves that are going to be removed.
However, these staves won't be removed until after line-breaking. Now,
they're thrown away because they have an empty skyline but if you change
that, you'll probably need to add a check here, before including padding
and min_distance.

A good way to check that it's working would be to add annotate-spacing =
##t to input/regression/hara-kiri-staff.ly and check that the
Y-extent-estimates are accurate.

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode229
lily/align-interface.cc:229: // So if stretchability=0, treat
basic-distance as a minimum-distance.
This is no longer needed, right?

http://codereview.appspot.com/4515158/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Vertical spacing, distinguish stretchability/compressibility (issue4517136)

[joeneeman]

http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc
File lily/align-interface.cc (left):

http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc#oldcode230
lily/align-interface.cc:230: dy = max (dy,
Page_layout_problem::get_fixed_spacing (elems[j-1], elems[j],
spaceable_count, pure, start, end));
I think this is still necessary because of alignment-distances.

http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc
File lily/simple-spacer.cc (right):

http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode230
lily/simple-spacer.cc:230: for (++i; i  sorted_springs.size (); i++)
It seems like this loop will never see i=0, even if it is active.

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc
File lily/spring.cc (left):

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#oldcode55
lily/spring.cc:55: // -infinity_f works fine for now.
So now, you make fixed springs have a blocking_force_ of zero instead of
-infinity_f. Does that work properly in simple-spacer? It seems to me
like it will sort springs in the wrong order (and also line 237 won't
work as intended).

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc
File lily/spring.cc (right):

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#newcode51
lily/spring.cc:51: // Simple_spacer::compress_line() depends on the
contition above.
condition

http://codereview.appspot.com/4517136/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)

[joeneeman]

http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc
File lily/simple-spacer.cc (right):

http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode230
lily/simple-spacer.cc:230: for (++i; i  sorted_springs.size (); i++)
On 2011/06/04 10:48:09, Keith wrote:

On 2011/06/04 07:09:05, joeneeman wrote:
 It seems like this loop will never see i=0, even if it is active.



If springs[0] is active, the i-- above leaves i at UINT_MAX on loop

exit, so ++i

would be zero.



I don't know a good loop idiom for a down-loop using an unsigned type

(vsize)

for the index.  I'll re-write as two up-loops starting with

first_active or

something.



(In the old code, some inactive springs were added to inv_hooke and

never

removed, but other functions made sure these had zero flexibility so

it didn't

matter.  That seems too tricky to maintain, now keeping track of

compressibility

distinct from stretchability. So my reason for change here was an

attempt to

make the code /easier/ to figure out.)


Sorry, my mistake. I wouldn't bother rewriting this if I were you.
Perhaps it's worth a comment, though, just to point out the corner case.

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc
File lily/spring.cc (left):

http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#oldcode55
lily/spring.cc:55: // -infinity_f works fine for now.
On 2011/06/04 10:48:09, Keith wrote:

On 2011/06/04 07:09:05, joeneeman wrote:
 fixed springs have a blocking_force_ of zero instead
 of -infinity_f. Does that work properly in simple-spacer?



My comment tries to state the contitions ^H^H conditions that I can

see as

required for simple-spacer to work properly.



With blocking_force 0, the unstretchable springs are on the active

list when

stretching the layout, but with zero stretchability they do nothing.

When

compressing, unstretchable springs never get put on the active list,

since their

blocking_force is 0.


It seems, though, that unstretchable but compressible springs should be
on the active list when compressing.


With the former blocking force -inf, infinite compression, such

springs sorted

to be the very last ones removed from the active list.  I could see no

reason to

keep them active until the end.


What about +inf? I'm not vehemently opposed to 0.0, but it seems strange
to me that springs with are always blocking (or never blocking,
depending on how you see it) should be sorted in the middle instead of
at one end.

http://codereview.appspot.com/4517136/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Emit not-quite-cross-staff beams in the right context. (issue4564041)

[joeneeman]

On 2011/06/01 05:59:21, Keith wrote:

On 2011/05/31 08:19:42, joeneeman wrote:
 the Beam's Y-parent will be the VerticalAxisGroup of the bottom

staff

 and Beam::calc_cross_staff will return false.



Old patch didn't; but new patch does.  LGTM


Thanks, pushed. Also removed the 3 lines suggested by Mike.

http://codereview.appspot.com/4564041/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Emit not-quite-cross-staff beams in the right context. (issue4564041)

[joeneeman]

Reviewers: Keith,

Message:
On 2011/05/28 18:43:19, Keith wrote:

I would love to test, but don't know enough of the internals to see

what this

does.


If you take your example from comment 5 of issue 1043, without manual
beaming, the Beam's Y-parent will be the VerticalAxisGroup of the top
staff and Beam::calc_cross_staff will return true. With manual beaming
(or with this patch), the Beam's Y-parent will be the VerticalAxisGroup
of the bottom staff and Beam::calc_cross_staff will return false.


Description:
Emit not-quite-cross-staff beams in the right context.

This is related to 1043 and possibly other bugs.  Previously,
if a staff change happened immediately after the termination of
an auto-engraved cross-staff beam, then the beam was parented
to the wrong staff.  Now, every beam is parented to the context
in which it began.

Please review this at http://codereview.appspot.com/4564041/

Affected files:
  M lily/auto-beam-engraver.cc
  M lily/engraver-group.cc
  M lily/engraver.cc
  M lily/grob-info.cc
  M lily/include/engraver.hh
  M lily/include/grob-info.hh


Index: lily/auto-beam-engraver.cc
diff --git a/lily/auto-beam-engraver.cc b/lily/auto-beam-engraver.cc
index  
66b79fe008c59b0c2612c03425c2871af991c9cd..3fb76af9b6e7928e9f8f997c99cb0adde0cd0646  
100644

--- a/lily/auto-beam-engraver.cc
+++ b/lily/auto-beam-engraver.cc
@@ -80,6 +80,7 @@ private:
   Moment extend_mom_;
   Moment beam_start_moment_;
   Moment beam_start_location_;
+  Context *beam_start_context_;

   // We act as if beam were created, and start a grouping anyway.
   Beaming_pattern *grouping_;
@@ -219,7 +220,9 @@ Auto_beam_engraver::create_beam ()
   for (vsize i = 0; i  stems_-size (); i++)
 Beam::add_stem (beam, (*stems_)[i]);

-  announce_grob (beam, (*stems_)[0]-self_scm ());
+  Grob_info i = make_grob_info (beam, (*stems_)[0]-self_scm ());
+  i.rerouting_daddy_context_ = beam_start_context_;
+  announce_grob (i);

   return beam;
 }
@@ -238,6 +241,7 @@ Auto_beam_engraver::begin_beam ()
   beaming_options_.from_context (context ());
   beam_settings_ = updated_grob_properties (context (), ly_symbol2scm  
(Beam));


+  beam_start_context_ = context ();
   beam_start_moment_ = now_mom ();
   beam_start_location_
 = robust_scm2moment (get_property (measurePosition), Moment (0));
@@ -269,7 +273,10 @@ Auto_beam_engraver::end_beam ()

   if (finished_beam_)
{
- announce_end_grob (finished_beam_, SCM_EOL);
+ Grob_info i = make_grob_info (finished_beam_, SCM_EOL);
+ i.rerouting_daddy_context_ = beam_start_context_;
+
+ announce_end_grob (i);
  finished_grouping_ = grouping_;
  finished_beaming_options_ = beaming_options_;
}
Index: lily/engraver-group.cc
diff --git a/lily/engraver-group.cc b/lily/engraver-group.cc
index  
07a0ef50914b4f235093ae222e3c8637e604964d..a5782d23b0b8fbbcb8fa5272fb2cbdef77b389d2  
100644

--- a/lily/engraver-group.cc
+++ b/lily/engraver-group.cc
@@ -70,9 +70,16 @@ Engraver_group::announce_grob (Grob_info info)
 {
   announce_infos_.push_back (info);

+  Context *dad_con = context_-get_parent_context ();
+  if (info.rerouting_daddy_context_)
+{
+  dad_con = info.rerouting_daddy_context_;
+  info.rerouting_daddy_context_ = 0;
+}
+
   Engraver_group *dad_eng
-= context_-get_parent_context ()
-? dynamic_castEngraver_group * (context_-get_parent_context  
()-implementation ())

+= dad_con
+? dynamic_castEngraver_group * (dad_con-implementation ())
 : 0;

   if (dad_eng)
Index: lily/engraver.cc
diff --git a/lily/engraver.cc b/lily/engraver.cc
index  
8e49280fd8f928413eb810b4b1428da66e806691..12615eb8d6cbc780fc96a8e95cdee2fab6984bc8  
100644

--- a/lily/engraver.cc
+++ b/lily/engraver.cc
@@ -43,29 +43,33 @@ Engraver::announce_grob (Grob_info inf)
 void
 Engraver::announce_end_grob (Grob_info inf)
 {
+  inf.start_end_ = STOP;
   get_daddy_engraver ()-announce_grob (inf);
 }

-/*
-  CAUSE is the object (typically a Stream_event object)  that
-  was the reason for making E.
-*/
-void
-Engraver::announce_grob (Grob *e, SCM cause)
+Grob_info
+Engraver::make_grob_info(Grob *e, SCM cause)
 {
   /* TODO: Remove Music code when it's no longer needed */
   if (Music *m = unsmob_music (cause))
 {
   cause = m-to_event ()-unprotect ();
 }
-  if (unsmob_stream_event (cause) || unsmob_grob (cause))
+  if (e-get_property (cause) == SCM_EOL
+   (unsmob_stream_event (cause) || unsmob_grob (cause)))
 e-set_property (cause, cause);

-  Grob_info i (this, e);
+  return Grob_info (this, e);
+}

-  Engraver_group *g = get_daddy_engraver ();
-  if (g)
-g-announce_grob (i);
+/*
+  CAUSE is the object (typically a Stream_event object)  that
+  was the reason for making E.
+*/
+void
+Engraver::announce_grob (Grob *e, SCM cause)
+{
+  announce_grob (make_grob_info (e, cause));
 }


@@ -75,21 +79,7 @@ Engraver::announce_grob (Grob *e, SCM cause)
 void
 Engraver::announce_end_grob (Grob *e, SCM cause)
 {
-  

Re: Emit not-quite-cross-staff beams in the right context. (issue4564041)

[joeneeman]

Hey Joe,



Lines 1207-1209 in beam.cc are a kludge that I believe your patch

resolves.  You

may want to consider removing these lines and testing to see if it

still passes

regtests.  If so, I think you can nix these lines permanently.



After fixing my patch, I can indeed remove those lines.

Cheers,
Joe


http://codereview.appspot.com/4564041/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing staves with dynamics between; issue 1668 (issue4536088)

[joeneeman]

lgtm


http://codereview.appspot.com/4536088/diff/15003/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (left):

http://codereview.appspot.com/4536088/diff/15003/lily/page-layout-problem.cc#oldcode543
lily/page-layout-problem.cc:543: // using a scheme similar to the one in
set_column_rods.
I'd prefer to leave this comment in

http://codereview.appspot.com/4536088/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing staves with dynamics between; issue 1442 (issue4536088)

[joeneeman]

On 2011/05/27 18:02:23, Keith wrote:

Joe just pushed half of this, fixing 1442, so never mind for now.
I'll merge and repost next weekend.


Sorry, I didn't realize we were competing for this issue. But it seems
like there's still an outstanding issue in the handling of
minimum-distance together with padding?

http://codereview.appspot.com/4536088/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Loose-lines honor padding between systems (issue4553060)

[joeneeman]

lgtm

http://codereview.appspot.com/4553060/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Avoid repeats of 'staff-affinity' warning; change text. (issue4278058)

[joeneeman]

On 2011/03/20 07:45:11, Keith wrote:

On Sun, 20 Mar 2011 00:17:48 -0700, Joe Neeman

mailto:joenee...@gmail.com wrote:

 On Sat, Mar 19, 2011 at 11:58 PM, mailto:k-ohara5...@oco.net

wrote:


 In that case, is there any need to set after_affinity at all?

I could ask the author of the original code the same question :)


Surely you aren't asking me to understand code I wrote months ago? :P


I think yes.  As commentary.
It assures the reader that LilyPond is proceeding as if after_affinity

were

changed.


Wouldn't a comment be better then? The current discussion suggests that
redundant code can actually create confusion...



http://codereview.appspot.com/4278058/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Reduces algorithm time by prefinding footnoted grobs (issue4213042)

[joeneeman]

I haven't had time to look at this carefully, but I'll have closer look
later. What I don't understand, though, is why this problem needs such
extensive changes. If it's just a matter of preventing repeated
footnotes at the beginning/end of a line, it should be enough to examine
footnote-get_column ()-get_rank ()
and possibly
footnote-get_parent (X_AXIS)-get_property (break-visibility)
when you're constructing a Line_details.

Also, as a matter of convention, we don't pass non-const references to
functions. If it has to be non-const, it should be a pointer.


http://codereview.appspot.com/4213042/diff/27049/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/4213042/diff/27049/lily/system.cc#newcode257
lily/system.cc:257: }
I don't get it. Why does filtering out grobs with an empty X-extent
solve the problem? And why do you have to redo this every time
Page_spacing::append_system is called?

http://codereview.appspot.com/4213042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Draws a line above footnotes (issue4167063)

[joeneeman]

On 2011/02/22 15:23:00, MikeSol wrote:

On 2011/02/22 01:05:49, joeneeman wrote:
 Why not make the separator a (stencil-valued) property of the

paper-book?


I could, but currently, this patch employs its current stencil kludge

for the

following (perhaps-unfounded) reason.  Please let me know if I am

correct in

worrying about this.



The page spacer, when it is calculating rod_height_, takes into

account the

height of footnotes in the current patch.  However, it also needs to

take into

account the height of the footnote separator.  If the height and

padding of this

separator is encoded in the paper-book, there will be no way for the

page spacer

to have access to this information (I think...).  Is there a good way

to get the

spacer this info w/o more kludgery?


If you give Page_breaking a public accessor for its paper-book, then you
can access it in Page_spacing. For performance (because
Page_spacing::calc_force gets called a lot), it's probably best to
access the stencil and cache its height in Page_spacing's constructor.


On 2011/02/22 01:05:49, joeneeman wrote:
 Have you checked the performance of this? This part is linear and so

it makes

 break_into_pieces quadratic. Also, you can make it simpler by

iterating

through
 all-elements instead of recursing through elements.



I've made the change to iterate through all-elements, but I still need

to

measure the performance of the algorithm.  How would I do this?


The change to all-elements was just for simplicity; I still think it
will be quadratic. You can do a profiling build with
./configure --enable-conf=prof --enable-profiling --disable-optimising
make conf=prof
(and the binary will be out-prof/bin/lilypond). You can also just time
lilypond on a largish score (ie. multiple minutes of processing time).
You can find such scores on the mailing list if you don't have one; Hu
Haipeng often sends them. Or you can try Valentin's opera, but that's
more like multiple hours of processing time...



http://codereview.appspot.com/4167063/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Draws a line above footnotes (issue4167063)

[joeneeman]

http://codereview.appspot.com/4167063/diff/3018/lily/note-head.cc
File lily/note-head.cc (right):

http://codereview.appspot.com/4167063/diff/3018/lily/note-head.cc#newcode189
lily/note-head.cc:189: footnote 
Any particular reason this belongs to note-head-interface (rather than,
say, item-interface)?

http://codereview.appspot.com/4167063/diff/3018/lily/page-breaking.cc
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4167063/diff/3018/lily/page-breaking.cc#newcode516
lily/page-breaking.cc:516: Page_breaking::draw_page (SCM systems, SCM
footnotes, SCM configuration, int page_num, bool last)
Surely draw_page can extract the footnotes from the systems rather than
taking them as an argument? Then you don't have to keep passing them
around, and you get support for the other page breaking algorithms for
free.

http://codereview.appspot.com/4167063/diff/3018/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/4167063/diff/3018/lily/page-layout-problem.cc#newcode65
lily/page-layout-problem.cc:65: Stencil mol;
Why not make the separator a (stencil-valued) property of the
paper-book?

http://codereview.appspot.com/4167063/diff/3018/lily/page-spacing.cc
File lily/page-spacing.cc (right):

http://codereview.appspot.com/4167063/diff/3018/lily/page-spacing.cc#newcode81
lily/page-spacing.cc:81: for (vsize i = 0; i  line.footnotes_.size ();
i++)
You'll need to do this in append_system also.

http://codereview.appspot.com/4167063/diff/3018/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/4167063/diff/3018/lily/system.cc#newcode623
lily/system.cc:623: footnote_search (Grob *g, vsize start, vsize end,
vectorStencil** s)
Have you checked the performance of this? This part is linear and so it
makes break_into_pieces quadratic. Also, you can make it simpler by
iterating through all-elements instead of recursing through
elements.

http://codereview.appspot.com/4167063/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix Issue 1290 (issue3832046)

[joeneeman]

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i-slope_ == 0)  !isinf (i-y_intercept_) 
i-y_intercept_ = 0)
On 2011/01/03 03:48:09, Carl wrote:

On 2011/01/02 05:19:58, joeneeman wrote:
 Why restrict to y_intercept_  0?
  Because if I have a y intercept that is less than zero, and create a

box with

that y intercept, the padded skyline comes out with all zero heights.

I don't

know why it works that way, but it does.


That's very strange. Do you have an example file that triggers this
behaviour?


I tried several things to get it to behave differently, but was

unsuccessful.

And I don't think that a negative skyline will ever be limiting in

inter-system

spacing, so I didn't feel that it was a problem.


Probably not, but I still feel that it's bad to have methods with
strange corner cases. At the very least, there should be a comment
explaining that the distance function will throw away any constraints
with negative height.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
lily/skyline.cc:391: {
I still don't understand why this function is so complicated. Does this
not work?

Real start = -infinity_f;
vectorBox boxes;
listBuilding::const_iterator end = src.buildings_.end ();
for (listBuilding::const_iterator i = src.buildings_.begin (); i !=
end; sta
 rt=i-end_, i++)
  if ((i-slope_ == 0)  !isinf (i-y_intercept_))
boxes.push_back (Box (Interval (start, i-end_),
  Interval (-infinity_f, i-y_intercept_)));

buildings_ = internal_build_skyline (boxes, horizon_padding, X_AXIS,
UP);
sky_ = src.sky_;

By the way, the above code (and your version also) will break if we
every switch to using more accurate outlines for grobs (ie. more
accurate than bounding boxes) because it could be throwing away
interesting things when it discards non-horizontal segments.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
lily/skyline.cc:419: Axis vert_axis = other_axis (horizon_axis);
This should have X_AXIS instead of a, because you put the horizon axis
into the X_AXIS of your boxes.

http://codereview.appspot.com/3832046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix Issue 1290 (issue3832046)

[joeneeman]

I don't know if it's important, but it may be worth mentioning somewhere
that skyline-horizontal-padding works differently for VerticalAxisGroup
and System. (Because in VerticalAxisGroup, skyline-horizontal-padding
takes effect while the outside-staff-grobs are being placed).


http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#newcode274
lily/page-layout-problem.cc:274: minimum_distance = (*sky)[UP].distance
(bottom_skyline_, scm_to_double (prob-get_property
(skyline-horizontal-padding)));
robust_scm2double is probably better

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i-slope_ == 0)  !isinf (i-y_intercept_) 
i-y_intercept_ = 0)
Why restrict to y_intercept_  0?

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i-y_intercept_  0 ? i-y_intercept_ : 0
, i-y_intercept_)));
...and if you do restrict to y_intercept_  0, then this is redundant.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding,
a, (Direction) 1);
pad_sky instead of padSky
UP instead of (Direction) 1
and perhaps a comment about why you need to use UP instead of sky_
(which would seem intuitive)

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = other;
I'm not sure, but I think Skyline const * is more consistent with the
rest of the code.

http://codereview.appspot.com/3832046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix bookpart identifier crash with page-marker. (issue3585042)

[joeneeman]

lgtm


http://codereview.appspot.com/3585042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 1252 by compressing page (issue3422041)

[joeneeman]

On 2010/12/02 20:52:00, Carl wrote:

Here's a patch to fix issue 1252 (music overflows page) by compressing

music

together.  It may cause collisions, but it should solve the critical

overflow

error.


LGTM. It would still be nice to better estimate the heights (I actually
have a little code for this, but I haven't had time to finish it...).
But that can be a non-critical bug.

http://codereview.appspot.com/3422041/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: NR 4.1: Reorganize, clarify details. (issue2758042)

[joeneeman]

http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.itely
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.itely#newcode428
Documentation/notation/spacing.itely:428: @c TODO: Where do
headers/footers fit in? -mp
headers/footers have no effect on reference points (ie. the reference
point of the top/bottom of the page is the bottom/top of the appropriate
margin). Their only effect comes from avoiding overlaps. That is, they
affect the behaviour of padding in the same way that a low note on a
staff does.

http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.itely#newcode775
Documentation/notation/spacing.itely:775: TODO: this variable is used,
but I don't know what it does. -pm
The penalty for having a blank page after the end of one score and
before the next. By default, this is smaller than blank-page-force, so
that we prefer blank pages after scores to blank pages within a score.

http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.itely#newcode854
Documentation/notation/spacing.itely:854: @funindex
page-limit-inter-system-space
I don't think this exists any more...

http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.itely#newcode861
Documentation/notation/spacing.itely:861: @funindex
page-limit-inter-system-space-factor
I think this is gone too

http://codereview.appspot.com/2758042/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: NR 4.4.1: Rewrite. (issue2642043)

[joeneeman]

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1662
Documentation/notation/spacing.itely:1662:
@code{after-last-staff-spacing}).
On 2010/10/29 04:12:45, Keith wrote:

On 2010/10/27 08:44:41, Mark Polesky wrote:
 What's a better way to word it?
... leave this property unset, because if set it will be used instead

of any

StaffGrouper property that would have otherwise applied.


There are some situations where you might want to override this, so I'm
not sure if the docs have such a blanket statement. The point is that
all of the logic involving StaffGrouper and its spacing variables is
contained in the default callback associated with next-staff-spacing
(ly:axis-group-interface::calc-next-staff-spacing). If you override that
default callback, you will disable that logic for the relevant staff.

However (and this is the source of my original comment) you will not
actually override any of the properties in StaffGrouper, as the
following example demonstrates:

\new StaffGroup 
% The override doesn't change the value of anything in
% StaffGrouper. You can see this because the original
% value of between-staff-spacing is used for all staves
% but the first. Also, you can't achieve this effect by
% changing 'default-next-staff-spacing, which shows that it
% is sometimes useful to override 'next-staff-spacing.
\new Staff \with { \override VerticalAxisGroup #'next-staff-spacing
#'space = #30 } { c'1 }
\new Staff { c'1 }
\new Staff { c'1 }


The above is probably an excessively long explanation for the docs. I
would suggest something like ...since setting it will prevent
StaffGrouper variables (such as ...) from having any effect on the
current staff.

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1679
Documentation/notation/spacing.itely:1679: either side.  Adjacent
staff-like contexts should have
On 2010/10/27 08:44:41, Mark Polesky wrote:

On 2010/10/26 22:28:16, joeneeman wrote:
 I'm not sure how precise you want to be here, but this
 isn't quite true: if the upper staff, for example, has a
 very low note then a lyrics line with CENTER affinity
 might be placed closer to the lower staff.

 Also, by equidistant between the ... staves, you mean
 equidistant between the refpoints of the staves, right?



That's what I meant, but based on your previous statement, I
assume even that would be incorrect.  In fact, this is
clearly wrong, since the refpoints of the staves are the
midlines, right?  So is it centered between the skylines?


It is centered between refpoints in the sense that its desired
position is for its refpoint to be exactly centered between the
refpoints of the surrounding staves. But it may not achieve this desired
position due to other constraints like collisions or minimum-distances.

http://codereview.appspot.com/2642043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: NR 4.4.1: Rewrite. (issue2642043)

[joeneeman]

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1624
Documentation/notation/spacing.itely:1624: size) will always reset all
its default key-values.
Everything from the subsubheading until here applies to any grob
property with an alist (eg. Beam 'details, Slur 'details,
NonMusicalPaperColumn 'line-break-system-details). It may be more
appropriate (long-term) to put this detailed discussion elsewhere.

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1662
Documentation/notation/spacing.itely:1662:
@code{after-last-staff-spacing}).
I find this wording a little confusing, since
\override VerticalAxisGroup #'next-staff-spacing ...
has a quite different effect from
\override StaffGrouper #'...

That is, overriding next-staff-spacing does not override any
StaffGrouper properties for the usual use of override in lilypond.

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1672
Documentation/notation/spacing.itely:1672: system.
default-next-staff-spacing also applies to staves which are not part of
a staff group

http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1679
Documentation/notation/spacing.itely:1679: either side.  Adjacent
staff-like contexts should have
I'm not sure how precise you want to be here, but this isn't quite true:
if the upper staff, for example, has a very low note then a lyrics line
with CENTER affinity might be placed closer to the lower staff.

Also, by equidistant between the ... staves, you mean equidistant
between the refpoints of the staves, right?

http://codereview.appspot.com/2642043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 1240. (issue2065041)

[joeneeman]

Reviewers: carl.d.sorensen_gmail.com,

Message:
Thanks. In the end, I committed this mostly as-is, but a change I made
while working on 1252 allowed me to do without the dummy Paper_columns.


Description:
Fix 1240.

Include fixed spacings in the calculation of minimum spacings.

Please review this at http://codereview.appspot.com/2065041/

Affected files:
  M lily/align-interface.cc
  M lily/grob.cc
  M lily/include/align-interface.hh
  M lily/include/grob.hh
  M lily/include/page-layout-problem.hh
  M lily/include/system.hh
  M lily/page-layout-problem.cc
  M lily/system.cc



___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make pure-print-callbacks public (issue1983047)

[joeneeman]

While this patch looks fine to me, there are a bunch of other lists with
related functionality. You may want to make some of them public too (or
add public accessor functions).

http://codereview.appspot.com/1983047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 442. (issue1888042)

[joeneeman]

Reviewers: Neil Puttock,

Message:
On 2010/08/04 21:57:20, Neil Puttock wrote:

Hi Joe,



LGTM.



I'd be quite happy with this method, even though it loses a bit of

flexibility

in comparison with your original patch.


Thanks, fixed and pushed. I don't particularly see much use for the
original flexibility, but we can always add it on later. It would just
be a new interface anyway to the same backend code anyway.

Cheers,
Joe


Description:
Fix 442.

Add an engraver which keeps all of the staves below
it alive together.

Please review this at http://codereview.appspot.com/1888042/show

Affected files:
  A input/regression/hara-kiri-alive-with.ly
  M lily/hara-kiri-group-spanner.cc
  M lily/include/hara-kiri-group-spanner.hh
  A lily/keep-alive-together-engraver.cc
  M ly/engraver-init.ly
  M scm/define-grob-properties.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix #1192. (issue1665053)

[joeneeman]

lgtm

http://codereview.appspot.com/1665053/show

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Optimizations for pure-height approximations. (issue1817045)

[joeneeman]

On 2010/07/14 23:15:13, lemzwerg wrote:

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



http://codereview.appspot.com/1817045/diff/1/4#newcode1059
scm/define-grob-properties.scm:1059:
Since the properties in this file are sorted alphabetically, this is

the wrong

place...


Oops, turns out that 'r' comes before 's'...

http://codereview.appspot.com/1817045/show

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Optimizations for pure-height approximations. (issue1817045)

[joeneeman]

Reviewers: lemzwerg, Neil Puttock,

Message:
On 2010/07/15 22:04:58, Neil Puttock wrote:

http://codereview.appspot.com/1817045/diff/1/2
File lily/axis-group-interface.cc (right):



http://codereview.appspot.com/1817045/diff/1/2#newcode125
lily/axis-group-interface.cc:125: pure_height_cache =

scm_c_make_hash_table

(1000);
This default size appears to be too big (at least for my poor

computer, which

imploded trying to run `make check': it swallowed all the memory (2

Gb) and

virtual memory shot up to 4 Gb before I put it out of its misery :)



I tried reducing it to 100, which incurred a slight performance

penalty due to

rehashing, but allowed the regtest checks to finish.


I can change it to 100, but it's still a bit worrying that a few measly
hash tables end up taking so much memory (there should only be one per
staff). Can you see a noticeable memory increase between git master and
the patch (once you reduce the hash-tables to size 100)?


Description:
Optimizations for pure-height approximations.

Since we end up querying the height of each VerticalAxisGroup
multiple times for each line, cache the intermediate results.

Please review this at http://codereview.appspot.com/1817045/show

Affected files:
  M lily/axis-group-interface.cc
  M lily/include/axis-group-interface.hh
  M scm/define-grob-properties.scm
  M stepmake/aclocal.m4



___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Patch for issue #1116 (one stencil in fill-line) (issue1689041)

[joeneeman]

Are you still waiting for someone to review this? If so, here are a
couple minor things:


http://codereview.appspot.com/1689041/diff/2001/3001
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1689041/diff/2001/3001#newcode848
scm/define-markup-commands.scm:848: X RIGHT fill-space line-stencils)))
These two set!s could be written more concisely as
(set! line-stencils
  (stack-stencils-padding-list
   X RIGHT fill-space empty-stencil)))

http://codereview.appspot.com/1689041/diff/2001/3001#newcode850
scm/define-markup-commands.scm:850: (if ( word-count 1)
I know there aren't many comments in the code, but that doesn't mean you
can't add one...

http://codereview.appspot.com/1689041/show

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix 1112. (issue1670042)

[joeneeman]

Reviewers: Neil Puttock,

Message:
Thanks, fixed. I'll push after make check finishes...


http://codereview.appspot.com/1670042/diff/1/3
File lily/constrained-breaking.cc (right):

http://codereview.appspot.com/1670042/diff/1/3#newcode384
lily/constrained-breaking.cc:384: Line_details for anything.  That's
because the approximations
On 2010/06/20 20:12:52, Neil Puttock wrote:

Is this still true?  It appears to be used in several places.


We read it in several places, but we never set it to anything other than
zero. So it's ready to be used as soon as we decide to set it to
something better than zero, but I don't know what that is yet...

http://codereview.appspot.com/1670042/diff/1/3#newcode406
lily/constrained-breaking.cc:406: SCM page_breaking_spacing_spec =
l-c_variable (page-breaking-between-system-spacing);
On 2010/06/20 20:12:52, Neil Puttock wrote:

Is this here for future use or is it left over from your original

spacing patch?

It's read in lines 413 and 425 (but I see that it isn't documented
correctly in the NR...)

Description:
Fix 1112.
Add support for minimum-distance into the page-breaker.

Please review this at http://codereview.appspot.com/1670042/show

Affected files:
  A input/regression/page-breaking-min-distance.ly
  M lily/constrained-breaking.cc
  M lily/include/constrained-breaking.hh
  M lily/page-breaking.cc
  M lily/page-spacing.cc



___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix #943 (input/regression/slur-broken-trend.ly broken) (issue203054)

[joeneeman]

Just for the record, I posted to lily-devel (because I wanted to attach
a file and I can't seem to do that here) to point out that this patch
breaks input/regression/page-spacing-rehearsal-mark.ly.

http://codereview.appspot.com/203054/show


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix #943 (input/regression/slur-broken-trend.ly broken) (issue203054)

[joeneeman]

http://codereview.appspot.com/203054/diff/1/2
File lily/system.cc (left):

http://codereview.appspot.com/203054/diff/1/2#oldcode193
lily/system.cc:193: }
For vertical positioning to work, it's important that
after-line-breaking be called before Page_layout_problem does its work.
Can you check that this is still the case?

http://codereview.appspot.com/203054/show


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Avoid orphan/widow lines (issue190102)

[joeneeman]

lgtm, modulo some more formatting nitpicking. If you fix the formatting
and mail me the patch, I'll push it.

Also, in the future, please add lilypond-devel@gnu.org to the CC list (I
should have mentioned it, sorry).



http://codereview.appspot.com/190102/diff/1/2
File lily/constrained-breaking.cc (right):

http://codereview.appspot.com/190102/diff/1/2#newcode529
lily/constrained-breaking.cc:529: last_markup_line_  =
to_boolean(last_scm);
space before (

http://codereview.appspot.com/190102/diff/1/2#newcode531
lily/constrained-breaking.cc:531: first_markup_line_ =
to_boolean(first_scm);
space before (

http://codereview.appspot.com/190102/diff/1/7
File lily/paper-book.cc (right):

http://codereview.appspot.com/190102/diff/1/7#newcode554
lily/paper-book.cc:554: {
indentation should be
if (blah)
  {
foo();
  }

http://codereview.appspot.com/190102/diff/1/7#newcode558
lily/paper-book.cc:558: ps-set_property
(first-markup-line, SCM_BOOL_F);
extra spaces?

http://codereview.appspot.com/190102/show


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix DynamicTextSpanner left alignment. (issue186112)

[joeneeman]

lgtm

http://codereview.appspot.com/186112


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add -dmusic-strings-to-paths and enable for SVG backend. (issue186054)

[joeneeman]

On 2010/01/10 06:40:56, Patrick McCarty wrote:

On 2010/01/10 05:03:30, joeneeman wrote:
 http://codereview.appspot.com/186054/diff/1/2
 File lily/pango-font.cc (right):

 http://codereview.appspot.com/186054/diff/1/2#newcode384
 lily/pango-font.cc:384: bool to_paths = get_program_option
 (music-strings-to-paths);
 It would be more consistent, I think, to use a global variable here.



Done.



 http://codereview.appspot.com/186054/diff/1/2#newcode386
 lily/pango-font.cc:386: if (has_utf8_string  ((to_paths 

!music_string) ||

 !to_paths))
 If I understand this correctly, the SVG backend will now output feta
characters
 using utf-8-string by default (ie. unless the user manually

specifies

 -dmusic-strings-to-paths)?



It's actually the opposite.  -dmusic-strings-to-paths is true by

default for the

SVG backend, which is now set in scm/lily.scm.  The backend will only

use

utf-8-string when a non-music string is encountered.


Oops, I missed the second chunk of lily.scm. LGTM


http://codereview.appspot.com/186054


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Add -dmusic-strings-to-paths and enable for SVG backend. (issue186054)

[joeneeman]

http://codereview.appspot.com/186054/diff/1/2
File lily/pango-font.cc (right):

http://codereview.appspot.com/186054/diff/1/2#newcode384
lily/pango-font.cc:384: bool to_paths = get_program_option
(music-strings-to-paths);
It would be more consistent, I think, to use a global variable here.

http://codereview.appspot.com/186054/diff/1/2#newcode386
lily/pango-font.cc:386: if (has_utf8_string  ((to_paths 
!music_string) || !to_paths))
If I understand this correctly, the SVG backend will now output feta
characters using utf-8-string by default (ie. unless the user manually
specifies -dmusic-strings-to-paths)?

http://codereview.appspot.com/186054


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes issue 786, Extenders in lyrics stop prematurely if a single underscore is found.

[joeneeman]

lgtm


http://codereview.appspot.com/150067/diff/3018/4006
File lily/extender-engraver.cc (right):

http://codereview.appspot.com/150067/diff/3018/4006#newcode118
lily/extender-engraver.cc:118: Moment *start_mom = column ?
unsmob_moment (column-get_property (when) ) : 0;
no space between ) )

http://codereview.appspot.com/150067


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: New twoside mode.

[joeneeman]

http://codereview.appspot.com/144049/diff/1/4
File ly/paper-defaults-init.ly (right):

http://codereview.appspot.com/144049/diff/1/4#newcode90
Line 90: twoside = ##f
On 2009/10/30 22:19:50, Neil Puttock wrote:

I'm not too fond of this name, but can't think of anything better. :)


I think two-side would be at least a slight improvement.

http://codereview.appspot.com/144049


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Manual beaming forced directions (^[ _[).

[joeneeman]

lgtm

http://codereview.appspot.com/120060


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Use our own ~s ly:format placeholder, since guile is broken with wide chars

[joeneeman]

On 2009/08/22 00:11:34, Reinhold wrote:

http://codereview.appspot.com/109070/diff/1/2
File lily/general-scheme.cc (right):



http://codereview.appspot.com/109070/diff/1/2#newcode437
Line 437: replace_all (s, \, \\\);
On 2009/08/21 22:59:33, joeneeman wrote:
 Don't forget to escape $. Or else use single quotes rather than

double quotes.


Good catch. $ needs to be escaped, too. Using single quotes does not

work with

gs.


Well, it should be the shell that sees the quotes, not gs. Perhaps
you're using cmd.exe and it doesn't like single quotes? Which brings up
the issue that the escaping/quoting rules are different between windows
and linux, so you'd need to test which system lilypond is running on...

Unless you use guile's system* function instead of system, which doesn't
run a shell and hence needs no escaping or quoting. That's probably a
safer and simpler approach.

http://codereview.appspot.com/109070


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Implement new handling for margin and line-width settings.

[joeneeman]

Could you add some regression tests (doesn't have to be in this commit)
that demonstrate some of the possible combinations of margin settings?
There should also be some tests that demonstrate the warnings.


http://codereview.appspot.com/104085/diff/1/4
File lily/output-def.cc (right):

http://codereview.appspot.com/104085/diff/1/4#newcode149
Line 149: if ((lookup_variable (ly_symbol2scm (is-layout)) ==
SCM_BOOL_T)  (scm_line_width != SCM_UNDEFINED)) return;
put the return on a separate line

http://codereview.appspot.com/104085/diff/1/4#newcode183
Line 183: if (paper_width != (line_width + left_margin + right_margin))
In case there is rounding, it is better to check
if (abs (paper_width - line_width - left_margin - right_margin)  1e-6)

http://codereview.appspot.com/104085/diff/1/4#newcode205
Line 205: scm_from_double(paper_width - left_margin_default -
right_margin_default));
indentation

http://codereview.appspot.com/104085


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

SVG backend: Output a single SVG file for each page

[joeneeman]

lgtm. You can ignore my nitpicking if you disagree.


http://codereview.appspot.com/105045/diff/5/1005
File scm/framework-svg.scm (right):

http://codereview.appspot.com/105045/diff/5/1005#newcode60
Line 60: (dump (ec 'svg))
Just a very very minor thing: IWBN to have eo and ec closer together.
For example, if svg-header just returned the alist of attributes then
you could write
(dump (eo (cons 'svg (svg-header paper)))
...
(dump (ec 'svg))
and it would be totally obvious that they match.

http://codereview.appspot.com/105045


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: SVG backend: On-the-fly conversion of Emmentaler/Aybabtu glyphs to paths

[joeneeman]

On 2009/07/24 23:30:00, Patrick McCarty wrote:

On 2009/07/21 18:43:10, joeneeman wrote:
 I think it would be helpful if you gave an example or 2 of the sort

of string

 you expect to match here. I'm a bit worried also about the fact that

you

require
 the attributes to be in a specific order.



The new code uses a generalized regular expression that will match a

glyph

element with attributes in any order.



The code is slower now, I think due to the complexity of the new

regular

expression.  It might be a better idea if I rewrite this in C++

eventually.

How slow is it? Are we talking would-be-nice-if-it-were-faster slow or
nobody-will-want-to-use-it slow? The reason I ask is that I think the
_real_ solution would be to use an actual XML parser to parse the whole
svg font and dump all the glyphs into a hash table. As it is, you're
scanning a complicated regexp through the whole font file every time you
want a glyph.

Of course, that's a bunch of work and it involves, for example, deciding
which XML parsing lib to use and whether to use it from C++ and scheme.
So unless it's unusably slow, I'd suggest adding a TODO and committing
this patch.


http://codereview.appspot.com/96083


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: SVG backend: On-the-fly conversion of Emmentaler/Aybabtu glyphs to paths

[joeneeman]

It seems as though rietveld wants to send my code-comments and my
reply-to-comments comments in two separate emails. Sorry for the noise.


http://codereview.appspot.com/96083/diff/1015/20
File lily/pango-font.cc (right):

http://codereview.appspot.com/96083/diff/1015/20#newcode354
Line 354: bool has_utf8_string = false;
I like this way of dealing with the ps backend much better...

http://codereview.appspot.com/96083/diff/1015/20#newcode367
Line 367: if ((name == svg  !feta) || (name != svg 
has_utf8_string))
...but I think it would be ideal if the text_stencil code was totally
unaware of the backend (maybe using something like
-ddump-music-strings-as-paths, which defaults to true when
-dbackend=svg). I'm not suggesting you do it now, but it would be nice
to have a NOTE or TODO here.

http://codereview.appspot.com/96083


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: New format for autobeaming rules

[joeneeman]

I think this is pretty much ready to commit


http://codereview.appspot.com/88155/diff/3101/4032
File lily/beam-scheme.cc (right):

http://codereview.appspot.com/88155/diff/3101/4032#newcode2
Line 2: beam-scheme.cc -- Retrieving beam settings
could you call this beam-grouping-scheme.cc or something like that?
beam-scheme sounds like it contains routines for manipulating Beam
grobs.

http://codereview.appspot.com/88155/diff/3101/4032#newcode12
Line 12: LY_DEFINE (ly_beam_settings, ly:beam-settings,
is this function really necessary?

http://codereview.appspot.com/88155/diff/3101/4032#newcode49
Line 49: ly_grouping_rules(settings,time_signature,rule_type),
formatting

http://codereview.appspot.com/88155/diff/3101/4032#newcode61
Line 61: SCM settings = ly_beam_settings(context);
formatting

http://codereview.appspot.com/88155


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: New format for autobeaming rules

[joeneeman]

very nice!


http://codereview.appspot.com/88155/diff/3092/2039
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/88155/diff/3092/2039#newcode699
Line 699: (make-music 'SequentialMusic 'void #t))
I'd feel better if this were finished. At least add FIXME so it can be
easily grepped for.

http://codereview.appspot.com/88155/diff/3092/2047
File scm/music-functions.scm (right):

http://codereview.appspot.com/88155/diff/3092/2047#newcode546
Line 546: (define (make-default-beaming-rule context)
this doesn't seem to be used...

http://codereview.appspot.com/88155


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

SVG backend: On-the-fly conversion of Emmentaler/Aybabtu glyphs to paths

[joeneeman]

http://codereview.appspot.com/96083/diff/1/8
File lily/pango-font.cc (right):

http://codereview.appspot.com/96083/diff/1/8#newcode351
Line 351: // options.
Could you please have a look at this? (after applying this patch, if you
prefer). The more special cases we add for different backends, the
messier this gets.

http://codereview.appspot.com/96083/diff/1/9
File lily/text-interface.cc (right):

http://codereview.appspot.com/96083/diff/1/9#newcode80
Line 80: if (ly_symbol2string (encoding) == latin1)
isn't there some possibility that we will have an encoding other than
latin1, fetaNumber or fetaDynamic?

http://codereview.appspot.com/96083/diff/1/12
File scm/output-svg.scm (right):

http://codereview.appspot.com/96083/diff/1/12#newcode184
Line 184: (make-regexp (string-append glyph-name=\(
I think it would be helpful if you gave an example or 2 of the sort of
string you expect to match here. I'm a bit worried also about the fact
that you require the attributes to be in a specific order.

http://codereview.appspot.com/96083


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix crash when a stencil routine is missing

[joeneeman]

lgtm

http://codereview.appspot.com/83046


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

New instrument name positioning in Scheme.

[joeneeman]

Just one corner case, otherwise lgtm


http://codereview.appspot.com/91119/diff/1/10
File scm/output-lib.scm (right):

http://codereview.appspot.com/91119/diff/1/10#newcode833
Line 833: (interval-center extent
If (not (pair? live-elts)) then (interval-center extent) will be NaN,
instead of 0 which would be more sensible.

http://codereview.appspot.com/91119


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Improvements for the SVG backend

[joeneeman]

http://codereview.appspot.com/91075/diff/1/3
File scm/output-svg.scm (right):

http://codereview.appspot.com/91075/diff/1/3#newcode140
Line 140: (match:substring match 1)))
Why not just (set-attribute 'font-weight bold)?
Similarly below.

http://codereview.appspot.com/91075/diff/1/3#newcode155
Line 155: (set! alist (reverse alist))
You can use reverse! here

http://codereview.appspot.com/91075


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix crash when a stencil routine is missing

[joeneeman]

http://codereview.appspot.com/83046/diff/1/3
File scm/output-ps.scm (right):

http://codereview.appspot.com/83046/diff/1/3#newcode58
Line 58: (ly:all-output-backend-commands)
Perhaps this could be a macro (so that you don't need to cp for every
backend). And if you add a -d option like Han-Wen suggested then you can
get rid of the (if #t ...)

http://codereview.appspot.com/83046


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

First release of ly/tablature.ly

[joeneeman]

I don't know much about the issues here, but here's a review, fwiw. Of
course, you'll need to update the docs also (and a regression test or 2
would be nice).


http://codereview.appspot.com/67174/diff/1/2
File ly/tablature.ly (right):

http://codereview.appspot.com/67174/diff/1/2#newcode205
Line 205: tabFullNotation = {
Why not create a new context, FullTabVoice, or something like that? Do
you expect people to use \tabFullNotation mid-piece?

http://codereview.appspot.com/67174/diff/1/2#newcode237
Line 237: \layout {
It seems to me that we have 3 versions of tablature now: the default
version in engraver-init.ly, the version that appears if the user
includes tablature.ly and the full version that the user gets by
including tablature.ly and using the tabFullNotation. Is there a reason
for keeping the old versions in engraver-init.ly? Perhaps we should
scrap them altogether and require that the user include tablature.ly in
order to get tabs.

http://codereview.appspot.com/67174


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix key signatures with accidentals in specific octave.

[joeneeman]

http://codereview.appspot.com/11052/diff/3409/2410
File scm/music-functions.scm (right):

http://codereview.appspot.com/11052/diff/3409/2410#newcode1047
Line 1047: ((and (equal? ignore-octave #f)
I think eq? is more appropriate here

http://codereview.appspot.com/11052/diff/3409/2410#newcode1048
Line 1048: (and
use (and a b c) instead of (and a (and b c))

http://codereview.appspot.com/11052/diff/3409/2410#newcode1073
Line 1073: (if (and (not (= (sign this-alt) 0))
Surely (= (sign x) 0) is the same as (= x 0)

http://codereview.appspot.com/11052/diff/3409/2410#newcode1075
Line 1075: ( (sign (* prev-alt this-alt)) 0)))
Surely ( (sign x) 0) is the same as ( x 0)

http://codereview.appspot.com/11052


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Improve implementation of dashed slurs

[joeneeman]

Very pretty slurs!


http://codereview.appspot.com/41099/diff/1021/59
File lily/bezier.cc (right):

http://codereview.appspot.com/41099/diff/1021/59#newcode275
Line 275: Bezier::subdivide (Real t, Bezier left_part, Bezier
right_part)
We only use references if they are const (for clarity), so please change
the arguments to pointers. Also, this function can be marked as const.

http://codereview.appspot.com/41099/diff/1021/59#newcode296
Line 296: Bezier::extract (Real t_min, Real t_max)
const here too

http://codereview.appspot.com/41099/diff/1021/59#newcode302
Line 302: bez2.control_[i] = control_[i];
bez2 = *this

http://codereview.appspot.com/41099/diff/1021/62
File lily/lookup.cc (right):

http://codereview.appspot.com/41099/diff/1021/62#newcode347
Line 347: SCM dash_details)
Since dash_details seems to just be a list of Reals, perhaps its better
to have a vectorReal const (with an empty vector to signify a solid
slur).

http://codereview.appspot.com/41099


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Move left-broken line-spanner check to callback.

[joeneeman]

On 2009/04/11 12:31:10, Neil Puttock wrote:

On 2009/04/08 20:42:51, joeneeman wrote:
 http://codereview.appspot.com/32148/diff/1/7#newcode426
 Line 426: Spanner::after_line_breaking (SCM grob)
 Can you think of a name that describes what the function actually

does? Like

 Spanner::suicide_if_spanned_time_is_empty, but maybe less verbose...



How about Spanner::kill_zero_length_span or

Spanner::kill_zero_spanned_time?

I prefer the second one.

http://codereview.appspot.com/32148


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix #670: Chained trills

[joeneeman]

lgtm

http://codereview.appspot.com/32142


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Move left-broken line-spanner check to callback.

[joeneeman]

One minor comment, otherwise lgtm


http://codereview.appspot.com/32148/diff/1/7
File lily/spanner.cc (right):

http://codereview.appspot.com/32148/diff/1/7#newcode426
Line 426: Spanner::after_line_breaking (SCM grob)
Can you think of a name that describes what the function actually does?
Like Spanner::suicide_if_spanned_time_is_empty, but maybe less
verbose...

http://codereview.appspot.com/32148


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix spacing for accidentals in tied chords

[joeneeman]

http://codereview.appspot.com/28134/diff/1/3
File lily/accidental-engraver.cc (right):

http://codereview.appspot.com/28134/diff/1/3#newcode431
Line 431: Grob *newa = 0;
Please use a more descriptive name. Also, add a comment regarding why
you need two copies of the accidental.

http://codereview.appspot.com/28134/diff/1/4
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/28134/diff/1/4#newcode30
Line 30: Accidental_placement::add_accidental (Grob *me, Grob *a, Grob
*newa)
Please use more descriptive parameter names.

http://codereview.appspot.com/28134/diff/1/4#newcode71
Line 71: if (break_reminder)
slightly cleaner, perhaps, to do
SCM accidental_sym = ly_symbol2scm(break_reminder ?
accidental-break-reminder-grobs : accidental_grobs);

and then
accs = me-get_object(accidental_sym);
...
me-set_object(accidental_sym, accs);

http://codereview.appspot.com/28134/diff/1/4#newcode97
Line 97: ret.push_back(a);
space before (

http://codereview.appspot.com/28134/diff/1/4#newcode433
Line 433: return;
Don't need the return here

http://codereview.appspot.com/28134/diff/1/4#newcode436
Line 436: void Accidental_placement::filter_tied_accidentals (Grob * me)
Rather than coping every accidental and then filtering out some, have
you considered delaying the copying until calc_positioning_done and only
copying the accidentals you care about? This also has the advantage of
hiding more stuff in accidental-placement instead of modifying
accidental-engraver.

http://codereview.appspot.com/28134/diff/1/4#newcode460
Line 460: accidental-break-reminder-grobs 
Only properties go here, not objects.

http://codereview.appspot.com/28134/diff/1/5
File lily/accidental.cc (right):

http://codereview.appspot.com/28134/diff/1/5#newcode181
Line 181: }
I'm a little confused about this. Depending on whether a group of
accidentals is at the beginning of a line or not, you want either all of
the break-reminder accidentals or all of the non-break-reminder
accidentals to suicide. It's not clear to me that this is achieved by
the above code.

http://codereview.appspot.com/28134/diff/1/8
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/28134/diff/1/8#newcode237
Line 237: if (Grob *g = unsmob_grob (me-get_property
(accidental-grob)))
get_object (and subsequently, too)

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

http://codereview.appspot.com/28134/diff/1/13#newcode835
Line 835: @var{groblist})} entries. Includes accidentals that will be
printed if at the beginning of the line.)
Consists of is more accurate than Includes, I think.

http://codereview.appspot.com/28134/diff/1/13#newcode841
Line 841: @var{groblist})} entries. Does not include tied accidentals
that won't be printed mid-line.)
Can you rephrase to avoid the double-negative?

http://codereview.appspot.com/28134


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix key signatures with accidentals in specific octave.

[joeneeman]

lgtm, except for the description move check_pitch_against_signature ()
to SCM, since ly:check-pitch-against-signature is still implemented in
C++.

http://codereview.appspot.com/11052


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Add chordChanges capability to FretBoard grobs

[joeneeman]

lgtm

http://codereview.appspot.com/24044


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Implement \eyeglasses as markup command

[joeneeman]

Could you add the command to appendix B.8.3 of the manual? And maybe
change the example for the \postscript command. Otherwise, lgtm

http://codereview.appspot.com/12660


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Updates to fret-diagrams

[joeneeman]

Reviewers: Carl.D.Sorensen,


http://codereview.appspot.com/11857/diff/1/2
File input/regression/fret-diagrams.ly (right):

http://codereview.appspot.com/11857/diff/1/2#newcode1
Line 1: \version 2.12.0
This regtest is getting quite large. Is there a logical way to split it
up (eg. fret-diagrams-landscape, fret-diagrams-string-count, etc)?

http://codereview.appspot.com/11857/diff/1/4
File scm/fret-diagrams.scm (right):

http://codereview.appspot.com/11857/diff/1/4#newcode1
Line 1:  fret-diagrams.scm --
I don't know enough about fret diagrams to really understand what's
going on, but I have one fairly general comment: the code is sprinkled
with
(cond ((eq? orientation 'landscape)
   foo)
  ((eq? orientation 'opposing-landscape)
   bar)
  (else
   baz))
where foo, bar and baz are almost the same except that they have
differences in signs and the X,Y are swapped around. It would be great
if this cond could be isolated to a few functions. For example:

(define (real-coordinate string-coordinate fret-coordinate orientation)
  (cond
   ((eq? orientation 'landscape)
(cons fret-coordinate string-coordinate))
   ((eq? orientation 'opposing-landscape)
(cons (-fret-coordinate string-coordinate))
   (else
(cons string-coordinate fret-coordinate

and then make-straight-barre-stencil (for example) becomes

(define (make-straight-barre-stencil
 size half-thickness fret-coordinate
 start-string-coordinate end-string-coordinate
 orientation)
  (let ((one-point (real-coordinate start-string-coordinate
fret-coordinate orientation))
(other-point (real-coordinate end-string-coordinate
fret-coordinate orientation)))
   (ly:make-line-stencil
 half-thickness
 (car one-point) (cdr one-point)
 (car other-point) (cdr other-point)))

If you're in a hurry to get the functionality in, don't let this hold
you up.

http://codereview.appspot.com/11857/diff/1/4#newcode91
Line 91: (cond
Here again you have cond leading to duplicate code. If you had
(define (combine-stencils a b c d) (ly:stencil-combine-at-edge a (car b)
(cdr b) c d))
then you could move the cond inside:

(combine-stencils string-stencil
  (cond
((eq? orientation 'landscape) (cons Y UP))
((eq? orientation 'opposing-landscape) (cons Y DOWN))
(else (cons X RIGHT)))
  (draw-strings (- string-count 1) fret-range th size orientation)
  gap))

Description:
Updates to fret-diagrams
Add new orientation opposing-landscape as requested by user
Revise orientation code so 'normal is always a default (in
the else clause of a cond)
Adjust the origin of the fret diagram so that the top fret
  on the diagram is always at the origin
Clean up calls for fret-diagram-details so that merge-details is always
  used.
Clean up text stencil alignments
Fix bug in thick top fret
Revise input/regression/fret-diagrams.ly so that it tests all
fret-diagram code functionality.

Please review this at http://codereview.appspot.com/11857

Affected files:
  M input/regression/fret-diagrams.ly
  M scm/define-grob-properties.scm
  M scm/fret-diagrams.scm




___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel