[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Allow a markup to replace the default LyricHyphen (issue 325470043 b
From: |
knupero |
Subject: |
Re: Allow a markup to replace the default LyricHyphen (issue 325470043 by address@hidden) |
Date: |
Sun, 17 Sep 2017 03:16:01 -0700 |
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc
File lily/lyric-hyphen.cc (right):
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newcode125
lily/lyric-hyphen.cc:125: Box b (Interval (0, dash_length), Interval (h,
h + th));
On 2017/09/16 19:53:56, dak wrote:
It seems wasteful to put these in the loop, particularly since
dash_mol is then
additionally copied before shifting. Maybe pull if
(Text_interface::is_markup
...) outside of the loop and have a separate loop for each branch?
There is no
shared loop code outside of if/else in the current implementation
anyway.
Well, you are right.
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newcode126
lily/lyric-hyphen.cc:126: Stencil dash_mol (Lookup::round_filled_box (b,
0.8 * lt));
Shouldn't the hardcoded 0.8 be replaced by a property?
https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm
File scm/define-grobs.scm (right):
https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm#newcode1403
scm/define-grobs.scm:1403: (text . '())
On 2017/09/16 19:53:56, dak wrote:
That's the default when unset. For properties that may optionally be
set, in
particular when this default formally does not match the predicate, I
think we
tend not to explicitly specify it.
Well, omitting it here means that there is no indication in the
internals reference that LyricHyphen uses that property.
https://codereview.appspot.com/325470043/